1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()

Fei Li posted 1 patch 5 years, 3 months ago
Test asan passed
Test checkpatch passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190113143641.38936-1-lifei1214@126.com
Maintainers: Jiri Slaby <jslaby@suse.cz>
There is a newer version of this series
hw/misc/edu.c | 1 +
1 file changed, 1 insertion(+)
[Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Fei Li 5 years, 3 months ago 
From: Fei Li <shirley17fei@gmail.com>

Let's supplement the msi_uninit() when failing to realize
the pci edu device.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Fei Li <shirley17fei@gmail.com>
---
 hw/misc/edu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index cdcf550dd7..4feb7503de 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
 {
     EduState *edu = EDU(pdev);
 
+    msi_uninit(pdev);
     qemu_mutex_lock(&edu->thr_mutex);
     edu->stopping = true;
     qemu_mutex_unlock(&edu->thr_mutex);
-- 
2.17.2 (Apple Git-113)
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Marcel Apfelbaum 5 years, 3 months ago 

On 1/13/19 4:36 PM, Fei Li wrote:
> From: Fei Li <shirley17fei@gmail.com>
>
> Let's supplement the msi_uninit() when failing to realize
> the pci edu device.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Fei Li <shirley17fei@gmail.com>
> ---
>   hw/misc/edu.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index cdcf550dd7..4feb7503de 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
>   {
>       EduState *edu = EDU(pdev);
>   
> +    msi_uninit(pdev);
>       qemu_mutex_lock(&edu->thr_mutex);
>       edu->stopping = true;
>       qemu_mutex_unlock(&edu->thr_mutex);

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Peter Xu 5 years, 3 months ago 
On Sun, Jan 13, 2019 at 10:36:41PM +0800, Fei Li wrote:
> From: Fei Li <shirley17fei@gmail.com>
> 
> Let's supplement the msi_uninit() when failing to realize
> the pci edu device.
> 
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Fei Li <shirley17fei@gmail.com>
> ---
>  hw/misc/edu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index cdcf550dd7..4feb7503de 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
>  {
>      EduState *edu = EDU(pdev);
>  
> +    msi_uninit(pdev);

It would be cleaner to me to call this after the join() since
edu_fact_thread() could potentially use msi_*() helpers then the
destructions follow the reverse order of init.

Reviewed-by: Peter Xu <peterx@redhat.com>

>      qemu_mutex_lock(&edu->thr_mutex);
>      edu->stopping = true;
>      qemu_mutex_unlock(&edu->thr_mutex);
> -- 
> 2.17.2 (Apple Git-113)
> 

Regards,

-- 
Peter Xu
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Markus Armbruster 5 years, 3 months ago 
Peter Xu <peterx@redhat.com> writes:

> On Sun, Jan 13, 2019 at 10:36:41PM +0800, Fei Li wrote:
>> From: Fei Li <shirley17fei@gmail.com>
>> 
>> Let's supplement the msi_uninit() when failing to realize
>> the pci edu device.
>> 
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Signed-off-by: Fei Li <shirley17fei@gmail.com>
>> ---
>>  hw/misc/edu.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>> index cdcf550dd7..4feb7503de 100644
>> --- a/hw/misc/edu.c
>> +++ b/hw/misc/edu.c
>> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
>>  {
>>      EduState *edu = EDU(pdev);
>>  
>> +    msi_uninit(pdev);
>
> It would be cleaner to me to call this after the join() since
> edu_fact_thread() could potentially use msi_*() helpers then the
> destructions follow the reverse order of init.

Destruction in reverse creation order is good practice.

This being the "QEMU educational PCI device", good practice is even more
desirable.

> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>>      qemu_mutex_lock(&edu->thr_mutex);
>>      edu->stopping = true;
>>      qemu_mutex_unlock(&edu->thr_mutex);
>> -- 
>> 2.17.2 (Apple Git-113)
>> 
>
> Regards,
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Peter Xu 5 years, 3 months ago 
On Mon, Jan 14, 2019 at 08:02:23AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Sun, Jan 13, 2019 at 10:36:41PM +0800, Fei Li wrote:
> >> From: Fei Li <shirley17fei@gmail.com>
> >> 
> >> Let's supplement the msi_uninit() when failing to realize
> >> the pci edu device.
> >> 
> >> Cc: Markus Armbruster <armbru@redhat.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >> Signed-off-by: Fei Li <shirley17fei@gmail.com>
> >> ---
> >>  hw/misc/edu.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> >> index cdcf550dd7..4feb7503de 100644
> >> --- a/hw/misc/edu.c
> >> +++ b/hw/misc/edu.c
> >> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
> >>  {
> >>      EduState *edu = EDU(pdev);
> >>  
> >> +    msi_uninit(pdev);
> >
> > It would be cleaner to me to call this after the join() since
> > edu_fact_thread() could potentially use msi_*() helpers then the
> > destructions follow the reverse order of init.
> 
> Destruction in reverse creation order is good practice.
> 
> This being the "QEMU educational PCI device", good practice is even more
> desirable.

Very persuasive.  With that, I'd like to withdraw my excuse of
"msi_uninit() is optional" too. :)

Fei, please feel free to pick my r-b if you want to repost, and IMHO
you can also add:

Reported-by: Markus Armbruster <armbru@redhat.com>

Thanks,

-- 
Peter Xu
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Philippe Mathieu-Daudé 5 years, 3 months ago 
On 1/14/19 8:18 AM, Peter Xu wrote:
> On Mon, Jan 14, 2019 at 08:02:23AM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>>> On Sun, Jan 13, 2019 at 10:36:41PM +0800, Fei Li wrote:
>>>> From: Fei Li <shirley17fei@gmail.com>
>>>>
>>>> Let's supplement the msi_uninit() when failing to realize
>>>> the pci edu device.
>>>>
>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>> Cc: Peter Xu <peterx@redhat.com>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>> Signed-off-by: Fei Li <shirley17fei@gmail.com>
>>>> ---
>>>>  hw/misc/edu.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>>>> index cdcf550dd7..4feb7503de 100644
>>>> --- a/hw/misc/edu.c
>>>> +++ b/hw/misc/edu.c
>>>> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
>>>>  {
>>>>      EduState *edu = EDU(pdev);
>>>>  
>>>> +    msi_uninit(pdev);
>>>
>>> It would be cleaner to me to call this after the join() since
>>> edu_fact_thread() could potentially use msi_*() helpers then the
>>> destructions follow the reverse order of init.
>>
>> Destruction in reverse creation order is good practice.
>>
>> This being the "QEMU educational PCI device", good practice is even more
>> desirable.
> 
> Very persuasive.  With that, I'd like to withdraw my excuse of
> "msi_uninit() is optional" too. :)
> 
> Fei, please feel free to pick my r-b if you want to repost, and IMHO
> you can also add:
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> 
> Thanks,
> 

Moving msi_uninit() after timer_del():
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Re: [Qemu-devel] [PATCH] hw/misc/edu: add msi_uninit() for pci_edu_uninit()
Posted by Fei Li 5 years, 3 months ago 
在 2019/1/14 下午6:40, Philippe Mathieu-Daudé 写道:
> On 1/14/19 8:18 AM, Peter Xu wrote:
>> On Mon, Jan 14, 2019 at 08:02:23AM +0100, Markus Armbruster wrote:
>>> Peter Xu <peterx@redhat.com> writes:
>>>
>>>> On Sun, Jan 13, 2019 at 10:36:41PM +0800, Fei Li wrote:
>>>>> From: Fei Li <shirley17fei@gmail.com>
>>>>>
>>>>> Let's supplement the msi_uninit() when failing to realize
>>>>> the pci edu device.
>>>>>
>>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>>> Cc: Peter Xu <peterx@redhat.com>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>>> Signed-off-by: Fei Li <shirley17fei@gmail.com>
>>>>> ---
>>>>>   hw/misc/edu.c | 1 +
>>>>>   1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>>>>> index cdcf550dd7..4feb7503de 100644
>>>>> --- a/hw/misc/edu.c
>>>>> +++ b/hw/misc/edu.c
>>>>> @@ -367,6 +367,7 @@ static void pci_edu_uninit(PCIDevice *pdev)
>>>>>   {
>>>>>       EduState *edu = EDU(pdev);
>>>>>   
>>>>> +    msi_uninit(pdev);
>>>> It would be cleaner to me to call this after the join() since
>>>> edu_fact_thread() could potentially use msi_*() helpers then the
>>>> destructions follow the reverse order of init.
>>> Destruction in reverse creation order is good practice.
>>>
>>> This being the "QEMU educational PCI device", good practice is even more
>>> desirable.
>> Very persuasive.  With that, I'd like to withdraw my excuse of
>> "msi_uninit() is optional" too. :)
>>
>> Fei, please feel free to pick my r-b if you want to repost, and IMHO
>> you can also add:
>>
>> Reported-by: Markus Armbruster <armbru@redhat.com>
>>
>> Thanks,
>>
> Moving msi_uninit() after timer_del():
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Ok, thanks all!

Will do the update and send a new version soon.

Have a nice day
Fei

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.