Patchwork [2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

login
register
mail settings
Submitter Wei Yang
Date Aug. 16, 2013, 10:08 a.m.
Message ID <1376647687-20550-3-git-send-email-weiyang@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/267608/
State Not Applicable
Headers show

Comments

Wei Yang - Aug. 16, 2013, 10:08 a.m.
---
 arch/powerpc/kernel/iommu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Alexey Kardashevskiy - Aug. 16, 2013, 10:15 a.m.
On 08/16/2013 08:08 PM, Wei Yang wrote:
> ---
>  arch/powerpc/kernel/iommu.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index b20ff17..5abf7c3 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>  	case BUS_NOTIFY_ADD_DEVICE:
>  		return iommu_add_device(dev);
>  	case BUS_NOTIFY_DEL_DEVICE:
> -		iommu_del_device(dev);
> +		if (dev->iommu_group)
> +			iommu_del_device(dev);
>  		return 0;
>  	default:
>  		return 0;
> 

This one seems redundant, no?
Wei Yang - Aug. 19, 2013, 1:29 a.m.
On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>On 08/16/2013 08:08 PM, Wei Yang wrote:
>> ---
>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> index b20ff17..5abf7c3 100644
>> --- a/arch/powerpc/kernel/iommu.c
>> +++ b/arch/powerpc/kernel/iommu.c
>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>  	case BUS_NOTIFY_ADD_DEVICE:
>>  		return iommu_add_device(dev);
>>  	case BUS_NOTIFY_DEL_DEVICE:
>> -		iommu_del_device(dev);
>> +		if (dev->iommu_group)
>> +			iommu_del_device(dev);
>>  		return 0;
>>  	default:
>>  		return 0;
>> 
>
>This one seems redundant, no?

Sorry for the late.

Yes, these two patches have the same purpose to guard the system, while in two
different places.  One is in powernv platform, the other is in the generic iommu 
driver.

The one in powernv platform is used to correct the original logic.

The one in generic iommu driver is to keep system safe in case other platform to
call iommu_group_remove_device() without the check.

>
>
>-- 
>Alexey
Alexey Kardashevskiy - Aug. 19, 2013, 1:39 a.m.
On 08/19/2013 11:29 AM, Wei Yang wrote:
> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>> ---
>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>> index b20ff17..5abf7c3 100644
>>> --- a/arch/powerpc/kernel/iommu.c
>>> +++ b/arch/powerpc/kernel/iommu.c
>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>  		return iommu_add_device(dev);
>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>> -		iommu_del_device(dev);
>>> +		if (dev->iommu_group)
>>> +			iommu_del_device(dev);
>>>  		return 0;
>>>  	default:
>>>  		return 0;
>>>
>>
>> This one seems redundant, no?
> 
> Sorry for the late.
> 
> Yes, these two patches have the same purpose to guard the system, while in two
> different places.  One is in powernv platform, the other is in the generic iommu 
> driver.
> 
> The one in powernv platform is used to correct the original logic.
> 
> The one in generic iommu driver is to keep system safe in case other platform to
> call iommu_group_remove_device() without the check.


But I am moving bus notifier to powernv code (posted a patch last week,
otherwise Freescale's IOMMU conflicted) so this won't be the case.
Wei Yang - Aug. 19, 2013, 1:55 a.m.
On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>On 08/19/2013 11:29 AM, Wei Yang wrote:
>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>> ---
>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>> index b20ff17..5abf7c3 100644
>>>> --- a/arch/powerpc/kernel/iommu.c
>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>  		return iommu_add_device(dev);
>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>> -		iommu_del_device(dev);
>>>> +		if (dev->iommu_group)
>>>> +			iommu_del_device(dev);
>>>>  		return 0;
>>>>  	default:
>>>>  		return 0;
>>>>
>>>
>>> This one seems redundant, no?
>> 
>> Sorry for the late.
>> 
>> Yes, these two patches have the same purpose to guard the system, while in two
>> different places.  One is in powernv platform, the other is in the generic iommu 
>> driver.
>> 
>> The one in powernv platform is used to correct the original logic.
>> 
>> The one in generic iommu driver is to keep system safe in case other platform to
>> call iommu_group_remove_device() without the check.
>
>
>But I am moving bus notifier to powernv code (posted a patch last week,
>otherwise Freescale's IOMMU conflicted) so this won't be the case.

Yes, I see the patch.

This means other platforms, besides powernv, will check the dev->iommu_group
before remove the device? This would be a convention?

If this is the case, the second patch is enough. We don't need to check it in
generic iommu driver.

Since I am not very familiar with the code convention, I post these two
patches together. This doesn't mean I need to push both of them. Your comments
are welcome, lets me understand which one is more suitable in this case.

>
>
>
>-- 
>Alexey
Alexey Kardashevskiy - Aug. 22, 2013, 7:23 a.m.
On 08/19/2013 11:55 AM, Wei Yang wrote:
> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>>> ---
>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>>> index b20ff17..5abf7c3 100644
>>>>> --- a/arch/powerpc/kernel/iommu.c
>>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>>  		return iommu_add_device(dev);
>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>>> -		iommu_del_device(dev);
>>>>> +		if (dev->iommu_group)
>>>>> +			iommu_del_device(dev);
>>>>>  		return 0;
>>>>>  	default:
>>>>>  		return 0;
>>>>>
>>>>
>>>> This one seems redundant, no?
>>>
>>> Sorry for the late.
>>>
>>> Yes, these two patches have the same purpose to guard the system, while in two
>>> different places.  One is in powernv platform, the other is in the generic iommu 
>>> driver.
>>>
>>> The one in powernv platform is used to correct the original logic.
>>>
>>> The one in generic iommu driver is to keep system safe in case other platform to
>>> call iommu_group_remove_device() without the check.
>>
>>
>> But I am moving bus notifier to powernv code (posted a patch last week,
>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> 
> Yes, I see the patch.
> 
> This means other platforms, besides powernv, will check the dev->iommu_group
> before remove the device? This would be a convention?
> 
> If this is the case, the second patch is enough. We don't need to check it in
> generic iommu driver.
> 
> Since I am not very familiar with the code convention, I post these two
> patches together. This doesn't mean I need to push both of them. Your comments
> are welcome, lets me understand which one is more suitable in this case.


Ok. So. I included the check in the bus notifier which I moved to powernv
platform, I guess I'll repost the series soon.

Good luck with pushing the fix for drivers/iommu/iommu.c :)
Wei Yang - Aug. 22, 2013, 7:52 a.m.
On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
>On 08/19/2013 11:55 AM, Wei Yang wrote:
>> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>>>> ---
>>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>>>> index b20ff17..5abf7c3 100644
>>>>>> --- a/arch/powerpc/kernel/iommu.c
>>>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>>>  		return iommu_add_device(dev);
>>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>>>> -		iommu_del_device(dev);
>>>>>> +		if (dev->iommu_group)
>>>>>> +			iommu_del_device(dev);
>>>>>>  		return 0;
>>>>>>  	default:
>>>>>>  		return 0;
>>>>>>
>>>>>
>>>>> This one seems redundant, no?
>>>>
>>>> Sorry for the late.
>>>>
>>>> Yes, these two patches have the same purpose to guard the system, while in two
>>>> different places.  One is in powernv platform, the other is in the generic iommu 
>>>> driver.
>>>>
>>>> The one in powernv platform is used to correct the original logic.
>>>>
>>>> The one in generic iommu driver is to keep system safe in case other platform to
>>>> call iommu_group_remove_device() without the check.
>>>
>>>
>>> But I am moving bus notifier to powernv code (posted a patch last week,
>>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
>> 
>> Yes, I see the patch.
>> 
>> This means other platforms, besides powernv, will check the dev->iommu_group
>> before remove the device? This would be a convention?
>> 
>> If this is the case, the second patch is enough. We don't need to check it in
>> generic iommu driver.
>> 
>> Since I am not very familiar with the code convention, I post these two
>> patches together. This doesn't mean I need to push both of them. Your comments
>> are welcome, lets me understand which one is more suitable in this case.
>
>
>Ok. So. I included the check in the bus notifier which I moved to powernv
>platform, I guess I'll repost the series soon.

Thanks, this check will guard the powernv platform.

>
>Good luck with pushing the fix for drivers/iommu/iommu.c :)
>

Alex,

Sorry for not including you in the very beginning, which may spend you more
efforts to track previous mails in this thread.

Do you think it is reasonable to check the dev->iommu_group in
iommu_group_remove_device()? Or we can count on the bus notifier to check it?

Welcome your suggestions~

>
>
>-- 
>Alexey
Alex Williamson - Aug. 22, 2013, 3:28 p.m.
On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
> >On 08/19/2013 11:55 AM, Wei Yang wrote:
> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
> >>>>>> ---
> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >>>>>> index b20ff17..5abf7c3 100644
> >>>>>> --- a/arch/powerpc/kernel/iommu.c
> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
> >>>>>>  		return iommu_add_device(dev);
> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
> >>>>>> -		iommu_del_device(dev);
> >>>>>> +		if (dev->iommu_group)
> >>>>>> +			iommu_del_device(dev);
> >>>>>>  		return 0;
> >>>>>>  	default:
> >>>>>>  		return 0;
> >>>>>>
> >>>>>
> >>>>> This one seems redundant, no?
> >>>>
> >>>> Sorry for the late.
> >>>>
> >>>> Yes, these two patches have the same purpose to guard the system, while in two
> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
> >>>> driver.
> >>>>
> >>>> The one in powernv platform is used to correct the original logic.
> >>>>
> >>>> The one in generic iommu driver is to keep system safe in case other platform to
> >>>> call iommu_group_remove_device() without the check.
> >>>
> >>>
> >>> But I am moving bus notifier to powernv code (posted a patch last week,
> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> >> 
> >> Yes, I see the patch.
> >> 
> >> This means other platforms, besides powernv, will check the dev->iommu_group
> >> before remove the device? This would be a convention?
> >> 
> >> If this is the case, the second patch is enough. We don't need to check it in
> >> generic iommu driver.
> >> 
> >> Since I am not very familiar with the code convention, I post these two
> >> patches together. This doesn't mean I need to push both of them. Your comments
> >> are welcome, lets me understand which one is more suitable in this case.
> >
> >
> >Ok. So. I included the check in the bus notifier which I moved to powernv
> >platform, I guess I'll repost the series soon.
> 
> Thanks, this check will guard the powernv platform.
> 
> >
> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
> >
> 
> Alex,
> 
> Sorry for not including you in the very beginning, which may spend you more
> efforts to track previous mails in this thread.
> 
> Do you think it is reasonable to check the dev->iommu_group in
> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
> 
> Welcome your suggestions~

I don't really see the point of patch 1/2. iommu_group_remove_device()
is specifically to remove a device from an iommu_group, so why would you
call it on a device that's not part of an iommu_group.  If you want to
avoid testing dev->iommu_group, then implement the .remove_device
callback rather than using the notifier.  Thanks,

Alex
Wei Yang - Aug. 22, 2013, 3:41 p.m.
On Thu, Aug 22, 2013 at 09:28:23AM -0600, Alex Williamson wrote:
>On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
>> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
>> >On 08/19/2013 11:55 AM, Wei Yang wrote:
>> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>> >>>>>> ---
>> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> >>>>>> index b20ff17..5abf7c3 100644
>> >>>>>> --- a/arch/powerpc/kernel/iommu.c
>> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
>> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>> >>>>>>  		return iommu_add_device(dev);
>> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>> >>>>>> -		iommu_del_device(dev);
>> >>>>>> +		if (dev->iommu_group)
>> >>>>>> +			iommu_del_device(dev);
>> >>>>>>  		return 0;
>> >>>>>>  	default:
>> >>>>>>  		return 0;
>> >>>>>>
>> >>>>>
>> >>>>> This one seems redundant, no?
>> >>>>
>> >>>> Sorry for the late.
>> >>>>
>> >>>> Yes, these two patches have the same purpose to guard the system, while in two
>> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
>> >>>> driver.
>> >>>>
>> >>>> The one in powernv platform is used to correct the original logic.
>> >>>>
>> >>>> The one in generic iommu driver is to keep system safe in case other platform to
>> >>>> call iommu_group_remove_device() without the check.
>> >>>
>> >>>
>> >>> But I am moving bus notifier to powernv code (posted a patch last week,
>> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
>> >> 
>> >> Yes, I see the patch.
>> >> 
>> >> This means other platforms, besides powernv, will check the dev->iommu_group
>> >> before remove the device? This would be a convention?
>> >> 
>> >> If this is the case, the second patch is enough. We don't need to check it in
>> >> generic iommu driver.
>> >> 
>> >> Since I am not very familiar with the code convention, I post these two
>> >> patches together. This doesn't mean I need to push both of them. Your comments
>> >> are welcome, lets me understand which one is more suitable in this case.
>> >
>> >
>> >Ok. So. I included the check in the bus notifier which I moved to powernv
>> >platform, I guess I'll repost the series soon.
>> 
>> Thanks, this check will guard the powernv platform.
>> 
>> >
>> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
>> >
>> 
>> Alex,
>> 
>> Sorry for not including you in the very beginning, which may spend you more
>> efforts to track previous mails in this thread.
>> 
>> Do you think it is reasonable to check the dev->iommu_group in
>> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
>> 
>> Welcome your suggestions~
>
>I don't really see the point of patch 1/2. iommu_group_remove_device()
>is specifically to remove a device from an iommu_group, so why would you
>call it on a device that's not part of an iommu_group.  If you want to
>avoid testing dev->iommu_group, then implement the .remove_device
>callback rather than using the notifier.  Thanks,
>

You mean the .remove_device like intel_iommu_remove_device()? 

Hmm... this function didn't check the dev->iommu_group and just call
iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
which will check dev->iommu_group before invoke .remove_device.

Let me explain the case to triger the problem a little. 

On some platform, like powernv, we implement another bus notifier when devices
are added or removed in the system. Like Alexey mentioned, he missed the check
for dev->iommu_group in the notifier before removing it from iommu_group. This
trigger the crash.

So do you think it is reasonable to guard the kernel in
iommu_group_remove_device(), or we give the platform developers the
responsibility to check the dev->iommu_group before calling it?

Thanks~

>Alex
Alex Williamson - Aug. 22, 2013, 4:17 p.m.
On Thu, 2013-08-22 at 23:41 +0800, Wei Yang wrote:
> On Thu, Aug 22, 2013 at 09:28:23AM -0600, Alex Williamson wrote:
> >On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
> >> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
> >> >On 08/19/2013 11:55 AM, Wei Yang wrote:
> >> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
> >> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
> >> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
> >> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
> >> >>>>>> ---
> >> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
> >> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
> >> >>>>>>
> >> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >> >>>>>> index b20ff17..5abf7c3 100644
> >> >>>>>> --- a/arch/powerpc/kernel/iommu.c
> >> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
> >> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> >> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
> >> >>>>>>  		return iommu_add_device(dev);
> >> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
> >> >>>>>> -		iommu_del_device(dev);
> >> >>>>>> +		if (dev->iommu_group)
> >> >>>>>> +			iommu_del_device(dev);
> >> >>>>>>  		return 0;
> >> >>>>>>  	default:
> >> >>>>>>  		return 0;
> >> >>>>>>
> >> >>>>>
> >> >>>>> This one seems redundant, no?
> >> >>>>
> >> >>>> Sorry for the late.
> >> >>>>
> >> >>>> Yes, these two patches have the same purpose to guard the system, while in two
> >> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
> >> >>>> driver.
> >> >>>>
> >> >>>> The one in powernv platform is used to correct the original logic.
> >> >>>>
> >> >>>> The one in generic iommu driver is to keep system safe in case other platform to
> >> >>>> call iommu_group_remove_device() without the check.
> >> >>>
> >> >>>
> >> >>> But I am moving bus notifier to powernv code (posted a patch last week,
> >> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> >> >> 
> >> >> Yes, I see the patch.
> >> >> 
> >> >> This means other platforms, besides powernv, will check the dev->iommu_group
> >> >> before remove the device? This would be a convention?
> >> >> 
> >> >> If this is the case, the second patch is enough. We don't need to check it in
> >> >> generic iommu driver.
> >> >> 
> >> >> Since I am not very familiar with the code convention, I post these two
> >> >> patches together. This doesn't mean I need to push both of them. Your comments
> >> >> are welcome, lets me understand which one is more suitable in this case.
> >> >
> >> >
> >> >Ok. So. I included the check in the bus notifier which I moved to powernv
> >> >platform, I guess I'll repost the series soon.
> >> 
> >> Thanks, this check will guard the powernv platform.
> >> 
> >> >
> >> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
> >> >
> >> 
> >> Alex,
> >> 
> >> Sorry for not including you in the very beginning, which may spend you more
> >> efforts to track previous mails in this thread.
> >> 
> >> Do you think it is reasonable to check the dev->iommu_group in
> >> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
> >> 
> >> Welcome your suggestions~
> >
> >I don't really see the point of patch 1/2. iommu_group_remove_device()
> >is specifically to remove a device from an iommu_group, so why would you
> >call it on a device that's not part of an iommu_group.  If you want to
> >avoid testing dev->iommu_group, then implement the .remove_device
> >callback rather than using the notifier.  Thanks,
> >
> 
> You mean the .remove_device like intel_iommu_remove_device()? 
> 
> Hmm... this function didn't check the dev->iommu_group and just call
> iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
> which will check dev->iommu_group before invoke .remove_device.
> 
> Let me explain the case to triger the problem a little. 
> 
> On some platform, like powernv, we implement another bus notifier when devices
> are added or removed in the system. Like Alexey mentioned, he missed the check
> for dev->iommu_group in the notifier before removing it from iommu_group. This
> trigger the crash.
> 
> So do you think it is reasonable to guard the kernel in
> iommu_group_remove_device(), or we give the platform developers the
> responsibility to check the dev->iommu_group before calling it?

I don't see it as we need either patch 1/2 or patch 2/2.  We absolutely
need some form of patch 2/2.  Patch 1/2 isn't necessarily bad, but it
facilitates sloppy usage.  The iommu driver shouldn't be calling
iommu_group_remove_device() on arbitrary devices that may or may not be
part of an iommu_group.  Perhaps patch 1/2 should be:

if (WARN_ON(!group))
	return;

Thanks,

Alex
Wei Yang - Aug. 23, 2013, 1:30 a.m.
On Thu, Aug 22, 2013 at 10:17:20AM -0600, Alex Williamson wrote:
>On Thu, 2013-08-22 at 23:41 +0800, Wei Yang wrote:
>> >> 
>> >> Alex,
>> >> 
>> >> Sorry for not including you in the very beginning, which may spend you more
>> >> efforts to track previous mails in this thread.
>> >> 
>> >> Do you think it is reasonable to check the dev->iommu_group in
>> >> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
>> >> 
>> >> Welcome your suggestions~
>> >
>> >I don't really see the point of patch 1/2. iommu_group_remove_device()
>> >is specifically to remove a device from an iommu_group, so why would you
>> >call it on a device that's not part of an iommu_group.  If you want to
>> >avoid testing dev->iommu_group, then implement the .remove_device
>> >callback rather than using the notifier.  Thanks,
>> >
>> 
>> You mean the .remove_device like intel_iommu_remove_device()? 
>> 
>> Hmm... this function didn't check the dev->iommu_group and just call
>> iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
>> which will check dev->iommu_group before invoke .remove_device.
>> 
>> Let me explain the case to triger the problem a little. 
>> 
>> On some platform, like powernv, we implement another bus notifier when devices
>> are added or removed in the system. Like Alexey mentioned, he missed the check
>> for dev->iommu_group in the notifier before removing it from iommu_group. This
>> trigger the crash.
>> 
>> So do you think it is reasonable to guard the kernel in
>> iommu_group_remove_device(), or we give the platform developers the
>> responsibility to check the dev->iommu_group before calling it?
>
>I don't see it as we need either patch 1/2 or patch 2/2.  We absolutely
>need some form of patch 2/2.  Patch 1/2 isn't necessarily bad, but it
>facilitates sloppy usage.  The iommu driver shouldn't be calling
>iommu_group_remove_device() on arbitrary devices that may or may not be
>part of an iommu_group.  Perhaps patch 1/2 should be:
>
>if (WARN_ON(!group))
>	return;
>

Agree, this one sounds more reasonable. :-)

Since patch 2/2 is merged by Alexey, I will re-send patch 1/2 alone.

Thanks for your comments ~

>Thanks,
>
>Alex
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev

Patch

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index b20ff17..5abf7c3 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -1149,7 +1149,8 @@  static int iommu_bus_notifier(struct notifier_block *nb,
 	case BUS_NOTIFY_ADD_DEVICE:
 		return iommu_add_device(dev);
 	case BUS_NOTIFY_DEL_DEVICE:
-		iommu_del_device(dev);
+		if (dev->iommu_group)
+			iommu_del_device(dev);
 		return 0;
 	default:
 		return 0;