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

Groups > linux.kernel > #1658896 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/7] driver core/USB/thermal: fix device-tree node reuse Johan Hovold <johan@kernel.org> - 2017-06-06 18:10 +0200
    [PATCH v2 1/7] USB: core: fix device node leak Johan Hovold <johan@kernel.org> - 2017-06-06 18:10 +0200
    [PATCH v2 3/7] driver core: add helper to reuse a device-tree node Johan Hovold <johan@kernel.org> - 2017-06-06 18:10 +0200
    [PATCH v2 2/7] USB: of: document reference taken by child-lookup helper Johan Hovold <johan@kernel.org> - 2017-06-06 18:10 +0200
    [PATCH v2 6/7] thermal: max77620: fix device-node reference imbalance Johan Hovold <johan@kernel.org> - 2017-06-06 18:10 +0200

#1658896 — [PATCH v2 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")

Greg, I'm not sure how best to handle merging this, but I guess all
could go in through driver-core-next to make handling dependencies
easier even if patches 1,2 and 6 could otherwise be considered for
4.12-rc.

Johan


Changes in v2
 - update kernel doc comment (2/7)
 - add missing kernel doc comment (3/7)
 - add Peter's and Linus's acks (1/7 and 4/7)


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             |  4 ++++
 7 files changed, 33 insertions(+), 5 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1658897 — [PATCH v2 1/7] USB: core: fix device node leak

Make sure to release any OF device-node reference taken when creating
the USB device.

Note that we currently do not hold a reference to the root hub
device-tree node (i.e. the parent controller node).

Fixes: 69bec7259853 ("USB: core: let USB device know device node")
Cc: stable <stable@vger.kernel.org>	# v4.6
Acked-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/usb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 28b053cacc90..62e1906bb2f3 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -416,6 +416,8 @@ static void usb_release_dev(struct device *dev)
 
 	usb_destroy_configuration(udev);
 	usb_release_bos_descriptor(udev);
+	if (udev->parent)
+		of_node_put(dev->of_node);
 	usb_put_hcd(hcd);
 	kfree(udev->product);
 	kfree(udev->manufacturer);
-- 
2.13.0

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

#1658899 — [PATCH v2 3/7] driver core: add helper to reuse a device-tree node

Add a helper function to be used when reusing the device-tree node of
another device.

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.

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 the
recently added DMA-setup in driver core.

Note that for most examples above this is currently not an issue as the
devices are never probed, but this is a problem for the USB bus which
has recently gained device-tree support. This was discovered and
worked-around in a rather ad-hoc fashion by commit dc5878abf49c ("usb:
core: move root hub's device node assignment after it is added to bus")
by not setting the of_node pointer until after the root-hub device has
been registered.

Instead 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.

Note that the helper also grabs an extra reference to the device node,
which specifically balances the unconditional put in the platform-device
destructor.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/base/core.c    | 16 ++++++++++++++++
 include/linux/device.h |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bbecaf9293be..c3064ff09af5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2884,3 +2884,19 @@ void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
 	else
 		dev->fwnode = fwnode;
 }
+
+/**
+ * device_set_of_node_from_dev - reuse device-tree node of another device
+ * @dev: device whose device-tree node is being set
+ * @dev2: device whose device-tree node is being reused
+ *
+ * Takes another reference to the new device-tree node after first dropping
+ * any reference held to the old node.
+ */
+void device_set_of_node_from_dev(struct device *dev, const struct device *dev2)
+{
+	of_node_put(dev->of_node);
+	dev->of_node = of_node_get(dev2->of_node);
+	dev->of_node_reused = true;
+}
+EXPORT_SYMBOL_GPL(device_set_of_node_from_dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 9ef518af5515..60ab00b13095 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -879,6 +879,8 @@ struct dev_links_info {
  *
  * @offline_disabled: If set, the device is permanently online.
  * @offline:	Set after successful invocation of bus type's .offline().
+ * @of_node_reused: Set if the device-tree node is shared with an ancestor
+ *              device.
  *
  * At the lowest level, every device in a Linux system is represented by an
  * instance of struct device. The device structure contains the information
@@ -966,6 +968,7 @@ struct device {
 
 	bool			offline_disabled:1;
 	bool			offline:1;
+	bool			of_node_reused:1;
 };
 
 static inline struct device *kobj_to_dev(struct kobject *kobj)
@@ -1144,6 +1147,7 @@ extern int device_offline(struct device *dev);
 extern int device_online(struct device *dev);
 extern void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
 extern void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+void device_set_of_node_from_dev(struct device *dev, const struct device *dev2);
 
 static inline int dev_num_vf(struct device *dev)
 {
-- 
2.13.0

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

#1658900 — [PATCH v2 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, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index d563cbcf76cf..3863bb1ce8c5 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -28,7 +28,8 @@
  *
  * Find the node from device tree according to its port number.
  *
- * Return: On success, a pointer to the device node, %NULL on failure.
+ * Return: A pointer to the node with incremented refcount if found, or
+ * %NULL otherwise.
  */
 struct device_node *usb_of_get_child_node(struct device_node *parent,
 					int portnum)
-- 
2.13.0

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

#1658903 — [PATCH v2 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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web