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

Groups > linux.kernel > #1653366 > unrolled thread

[PATCH 0/7] driver core/USB/thermal: fix device-tree node reuse

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] driver core/USB/thermal: fix device-tree node reuse Johan Hovold <johan@kernel.org> - 2017-05-30 18:30 +0200
    [PATCH 4/7] driver core: fix automatic pinctrl management Johan Hovold <johan@kernel.org> - 2017-05-30 18:30 +0200
      Re: [PATCH 4/7] driver core: fix automatic pinctrl management Linus Walleij <linus.walleij@linaro.org> - 2017-05-31 02:40 +0200
        Re: [PATCH 4/7] driver core: fix automatic pinctrl management Johan Hovold <johan@kernel.org> - 2017-05-31 10:40 +0200
    [PATCH 2/7] USB: of: document reference taken by child-lookup helper Johan Hovold <johan@kernel.org> - 2017-05-30 18:40 +0200
      Re: [PATCH 2/7] USB: of: document reference taken by child-lookup  helper Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-05-31 00:50 +0200
        Re: [PATCH 2/7] USB: of: document reference taken by child-lookup  helper Johan Hovold <johan@kernel.org> - 2017-06-06 17:40 +0200
    [PATCH 6/7] thermal: max77620: fix device-node reference imbalance Johan Hovold <johan@kernel.org> - 2017-05-30 18:40 +0200
      Re: [PATCH 6/7] thermal: max77620: fix device-node reference  imbalance Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-05-31 01:00 +0200
        Re: [PATCH 6/7] thermal: max77620: fix device-node reference  imbalance Johan Hovold <johan@kernel.org> - 2017-05-31 10:30 +0200

#1653366 — [PATCH 0/7] driver core/USB/thermal: fix device-tree node reuse

This series fixes a few issues related to device-tree node reuse.

It is fairly common for drivers to reuse the device-tree node of a
parent (or other ancestor) device when creating class or bus devices
(e.g. gpio chips, i2c adapters, iio chips, spi masters, serdev, phys,
usb root hubs). But reusing a device-tree node may cause problems *if*
the new device is later probed as for example driver core would
currently attempt to reinitialise an already active associated pinmux
configuration.

[ NB: For most examples above this is currently not a problem as the
devices reusing a node are never probed. ]

Other potential issues include the platform-bus code unconditionally
dropping the device-tree-node reference in its device destructor,
reinitialisation of other bus-managed resources such as clocks, and
(possibly) the recently added DMA-setup in driver core.

Instead of having drivers try to work around this (as is currently done
for USB root hubs), we can allow devices to reuse a device-tree node by
setting a flag in their struct device that can be used by core, bus and
driver code to avoid resources from being over-allocated.

The first two patches fix a device-tree node reference leak for
non-root-hub devices in USB. These were submitted yesterday to the USB
list, but are included here for completeness.

The third and fourth patches add a helper that can be used when reusing
a device-tree node of another device and that specifically sets a new
of_node_reused flag which is then used by driver core to skip the
automatic pinctrl configuration during probe.

The fifth patch removes the pinctrl over-allocation workaround from USB
core, which also had some undesirable side-effects.

The final two patches fix a device-tree node imbalance and
use-after-free in an thermal driver, where a platform device was reusing
the device-tree node of its parent mfd during probe. This would also
prevent the child device from being reprobed (e.g. due to probe
deferral) if the parent node defines a pinctrl configuration.

Note that this series is against 4.12-rc3.

For reference, here is a list of relevant commits leading up to the
current situation:

 - [2013-01-22] ab78029ecc34 ("drivers/pinctrl: grab default handles from device core")
 - [2016-02-19] 69bec7259853 ("USB: core: let USB device know device node")
 - [2016-04-25] dc5878abf49c ("usb: core: move root hub's device node assignment after it is added to bus")
 - [2016-10-06] 51fa91475e43 ("usb/core: Added devspec sysfs entry for devices behind the usb hub")
 - [2017-03-13] a8c06e407ef9 ("usb: separate out sysdev pointer from usb_bus")
 - [2017-04-10] 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")

Johan


Johan Hovold (7):
  USB: core: fix device node leak
  USB: of: document reference taken by child-lookup helper
  driver core: add helper to reuse a device-tree node
  driver core: fix automatic pinctrl management
  USB: of: fix root-hub device-tree node handling
  thermal: max77620: fix device-node reference imbalance
  thermal: max77620: fix pinmux conflict on reprobe

 drivers/base/core.c                | 16 ++++++++++++++++
 drivers/base/pinctrl.c             |  3 +++
 drivers/thermal/max77620_thermal.c |  8 ++++++--
 drivers/usb/core/hcd.c             |  2 --
 drivers/usb/core/of.c              |  3 +++
 drivers/usb/core/usb.c             |  2 ++
 include/linux/device.h             |  2 ++
 7 files changed, 32 insertions(+), 4 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1653368 — [PATCH 4/7] driver core: fix automatic pinctrl management

Commit ab78029ecc34 ("drivers/pinctrl: grab default handles from device
core") added automatic pin-control management to driver core by looking
up and setting any default pinctrl state found in device tree while a
device is being probed.

This obviously runs into problems as soon as device-tree nodes are
reused for child devices which are later also probed as pins would
already have been claimed by the ancestor device.

For example if a USB host controller claims a pin, its root hub would
consequently fail to probe when its device-tree node is set to the node
of the controller:

    pinctrl-single 48002030.pinmux: pin PIN204 already requested by 48064800.ehci; cannot claim for usb1
    pinctrl-single 48002030.pinmux: pin-204 (usb1) status -22
    pinctrl-single 48002030.pinmux: could not request pin 204 (PIN204) from group usb_dbg_pins  on device pinctrl-single
    usb usb1: Error applying setting, reverse things back
    usb: probe of usb1 failed with error -22

Fix this by checking the new of_node_reused flag and skipping automatic
pinctrl configuration during probe if set.

Note that the flag is checked in driver core rather than in pinctrl
(e.g. in pinctrl_dt_to_map()) which would specifically have prevented
intentional use of a parent's pinctrl properties by a child device
(should such a need ever arise).

Fixes: ab78029ecc34 ("drivers/pinctrl: grab default handles from device core")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/base/pinctrl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 5917b4b5fb99..eb929dd6ef1e 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -23,6 +23,9 @@ int pinctrl_bind_pins(struct device *dev)
 {
 	int ret;
 
+	if (dev->of_node_reused)
+		return 0;
+
 	dev->pins = devm_kzalloc(dev, sizeof(*(dev->pins)), GFP_KERNEL);
 	if (!dev->pins)
 		return -ENOMEM;
-- 
2.13.0

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

#1653740 — Re: [PATCH 4/7] driver core: fix automatic pinctrl management

On Tue, May 30, 2017 at 6:25 PM, Johan Hovold <johan@kernel.org> wrote:

> Commit ab78029ecc34 ("drivers/pinctrl: grab default handles from device
> core") added automatic pin-control management to driver core by looking
> up and setting any default pinctrl state found in device tree while a
> device is being probed.

Actually we do not just support device tree, but also passing pin control
states from board files. It is handled by the core all the same.
So it's not a device tree thing.

One of those days we will have ACPI passing state tables too...

But I understand what you mean.

> Fix this by checking the new of_node_reused flag and skipping automatic
> pinctrl configuration during probe if set.

Seems like a solid idea. I hope we don't need another quirk for ACPI.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

#1653996 — Re: [PATCH 4/7] driver core: fix automatic pinctrl management

On Wed, May 31, 2017 at 02:39:28AM +0200, Linus Walleij wrote:
> On Tue, May 30, 2017 at 6:25 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > Commit ab78029ecc34 ("drivers/pinctrl: grab default handles from device
> > core") added automatic pin-control management to driver core by looking
> > up and setting any default pinctrl state found in device tree while a
> > device is being probed.
> 
> Actually we do not just support device tree, but also passing pin control
> states from board files. It is handled by the core all the same.
> So it's not a device tree thing.
> 
> One of those days we will have ACPI passing state tables too...
> 
> But I understand what you mean.

Yes, I could have mentioned board files, but this problem only applies
to device-tree descriptions (for the time being at least).

> > Fix this by checking the new of_node_reused flag and skipping automatic
> > pinctrl configuration during probe if set.
> 
> Seems like a solid idea. I hope we don't need another quirk for ACPI.

We should be able to just generalise and rename the flag (or add a
second one) if it turns out ACPI needs this too.

> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Johan

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

#1653375 — [PATCH 2/7] USB: of: document reference taken by child-lookup helper

Document that the child-node lookup helper takes a reference to the
device-tree node which needs to be dropped after use.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/of.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index d563cbcf76cf..17a4af02cf5b 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -28,6 +28,9 @@
  *
  * Find the node from device tree according to its port number.
  *
+ * Takes a reference to the returned device-tree node, which needs to be
+ * dropped after use.
+ *
  * Return: On success, a pointer to the device node, %NULL on failure.
  */
 struct device_node *usb_of_get_child_node(struct device_node *parent,
-- 
2.13.0

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

#1653694 — Re: [PATCH 2/7] USB: of: document reference taken by child-lookup helper

On 05/30/2017 09:25 AM, Johan Hovold wrote:
> Document that the child-node lookup helper takes a reference to the
> device-tree node which needs to be dropped after use.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/core/of.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> index d563cbcf76cf..17a4af02cf5b 100644
> --- a/drivers/usb/core/of.c
> +++ b/drivers/usb/core/of.c
> @@ -28,6 +28,9 @@
>   *
>   * Find the node from device tree according to its port number.
>   *
> + * Takes a reference to the returned device-tree node, which needs to be
> + * dropped after use.
> + *
>   * Return: On success, a pointer to the device node, %NULL on failure.

I would use the same blurb used throughout drivers/of/* for consistency.

 *      Returns a node pointer with refcount incremented, use
 *      of_node_put() on it when done.
 */

Just my 2-cents

-Tyrel

>   */
>  struct device_node *usb_of_get_child_node(struct device_node *parent,
> 

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

#1658872 — Re: [PATCH 2/7] USB: of: document reference taken by child-lookup helper

On Tue, May 30, 2017 at 03:40:03PM -0700, Tyrel Datwyler wrote:
> On 05/30/2017 09:25 AM, Johan Hovold wrote:
> > Document that the child-node lookup helper takes a reference to the
> > device-tree node which needs to be dropped after use.
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/core/of.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> > index d563cbcf76cf..17a4af02cf5b 100644
> > --- a/drivers/usb/core/of.c
> > +++ b/drivers/usb/core/of.c
> > @@ -28,6 +28,9 @@
> >   *
> >   * Find the node from device tree according to its port number.
> >   *
> > + * Takes a reference to the returned device-tree node, which needs to be
> > + * dropped after use.
> > + *
> >   * Return: On success, a pointer to the device node, %NULL on failure.
> 
> I would use the same blurb used throughout drivers/of/* for consistency.
> 
>  *      Returns a node pointer with refcount incremented, use
>  *      of_node_put() on it when done.
>  */
> 
> Just my 2-cents

I updated the comment to use a version of this also found in drivers/of:

 * Return: A pointer to the node with incremented refcount if found, or
 * %NULL otherwise.

which is on kernel-doc format. I don't think we need to explicitly
mention which function to manipulate the refcount with in every header.

Thanks,
Johan

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

#1653377 — [PATCH 6/7] thermal: max77620: fix device-node reference imbalance

The thermal child device reuses the parent MFD-device device-tree node
when registering a thermal zone, but did not take a reference to the
node.

This leads to a reference imbalance, and potential use-after-free, when
the node reference is dropped by the platform-bus device destructor
(once for the child and later again for the parent).

Fix this by dropping any reference already held to a device-tree node
and getting a reference to the parent's node which will be balanced on
reprobe or on platform-device release, whichever comes first.

Note that simply clearing the of_node pointer on probe errors and on
driver unbind would not allow the use of device-managed resources as
specifically thermal_zone_of_sensor_unregister() claims that a valid
device-tree node pointer is needed during deregistration (even if it
currently does not seem to use it).

Fixes: ec4664b3fd6d ("thermal: max77620: Add thermal driver for reporting junction temp")
Cc: stable <stable@vger.kernel.org>     # 4.9
Cc: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/thermal/max77620_thermal.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/max77620_thermal.c b/drivers/thermal/max77620_thermal.c
index e9a1fe342760..71d35f3c9215 100644
--- a/drivers/thermal/max77620_thermal.c
+++ b/drivers/thermal/max77620_thermal.c
@@ -104,8 +104,6 @@ static int max77620_thermal_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	pdev->dev.of_node = pdev->dev.parent->of_node;
-
 	mtherm->dev = &pdev->dev;
 	mtherm->rmap = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!mtherm->rmap) {
@@ -113,6 +111,14 @@ static int max77620_thermal_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	/*
+	 * Drop any current reference to a device-tree node and get a
+	 * reference to the parent's node which will be balanced on reprobe or
+	 * on platform-device release.
+	 */
+	of_node_put(pdev->dev.of_node);
+	pdev->dev.of_node = of_node_get(pdev->dev.parent->of_node);
+
 	mtherm->tz_device = devm_thermal_zone_of_sensor_register(&pdev->dev, 0,
 				mtherm, &max77620_thermal_ops);
 	if (IS_ERR(mtherm->tz_device)) {
-- 
2.13.0

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

#1653700 — Re: [PATCH 6/7] thermal: max77620: fix device-node reference imbalance

On 05/30/2017 09:25 AM, Johan Hovold wrote:
> The thermal child device reuses the parent MFD-device device-tree node
> when registering a thermal zone, but did not take a reference to the
> node.
> 
> This leads to a reference imbalance, and potential use-after-free, when
> the node reference is dropped by the platform-bus device destructor
> (once for the child and later again for the parent).
> 
> Fix this by dropping any reference already held to a device-tree node
> and getting a reference to the parent's node which will be balanced on
> reprobe or on platform-device release, whichever comes first.
> 
> Note that simply clearing the of_node pointer on probe errors and on
> driver unbind would not allow the use of device-managed resources as
> specifically thermal_zone_of_sensor_unregister() claims that a valid
> device-tree node pointer is needed during deregistration (even if it
> currently does not seem to use it).
> 
> Fixes: ec4664b3fd6d ("thermal: max77620: Add thermal driver for reporting junction temp")
> Cc: stable <stable@vger.kernel.org>     # 4.9
> Cc: Laxman Dewangan <ldewangan@nvidia.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/thermal/max77620_thermal.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/max77620_thermal.c b/drivers/thermal/max77620_thermal.c
> index e9a1fe342760..71d35f3c9215 100644
> --- a/drivers/thermal/max77620_thermal.c
> +++ b/drivers/thermal/max77620_thermal.c
> @@ -104,8 +104,6 @@ static int max77620_thermal_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	pdev->dev.of_node = pdev->dev.parent->of_node;
> -
>  	mtherm->dev = &pdev->dev;
>  	mtherm->rmap = dev_get_regmap(pdev->dev.parent, NULL);
>  	if (!mtherm->rmap) {
> @@ -113,6 +111,14 @@ static int max77620_thermal_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	/*
> +	 * Drop any current reference to a device-tree node and get a
> +	 * reference to the parent's node which will be balanced on reprobe or
> +	 * on platform-device release.
> +	 */
> +	of_node_put(pdev->dev.of_node);
> +	pdev->dev.of_node = of_node_get(pdev->dev.parent->of_node);
> +

This seems like needless churn. Can't this just be squashed into patch #7?

-Tyrel

>  	mtherm->tz_device = devm_thermal_zone_of_sensor_register(&pdev->dev, 0,
>  				mtherm, &max77620_thermal_ops);
>  	if (IS_ERR(mtherm->tz_device)) {
> 

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

#1653976 — Re: [PATCH 6/7] thermal: max77620: fix device-node reference imbalance

On Tue, May 30, 2017 at 03:59:10PM -0700, Tyrel Datwyler wrote:
> On 05/30/2017 09:25 AM, Johan Hovold wrote:
> > The thermal child device reuses the parent MFD-device device-tree node
> > when registering a thermal zone, but did not take a reference to the
> > node.
> > 
> > This leads to a reference imbalance, and potential use-after-free, when
> > the node reference is dropped by the platform-bus device destructor
> > (once for the child and later again for the parent).
> > 
> > Fix this by dropping any reference already held to a device-tree node
> > and getting a reference to the parent's node which will be balanced on
> > reprobe or on platform-device release, whichever comes first.
> > 
> > Note that simply clearing the of_node pointer on probe errors and on
> > driver unbind would not allow the use of device-managed resources as
> > specifically thermal_zone_of_sensor_unregister() claims that a valid
> > device-tree node pointer is needed during deregistration (even if it
> > currently does not seem to use it).
> > 
> > Fixes: ec4664b3fd6d ("thermal: max77620: Add thermal driver for reporting junction temp")
> > Cc: stable <stable@vger.kernel.org>     # 4.9
> > Cc: Laxman Dewangan <ldewangan@nvidia.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/thermal/max77620_thermal.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/max77620_thermal.c b/drivers/thermal/max77620_thermal.c
> > index e9a1fe342760..71d35f3c9215 100644
> > --- a/drivers/thermal/max77620_thermal.c
> > +++ b/drivers/thermal/max77620_thermal.c
> > @@ -104,8 +104,6 @@ static int max77620_thermal_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	pdev->dev.of_node = pdev->dev.parent->of_node;
> > -
> >  	mtherm->dev = &pdev->dev;
> >  	mtherm->rmap = dev_get_regmap(pdev->dev.parent, NULL);
> >  	if (!mtherm->rmap) {
> > @@ -113,6 +111,14 @@ static int max77620_thermal_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> >  
> > +	/*
> > +	 * Drop any current reference to a device-tree node and get a
> > +	 * reference to the parent's node which will be balanced on reprobe or
> > +	 * on platform-device release.
> > +	 */
> > +	of_node_put(pdev->dev.of_node);
> > +	pdev->dev.of_node = of_node_get(pdev->dev.parent->of_node);
> > +
> 
> This seems like needless churn. Can't this just be squashed into patch
> #7?

As I just replied to Eduardo, I'm fixing two separate issues here of
which this one might qualify for stable while the other (7/7) does not.

Thanks,
Johan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web