This patchset fixes mysterious boot hang on Exynos 4210 SoCs, when IOMMU
is enabled. There is no direct dependency between IOMMU devices and
MDMA1. However enabling IOMMU changes the device probe order, what
results in LCD0 power domain being turned off for some time. During that
time the registration of MDMA1 device happens, what results in system
hangs, because the common bus code tries to read PID/CID registers from
turned-off device.
Changes since v2 include some code refactoring done to fix issues
pointed by Ulf Hansson.
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
Changelog:
v3:
- fixed issues pointed by Ulf Hansson
- dropped patch for exynos4210 dts, because it already got queued for merging
v2: https://lkml.org/lkml/2015/11/26/229
- added 2 patches from 'On-demand device probing' thread
(https://lkml.org/lkml/2015/9/29/189), which move PID/CIR reading
from amba_device_add() to amba_match()
- moved dev_pm_domain_attach() to amba_match(), which is allowed to
return -EPROBE_DEFER
v1: http://www.spinics.net/lists/arm-kernel/msg463185.html
- initial version
Patch summary:
Marek Szyprowski (1):
ARM: amba: Properly handle devices with power domains
Tomeu Vizoso (2):
driver core: handle -EPROBE_DEFER from bus_type.match()
ARM: amba: Move reading of periphid to amba_match()
Documentation/driver-model/porting.txt | 6 ++-
drivers/amba/bus.c | 83 +++++++++++++++++++---------------
drivers/base/dd.c | 24 +++++++++-
include/linux/device.h | 2 +-
4 files changed, 74 insertions(+), 41 deletions(-)
--
1.9.2
From: Tomeu Vizoso <[email protected]>
Allow implementations of the match() callback in struct bus_type to
return errors and if it's -EPROBE_DEFER then queue the device for
deferred probing.
This is useful to buses such as AMBA in which devices are registered
before their matching information can be retrieved from the HW
(typically because a clock driver hasn't probed yet).
Signed-off-by: Tomeu Vizoso <[email protected]>
[changed if-else code structure, adjusted documentation to match the code]
Signed-off-by: Marek Szyprowski <[email protected]>
---
Documentation/driver-model/porting.txt | 6 ++++--
drivers/base/dd.c | 24 ++++++++++++++++++++++--
include/linux/device.h | 2 +-
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/Documentation/driver-model/porting.txt b/Documentation/driver-model/porting.txt
index 92d86f7..453053f 100644
--- a/Documentation/driver-model/porting.txt
+++ b/Documentation/driver-model/porting.txt
@@ -340,8 +340,10 @@ comparison:
int (*match)(struct device * dev, struct device_driver * drv);
-match should return '1' if the driver supports the device, and '0'
-otherwise.
+match should return positive value if the driver supports the device,
+and zero otherwise. It may also return error code (for example
+-EPROBE_DEFER) if determining that given driver supports the device is
+not possible.
When a device is registered, the bus's list of drivers is iterated
over. bus->match() is called for each one until a match is found.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a641cf3..793e915 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -490,6 +490,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
struct device_attach_data *data = _data;
struct device *dev = data->dev;
bool async_allowed;
+ int ret;
/*
* Check if device has already been claimed. This may
@@ -500,8 +501,17 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
if (dev->driver)
return -EBUSY;
- if (!driver_match_device(drv, dev))
+ ret = driver_match_device(drv, dev);
+ if (ret == 0) {
+ /* no match */
return 0;
+ } else if (ret == -EPROBE_DEFER) {
+ dev_dbg(dev, "Device match requests probe deferral\n");
+ driver_deferred_probe_add(dev);
+ } else if (ret < 0) {
+ dev_dbg(dev, "Bus failed to match device: %d", ret);
+ return ret;
+ } /* ret > 0 means positive match */
async_allowed = driver_allows_async_probing(drv);
@@ -621,6 +631,7 @@ void device_initial_probe(struct device *dev)
static int __driver_attach(struct device *dev, void *data)
{
struct device_driver *drv = data;
+ int ret;
/*
* Lock device and try to bind to it. We drop the error
@@ -632,8 +643,17 @@ static int __driver_attach(struct device *dev, void *data)
* is an error.
*/
- if (!driver_match_device(drv, dev))
+ ret = driver_match_device(drv, dev);
+ if (ret == 0) {
+ /* no match */
return 0;
+ } else if (ret == -EPROBE_DEFER) {
+ dev_dbg(dev, "Device match requests probe deferral\n");
+ driver_deferred_probe_add(dev);
+ } else if (ret < 0) {
+ dev_dbg(dev, "Bus failed to match device: %d", ret);
+ return ret;
+ } /* ret > 0 means positive match */
if (dev->parent) /* Needed for USB */
device_lock(dev->parent);
diff --git a/include/linux/device.h b/include/linux/device.h
index b8f411b..d4e7d1f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -70,7 +70,7 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* @dev_groups: Default attributes of the devices on the bus.
* @drv_groups: Default attributes of the device drivers on the bus.
* @match: Called, perhaps multiple times, whenever a new device or driver
- * is added for this bus. It should return a nonzero value if the
+ * is added for this bus. It should return a positive value if the
* given device can be handled by the given driver.
* @uevent: Called when a device is added, removed, or a few other things
* that generate uevents to add the environment variables.
--
1.9.2
From: Tomeu Vizoso <[email protected]>
Reading the periphid when the Primecell device is registered means that
the apb pclk must be available by then or the device won't be registered
at all.
By reading the periphid in amba_match() we can return -EPROBE_DEFER if
the apb pclk isn't there yet and the device will be retried later.
Signed-off-by: Tomeu Vizoso <[email protected]>
[minor code adjustments, added missing comment]
Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/amba/bus.c | 77 +++++++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 36 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f009936..879a421 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -24,6 +24,8 @@
#define to_amba_driver(d) container_of(d, struct amba_driver, drv)
+static int amba_read_periphid(struct amba_device *dev);
+
static const struct amba_id *
amba_lookup(const struct amba_id *table, struct amba_device *dev)
{
@@ -43,11 +45,23 @@ static int amba_match(struct device *dev, struct device_driver *drv)
{
struct amba_device *pcdev = to_amba_device(dev);
struct amba_driver *pcdrv = to_amba_driver(drv);
+ int ret;
/* When driver_override is set, only bind to the matching driver */
if (pcdev->driver_override)
return !strcmp(pcdev->driver_override, drv->name);
+ /* Do plug-n-play if no hard-coded primecell ID has been provided */
+ if (!pcdev->periphid) {
+ ret = amba_read_periphid(pcdev);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Failed to read periphid: %d",
+ ret);
+ return ret;
+ }
+ }
+
return amba_lookup(pcdrv->id_table, pcdev) != NULL;
}
@@ -336,31 +350,11 @@ static void amba_device_release(struct device *dev)
kfree(d);
}
-/**
- * amba_device_add - add a previously allocated AMBA device structure
- * @dev: AMBA device allocated by amba_device_alloc
- * @parent: resource parent for this devices resources
- *
- * Claim the resource, and read the device cell ID if not already
- * initialized. Register the AMBA device with the Linux device
- * manager.
- */
-int amba_device_add(struct amba_device *dev, struct resource *parent)
+static int amba_read_periphid(struct amba_device *dev)
{
u32 size;
void __iomem *tmp;
- int i, ret;
-
- WARN_ON(dev->irq[0] == (unsigned int)-1);
- WARN_ON(dev->irq[1] == (unsigned int)-1);
-
- ret = request_resource(parent, &dev->res);
- if (ret)
- goto err_out;
-
- /* Hard-coded primecell ID instead of plug-n-play */
- if (dev->periphid != 0)
- goto skip_probe;
+ int i, ret = 0;
/*
* Dynamically calculate the size of the resource
@@ -368,10 +362,8 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
*/
size = resource_size(&dev->res);
tmp = ioremap(dev->res.start, size);
- if (!tmp) {
- ret = -ENOMEM;
- goto err_release;
- }
+ if (!tmp)
+ return -ENOMEM;
ret = amba_get_enable_pclk(dev);
if (ret == 0) {
@@ -399,26 +391,39 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
iounmap(tmp);
+ return ret;
+}
+
+/**
+ * amba_device_add - add a previously allocated AMBA device structure
+ * @dev: AMBA device allocated by amba_device_alloc
+ * @parent: resource parent for this devices resources
+ *
+ * Claim the resource, and register the AMBA device with the Linux device
+ * manager.
+ */
+int amba_device_add(struct amba_device *dev, struct resource *parent)
+{
+ int ret;
+
+ WARN_ON(dev->irq[0] == (unsigned int)-1);
+ WARN_ON(dev->irq[1] == (unsigned int)-1);
+
+ ret = request_resource(parent, &dev->res);
if (ret)
- goto err_release;
+ return ret;
- skip_probe:
ret = device_add(&dev->dev);
if (ret)
- goto err_release;
+ return ret;
if (dev->irq[0])
ret = device_create_file(&dev->dev, &dev_attr_irq0);
if (ret == 0 && dev->irq[1])
ret = device_create_file(&dev->dev, &dev_attr_irq1);
- if (ret == 0)
- return ret;
-
- device_unregister(&dev->dev);
+ if (ret)
+ device_unregister(&dev->dev);
- err_release:
- release_resource(&dev->res);
- err_out:
return ret;
}
EXPORT_SYMBOL_GPL(amba_device_add);
--
1.9.2
To read pid/cid registers, the probed device need to be properly turned on.
When it is inside a power domain, the bus code should ensure that the
given power domain is enabled before trying to access device's registers.
Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/amba/bus.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 879a421..8d182a4 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -365,6 +365,10 @@ static int amba_read_periphid(struct amba_device *dev)
if (!tmp)
return -ENOMEM;
+ ret = dev_pm_domain_attach(&dev->dev, true);
+ if (ret == -EPROBE_DEFER)
+ goto err_unmap;
+
ret = amba_get_enable_pclk(dev);
if (ret == 0) {
u32 pid, cid;
@@ -389,6 +393,8 @@ static int amba_read_periphid(struct amba_device *dev)
ret = -ENODEV;
}
+ dev_pm_domain_detach(&dev->dev, true);
+err_unmap:
iounmap(tmp);
return ret;
--
1.9.2
On 1 December 2015 at 14:34, Marek Szyprowski <[email protected]> wrote:
> From: Tomeu Vizoso <[email protected]>
>
> Allow implementations of the match() callback in struct bus_type to
> return errors and if it's -EPROBE_DEFER then queue the device for
> deferred probing.
>
> This is useful to buses such as AMBA in which devices are registered
> before their matching information can be retrieved from the HW
> (typically because a clock driver hasn't probed yet).
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> [changed if-else code structure, adjusted documentation to match the code]
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> Documentation/driver-model/porting.txt | 6 ++++--
> drivers/base/dd.c | 24 ++++++++++++++++++++++--
> include/linux/device.h | 2 +-
> 3 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/driver-model/porting.txt b/Documentation/driver-model/porting.txt
> index 92d86f7..453053f 100644
> --- a/Documentation/driver-model/porting.txt
> +++ b/Documentation/driver-model/porting.txt
> @@ -340,8 +340,10 @@ comparison:
>
> int (*match)(struct device * dev, struct device_driver * drv);
>
> -match should return '1' if the driver supports the device, and '0'
> -otherwise.
> +match should return positive value if the driver supports the device,
> +and zero otherwise. It may also return error code (for example
> +-EPROBE_DEFER) if determining that given driver supports the device is
> +not possible.
>
> When a device is registered, the bus's list of drivers is iterated
> over. bus->match() is called for each one until a match is found.
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a641cf3..793e915 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -490,6 +490,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
> struct device_attach_data *data = _data;
> struct device *dev = data->dev;
> bool async_allowed;
> + int ret;
>
> /*
> * Check if device has already been claimed. This may
> @@ -500,8 +501,17 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
> if (dev->driver)
> return -EBUSY;
>
> - if (!driver_match_device(drv, dev))
> + ret = driver_match_device(drv, dev);
> + if (ret == 0) {
> + /* no match */
> return 0;
> + } else if (ret == -EPROBE_DEFER) {
> + dev_dbg(dev, "Device match requests probe deferral\n");
> + driver_deferred_probe_add(dev);
> + } else if (ret < 0) {
> + dev_dbg(dev, "Bus failed to match device: %d", ret);
> + return ret;
> + } /* ret > 0 means positive match */
>
> async_allowed = driver_allows_async_probing(drv);
>
> @@ -621,6 +631,7 @@ void device_initial_probe(struct device *dev)
> static int __driver_attach(struct device *dev, void *data)
> {
> struct device_driver *drv = data;
> + int ret;
>
> /*
> * Lock device and try to bind to it. We drop the error
> @@ -632,8 +643,17 @@ static int __driver_attach(struct device *dev, void *data)
> * is an error.
> */
>
> - if (!driver_match_device(drv, dev))
> + ret = driver_match_device(drv, dev);
> + if (ret == 0) {
> + /* no match */
> return 0;
> + } else if (ret == -EPROBE_DEFER) {
> + dev_dbg(dev, "Device match requests probe deferral\n");
> + driver_deferred_probe_add(dev);
> + } else if (ret < 0) {
> + dev_dbg(dev, "Bus failed to match device: %d", ret);
> + return ret;
> + } /* ret > 0 means positive match */
>
> if (dev->parent) /* Needed for USB */
> device_lock(dev->parent);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b8f411b..d4e7d1f 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -70,7 +70,7 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> * @dev_groups: Default attributes of the devices on the bus.
> * @drv_groups: Default attributes of the device drivers on the bus.
> * @match: Called, perhaps multiple times, whenever a new device or driver
> - * is added for this bus. It should return a nonzero value if the
> + * is added for this bus. It should return a positive value if the
Perhaps add some more information about what happens when returning
error codes!?
> * given device can be handled by the given driver.
> * @uevent: Called when a device is added, removed, or a few other things
> * that generate uevents to add the environment variables.
> --
> 1.9.2
>
Besides the minor nit above, you may add my:
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
On 1 December 2015 at 14:34, Marek Szyprowski <[email protected]> wrote:
> From: Tomeu Vizoso <[email protected]>
>
> Reading the periphid when the Primecell device is registered means that
> the apb pclk must be available by then or the device won't be registered
> at all.
>
> By reading the periphid in amba_match() we can return -EPROBE_DEFER if
> the apb pclk isn't there yet and the device will be retried later.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> [minor code adjustments, added missing comment]
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/amba/bus.c | 77 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f009936..879a421 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -24,6 +24,8 @@
>
> #define to_amba_driver(d) container_of(d, struct amba_driver, drv)
>
> +static int amba_read_periphid(struct amba_device *dev);
> +
> static const struct amba_id *
> amba_lookup(const struct amba_id *table, struct amba_device *dev)
> {
> @@ -43,11 +45,23 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(drv);
> + int ret;
>
> /* When driver_override is set, only bind to the matching driver */
> if (pcdev->driver_override)
> return !strcmp(pcdev->driver_override, drv->name);
>
> + /* Do plug-n-play if no hard-coded primecell ID has been provided */
> + if (!pcdev->periphid) {
> + ret = amba_read_periphid(pcdev);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to read periphid: %d",
> + ret);
If you anyway are printing errors in the driver core, why do it here as well?
I guess you need to make a call, either here or in driver core. I am
fine with whatever way, just not both.
> + return ret;
> + }
> + }
> +
Besides the minor comment above, you may add my:
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
On 1 December 2015 at 14:34, Marek Szyprowski <[email protected]> wrote:
> To read pid/cid registers, the probed device need to be properly turned on.
> When it is inside a power domain, the bus code should ensure that the
> given power domain is enabled before trying to access device's registers.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> drivers/amba/bus.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 879a421..8d182a4 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -365,6 +365,10 @@ static int amba_read_periphid(struct amba_device *dev)
> if (!tmp)
> return -ENOMEM;
>
> + ret = dev_pm_domain_attach(&dev->dev, true);
> + if (ret == -EPROBE_DEFER)
> + goto err_unmap;
> +
> ret = amba_get_enable_pclk(dev);
> if (ret == 0) {
> u32 pid, cid;
> @@ -389,6 +393,8 @@ static int amba_read_periphid(struct amba_device *dev)
> ret = -ENODEV;
> }
>
> + dev_pm_domain_detach(&dev->dev, true);
> +err_unmap:
> iounmap(tmp);
>
> return ret;
> --
> 1.9.2
>
On Tue, Dec 01, 2015 at 02:34:25PM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso <[email protected]>
>
> Reading the periphid when the Primecell device is registered means that
> the apb pclk must be available by then or the device won't be registered
> at all.
>
> By reading the periphid in amba_match() we can return -EPROBE_DEFER if
> the apb pclk isn't there yet and the device will be retried later.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> [minor code adjustments, added missing comment]
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/amba/bus.c | 77 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f009936..879a421 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -24,6 +24,8 @@
>
> #define to_amba_driver(d) container_of(d, struct amba_driver, drv)
>
> +static int amba_read_periphid(struct amba_device *dev);
> +
Please avoid forward declarations where possible.
> static const struct amba_id *
> amba_lookup(const struct amba_id *table, struct amba_device *dev)
> {
> @@ -43,11 +45,23 @@ static int amba_match(struct device *dev, struct device_driver *drv)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(drv);
> + int ret;
>
> /* When driver_override is set, only bind to the matching driver */
> if (pcdev->driver_override)
> return !strcmp(pcdev->driver_override, drv->name);
>
> + /* Do plug-n-play if no hard-coded primecell ID has been provided */
> + if (!pcdev->periphid) {
> + ret = amba_read_periphid(pcdev);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to read periphid: %d",
> + ret);
> + return ret;
> + }
> + }
> +
> return amba_lookup(pcdrv->id_table, pcdev) != NULL;
> }
>
> @@ -336,31 +350,11 @@ static void amba_device_release(struct device *dev)
> kfree(d);
> }
>
> -/**
> - * amba_device_add - add a previously allocated AMBA device structure
> - * @dev: AMBA device allocated by amba_device_alloc
> - * @parent: resource parent for this devices resources
> - *
> - * Claim the resource, and read the device cell ID if not already
> - * initialized. Register the AMBA device with the Linux device
> - * manager.
> - */
> -int amba_device_add(struct amba_device *dev, struct resource *parent)
> +static int amba_read_periphid(struct amba_device *dev)
> {
> u32 size;
> void __iomem *tmp;
> - int i, ret;
> -
> - WARN_ON(dev->irq[0] == (unsigned int)-1);
> - WARN_ON(dev->irq[1] == (unsigned int)-1);
> -
> - ret = request_resource(parent, &dev->res);
> - if (ret)
> - goto err_out;
> -
> - /* Hard-coded primecell ID instead of plug-n-play */
> - if (dev->periphid != 0)
> - goto skip_probe;
> + int i, ret = 0;
>
> /*
> * Dynamically calculate the size of the resource
> @@ -368,10 +362,8 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
> */
> size = resource_size(&dev->res);
> tmp = ioremap(dev->res.start, size);
> - if (!tmp) {
> - ret = -ENOMEM;
> - goto err_release;
> - }
> + if (!tmp)
> + return -ENOMEM;
>
> ret = amba_get_enable_pclk(dev);
> if (ret == 0) {
> @@ -399,26 +391,39 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
>
> iounmap(tmp);
>
> + return ret;
> +}
> +
> +/**
> + * amba_device_add - add a previously allocated AMBA device structure
> + * @dev: AMBA device allocated by amba_device_alloc
> + * @parent: resource parent for this devices resources
> + *
> + * Claim the resource, and register the AMBA device with the Linux device
> + * manager.
> + */
> +int amba_device_add(struct amba_device *dev, struct resource *parent)
> +{
> + int ret;
> +
> + WARN_ON(dev->irq[0] == (unsigned int)-1);
> + WARN_ON(dev->irq[1] == (unsigned int)-1);
> +
> + ret = request_resource(parent, &dev->res);
> if (ret)
> - goto err_release;
> + return ret;
>
> - skip_probe:
> ret = device_add(&dev->dev);
> if (ret)
> - goto err_release;
> + return ret;
NAK.
The original code handled error conditions correctly. This new code
completely breaks the error handling here by removing the release of
the above requested resource.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.