Patchwork [3/4] iommu/of: Don't call iommu_ops->add_device directly

login
register
mail settings
Submitter Joerg Roedel
Date Dec. 5, 2018, 2:36 p.m.
Message ID <20181205143646.4876-4-joro@8bytes.org>
Download mbox | patch
Permalink /patch/673169/
State New
Headers show

Comments

Joerg Roedel - Dec. 5, 2018, 2:36 p.m.
From: Joerg Roedel <jroedel@suse.de>

Make sure to invoke this call-back through the proper
function of the IOMMU-API.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Robin Murphy - Dec. 5, 2018, 6:49 p.m.
On 05/12/2018 14:36, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/of_iommu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..4d4847de727e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>   		ops = dev->iommu_fwspec->ops;
>   	/*
>   	 * If we have reason to believe the IOMMU driver missed the initial
> -	 * add_device callback for dev, replay it to get things in order.
> +	 * probe for dev, replay it to get things in order.
>   	 */
> -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)

Ugh, code I wrote...

I think that first ops test is serving double-duty - it's not just an 
"is ops->add_device safe to call?" check, but also the specific "do we 
need to try doing this at all?" check, since ops is only (potentially) 
set in the !err case at the top of this context.

> -		err = ops->add_device(dev);
> +	if (dev->bus && !dev->iommu_group)

Thus to avoid calling add_device erroneously in either of the "no valid 
IOMMU" cases (since dev->bus->iommu_ops may well be non-NULL), this 
still needs to be at least:

	if (ops && dev->bus && !dev->iommu_group)

What I can't quite remember just now is whether it's actually valid to 
get here with err == 0 but dev->iommu_fwspec->ops == NULL, so it *might* 
be OK to use "!err" instead of "ops" to make things slightly more 
obvious - I'll work through the flow tomorrow to double-check.

Robin.

> +		err = iommu_probe_device(dev);
>   
>   	/* Ignore all other errors apart from EPROBE_DEFER */
>   	if (err == -EPROBE_DEFER) {
>
Joerg Roedel - Dec. 6, 2018, 4:19 p.m.
On Wed, Dec 05, 2018 at 06:49:26PM +0000, Robin Murphy wrote:
> 	if (ops && dev->bus && !dev->iommu_group)
> 
> What I can't quite remember just now is whether it's actually valid to get
> here with err == 0 but dev->iommu_fwspec->ops == NULL, so it *might* be OK
> to use "!err" instead of "ops" to make things slightly more obvious - I'll
> work through the flow tomorrow to double-check.

Yeah, adding an err == 0 check seems to be the best option here, so that
iommu-drivers don't get confused when we try to add a device that is not
managed by an iommu.


Regards,

	Joerg

Patch

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..4d4847de727e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -218,10 +218,10 @@  const struct iommu_ops *of_iommu_configure(struct device *dev,
 		ops = dev->iommu_fwspec->ops;
 	/*
 	 * If we have reason to believe the IOMMU driver missed the initial
-	 * add_device callback for dev, replay it to get things in order.
+	 * probe for dev, replay it to get things in order.
 	 */
-	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
-		err = ops->add_device(dev);
+	if (dev->bus && !dev->iommu_group)
+		err = iommu_probe_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {