[RFC PATCH 0/2] driver core: add new dl device status DL_DEV_PROBE_FAILED

[RFC PATCH 0/2] driver core: add new dl device status DL_DEV_PROBE_FAILED

[Wang Dongsheng]

Recently we found the master of SMMU retries to probe endlessly.

[    3.658956] pci 0000:00:00.0: Retrying from deferred list
[    3.658969] pci 0000:00:00.0: Added to deferred list
[    3.658987] pci 0004:00:00.0: Retrying from deferred list
[    3.658994] pci 0004:00:00.0: Added to deferred list
[    3.659003] pci 0005:00:00.0: Retrying from deferred list
[    3.659010] pci 0005:00:00.0: Added to deferred list
[    3.659019] pci 0004:01:00.0: Retrying from deferred list
[    3.659029] pci 0004:01:00.0: Added to deferred list

The retrying should only happen when the IOMMU instance hasn't been
probed yet.
However, dma_configure() simply return -EPROBE_DEFER when failed to get
an IOMMU instance even if the IOMMU instance is failed to probe.

This patchset tries to fix the issue by distinguishing probe failures and
haven't-been-probed-yet state.

Wang Dongsheng (2):
  driver core: add new dl device status DL_DEV_PROBE_FAILED
  ACPI/IORT: use swiotlb_dma_ops when smmu probe failed

 Documentation/driver-api/device_link.rst |  2 +-
 drivers/acpi/arm64/iort.c                | 39 +++++++++++++++++++++++++++-----
 drivers/base/base.h                      |  2 +-
 drivers/base/core.c                      | 22 ++++++++++++++++--
 drivers/base/dd.c                        |  2 +-
 include/linux/device.h                   |  1 +
 6 files changed, 57 insertions(+), 11 deletions(-)

-- 
2.7.4

[RFC PATCH 1/2] driver core: add new dl device status DL_DEV_PROBE_FAILED

[Wang Dongsheng]

Currently the initialization state of device is DL_DEV_NO_DRIVER.
The problem is, after probe failure the state will also be set to
DL_DEV_NO_DRIVER as well. And the device is not linked, it has no
supplier or consumer. Thus adding a new state to distinguish
probe failure and not-probed-yet.

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 Documentation/driver-api/device_link.rst |  2 +-
 drivers/base/base.h                      |  2 +-
 drivers/base/core.c                      | 22 ++++++++++++++++++++--
 drivers/base/dd.c                        |  2 +-
 include/linux/device.h                   |  1 +
 5 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index 70e328e..9054403 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -247,7 +247,7 @@ State machine
   :c:func:`device_links_unbind_consumers()`.)
 
 * If the probe fails, links to suppliers revert back to ``DL_STATE_AVAILABLE``.
-  (Call to :c:func:`device_links_no_driver()` from :c:func:`really_probe()`.)
+  (Call to :c:func:`device_links_probe_failed()` from :c:func:`really_probe()`.)
 
 * If the probe succeeds, links to suppliers progress to ``DL_STATE_ACTIVE``.
   (Call to :c:func:`device_links_driver_bound()` from :c:func:`driver_bound()`.)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index d800de6..f9931d9 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -158,6 +158,6 @@ extern void device_links_read_unlock(int idx);
 extern int device_links_check_suppliers(struct device *dev);
 extern void device_links_driver_bound(struct device *dev);
 extern void device_links_driver_cleanup(struct device *dev);
-extern void device_links_no_driver(struct device *dev);
+extern void device_links_probe_failed(struct device *dev);
 extern bool device_links_busy(struct device *dev);
 extern void device_links_unbind_consumers(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5847364..31d4f68 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -452,10 +452,28 @@ static void __device_links_no_driver(struct device *dev)
 	dev->links.status = DL_DEV_NO_DRIVER;
 }
 
-void device_links_no_driver(struct device *dev)
+static void __device_links_probe_failed(struct device *dev)
+{
+	struct device_link *link, *ln;
+
+	list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers,
+					 c_node) {
+		if (link->flags & DL_FLAG_STATELESS)
+			continue;
+
+		if (link->flags & DL_FLAG_AUTOREMOVE)
+			__device_link_del(link);
+		else if (link->status != DL_STATE_SUPPLIER_UNBIND)
+			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
+	}
+
+	dev->links.status = DL_DEV_PROBE_FAILED;
+}
+
+void device_links_probe_failed(struct device *dev)
 {
 	device_links_write_lock();
-	__device_links_no_driver(dev);
+	__device_links_probe_failed(dev);
 	device_links_write_unlock();
 }
 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index de6fd09..90d57e0 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -492,7 +492,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
 pinctrl_bind_failed:
-	device_links_no_driver(dev);
+	device_links_probe_failed(dev);
 	devres_release_all(dev);
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
diff --git a/include/linux/device.h b/include/linux/device.h
index b093405..bf9630a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -794,6 +794,7 @@ struct device_link {
 enum dl_dev_state {
 	DL_DEV_NO_DRIVER = 0,
 	DL_DEV_PROBING,
+	DL_DEV_PROBE_FAILED,
 	DL_DEV_DRIVER_BOUND,
 	DL_DEV_UNBINDING,
 };
-- 
2.7.4

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

[Wang Dongsheng]

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.

Signed-off-by: Wang Dongsheng <dongsheng.wang@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

Re: [RFC PATCH 1/2] driver core: add new dl device status DL_DEV_PROBE_FAILED

[Rafael J. Wysocki]

On Thu, Mar 29, 2018 at 12:00 PM, Wang Dongsheng
<dongsheng.wang@hxt-semitech.com> wrote:
> Currently the initialization state of device is DL_DEV_NO_DRIVER.
> The problem is, after probe failure the state will also be set to
> DL_DEV_NO_DRIVER as well. And the device is not linked, it has no
> supplier or consumer. Thus adding a new state to distinguish
> probe failure and not-probed-yet.
>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>

I guess what you want is a cleanup after a failing probe, but after
that the state really is "no driver" again, isn't it?

> ---
>  Documentation/driver-api/device_link.rst |  2 +-
>  drivers/base/base.h                      |  2 +-
>  drivers/base/core.c                      | 22 ++++++++++++++++++++--
>  drivers/base/dd.c                        |  2 +-
>  include/linux/device.h                   |  1 +
>  5 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
> index 70e328e..9054403 100644
> --- a/Documentation/driver-api/device_link.rst
> +++ b/Documentation/driver-api/device_link.rst
> @@ -247,7 +247,7 @@ State machine
>    :c:func:`device_links_unbind_consumers()`.)
>
>  * If the probe fails, links to suppliers revert back to ``DL_STATE_AVAILABLE``.
> -  (Call to :c:func:`device_links_no_driver()` from :c:func:`really_probe()`.)
> +  (Call to :c:func:`device_links_probe_failed()` from :c:func:`really_probe()`.)
>
>  * If the probe succeeds, links to suppliers progress to ``DL_STATE_ACTIVE``.
>    (Call to :c:func:`device_links_driver_bound()` from :c:func:`driver_bound()`.)
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index d800de6..f9931d9 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -158,6 +158,6 @@ extern void device_links_read_unlock(int idx);
>  extern int device_links_check_suppliers(struct device *dev);
>  extern void device_links_driver_bound(struct device *dev);
>  extern void device_links_driver_cleanup(struct device *dev);
> -extern void device_links_no_driver(struct device *dev);
> +extern void device_links_probe_failed(struct device *dev);
>  extern bool device_links_busy(struct device *dev);
>  extern void device_links_unbind_consumers(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5847364..31d4f68 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -452,10 +452,28 @@ static void __device_links_no_driver(struct device *dev)
>         dev->links.status = DL_DEV_NO_DRIVER;
>  }
>
> -void device_links_no_driver(struct device *dev)
> +static void __device_links_probe_failed(struct device *dev)
> +{
> +       struct device_link *link, *ln;
> +
> +       list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers,
> +                                        c_node) {
> +               if (link->flags & DL_FLAG_STATELESS)
> +                       continue;
> +
> +               if (link->flags & DL_FLAG_AUTOREMOVE)
> +                       __device_link_del(link);
> +               else if (link->status != DL_STATE_SUPPLIER_UNBIND)
> +                       WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> +       }
> +
> +       dev->links.status = DL_DEV_PROBE_FAILED;
> +}
> +
> +void device_links_probe_failed(struct device *dev)
>  {
>         device_links_write_lock();
> -       __device_links_no_driver(dev);
> +       __device_links_probe_failed(dev);
>         device_links_write_unlock();
>  }
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index de6fd09..90d57e0 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -492,7 +492,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>                 blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>                                              BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
>  pinctrl_bind_failed:
> -       device_links_no_driver(dev);
> +       device_links_probe_failed(dev);
>         devres_release_all(dev);
>         driver_sysfs_remove(dev);
>         dev->driver = NULL;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b093405..bf9630a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -794,6 +794,7 @@ struct device_link {
>  enum dl_dev_state {
>         DL_DEV_NO_DRIVER = 0,
>         DL_DEV_PROBING,
> +       DL_DEV_PROBE_FAILED,
>         DL_DEV_DRIVER_BOUND,
>         DL_DEV_UNBINDING,
>  };
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] driver core: add new dl device status DL_DEV_PROBE_FAILED

[Wang, Dongsheng]

On Thu, 2018-03-29 at 12:51 +0200, Rafael J. Wysocki wrote:
> On Thu, Mar 29, 2018 at 12:00 PM, Wang Dongsheng
> <dongsheng.wang@hxt-semitech.com> wrote:
> > Currently the initialization state of device is DL_DEV_NO_DRIVER.
> > The problem is, after probe failure the state will also be set to
> > DL_DEV_NO_DRIVER as well. And the device is not linked, it has no
> > supplier or consumer. Thus adding a new state to distinguish
> > probe failure and not-probed-yet.
> > 
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> 
> I guess what you want is a cleanup after a failing probe, but after
> that the state really is "no driver" again, isn't it?
Yes, agree "no driver", device is never get a driver again.

But A depend on B successful probe. if B failed, A will never know:
1. B can't work.
2. B hasn't got a probe yet.

Like IOMMU. After SMMU successful probed, the driver add a resource
into "iommu_device_list". Master lookup the corresponding SMMU fwnode
from "iommu_device_list", after matched master will do probe. but if
the list is NULL master will get -EPROBE_DEFER, means SMMU device may
not probe yet, in fact SMMU may probe failed.

I try to use DL_DEV state to fix this issue, but NO_DRIVER does not
distinguish between the two cases.

Cheers,
-Dongsheng

> 
> > ---
> >  Documentation/driver-api/device_link.rst |  2 +-
> >  drivers/base/base.h                      |  2 +-
> >  drivers/base/core.c                      | 22
> > ++++++++++++++++++++--
> >  drivers/base/dd.c                        |  2 +-
> >  include/linux/device.h                   |  1 +
> >  5 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/device_link.rst
> > b/Documentation/driver-api/device_link.rst
> > index 70e328e..9054403 100644
> > --- a/Documentation/driver-api/device_link.rst
> > +++ b/Documentation/driver-api/device_link.rst
> > @@ -247,7 +247,7 @@ State machine
> >    :c:func:`device_links_unbind_consumers()`.)
> > 
> >  * If the probe fails, links to suppliers revert back to
> > ``DL_STATE_AVAILABLE``.
> > -  (Call to :c:func:`device_links_no_driver()` from
> > :c:func:`really_probe()`.)
> > +  (Call to :c:func:`device_links_probe_failed()` from
> > :c:func:`really_probe()`.)
> > 
> >  * If the probe succeeds, links to suppliers progress to
> > ``DL_STATE_ACTIVE``.
> >    (Call to :c:func:`device_links_driver_bound()` from
> > :c:func:`driver_bound()`.)
> > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > index d800de6..f9931d9 100644
> > --- a/drivers/base/base.h
> > +++ b/drivers/base/base.h
> > @@ -158,6 +158,6 @@ extern void device_links_read_unlock(int idx);
> >  extern int device_links_check_suppliers(struct device *dev);
> >  extern void device_links_driver_bound(struct device *dev);
> >  extern void device_links_driver_cleanup(struct device *dev);
> > -extern void device_links_no_driver(struct device *dev);
> > +extern void device_links_probe_failed(struct device *dev);
> >  extern bool device_links_busy(struct device *dev);
> >  extern void device_links_unbind_consumers(struct device *dev);
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 5847364..31d4f68 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -452,10 +452,28 @@ static void __device_links_no_driver(struct
> > device *dev)
> >         dev->links.status = DL_DEV_NO_DRIVER;
> >  }
> > 
> > -void device_links_no_driver(struct device *dev)
> > +static void __device_links_probe_failed(struct device *dev)
> > +{
> > +       struct device_link *link, *ln;
> > +
> > +       list_for_each_entry_safe_reverse(link, ln, &dev-
> > >links.suppliers,
> > +                                        c_node) {
> > +               if (link->flags & DL_FLAG_STATELESS)
> > +                       continue;
> > +
> > +               if (link->flags & DL_FLAG_AUTOREMOVE)
> > +                       __device_link_del(link);
> > +               else if (link->status != DL_STATE_SUPPLIER_UNBIND)
> > +                       WRITE_ONCE(link->status,
> > DL_STATE_AVAILABLE);
> > +       }
> > +
> > +       dev->links.status = DL_DEV_PROBE_FAILED;
> > +}
> > +
> > +void device_links_probe_failed(struct device *dev)
> >  {
> >         device_links_write_lock();
> > -       __device_links_no_driver(dev);
> > +       __device_links_probe_failed(dev);
> >         device_links_write_unlock();
> >  }
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index de6fd09..90d57e0 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -492,7 +492,7 @@ static int really_probe(struct device *dev,
> > struct device_driver *drv)
> >                 blocking_notifier_call_chain(&dev->bus->p-
> > >bus_notifier,
> >                                              BUS_NOTIFY_DRIVER_NOT_
> > BOUND, dev);
> >  pinctrl_bind_failed:
> > -       device_links_no_driver(dev);
> > +       device_links_probe_failed(dev);
> >         devres_release_all(dev);
> >         driver_sysfs_remove(dev);
> >         dev->driver = NULL;
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index b093405..bf9630a 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -794,6 +794,7 @@ struct device_link {
> >  enum dl_dev_state {
> >         DL_DEV_NO_DRIVER = 0,
> >         DL_DEV_PROBING,
> > +       DL_DEV_PROBE_FAILED,
> >         DL_DEV_DRIVER_BOUND,
> >         DL_DEV_UNBINDING,
> >  };
> > --
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lorenzo Pieralisi]

[+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.

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.

Lorenzo

> Signed-off-by: Wang Dongsheng <dongsheng.wang@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
> 

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

[Robin Murphy]

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.

> 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, 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".

Robin.

> 
> Lorenzo
> 
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@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
>>

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

[Robin Murphy]

On 08/04/18 09:10, Wang, Dongsheng wrote:
> 
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>> Sent: Thursday, April 05, 2018 2:57 AM
>> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Wang, Dongsheng
>> <dongsheng.wang@hxt-semitech.com>
>> Cc: rjw@rjwysocki.net; gregkh@linuxfoundation.org; hanjun.guo@linaro.org;
>> sudeep.holla@arm.com; Zheng, Joey <yu.zheng@hxt-semitech.com>;
>> linux-acpi@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 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. 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.

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.

Robin.

> 
> Cheers,
> -Dongsheng
> 
>> Robin.
>>
>>>
>>> Lorenzo
>>>
>>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@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
>>>>