> -----Original Message-----
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: Thursday, April 05, 2018 2:57 AM
> To: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>; Wang, Dongsheng
> <dongsheng.w...@hxt-semitech.com>
> Cc: r...@rjwysocki.net; gre...@linuxfoundation.org; hanjun....@linaro.org;
> sudeep.ho...@arm.com; Zheng, Joey <yu.zh...@hxt-semitech.com>;
> linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org
> 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.w...@hxt-semitech.com>
> >> ---
> >> 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
> >>
