Hi,
This is a continuation of my old patchset from 2019. [1]
Back then, few drivers set driver_override wrong. I fixed Exynos
in a different way after discussions. QCOM NGD was not fixed
and a new user appeared - IMX SCU.
It seems "char *" in driver_override looks too consty, so we
tend to make a mistake of storing there string literals.
Changes since latest v4
=======================
1. Correct commit msgs and comments after Andy's review.
2. Re-order code in new helper (patch #1) (Andy).
3. Add tags.
Changes since latest v3
=======================
1. Wrap comments, extend comment in driver_set_override() about newline.
2. Minor commit msg fixes.
3. Add tags.
Changes since latest v2
=======================
1. Make all driver_override fields as "const char *", just like SPI
and VDPA. (Mark)
2. Move "count" check to the new helper and add "count" argument. (Michael)
3. Fix typos in docs, patch subject. Extend doc. (Michael, Bjorn)
4. Compare pointers to reduce number of string readings in the helper.
5. Fix clk-imx return value.
Changes since latest v1 (not the old 2019 solution):
====================================================
https://lore.kernel.org/all/[email protected]/
1. Add helper for setting driver_override.
2. Use the helper.
Dependencies (and stable):
==========================
1. All patches, including last three fixes, depend on the first patch
introducing the helper.
2. The last three commits - fixes - are probably not backportable
directly, because of this dependency. I don't know how to express
this dependency here, since stable-kernel-rules.rst mentions only commits as
possible dependencies.
[1] https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof
Krzysztof Kozlowski (11):
driver: platform: Add helper for safer setting of driver_override
amba: Use driver_set_override() instead of open-coding
fsl-mc: Use driver_set_override() instead of open-coding
hv: Use driver_set_override() instead of open-coding
PCI: Use driver_set_override() instead of open-coding
s390/cio: Use driver_set_override() instead of open-coding
spi: Use helper for safer setting of driver_override
vdpa: Use helper for safer setting of driver_override
clk: imx: scu: Fix kfree() of static memory on setting driver_override
slimbus: qcom-ngd: Fix kfree() of static memory on setting
driver_override
rpmsg: Fix kfree() of static memory on setting driver_override
drivers/amba/bus.c | 28 +++--------------
drivers/base/driver.c | 56 +++++++++++++++++++++++++++++++++
drivers/base/platform.c | 28 +++--------------
drivers/bus/fsl-mc/fsl-mc-bus.c | 25 +++------------
drivers/clk/imx/clk-scu.c | 7 ++++-
drivers/hv/vmbus_drv.c | 28 +++--------------
drivers/pci/pci-sysfs.c | 28 +++--------------
drivers/rpmsg/rpmsg_core.c | 3 +-
drivers/rpmsg/rpmsg_internal.h | 11 +++++--
drivers/rpmsg/rpmsg_ns.c | 14 +++++++--
drivers/s390/cio/cio.h | 6 +++-
drivers/s390/cio/css.c | 28 +++--------------
drivers/slimbus/qcom-ngd-ctrl.c | 13 +++++++-
drivers/spi/spi.c | 26 +++------------
drivers/vdpa/vdpa.c | 29 +++--------------
include/linux/amba/bus.h | 6 +++-
include/linux/device/driver.h | 2 ++
include/linux/fsl/mc.h | 6 ++--
include/linux/hyperv.h | 6 +++-
include/linux/pci.h | 6 +++-
include/linux/platform_device.h | 6 +++-
include/linux/rpmsg.h | 6 ++--
include/linux/spi/spi.h | 2 ++
include/linux/vdpa.h | 4 ++-
24 files changed, 169 insertions(+), 205 deletions(-)
--
2.32.0
The driver_override field from platform driver should not be initialized
from static memory (string literal) because the core later kfree() it,
for example when driver_override is set via sysfs.
Use dedicated helper to set driver_override properly.
Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
Cc: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
drivers/rpmsg/rpmsg_core.c | 3 ++-
drivers/rpmsg/rpmsg_internal.h | 11 +++++++++--
drivers/rpmsg/rpmsg_ns.c | 14 ++++++++++++--
include/linux/rpmsg.h | 6 ++++--
4 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 79368a957d89..95fc283f6af7 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -400,7 +400,8 @@ field##_store(struct device *dev, struct device_attribute *attr, \
const char *buf, size_t sz) \
{ \
struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
- char *new, *old; \
+ const char *old; \
+ char *new; \
\
new = kstrndup(buf, sz, GFP_KERNEL); \
if (!new) \
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index d4b23fd019a8..dd1f4ed616b6 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -95,9 +95,16 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
{
strcpy(rpdev->id.name, "rpmsg_ctrl");
- rpdev->driver_override = "rpmsg_ctrl";
+ ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+ "rpmsg_ctrl", strlen("rpmsg_ctrl"));
+ if (ret)
+ return ret;
- return rpmsg_register_device(rpdev);
+ ret = rpmsg_register_device(rpdev);
+ if (ret)
+ kfree(rpdev->driver_override);
+
+ return ret;
}
#endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 762ff1ae279f..95a51543f5ad 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -20,12 +20,22 @@
*/
int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
{
+ int ret;
+
strcpy(rpdev->id.name, "rpmsg_ns");
- rpdev->driver_override = "rpmsg_ns";
+ ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+ "rpmsg_ns", strlen("rpmsg_ns"));
+ if (ret)
+ return ret;
+
rpdev->src = RPMSG_NS_ADDR;
rpdev->dst = RPMSG_NS_ADDR;
- return rpmsg_register_device(rpdev);
+ ret = rpmsg_register_device(rpdev);
+ if (ret)
+ kfree(rpdev->driver_override);
+
+ return ret;
}
EXPORT_SYMBOL(rpmsg_ns_register_device);
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 02fa9116cd60..20c8cd1cde21 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -41,7 +41,9 @@ struct rpmsg_channel_info {
* rpmsg_device - device that belong to the rpmsg bus
* @dev: the device struct
* @id: device id (used to match between rpmsg drivers and devices)
- * @driver_override: driver name to force a match
+ * @driver_override: driver name to force a match; do not set directly,
+ * because core frees it; use driver_set_override() to
+ * set or clear it.
* @src: local address
* @dst: destination address
* @ept: the rpmsg endpoint of this channel
@@ -51,7 +53,7 @@ struct rpmsg_channel_info {
struct rpmsg_device {
struct device dev;
struct rpmsg_device_id id;
- char *driver_override;
+ const char *driver_override;
u32 src;
u32 dst;
struct rpmsg_endpoint *ept;
--
2.32.0
Use a helper to set driver_override to the reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
---
drivers/hv/vmbus_drv.c | 28 ++++------------------------
include/linux/hyperv.h | 6 +++++-
2 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 60ee8b329f9e..66213ce5579d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -575,31 +575,11 @@ static ssize_t driver_override_store(struct device *dev,
const char *buf, size_t count)
{
struct hv_device *hv_dev = device_to_hv_device(dev);
- char *driver_override, *old, *cp;
-
- /* We need to keep extra room for a newline */
- if (count >= (PAGE_SIZE - 1))
- return -EINVAL;
-
- driver_override = kstrndup(buf, count, GFP_KERNEL);
- if (!driver_override)
- return -ENOMEM;
-
- cp = strchr(driver_override, '\n');
- if (cp)
- *cp = '\0';
-
- device_lock(dev);
- old = hv_dev->driver_override;
- if (strlen(driver_override)) {
- hv_dev->driver_override = driver_override;
- } else {
- kfree(driver_override);
- hv_dev->driver_override = NULL;
- }
- device_unlock(dev);
+ int ret;
- kfree(old);
+ ret = driver_set_override(dev, &hv_dev->driver_override, buf, count);
+ if (ret)
+ return ret;
return count;
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index fe2e0179ed51..12e2336b23b7 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1257,7 +1257,11 @@ struct hv_device {
u16 device_id;
struct device device;
- char *driver_override; /* Driver name to force a match */
+ /*
+ * Driver name to force a match. Do not set directly, because core
+ * frees it. Use driver_set_override() to set or clear it.
+ */
+ const char *driver_override;
struct vmbus_channel *channel;
struct kset *channels_kset;
--
2.32.0
The driver_override field from platform driver should not be initialized
from static memory (string literal) because the core later kfree() it,
for example when driver_override is set via sysfs.
Use dedicated helper to set driver_override properly.
Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
Cc: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
---
drivers/clk/imx/clk-scu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index 083da31dc3ea..4b2268b7d0d0 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -683,7 +683,12 @@ struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
return ERR_PTR(ret);
}
- pdev->driver_override = "imx-scu-clk";
+ ret = driver_set_override(&pdev->dev, &pdev->driver_override,
+ "imx-scu-clk", strlen("imx-scu-clk"));
+ if (ret) {
+ platform_device_put(pdev);
+ return ERR_PTR(ret);
+ }
ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
if (ret)
--
2.32.0
Use a helper to set driver_override to the reduce amount of duplicated
code. Make the driver_override field const char, because it is not
modified by the core and it matches other subsystems.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/pci/pci-sysfs.c | 28 ++++------------------------
include/linux/pci.h | 6 +++++-
2 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 602f0fb0b007..5c42965c32c2 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -567,31 +567,11 @@ static ssize_t driver_override_store(struct device *dev,
const char *buf, size_t count)
{
struct pci_dev *pdev = to_pci_dev(dev);
- char *driver_override, *old, *cp;
-
- /* We need to keep extra room for a newline */
- if (count >= (PAGE_SIZE - 1))
- return -EINVAL;
-
- driver_override = kstrndup(buf, count, GFP_KERNEL);
- if (!driver_override)
- return -ENOMEM;
-
- cp = strchr(driver_override, '\n');
- if (cp)
- *cp = '\0';
-
- device_lock(dev);
- old = pdev->driver_override;
- if (strlen(driver_override)) {
- pdev->driver_override = driver_override;
- } else {
- kfree(driver_override);
- pdev->driver_override = NULL;
- }
- device_unlock(dev);
+ int ret;
- kfree(old);
+ ret = driver_set_override(dev, &pdev->driver_override, buf, count);
+ if (ret)
+ return ret;
return count;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b957eeb89c7a..5ecbb845aa21 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -516,7 +516,11 @@ struct pci_dev {
u16 acs_cap; /* ACS Capability offset */
phys_addr_t rom; /* Physical address if not from BAR */
size_t romlen; /* Length if not from BAR */
- char *driver_override; /* Driver name to force a match */
+ /*
+ * Driver name to force a match. Do not set directly, because core
+ * frees it. Use driver_set_override() to set or clear it.
+ */
+ const char *driver_override;
unsigned long priv_flags; /* Private flags for the PCI driver */
--
2.32.0
On 16/03/2022 16:08, Krzysztof Kozlowski wrote:
> The driver_override field from platform driver should not be initialized
> from static memory (string literal) because the core later kfree() it,
> for example when driver_override is set via sysfs.
>
> Use dedicated helper to set driver_override properly.
>
> Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Cc: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> drivers/rpmsg/rpmsg_core.c | 3 ++-
> drivers/rpmsg/rpmsg_internal.h | 11 +++++++++--
> drivers/rpmsg/rpmsg_ns.c | 14 ++++++++++++--
> include/linux/rpmsg.h | 6 ++++--
> 4 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 79368a957d89..95fc283f6af7 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -400,7 +400,8 @@ field##_store(struct device *dev, struct device_attribute *attr, \
> const char *buf, size_t sz) \
> { \
> struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> - char *new, *old; \
> + const char *old; \
> + char *new; \
> \
> new = kstrndup(buf, sz, GFP_KERNEL); \
> if (!new) \
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index d4b23fd019a8..dd1f4ed616b6 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -95,9 +95,16 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
> static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
> {
> strcpy(rpdev->id.name, "rpmsg_ctrl");
> - rpdev->driver_override = "rpmsg_ctrl";
> + ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> + "rpmsg_ctrl", strlen("rpmsg_ctrl"));
I made here a mistake while rebasing. This will need a v6.
Best regards,
Krzysztof