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 of latest since 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 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
it 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 and use helper for safer setting of
driver_override
amba: use helper for safer setting of driver_override
fsl-mc: use helper for safer setting of driver_override
hv: vmbus: use helper for safer setting of driver_override
pci: use helper for safer setting of driver_override
s390: cio: use helper for safer setting of driver_override
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 | 24 +++---------------
drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++
drivers/base/platform.c | 24 +++---------------
drivers/bus/fsl-mc/fsl-mc-bus.c | 22 +++--------------
drivers/clk/imx/clk-scu.c | 7 +++++-
drivers/hv/vmbus_drv.c | 24 +++---------------
drivers/pci/pci-sysfs.c | 24 +++---------------
drivers/rpmsg/rpmsg_internal.h | 13 ++++++++--
drivers/rpmsg/rpmsg_ns.c | 14 +++++++++--
drivers/s390/cio/css.c | 24 +++---------------
drivers/slimbus/qcom-ngd-ctrl.c | 12 ++++++++-
drivers/spi/spi.c | 20 +++------------
drivers/vdpa/vdpa.c | 25 +++----------------
include/linux/device/driver.h | 1 +
include/linux/platform_device.h | 6 ++++-
include/linux/spi/spi.h | 2 +-
16 files changed, 123 insertions(+), 163 deletions(-)
--
2.32.0
Use a helper for seting driver_override to reduce amount of duplicated
code.
Remove also "const" from the definition of spi_device.driver_override,
because it is not correct. The SPI driver already treats it as
dynamic, not const, memory.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/spi/spi.c | 20 ++++----------------
include/linux/spi/spi.h | 2 +-
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4599b121d744..0c7e2c34f4a3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -74,27 +74,15 @@ static ssize_t driver_override_store(struct device *dev,
struct spi_device *spi = to_spi_device(dev);
const char *end = memchr(buf, '\n', count);
const size_t len = end ? end - buf : count;
- const char *driver_override, *old;
+ int ret;
/* We need to keep extra room for a newline when displaying value */
if (len >= (PAGE_SIZE - 1))
return -EINVAL;
- driver_override = kstrndup(buf, len, GFP_KERNEL);
- if (!driver_override)
- return -ENOMEM;
-
- device_lock(dev);
- old = spi->driver_override;
- if (len) {
- spi->driver_override = driver_override;
- } else {
- /* Empty string, disable driver override */
- spi->driver_override = NULL;
- kfree(driver_override);
- }
- device_unlock(dev);
- kfree(old);
+ ret = driver_set_override(dev, &spi->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 7ab3fed7b804..01224d07aaff 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -184,7 +184,7 @@ struct spi_device {
void *controller_state;
void *controller_data;
char modalias[SPI_NAME_SIZE];
- const char *driver_override;
+ char *driver_override;
int cs_gpio; /* LEGACY: chip select gpio */
struct gpio_desc *cs_gpiod; /* chip select gpio desc */
struct spi_delay word_delay; /* inter-word delay */
--
2.32.0
Use a helper for seting driver_override to reduce amount of duplicated
code.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 8fd4a356a86e..d93f4f680f82 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -166,8 +166,7 @@ static ssize_t driver_override_store(struct device *dev,
const char *buf, size_t count)
{
struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
- char *driver_override, *old = mc_dev->driver_override;
- char *cp;
+ int ret;
if (WARN_ON(dev->bus != &fsl_mc_bus_type))
return -EINVAL;
@@ -175,22 +174,9 @@ static ssize_t driver_override_store(struct device *dev,
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';
-
- if (strlen(driver_override)) {
- mc_dev->driver_override = driver_override;
- } else {
- kfree(driver_override);
- mc_dev->driver_override = NULL;
- }
-
- kfree(old);
+ ret = driver_set_override(dev, &mc_dev->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
--
2.32.0
Use a helper for seting driver_override to reduce amount of duplicated
code.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/s390/cio/css.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index fa8293335077..2ced49be1912 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -338,31 +338,15 @@ static ssize_t driver_override_store(struct device *dev,
const char *buf, size_t count)
{
struct subchannel *sch = to_subchannel(dev);
- char *driver_override, *old, *cp;
+ int ret;
/* 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 = sch->driver_override;
- if (strlen(driver_override)) {
- sch->driver_override = driver_override;
- } else {
- kfree(driver_override);
- sch->driver_override = NULL;
- }
- device_unlock(dev);
-
- kfree(old);
+ ret = driver_set_override(dev, &dev->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
--
2.32.0
Use a helper for seting driver_override to reduce amount of duplicated
code.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/pci/pci-sysfs.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 602f0fb0b007..16a163d4623e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -567,31 +567,15 @@ 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;
+ int ret;
/* 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);
-
- kfree(old);
+ ret = driver_set_override(dev, &pdev->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
--
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]>
---
drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
drivers/rpmsg/rpmsg_ns.c | 14 ++++++++++++--
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index b1245d3ed7c6..c7bd0a3c802d 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -92,10 +92,19 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
*/
static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
{
+ int ret;
+
strcpy(rpdev->id.name, "rpmsg_chrdev");
- rpdev->driver_override = "rpmsg_chrdev";
+ ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+ "rpmsg_chrdev");
+ if (ret)
+ return ret;
+
+ ret = rpmsg_register_device(rpdev);
+ if (ret)
+ kfree(rpdev->driver_override);
- return rpmsg_register_device(rpdev);
+ return ret;
}
#endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 762ff1ae279f..1c9f9cf065b0 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");
+ 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);
--
2.32.0
Several core drivers and buses expect that driver_override is a
dynamically allocated memory thus later they can kfree() it.
However such assumption is not documented, there were in the past and
there are already users setting it to a string literal. This leads to
kfree() of static memory during device release (e.g. in error paths or
during unbind):
kernel BUG at ../mm/slub.c:3960!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
...
(kfree) from [<c058da50>] (platform_device_release+0x88/0xb4)
(platform_device_release) from [<c0585be0>] (device_release+0x2c/0x90)
(device_release) from [<c0a69050>] (kobject_put+0xec/0x20c)
(kobject_put) from [<c0f2f120>] (exynos5_clk_probe+0x154/0x18c)
(exynos5_clk_probe) from [<c058de70>] (platform_drv_probe+0x6c/0xa4)
(platform_drv_probe) from [<c058b7ac>] (really_probe+0x280/0x414)
(really_probe) from [<c058baf4>] (driver_probe_device+0x78/0x1c4)
(driver_probe_device) from [<c0589854>] (bus_for_each_drv+0x74/0xb8)
(bus_for_each_drv) from [<c058b48c>] (__device_attach+0xd4/0x16c)
(__device_attach) from [<c058a638>] (bus_probe_device+0x88/0x90)
(bus_probe_device) from [<c05871fc>] (device_add+0x3dc/0x62c)
(device_add) from [<c075ff10>] (of_platform_device_create_pdata+0x94/0xbc)
(of_platform_device_create_pdata) from [<c07600ec>] (of_platform_bus_create+0x1a8/0x4fc)
(of_platform_bus_create) from [<c0760150>] (of_platform_bus_create+0x20c/0x4fc)
(of_platform_bus_create) from [<c07605f0>] (of_platform_populate+0x84/0x118)
(of_platform_populate) from [<c0f3c964>] (of_platform_default_populate_init+0xa0/0xb8)
(of_platform_default_populate_init) from [<c01031f8>] (do_one_initcall+0x8c/0x404)
(do_one_initcall) from [<c0f012c0>] (kernel_init_freeable+0x3d0/0x4d8)
(kernel_init_freeable) from [<c0a7def0>] (kernel_init+0x8/0x114)
(kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Provide a helper which clearly documents the usage of driver_override.
This will allow later to reuse the helper and reduce amount of
duplicated code.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++
drivers/base/platform.c | 24 +++---------------
include/linux/device/driver.h | 1 +
include/linux/platform_device.h | 6 ++++-
4 files changed, 54 insertions(+), 21 deletions(-)
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 8c0d33e182fd..79efe51bb4c0 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -30,6 +30,50 @@ static struct device *next_device(struct klist_iter *i)
return dev;
}
+/*
+ * set_driver_override() - Helper to set or clear driver override.
+ * @dev: Device to change
+ * @override: Address of string to change (e.g. &device->driver_override);
+ * The contents will be freed and hold newly allocated override.
+ * @s: NULL terminated string, new driver name to force a match, pass empty
+ * string to clear it
+ *
+ * Helper to setr or clear driver override in a device, intended for the cases
+ * when the driver_override field is allocated by driver/bus code.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int driver_set_override(struct device *dev, char **override, const char *s)
+{
+ char *new, *old, *cp;
+
+ if (!dev || !override || !s)
+ return -EINVAL;
+
+ new = kstrndup(s, strlen(s), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ cp = strchr(new, '\n');
+ if (cp)
+ *cp = '\0';
+
+ device_lock(dev);
+ old = *override;
+ if (strlen(new)) {
+ *override = new;
+ } else {
+ kfree(new);
+ *override = NULL;
+ }
+ device_unlock(dev);
+
+ kfree(old);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(driver_set_override);
+
/**
* driver_for_each_device - Iterator for devices bound to a driver.
* @drv: Driver we're iterating.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6cb04ac48bf0..d8853b32ea10 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1275,31 +1275,15 @@ static ssize_t driver_override_store(struct device *dev,
const char *buf, size_t count)
{
struct platform_device *pdev = to_platform_device(dev);
- char *driver_override, *old, *cp;
+ int ret;
/* 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);
-
- kfree(old);
+ ret = driver_set_override(dev, &pdev->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 15e7c5e15d62..81c0d9f65a40 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver,
extern void driver_remove_file(struct device_driver *driver,
const struct driver_attribute *attr);
+int driver_set_override(struct device *dev, char **override, const char *s);
extern int __must_check driver_for_each_device(struct device_driver *drv,
struct device *start,
void *data,
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 7c96f169d274..37ac14459499 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -31,7 +31,11 @@ struct platform_device {
struct resource *resource;
const struct platform_device_id *id_entry;
- char *driver_override; /* Driver name to force a match */
+ /*
+ * Driver name to force a match, use
+ * driver_set_override() to set or clear it.
+ */
+ char *driver_override;
/* MFD cell pointer */
struct mfd_cell *mfd_cell;
--
2.32.0
On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote:
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
> ...
> + * set_driver_override() - Helper to set or clear driver override.
Doesn't match actual function name.
> + * @dev: Device to change
> + * @override: Address of string to change (e.g. &device->driver_override);
> + * The contents will be freed and hold newly allocated override.
> + * @s: NULL terminated string, new driver name to force a match, pass empty
> + * string to clear it
> + *
> + * Helper to setr or clear driver override in a device, intended for the cases
> + * when the driver_override field is allocated by driver/bus code.
s/setr/set/
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int driver_set_override(struct device *dev, char **override, const char *s)
> +{
Use a helper for seting driver_override to reduce amount of duplicated
code.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/amba/bus.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index e1a5eca3ae3c..12410c05ec70 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -94,31 +94,15 @@ static ssize_t driver_override_store(struct device *_dev,
const char *buf, size_t count)
{
struct amba_device *dev = to_amba_device(_dev);
- char *driver_override, *old, *cp;
+ int ret;
/* 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 = dev->driver_override;
- if (strlen(driver_override)) {
- dev->driver_override = driver_override;
- } else {
- kfree(driver_override);
- dev->driver_override = NULL;
- }
- device_unlock(_dev);
-
- kfree(old);
+ ret = driver_set_override(_dev, &dev->driver_override, buf);
+ if (ret)
+ return ret;
return count;
}
--
2.32.0
On Wed, Feb 23, 2022 at 08:14:37PM +0100, Krzysztof Kozlowski wrote:
> Remove also "const" from the definition of spi_device.driver_override,
> because it is not correct. The SPI driver already treats it as
> dynamic, not const, memory.
We don't modify the string do we, we just allocate a new one?
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: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
Cc: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/slimbus/qcom-ngd-ctrl.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
index 7040293c2ee8..e1a8de4d41fb 100644
--- a/drivers/slimbus/qcom-ngd-ctrl.c
+++ b/drivers/slimbus/qcom-ngd-ctrl.c
@@ -1434,6 +1434,7 @@ static int of_qcom_slim_ngd_register(struct device *parent,
const struct of_device_id *match;
struct device_node *node;
u32 id;
+ int ret;
match = of_match_node(qcom_slim_ngd_dt_match, parent->of_node);
data = match->data;
@@ -1455,7 +1456,16 @@ static int of_qcom_slim_ngd_register(struct device *parent,
}
ngd->id = id;
ngd->pdev->dev.parent = parent;
- ngd->pdev->driver_override = QCOM_SLIM_NGD_DRV_NAME;
+
+ ret = driver_set_override(&ngd->pdev->dev,
+ &ngd->pdev->driver_override,
+ QCOM_SLIM_NGD_DRV_NAME);
+ if (ret) {
+ platform_device_put(ngd->pdev);
+ kfree(ngd);
+ of_node_put(node);
+ return ret;
+ }
ngd->pdev->dev.of_node = node;
ctrl->ngd = ngd;
--
2.32.0
On 23/02/2022 21:04, Mark Brown wrote:
> On Wed, Feb 23, 2022 at 08:14:37PM +0100, Krzysztof Kozlowski wrote:
>
>> Remove also "const" from the definition of spi_device.driver_override,
>> because it is not correct. The SPI driver already treats it as
>> dynamic, not const, memory.
>
> We don't modify the string do we, we just allocate a new one?
Actually you're right - the SPI and VDPA implementations operate on
"const char *". The others do not, so I can convert them to "const char
*". Thanks!
Best regards,
Krzysztof
On 23/02/2022 22:53, Bjorn Helgaas wrote:
> On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote:
>> Several core drivers and buses expect that driver_override is a
>> dynamically allocated memory thus later they can kfree() it.
>> ...
>
>> + * set_driver_override() - Helper to set or clear driver override.
>
> Doesn't match actual function name.
Good point. I wonder why build W=1 did not complain... I need to check.
>
>> + * @dev: Device to change
>> + * @override: Address of string to change (e.g. &device->driver_override);
>> + * The contents will be freed and hold newly allocated override.
>> + * @s: NULL terminated string, new driver name to force a match, pass empty
>> + * string to clear it
>> + *
>> + * Helper to setr or clear driver override in a device, intended for the cases
>> + * when the driver_override field is allocated by driver/bus code.
>
> s/setr/set/
Right. Thanks for checking.
>
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int driver_set_override(struct device *dev, char **override, const char *s)
>> +{
Best regards,
Krzysztof
On 24/02/2022 08:47, Krzysztof Kozlowski wrote:
> On 23/02/2022 22:53, Bjorn Helgaas wrote:
>> On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote:
>>> Several core drivers and buses expect that driver_override is a
>>> dynamically allocated memory thus later they can kfree() it.
>>> ...
>>
>>> + * set_driver_override() - Helper to set or clear driver override.
>>
>> Doesn't match actual function name.
>
> Good point. I wonder why build W=1 did not complain... I need to check.
>
I see why - I missed kerneldoc /** opener.
Best regards,
Krzysztof
On 25/02/2022 00:52, Bjorn Helgaas wrote:
> On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote:
>> On 23/02/2022 22:51, Bjorn Helgaas wrote:
>>> In subject, to match drivers/pci/ convention, do something like:
>>>
>>> PCI: Use driver_set_override() instead of open-coding
>>>
>>> On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote:
>>>> Use a helper for seting driver_override to reduce amount of duplicated
>>>> code.
>>>> @@ -567,31 +567,15 @@ 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;
>>>> + int ret;
>>>>
>>>> /* We need to keep extra room for a newline */
>>>> if (count >= (PAGE_SIZE - 1))
>>>> return -EINVAL;
>>>
>>> This check makes no sense in the new function. Michael alluded to
>>> this as well.
>>
>> I am not sure if I got your comment properly. You mean here:
>> 1. Move this check to driver_set_override()?
>> 2. Remove the check entirely?
>
> I was mistaken about the purpose of the comment and the check. I
> thought it had to do with *this* function, and this function doesn't
> add a newline, and there's no obvious connection with PAGE_SIZE.
>
> But looking closer, I think the "extra room for a newline" is really
> to make sure that *driver_override_show()* can add a newline and have
> it still fit within the PAGE_SIZE sysfs limit.
>
> Most driver_override_*() functions have the same comment, so maybe
> this was obvious to everybody except me :) I do see that spi.c adds
> "when displaying value" at the end, which helps a lot.
>
> Sorry for the wild goose chase.
I think I will move this check anyway to driver_set_override() helper,
because there is no particular benefit to have duplicated all over. The
helper will receive "count" argument so can perform all checks.
Best regards,
Krzysztof
On Fri, Feb 25, 2022 at 10:36:20AM +0100, Krzysztof Kozlowski wrote:
> On 25/02/2022 00:52, Bjorn Helgaas wrote:
> > On Thu, Feb 24, 2022 at 08:49:15AM +0100, Krzysztof Kozlowski wrote:
> >> On 23/02/2022 22:51, Bjorn Helgaas wrote:
> >>> In subject, to match drivers/pci/ convention, do something like:
> >>>
> >>> PCI: Use driver_set_override() instead of open-coding
> >>>
> >>> On Wed, Feb 23, 2022 at 08:13:04PM +0100, Krzysztof Kozlowski wrote:
> >>>> Use a helper for seting driver_override to reduce amount of duplicated
> >>>> code.
> >>>> @@ -567,31 +567,15 @@ 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;
> >>>> + int ret;
> >>>>
> >>>> /* We need to keep extra room for a newline */
> >>>> if (count >= (PAGE_SIZE - 1))
> >>>> return -EINVAL;
> >>>
> >>> This check makes no sense in the new function. Michael alluded to
> >>> this as well.
> >>
> >> I am not sure if I got your comment properly. You mean here:
> >> 1. Move this check to driver_set_override()?
> >> 2. Remove the check entirely?
> >
> > I was mistaken about the purpose of the comment and the check. I
> > thought it had to do with *this* function, and this function doesn't
> > add a newline, and there's no obvious connection with PAGE_SIZE.
> >
> > But looking closer, I think the "extra room for a newline" is really
> > to make sure that *driver_override_show()* can add a newline and have
> > it still fit within the PAGE_SIZE sysfs limit.
> >
> > Most driver_override_*() functions have the same comment, so maybe
> > this was obvious to everybody except me :) I do see that spi.c adds
> > "when displaying value" at the end, which helps a lot.
> >
> > Sorry for the wild goose chase.
>
> I think I will move this check anyway to driver_set_override() helper,
> because there is no particular benefit to have duplicated all over. The
> helper will receive "count" argument so can perform all checks.
Thanks, I think that would be good!
Bjorn