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

[Wang, Dongsheng]

> -----Original Message-----
> From: linux-acpi-owner@xxxxxxxxxxxxxxx
> [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Robin Murphy
> Sent: Monday, April 09, 2018 8:11 PM
> To: Wang, Dongsheng <dongsheng.wang@xxxxxxxxxxxxxxxx>; Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx>
> 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 08/04/18 09:10, Wang, Dongsheng wrote:
> >
> >> -----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 don't think it's particularly justifiable to make core API changes for
> the sake of contrived testcases. On real systems, the SMMU is a
> fundamental system component which is no more expected to fail probe
> than, say, the GIC, and as such if it *does* fail then further progress
> is on a best-effort basis at most.
Yes. Agree with you.


> Just because *your* system happens to work fine in this state doesn't make it true for every SMMU
> implementation and integration that Linux may ever run on.
:(, yes, this is my mistake.


> 			
> If you want the kernel to ignore an SMMU, either configure out the
> driver or don't describe that SMMU in firmware in the first place.
> 
> >>> 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.
> 
> Handle it by not deliberately breaking the SMMU driver. In all other
> cases, either re-triggering SMMU probe might make it succeed (i.e. the
> DL_DEV_PROBE_FAILED state is meaningless), or things are so broken that
> you're probably dead in the water anyway.
> 
Drop this patch.

Thanks for your review.

Cheers,
-Dongsheng

> Robin.
> 
> >
> > 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
> >>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html