Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Will Deacon]

On Fri, Oct 21, 2016 at 05:20:43PM +0100, Robin Murphy wrote:
> On 19/10/16 13:49, Will Deacon wrote:
> > On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
> >> We now delay installing our per-bus iommu_ops until we know an SMMU has
> >> successfully probed, as they don't serve much purpose beforehand, and
> >> doing so also avoids fights between multiple IOMMU drivers in a single
> >> kernel. However, the upshot of passing the return value of bus_set_iommu()
> >> back from our probe function is that if there happens to be more than
> >> one SMMUv3 device in a system, the second and subsequent probes will
> >> wind up returning -EBUSY to the driver core and getting torn down again.
> >>
> >> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
> >> 1. The bus already has iommu_ops installed
> >> 2. One of the add_device callbacks from the initial notifier failed
> >> 3. Allocating or installing the notifier itself failed
> >>
> >> The first two are down to devices other than the SMMU in question, so
> >> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
> >> indicative of the kind of catastrophic system failure which isn't going
> >> to get much further anyway. Consequently, there is little harm in
> >> ignoring the return value either way.
> >>
> >> CC: Lorenzo Pieralisi 
> >> Signed-off-by: Robin Murphy 
> >> ---
> >>  drivers/iommu/arm-smmu-v3.c | 11 ---
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> index 15c01c3cd540..74fbef384deb 100644
> >> --- a/drivers/iommu/arm-smmu-v3.c
> >> +++ b/drivers/iommu/arm-smmu-v3.c
> >> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
> >> platform_device *pdev)
> >>of_iommu_set_ops(dev->of_node, _smmu_ops);
> >>  #ifdef CONFIG_PCI
> >>pci_request_acs();
> >> -  ret = bus_set_iommu(_bus_type, _smmu_ops);
> >> -  if (ret)
> >> -  return ret;
> >> +  bus_set_iommu(_bus_type, _smmu_ops);
> >>  #endif
> >>  #ifdef CONFIG_ARM_AMBA
> >> -  ret = bus_set_iommu(_bustype, _smmu_ops);
> >> -  if (ret)
> >> -  return ret;
> >> +  bus_set_iommu(_bustype, _smmu_ops);
> >>  #endif
> >> -  return bus_set_iommu(_bus_type, _smmu_ops);
> >> +  bus_set_iommu(_bus_type, _smmu_ops);
> >> +  return 0;
> > 
> > In which case, we should probably add an iommu_present check, like we
> > have for the v2 driver.
> 
> As touched upon in the commit message, the first thing bus_set_iommu()
> does is perform that same check and return immediately if any ops are
> already set. Do you really want redundant checks added, or shall I spin
> that cleanup patch removing them from v2 that I proposed to Lorenzo?

A cleanup patch sounds good, to keep the two drivers consistent.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Hanjun Guo]

On 2016/10/17 19:06, Robin Murphy wrote:

We now delay installing our per-bus iommu_ops until we know an SMMU has
successfully probed, as they don't serve much purpose beforehand, and
doing so also avoids fights between multiple IOMMU drivers in a single
kernel. However, the upshot of passing the return value of bus_set_iommu()
back from our probe function is that if there happens to be more than
one SMMUv3 device in a system, the second and subsequent probes will
wind up returning -EBUSY to the driver core and getting torn down again.

There are essentially 3 cases in which bus_set_iommu() returns nonzero:
1. The bus already has iommu_ops installed
2. One of the add_device callbacks from the initial notifier failed
3. Allocating or installing the notifier itself failed

The first two are down to devices other than the SMMU in question, so
shouldn't abort an otherwise-successful SMMU probe, whilst the third is
indicative of the kind of catastrophic system failure which isn't going
to get much further anyway. Consequently, there is little harm in
ignoring the return value either way.

CC: Lorenzo Pieralisi 
Signed-off-by: Robin Murphy 


Tested-by: Hanjun Guo 

Thanks
Hanjun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Robin Murphy]

On 19/10/16 13:49, Will Deacon wrote:
> On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
>> We now delay installing our per-bus iommu_ops until we know an SMMU has
>> successfully probed, as they don't serve much purpose beforehand, and
>> doing so also avoids fights between multiple IOMMU drivers in a single
>> kernel. However, the upshot of passing the return value of bus_set_iommu()
>> back from our probe function is that if there happens to be more than
>> one SMMUv3 device in a system, the second and subsequent probes will
>> wind up returning -EBUSY to the driver core and getting torn down again.
>>
>> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
>> 1. The bus already has iommu_ops installed
>> 2. One of the add_device callbacks from the initial notifier failed
>> 3. Allocating or installing the notifier itself failed
>>
>> The first two are down to devices other than the SMMU in question, so
>> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
>> indicative of the kind of catastrophic system failure which isn't going
>> to get much further anyway. Consequently, there is little harm in
>> ignoring the return value either way.
>>
>> CC: Lorenzo Pieralisi 
>> Signed-off-by: Robin Murphy 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 11 ---
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 15c01c3cd540..74fbef384deb 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
>> platform_device *pdev)
>>  of_iommu_set_ops(dev->of_node, _smmu_ops);
>>  #ifdef CONFIG_PCI
>>  pci_request_acs();
>> -ret = bus_set_iommu(_bus_type, _smmu_ops);
>> -if (ret)
>> -return ret;
>> +bus_set_iommu(_bus_type, _smmu_ops);
>>  #endif
>>  #ifdef CONFIG_ARM_AMBA
>> -ret = bus_set_iommu(_bustype, _smmu_ops);
>> -if (ret)
>> -return ret;
>> +bus_set_iommu(_bustype, _smmu_ops);
>>  #endif
>> -return bus_set_iommu(_bus_type, _smmu_ops);
>> +bus_set_iommu(_bus_type, _smmu_ops);
>> +return 0;
> 
> In which case, we should probably add an iommu_present check, like we
> have for the v2 driver.

As touched upon in the commit message, the first thing bus_set_iommu()
does is perform that same check and return immediately if any ops are
already set. Do you really want redundant checks added, or shall I spin
that cleanup patch removing them from v2 that I proposed to Lorenzo?

Robin.

> 
> Will
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Will Deacon]

On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
> We now delay installing our per-bus iommu_ops until we know an SMMU has
> successfully probed, as they don't serve much purpose beforehand, and
> doing so also avoids fights between multiple IOMMU drivers in a single
> kernel. However, the upshot of passing the return value of bus_set_iommu()
> back from our probe function is that if there happens to be more than
> one SMMUv3 device in a system, the second and subsequent probes will
> wind up returning -EBUSY to the driver core and getting torn down again.
> 
> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
> 1. The bus already has iommu_ops installed
> 2. One of the add_device callbacks from the initial notifier failed
> 3. Allocating or installing the notifier itself failed
> 
> The first two are down to devices other than the SMMU in question, so
> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
> indicative of the kind of catastrophic system failure which isn't going
> to get much further anyway. Consequently, there is little harm in
> ignoring the return value either way.
> 
> CC: Lorenzo Pieralisi 
> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/arm-smmu-v3.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 15c01c3cd540..74fbef384deb 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
> platform_device *pdev)
>   of_iommu_set_ops(dev->of_node, _smmu_ops);
>  #ifdef CONFIG_PCI
>   pci_request_acs();
> - ret = bus_set_iommu(_bus_type, _smmu_ops);
> - if (ret)
> - return ret;
> + bus_set_iommu(_bus_type, _smmu_ops);
>  #endif
>  #ifdef CONFIG_ARM_AMBA
> - ret = bus_set_iommu(_bustype, _smmu_ops);
> - if (ret)
> - return ret;
> + bus_set_iommu(_bustype, _smmu_ops);
>  #endif
> - return bus_set_iommu(_bus_type, _smmu_ops);
> + bus_set_iommu(_bus_type, _smmu_ops);
> + return 0;

In which case, we should probably add an iommu_present check, like we
have for the v2 driver.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Lorenzo Pieralisi]

On Mon, Oct 17, 2016 at 03:19:46PM +0100, Robin Murphy wrote:
> Hi Lorenzo,
> 
> On 17/10/16 14:21, Lorenzo Pieralisi wrote:
> > On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
> >> We now delay installing our per-bus iommu_ops until we know an SMMU has
> >> successfully probed, as they don't serve much purpose beforehand, and
> >> doing so also avoids fights between multiple IOMMU drivers in a single
> >> kernel. However, the upshot of passing the return value of bus_set_iommu()
> >> back from our probe function is that if there happens to be more than
> >> one SMMUv3 device in a system, the second and subsequent probes will
> >> wind up returning -EBUSY to the driver core and getting torn down again.
> >>
> >> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
> >> 1. The bus already has iommu_ops installed
> >> 2. One of the add_device callbacks from the initial notifier failed
> >> 3. Allocating or installing the notifier itself failed
> >>
> >> The first two are down to devices other than the SMMU in question, so
> >> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
> >> indicative of the kind of catastrophic system failure which isn't going
> >> to get much further anyway. Consequently, there is little harm in
> >> ignoring the return value either way.
> >>
> >> CC: Lorenzo Pieralisi 
> >> Signed-off-by: Robin Murphy 
> >> ---
> >>  drivers/iommu/arm-smmu-v3.c | 11 ---
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> index 15c01c3cd540..74fbef384deb 100644
> >> --- a/drivers/iommu/arm-smmu-v3.c
> >> +++ b/drivers/iommu/arm-smmu-v3.c
> >> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
> >> platform_device *pdev)
> >>of_iommu_set_ops(dev->of_node, _smmu_ops);
> >>  #ifdef CONFIG_PCI
> >>pci_request_acs();
> >> -  ret = bus_set_iommu(_bus_type, _smmu_ops);
> >> -  if (ret)
> >> -  return ret;
> >> +  bus_set_iommu(_bus_type, _smmu_ops);
> >>  #endif
> >>  #ifdef CONFIG_ARM_AMBA
> >> -  ret = bus_set_iommu(_bustype, _smmu_ops);
> >> -  if (ret)
> >> -  return ret;
> >> +  bus_set_iommu(_bustype, _smmu_ops);
> >>  #endif
> >> -  return bus_set_iommu(_bus_type, _smmu_ops);
> >> +  bus_set_iommu(_bus_type, _smmu_ops);
> >> +  return 0;
> > 
> > Nit: I do not see why you would not take the same approach as
> > the ARM SMMUv1/v2, namely checking if ops are already set and
> > skip the call if that's the case.
> 
> Well, I'd say it really goes the other way around - since the very first
> thing bus_set_iommu() does is check if ops are present, and return if
> so, and the v2 driver already doesn't care about that return value,
> there's not really any need for it to duplicate the check either. I
> didn't change it at the time to avoid cluttering the gigantic rework any
> further, but I could spin a cleanup patch if you like.

No worries, it was to understand if there was a reason to keep
the code different and after another look I agree with what
you are saying (by checking if ops are present you could eg
avoid calling pci_request_acs() every probe but that's a detail).

Thanks for fixing it !
Lorenzo

> > Anyway:
> > 
> > Acked-by: Lorenzo Pieralisi 
> 
> Thanks!
> 
> Robin.
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Robin Murphy]

Hi Lorenzo,

On 17/10/16 14:21, Lorenzo Pieralisi wrote:
> On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
>> We now delay installing our per-bus iommu_ops until we know an SMMU has
>> successfully probed, as they don't serve much purpose beforehand, and
>> doing so also avoids fights between multiple IOMMU drivers in a single
>> kernel. However, the upshot of passing the return value of bus_set_iommu()
>> back from our probe function is that if there happens to be more than
>> one SMMUv3 device in a system, the second and subsequent probes will
>> wind up returning -EBUSY to the driver core and getting torn down again.
>>
>> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
>> 1. The bus already has iommu_ops installed
>> 2. One of the add_device callbacks from the initial notifier failed
>> 3. Allocating or installing the notifier itself failed
>>
>> The first two are down to devices other than the SMMU in question, so
>> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
>> indicative of the kind of catastrophic system failure which isn't going
>> to get much further anyway. Consequently, there is little harm in
>> ignoring the return value either way.
>>
>> CC: Lorenzo Pieralisi 
>> Signed-off-by: Robin Murphy 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 11 ---
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 15c01c3cd540..74fbef384deb 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
>> platform_device *pdev)
>>  of_iommu_set_ops(dev->of_node, _smmu_ops);
>>  #ifdef CONFIG_PCI
>>  pci_request_acs();
>> -ret = bus_set_iommu(_bus_type, _smmu_ops);
>> -if (ret)
>> -return ret;
>> +bus_set_iommu(_bus_type, _smmu_ops);
>>  #endif
>>  #ifdef CONFIG_ARM_AMBA
>> -ret = bus_set_iommu(_bustype, _smmu_ops);
>> -if (ret)
>> -return ret;
>> +bus_set_iommu(_bustype, _smmu_ops);
>>  #endif
>> -return bus_set_iommu(_bus_type, _smmu_ops);
>> +bus_set_iommu(_bus_type, _smmu_ops);
>> +return 0;
> 
> Nit: I do not see why you would not take the same approach as
> the ARM SMMUv1/v2, namely checking if ops are already set and
> skip the call if that's the case.

Well, I'd say it really goes the other way around - since the very first
thing bus_set_iommu() does is check if ops are present, and return if
so, and the v2 driver already doesn't care about that return value,
there's not really any need for it to duplicate the check either. I
didn't change it at the time to avoid cluttering the gigantic rework any
further, but I could spin a cleanup patch if you like.

> Anyway:
> 
> Acked-by: Lorenzo Pieralisi 

Thanks!

Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Sricharan]

Hi,

>-Original Message-
>From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] 
>On Behalf Of Robin Murphy
>Sent: Monday, October 17, 2016 4:36 PM
>To: will.dea...@arm.com; j...@8bytes.org
>Cc: iommu@lists.linux-foundation.org; Lorenzo Pieralisi 
>; linux-arm-ker...@lists.infradead.org
>Subject: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple 
>SMMUv3s
>
>We now delay installing our per-bus iommu_ops until we know an SMMU has
>successfully probed, as they don't serve much purpose beforehand, and
>doing so also avoids fights between multiple IOMMU drivers in a single
>kernel. However, the upshot of passing the return value of bus_set_iommu()
>back from our probe function is that if there happens to be more than
>one SMMUv3 device in a system, the second and subsequent probes will
>wind up returning -EBUSY to the driver core and getting torn down again.
>
>There are essentially 3 cases in which bus_set_iommu() returns nonzero:
>1. The bus already has iommu_ops installed
>2. One of the add_device callbacks from the initial notifier failed
>3. Allocating or installing the notifier itself failed
>
>The first two are down to devices other than the SMMU in question, so
>shouldn't abort an otherwise-successful SMMU probe, whilst the third is
>indicative of the kind of catastrophic system failure which isn't going
>to get much further anyway. Consequently, there is little harm in
>ignoring the return value either way.
>
>CC: Lorenzo Pieralisi 
>Signed-off-by: Robin Murphy 
>---
> drivers/iommu/arm-smmu-v3.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>index 15c01c3cd540..74fbef384deb 100644
>--- a/drivers/iommu/arm-smmu-v3.c
>+++ b/drivers/iommu/arm-smmu-v3.c
>@@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
>platform_device *pdev)
>   of_iommu_set_ops(dev->of_node, _smmu_ops);
> #ifdef CONFIG_PCI
>   pci_request_acs();
>-  ret = bus_set_iommu(_bus_type, _smmu_ops);
>-  if (ret)
>-  return ret;
>+  bus_set_iommu(_bus_type, _smmu_ops);
> #endif
> #ifdef CONFIG_ARM_AMBA
>-  ret = bus_set_iommu(_bustype, _smmu_ops);
>-  if (ret)
>-  return ret;
>+  bus_set_iommu(_bustype, _smmu_ops);
> #endif
>-  return bus_set_iommu(_bus_type, _smmu_ops);
>+  bus_set_iommu(_bus_type, _smmu_ops);
>+  return 0;
reviewed-by: sricha...@codeaurora.org

Regards,
 Sricharan
  

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/arm-smmu: Don't inadvertently reject multiple SMMUv3s

[Lorenzo Pieralisi]

On Mon, Oct 17, 2016 at 12:06:20PM +0100, Robin Murphy wrote:
> We now delay installing our per-bus iommu_ops until we know an SMMU has
> successfully probed, as they don't serve much purpose beforehand, and
> doing so also avoids fights between multiple IOMMU drivers in a single
> kernel. However, the upshot of passing the return value of bus_set_iommu()
> back from our probe function is that if there happens to be more than
> one SMMUv3 device in a system, the second and subsequent probes will
> wind up returning -EBUSY to the driver core and getting torn down again.
> 
> There are essentially 3 cases in which bus_set_iommu() returns nonzero:
> 1. The bus already has iommu_ops installed
> 2. One of the add_device callbacks from the initial notifier failed
> 3. Allocating or installing the notifier itself failed
> 
> The first two are down to devices other than the SMMU in question, so
> shouldn't abort an otherwise-successful SMMU probe, whilst the third is
> indicative of the kind of catastrophic system failure which isn't going
> to get much further anyway. Consequently, there is little harm in
> ignoring the return value either way.
> 
> CC: Lorenzo Pieralisi 
> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/arm-smmu-v3.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 15c01c3cd540..74fbef384deb 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2637,16 +2637,13 @@ static int arm_smmu_device_dt_probe(struct 
> platform_device *pdev)
>   of_iommu_set_ops(dev->of_node, _smmu_ops);
>  #ifdef CONFIG_PCI
>   pci_request_acs();
> - ret = bus_set_iommu(_bus_type, _smmu_ops);
> - if (ret)
> - return ret;
> + bus_set_iommu(_bus_type, _smmu_ops);
>  #endif
>  #ifdef CONFIG_ARM_AMBA
> - ret = bus_set_iommu(_bustype, _smmu_ops);
> - if (ret)
> - return ret;
> + bus_set_iommu(_bustype, _smmu_ops);
>  #endif
> - return bus_set_iommu(_bus_type, _smmu_ops);
> + bus_set_iommu(_bus_type, _smmu_ops);
> + return 0;

Nit: I do not see why you would not take the same approach as
the ARM SMMUv1/v2, namely checking if ops are already set and
skip the call if that's the case.

Anyway:

Acked-by: Lorenzo Pieralisi 

>  }
>  
>  static int arm_smmu_device_remove(struct platform_device *pdev)
> -- 
> 1.9.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu