> -----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 ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
