RE: Re: [RFC PATCH 2/2] ACPI/IORT: use swiotlb_dma_ops when smmu probe failed

[Wang, Dongsheng]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> Sent: Thursday, April 05, 2018 2:57 AM
> To: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>; Wang, Dongsheng
> <dongsheng.wang@xxxxxxxxxxxxxxxx>
> Cc: rjw@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; hanjun.guo@xxxxxxxxxx;
> sudeep.holla@xxxxxxx; Zheng, Joey <yu.zheng@xxxxxxxxxxxxxxxx>;
> linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [æéäåèååéé] Re: [RFC PATCH 2/2] ACPI/IORT: use
> swiotlb_dma_ops when smmu probe failed
> 
> On 04/04/18 17:01, Lorenzo Pieralisi wrote:
> > [+cc Robin]
> >
> > On Thu, Mar 29, 2018 at 03:01:00AM -0700, Wang Dongsheng wrote:
> >> If SMMU probe failed, master should use swiotlb as dma ops.
> >> SMMU may probe failed with specified environment, so there
> >> are not any iommu resources in iommu_device_list.
> >>
> >> The master will always get EPROBE_DEFER from really_probe
> >> (dma_configure) but in fact SMMU has probe failed. The issue
> >> causes all of masters failed to be driven.
> 
> Let's just take a step back - why is SMMU probe failing? That seems to
> be the primary issue here, because it implies that either your hardware,
> firmware or kernel is broken, any of which would make boot failure
> somewhat unsurprising anyway.
> 
It's actually not a hardware issue. This is my test case, just return
-EINVAL in arm_smmu_device_probe. The HW probe(arm_smmu_device_hw_probe)
is just part of SMMU driver probe and the failure may be caused by SW. So
I design this case, just make sure even if SMMU probe failed that cause by SW,
the MASTER also can work. _Because of our SMMU default mode is bypass._


> > I added Robin to pick his brain. An alternative would consist
> > in using a bus notifier to prevent deferred probing once the SMMU
> > driver probing failed but that seems backwards given that a major
> > reason to move to deferred probing was to remove the bus notifiers
> > dependency in the first place.
> >
> > It seems to me this is both an OF/ACPI issue - it is not an IORT
> > only problem.
> 
> Yes, this is just an instance of the general probe-deferral problem,
> e.g. once you have multiple dependencies it's possible to end up in a
> stalemate where everything including the IOMMU ends up on the deferred
> probe list with nothing to kick it and make progress again.
> 
> Furthermore it seems to me that the whole premise in this patch is
> flawed,
Ditto. :)


> since even genuine probe failure may well be transient - just
> because one attempt failed doesn't mean a later attempt can't succeed.
> Thus "the most recent probe attempt failed" cannot be considered a
> fundamentally different state from "no driver is currently bound".
> 
Agree, the genuine probe failure may well be transient. But there is
depend on SMMU probe(IOMMU instance) status. There are two situations:

1. MASTER probing, SMMU doesn't probe yet.
	This case will match "the transient failure".
	really_probe get an EPROBE_DEFER from IORT and the MASTER probe will be
	delayed until SMMU probe successful.
2. MASTER probing, SMMU probe has failed.
	really_probe will always get an EPROBE_DEFER from IORT, because kernel
	has build in SMMU driver.(iort_iommu_driver_enabled) And the master
	never cannot do probe.

The case 2 is I want to handle.

Cheers,
-Dongsheng

> Robin.
> 
> >
> > Lorenzo
> >
> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@xxxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/acpi/arm64/iort.c | 39
> +++++++++++++++++++++++++++++++++------
> >>   1 file changed, 33 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> index e2f7bdd..a6f4c27 100644
> >> --- a/drivers/acpi/arm64/iort.c
> >> +++ b/drivers/acpi/arm64/iort.c
> >> @@ -774,17 +774,45 @@ static int arm_smmu_iort_xlate(struct device
> *dev, u32 streamid,
> >>   	return ret;
> >>   }
> >>
> >> -static inline bool iort_iommu_driver_enabled(u8 type)
> >> +static int iort_check_dev_dl_status(struct device *dev, void *data)
> >>   {
> >> +	struct fwnode_handle *fwnode = data;
> >> +
> >> +	if (dev->fwnode != fwnode)
> >> +		return 0;
> >> +
> >> +	if (dev->links.status == DL_DEV_PROBE_FAILED)
> >> +		return -ENODEV;
> >> +
> >> +	return -EPROBE_DEFER;
> >> +}
> >> +
> >> +static int iort_iommu_driver_enabled(u8 type, struct fwnode_handle
> *fwnode)
> >> +{
> >> +	bool buildin;
> >> +	int ret;
> >> +
> >>   	switch (type) {
> >>   	case ACPI_IORT_NODE_SMMU_V3:
> >> -		return IS_BUILTIN(CONFIG_ARM_SMMU_V3);
> >> +		buildin = IS_BUILTIN(CONFIG_ARM_SMMU_V3);
> >> +		break;
> >>   	case ACPI_IORT_NODE_SMMU:
> >> -		return IS_BUILTIN(CONFIG_ARM_SMMU);
> >> +		buildin = IS_BUILTIN(CONFIG_ARM_SMMU);
> >> +		break;
> >>   	default:
> >>   		pr_warn("IORT node type %u does not describe an SMMU\n",
> type);
> >> -		return false;
> >> +		buildin = false;
> >>   	}
> >> +
> >> +	if (!buildin)
> >> +		return -ENODEV;
> >> +
> >> +	ret = bus_for_each_dev(&platform_bus_type, NULL, fwnode,
> >> +			       iort_check_dev_dl_status);
> >> +	if (!ret)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	return ret;
> >>   }
> >>
> >>   #ifdef CONFIG_IOMMU_API
> >> @@ -919,8 +947,7 @@ static int iort_iommu_xlate(struct device *dev,
> struct acpi_iort_node *node,
> >>   	 */
> >>   	ops = iommu_ops_from_fwnode(iort_fwnode);
> >>   	if (!ops)
> >> -		return iort_iommu_driver_enabled(node->type) ?
> >> -		       -EPROBE_DEFER : -ENODEV;
> >> +		return iort_iommu_driver_enabled(node->type, iort_fwnode);
> >>
> >>   	return arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
> >>   }
> >> --
> >> 2.7.4
> >>