Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331055 > unrolled thread

[PATCH v5 RESEND 0/5] AMBA: add complete support for power domains

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 RESEND 0/5] AMBA: add complete support for power domains Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
    [PATCH v5 RESEND 1/5] drivers: nvdimm: ensure no negative value gets  returned on positive match Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
    [PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from  bus_type.match() Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
      Re: [PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from  bus_type.match() Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-12 04:20 +0100
    [PATCH v5 RESEND 5/5] ARM: amba: Properly handle devices with power  domains Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
    [PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets  returned on positive match Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
      Re: [PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets  returned on positive match Ulf Hansson <ulf.hansson@linaro.org> - 2016-02-10 17:50 +0100

#1331055 — [PATCH v5 RESEND 0/5] AMBA: add complete support for power domains

(Old thread name: Exynos4210: fix power domain for MDMA1 device)

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.

Since the proposed change in the driver core had impact on existing
drivers (see '-next regression: "driver cohandle -EPROBE_DEFER from
bus_type.match()"' thread https://lkml.org/lkml/2015/12/17/390 ), I've
checked all the functions assigned to match callback of struct bus_type
objects and found that there are only 4 such functions that don't return
0/1 values:

1. arch/arm/common/sa1111.c: sa1111_match -> result of bitwise &
2. drivers/nvdimm/bus.c: nvdimm_bus_match -> result of test_bit()
3. drivers/sh/superhyway/superhyway.c: superhyway_bus_match -> error codes
     (this is really funny case, all errors are resolved to 'matched' case)
4. drivers/staging/unisys/visorbus/visorbus_main.c: visorbus_match -> 0 and
     some positive integer values, safe for now

The list of functions that I've examined has been generated by following
shell command:
$ git grep -Pp "\.match\s" | grep -A1 bus_type | grep "\.match\s" | tr -s "\t;&,=:" " " | cut -d" " -f1,3

Only the first two functions require potential fixing to ensure that
correct match will not result in negative return value, so the 2
additional patches have been added to v5 patchset.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Changelog:

v5:
- added 2 more patches to avoid regression with existing drivers (nvdimm and
  sa1111), for more information, see https://lkml.org/lkml/2015/12/17/390
- changed thread name to "AMBA: add complete support for power domains"

v4: https://lkml.org/lkml/2015/12/2/52
- fixed more issues pointed by Ulf Hansson and Russell King

v3: https://lkml.org/lkml/2015/12/1/334
- 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:

Dan Williams (1):
  drivers: nvdimm: ensure no negative value gets returned on positive
    match

Marek Szyprowski (2):
  ARM: sa1111: ensure no negative value gets returned on positive match
  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 +-
 arch/arm/common/sa1111.c               |   2 +-
 drivers/amba/bus.c                     | 152 ++++++++++++++++++---------------
 drivers/base/dd.c                      |  24 +++++-
 drivers/nvdimm/bus.c                   |   2 +-
 include/linux/device.h                 |   7 +-
 6 files changed, 114 insertions(+), 79 deletions(-)

-- 
1.9.2

[toc] | [next] | [standalone]

#1331056 — [PATCH v5 RESEND 1/5] drivers: nvdimm: ensure no negative value gets returned on positive match

From: Dan Williams <dan.j.williams@intel.com>

This patch ensures that existing bus match callbacks don't return
negative values (which might be interpreted as potential errors in the
future) in case of positive match.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/nvdimm/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..2b2181c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct nd_device_driver *nd_drv = to_nd_device_driver(drv);
 
-	return test_bit(to_nd_device_type(dev), &nd_drv->type);
+	return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
 }
 
 static struct module *to_bus_provider(struct device *dev)
-- 
1.9.2

[toc] | [prev] | [next] | [standalone]

#1331057 — [PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from bus_type.match()

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>

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 <tomeu.vizoso@collabora.com>
[changed if-else code structure, adjusted documentation to match the code,
extended comments]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/driver-model/porting.txt |  6 ++++--
 drivers/base/dd.c                      | 24 ++++++++++++++++++++++--
 include/linux/device.h                 |  7 +++++--
 3 files changed, 31 insertions(+), 6 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 7399be7..7c3f1f1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -544,6 +544,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
@@ -554,8 +555,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);
 
@@ -675,6 +685,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
@@ -686,8 +697,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 f627ba2..75a2cde 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -70,8 +70,11 @@ 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
- *		given device can be handled by the given driver.
+ *		is added for this bus. It should return a positive value if the
+ *		given device can be handled by the given driver and zero
+ *		otherwise. It may also return error code if determining that
+ *		the driver supports the device is not possible. In case of
+ *		-EPROBE_DEFER it will queue the device for deferred probing.
  * @uevent:	Called when a device is added, removed, or a few other things
  *		that generate uevents to add the environment variables.
  * @probe:	Called when a new device or driver add to this bus, and callback
-- 
1.9.2

[toc] | [prev] | [next] | [standalone]

#1332504 — Re: [PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from bus_type.match()

On Wed, Feb 10, 2016 at 11:47:28AM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> 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 <tomeu.vizoso@collabora.com>
> [changed if-else code structure, adjusted documentation to match the code,
> extended comments]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/driver-model/porting.txt |  6 ++++--
>  drivers/base/dd.c                      | 24 ++++++++++++++++++++++--
>  include/linux/device.h                 |  7 +++++--
>  3 files changed, 31 insertions(+), 6 deletions(-)


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[toc] | [prev] | [next] | [standalone]

#1331058 — [PATCH v5 RESEND 5/5] ARM: amba: Properly handle devices with power domains

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 <m.szyprowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 643127f..570033b 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -60,6 +60,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;
@@ -84,6 +88,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

[toc] | [prev] | [next] | [standalone]

#1331059 — [PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets returned on positive match

This patch ensures that existing bus match callbacks don't return
negative values (which might be interpreted as potential errors in the
future) in case of positive match.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/common/sa1111.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 3d22494..fb0a0a4 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, struct device_driver *_drv)
 	struct sa1111_dev *dev = SA1111_DEV(_dev);
 	struct sa1111_driver *drv = SA1111_DRV(_drv);
 
-	return dev->devid & drv->devid;
+	return !!(dev->devid & drv->devid);
 }
 
 static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
-- 
1.9.2

[toc] | [prev] | [next] | [standalone]

#1331324 — Re: [PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets returned on positive match

On 10 February 2016 at 11:47, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> This patch ensures that existing bus match callbacks don't return
> negative values (which might be interpreted as potential errors in the
> future) in case of positive match.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

FYI:
This one has actually been acked by Russell.
https://lkml.org/lkml/2015/12/23/111

Kind regards
Uffe

> ---
>  arch/arm/common/sa1111.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> index 3d22494..fb0a0a4 100644
> --- a/arch/arm/common/sa1111.c
> +++ b/arch/arm/common/sa1111.c
> @@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, struct device_driver *_drv)
>         struct sa1111_dev *dev = SA1111_DEV(_dev);
>         struct sa1111_driver *drv = SA1111_DRV(_drv);
>
> -       return dev->devid & drv->devid;
> +       return !!(dev->devid & drv->devid);
>  }
>
>  static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
> --
> 1.9.2
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web