Commit 723de0f9171e ("staging: most: remove device from interface
structure") moved registration of driver-provided struct device to
the most subsystem, but did not properly update dim2 driver to
work with that change.
After most subsystem passes driver's dev to register_device(), it
becomes refcounted, and can be only deallocated in the release method.
Provide that by:
- not using devres to allocate the device,
- moving shutdown actions from _remove() to the device release method,
- not calling shutdown actions in _probe() after the device becomes
refcounted.
Also, driver used to register it's dev itself, to provide a custom
attribute. With the modified most subsystem, this causes duplicate
registration of the same device object. Fix that by adding that custom
attribute to the platform device - that is a better location for
a platform-specific attribute anyway.
Fixes: 723de0f9171e ("staging: most: remove device from interface structure")
Signed-off-by: Nikita Yushchenko <[email protected]>
---
drivers/staging/most/dim2/dim2.c | 60 ++++++++++++++++++-------------
drivers/staging/most/dim2/sysfs.c | 5 ++-
2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index e8b03fa90e80..7ef142b9faef 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -717,6 +717,23 @@ static int get_dim2_clk_speed(const char *clock_speed, u8 *val)
return -EINVAL;
}
+static void dim2_release(struct device *d)
+{
+ struct dim2_hdm *dev = container_of(d, struct dim2_hdm, dev);
+ unsigned long flags;
+
+ kthread_stop(dev->netinfo_task);
+
+ spin_lock_irqsave(&dim_lock, flags);
+ dim_shutdown();
+ spin_unlock_irqrestore(&dim_lock, flags);
+
+ if (dev->disable_platform)
+ dev->disable_platform(to_platform_device(d->parent));
+
+ kfree(dev);
+}
+
/*
* dim2_probe - dim2 probe handler
* @pdev: platform device structure
@@ -738,7 +755,7 @@ static int dim2_probe(struct platform_device *pdev)
enum { MLB_INT_IDX, AHB0_INT_IDX };
- dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
@@ -750,19 +767,21 @@ static int dim2_probe(struct platform_device *pdev)
"microchip,clock-speed", &clock_speed);
if (ret) {
dev_err(&pdev->dev, "missing dt property clock-speed\n");
- return ret;
+ goto err_free_dev;
}
ret = get_dim2_clk_speed(clock_speed, &dev->clk_speed);
if (ret) {
dev_err(&pdev->dev, "bad dt property clock-speed\n");
- return ret;
+ goto err_free_dev;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->io_base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dev->io_base))
- return PTR_ERR(dev->io_base);
+ if (IS_ERR(dev->io_base)) {
+ ret = PTR_ERR(dev->io_base);
+ goto err_free_dev;
+ }
of_id = of_match_node(dim2_of_match, pdev->dev.of_node);
pdata = of_id->data;
@@ -770,7 +789,7 @@ static int dim2_probe(struct platform_device *pdev)
if (pdata->enable) {
ret = pdata->enable(pdev);
if (ret)
- return ret;
+ goto err_free_dev;
}
dev->disable_platform = pdata->disable;
if (pdata->fcnt)
@@ -865,32 +884,34 @@ static int dim2_probe(struct platform_device *pdev)
dev->most_iface.request_netinfo = request_netinfo;
dev->most_iface.driver_dev = &pdev->dev;
dev->most_iface.dev = &dev->dev;
- dev->dev.init_name = "dim2_state";
+ dev->dev.init_name = dev->name;
dev->dev.parent = &pdev->dev;
+ dev->dev.release = dim2_release;
ret = most_register_interface(&dev->most_iface);
if (ret) {
dev_err(&pdev->dev, "failed to register MOST interface\n");
- goto err_stop_thread;
+ /* cleanup handled by dim2_release() */
+ return ret;
}
- ret = dim2_sysfs_probe(&dev->dev);
+ ret = dim2_sysfs_probe(&pdev->dev);
if (ret) {
dev_err(&pdev->dev, "failed to create sysfs attribute\n");
- goto err_unreg_iface;
+ most_deregister_interface(&dev->most_iface);
+ /* cleanup handled by dim2_release() */
+ return ret;
}
return 0;
-err_unreg_iface:
- most_deregister_interface(&dev->most_iface);
-err_stop_thread:
- kthread_stop(dev->netinfo_task);
err_shutdown_dim:
dim_shutdown();
err_disable_platform:
if (dev->disable_platform)
dev->disable_platform(pdev);
+err_free_dev:
+ kfree(dev);
return ret;
}
@@ -904,18 +925,9 @@ static int dim2_probe(struct platform_device *pdev)
static int dim2_remove(struct platform_device *pdev)
{
struct dim2_hdm *dev = platform_get_drvdata(pdev);
- unsigned long flags;
- dim2_sysfs_destroy(&dev->dev);
+ dim2_sysfs_destroy(&pdev->dev);
most_deregister_interface(&dev->most_iface);
- kthread_stop(dev->netinfo_task);
-
- spin_lock_irqsave(&dim_lock, flags);
- dim_shutdown();
- spin_unlock_irqrestore(&dim_lock, flags);
-
- if (dev->disable_platform)
- dev->disable_platform(pdev);
return 0;
}
diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
index c85b2cdcdca3..22836c8ed554 100644
--- a/drivers/staging/most/dim2/sysfs.c
+++ b/drivers/staging/most/dim2/sysfs.c
@@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
int dim2_sysfs_probe(struct device *dev)
{
- dev->groups = dev_attr_groups;
- return device_register(dev);
+ return sysfs_create_groups(&dev->kobj, dev_attr_groups);
}
void dim2_sysfs_destroy(struct device *dev)
{
- device_unregister(dev);
+ sysfs_remove_groups(&dev->kobj, dev_attr_groups);
}
--
2.30.2
On Wed, Sep 29, 2021 at 11:56:20PM +0300, Nikita Yushchenko wrote:
> Commit 723de0f9171e ("staging: most: remove device from interface
> structure") moved registration of driver-provided struct device to
> the most subsystem, but did not properly update dim2 driver to
> work with that change.
>
> After most subsystem passes driver's dev to register_device(), it
> becomes refcounted, and can be only deallocated in the release method.
> Provide that by:
> - not using devres to allocate the device,
> - moving shutdown actions from _remove() to the device release method,
> - not calling shutdown actions in _probe() after the device becomes
> refcounted.
Should this be 3 patches?
> Also, driver used to register it's dev itself, to provide a custom
> attribute. With the modified most subsystem, this causes duplicate
> registration of the same device object. Fix that by adding that custom
> attribute to the platform device - that is a better location for
> a platform-specific attribute anyway.
Nope, it should be 4 patches. Whenever you have to list a bunch of
different things you are doing in a single change, that's a hint that
this should be more than one patch.
Also, why have you not cc:ed the original author of the commit you are
"fixing" here? They are the maintainer of this code, right?
One note on your change that would keep me from accepting it even if all
of the above was not an issue:
> diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
> index c85b2cdcdca3..22836c8ed554 100644
> --- a/drivers/staging/most/dim2/sysfs.c
> +++ b/drivers/staging/most/dim2/sysfs.c
> @@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
>
> int dim2_sysfs_probe(struct device *dev)
> {
> - dev->groups = dev_attr_groups;
> - return device_register(dev);
> + return sysfs_create_groups(&dev->kobj, dev_attr_groups);
No driver code should ever be calling a sysfs_* function, which is a
huge hint that this is incorrect.
You also just raced with userspace and lost, please use the default
attributes for the driver or bus for this, but NEVER manually add and
remove sysfs files, that way lies madness and hard to maintain code.
thanks,
greg k-h
>> Commit 723de0f9171e ("staging: most: remove device from interface
>> structure") moved registration of driver-provided struct device to
>> the most subsystem, but did not properly update dim2 driver to
>> work with that change.
>>
>> After most subsystem passes driver's dev to register_device(), it
>> becomes refcounted, and can be only deallocated in the release method.
>> Provide that by:
>> - not using devres to allocate the device,
>> - moving shutdown actions from _remove() to the device release method,
>> - not calling shutdown actions in _probe() after the device becomes
>> refcounted.
>
> Should this be 3 patches?
But these three items are deeply interconnected, and fix the issue together. Must not manually free
device structure passed to register_device(), thus must not allocate via devres (because otherwise,
devres will free it). Once not using devres for it, must deallocate it somehow else, thus must rework
the release paths.
Perhaps I just shall not go into these details in the commit message.
>> Also, driver used to register it's dev itself, to provide a custom
>> attribute. With the modified most subsystem, this causes duplicate
>> registration of the same device object. Fix that by adding that custom
>> attribute to the platform device - that is a better location for
>> a platform-specific attribute anyway.
>
> Nope, it should be 4 patches.
Unlike the above three, this item could be separated.
Will split into two patches now - the first for this (and with fix to the attributes issue noted below)
and the second for proper device releasing.
> Also, why have you not cc:ed the original author of the commit you are
> "fixing" here? They are the maintainer of this code, right?
I was under impression that "git send-email" does that automatically...
CCing them now.
> One note on your change that would keep me from accepting it even if all
> of the above was not an issue:
>
>> diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
>> index c85b2cdcdca3..22836c8ed554 100644
>> --- a/drivers/staging/most/dim2/sysfs.c
>> +++ b/drivers/staging/most/dim2/sysfs.c
>> @@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
>>
>> int dim2_sysfs_probe(struct device *dev)
>> {
>> - dev->groups = dev_attr_groups;
>> - return device_register(dev);
>> + return sysfs_create_groups(&dev->kobj, dev_attr_groups);
>
> No driver code should ever be calling a sysfs_* function, which is a
> huge hint that this is incorrect.
>
> You also just raced with userspace and lost, please use the default
> attributes for the driver or bus for this, but NEVER manually add and
> remove sysfs files, that way lies madness and hard to maintain code.
I'm aware of this race, but still creating attributes on device probe is under wide use in the kernel:
nikita@cobook:~/kernel$ grep -r device_create_file drivers | wc -l
448
Still, in case of dim2 driver, moving to driver's dev_groups is trivial. Preparing that patch now.
Nikita
On Tue, Oct 05, 2021 at 04:33:02PM +0300, Nikita Yushchenko wrote:
> > > Commit 723de0f9171e ("staging: most: remove device from interface
> > > structure") moved registration of driver-provided struct device to
> > > the most subsystem, but did not properly update dim2 driver to
> > > work with that change.
> > >
> > > After most subsystem passes driver's dev to register_device(), it
> > > becomes refcounted, and can be only deallocated in the release method.
> > > Provide that by:
> > > - not using devres to allocate the device,
> > > - moving shutdown actions from _remove() to the device release method,
> > > - not calling shutdown actions in _probe() after the device becomes
> > > refcounted.
> >
> > Should this be 3 patches?
>
> But these three items are deeply interconnected, and fix the issue together.
> Must not manually free device structure passed to register_device(), thus
> must not allocate via devres (because otherwise, devres will free it). Once
> not using devres for it, must deallocate it somehow else, thus must rework
> the release paths.
Ok, but that was obvious.
> > > Also, driver used to register it's dev itself, to provide a custom
> > > attribute. With the modified most subsystem, this causes duplicate
> > > registration of the same device object. Fix that by adding that custom
> > > attribute to the platform device - that is a better location for
> > > a platform-specific attribute anyway.
> >
> > Nope, it should be 4 patches.
>
> Unlike the above three, this item could be separated.
> Will split into two patches now - the first for this (and with fix to the
> attributes issue noted below) and the second for proper device releasing.
>
> > Also, why have you not cc:ed the original author of the commit you are
> > "fixing" here? They are the maintainer of this code, right?
>
> I was under impression that "git send-email" does that automatically...
Nope.
> CCing them now.
They don't see the change :(
> > One note on your change that would keep me from accepting it even if all
> > of the above was not an issue:
> >
> > > diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
> > > index c85b2cdcdca3..22836c8ed554 100644
> > > --- a/drivers/staging/most/dim2/sysfs.c
> > > +++ b/drivers/staging/most/dim2/sysfs.c
> > > @@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
> > > int dim2_sysfs_probe(struct device *dev)
> > > {
> > > - dev->groups = dev_attr_groups;
> > > - return device_register(dev);
> > > + return sysfs_create_groups(&dev->kobj, dev_attr_groups);
> >
> > No driver code should ever be calling a sysfs_* function, which is a
> > huge hint that this is incorrect.
> >
> > You also just raced with userspace and lost, please use the default
> > attributes for the driver or bus for this, but NEVER manually add and
> > remove sysfs files, that way lies madness and hard to maintain code.
> I'm aware of this race, but still creating attributes on device probe is under wide use in the kernel:
>
> nikita@cobook:~/kernel$ grep -r device_create_file drivers | wc -l
> 448
Just because there are bad examples, does not mean you should add more.
And not all of these are wrong, but do you know when they are not wrong?
> Still, in case of dim2 driver, moving to driver's dev_groups is
> trivial. Preparing that patch now.
That should have already been done, right?
thanks,
greg k-h
On Tue, Oct 05, 2021 at 03:49:09PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 05, 2021 at 04:33:02PM +0300, Nikita Yushchenko wrote:
> > > > Commit 723de0f9171e ("staging: most: remove device from interface
> > > > structure") moved registration of driver-provided struct device to
> > > > the most subsystem, but did not properly update dim2 driver to
> > > > work with that change.
> > > >
> > > > After most subsystem passes driver's dev to register_device(), it
> > > > becomes refcounted, and can be only deallocated in the release method.
> > > > Provide that by:
> > > > - not using devres to allocate the device,
> > > > - moving shutdown actions from _remove() to the device release method,
> > > > - not calling shutdown actions in _probe() after the device becomes
> > > > refcounted.
> > >
> > > Should this be 3 patches?
> >
> > But these three items are deeply interconnected, and fix the issue together.
> > Must not manually free device structure passed to register_device(), thus
> > must not allocate via devres (because otherwise, devres will free it). Once
> > not using devres for it, must deallocate it somehow else, thus must rework
> > the release paths.
>
> Ok, but that was obvious.
That was *not* obvious.
On Tue, Oct 05, 2021 at 04:33:02PM +0300, Nikita Yushchenko wrote:
> > Also, why have you not cc:ed the original author of the commit you are
> > "fixing" here? They are the maintainer of this code, right?
>
> I was under impression that "git send-email" does that automatically...
>
The ./scripts/get_maintainer.pl does try to CC them. It's working for
me when I use the script on your patch. I normally put the author in
the To: header so it's more likely they'll see my patch and Ack it.
regards,
dan carpenter
Commit 723de0f9171e ("staging: most: remove device from interface
structure") moved registration of driver-provided struct device to
the most subsystem.
Dim2 used to register the same struct device to provide a custom device
attribute. This causes double-registration of the same struct device.
Fix that by moving the custom attribute to driver's dev_groups.
This moves attribute to the platform_device object, which is a better
location for platform-specific attributes anyway.
Fixes: 723de0f9171e ("staging: most: remove device from interface structure")
Signed-off-by: Nikita Yushchenko <[email protected]>
---
drivers/staging/most/dim2/Makefile | 2 +-
drivers/staging/most/dim2/dim2.c | 31 ++++++++++++-------
drivers/staging/most/dim2/sysfs.c | 49 ------------------------------
drivers/staging/most/dim2/sysfs.h | 11 -------
4 files changed, 21 insertions(+), 72 deletions(-)
delete mode 100644 drivers/staging/most/dim2/sysfs.c
diff --git a/drivers/staging/most/dim2/Makefile b/drivers/staging/most/dim2/Makefile
index 861adacf6c72..5f9612af3fa3 100644
--- a/drivers/staging/most/dim2/Makefile
+++ b/drivers/staging/most/dim2/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_MOST_DIM2) += most_dim2.o
-most_dim2-objs := dim2.o hal.o sysfs.o
+most_dim2-objs := dim2.o hal.o
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index e8b03fa90e80..bb6dd508e531 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -118,7 +118,8 @@ struct dim2_platform_data {
(((p)[1] == 0x18) && ((p)[2] == 0x05) && ((p)[3] == 0x0C) && \
((p)[13] == 0x3C) && ((p)[14] == 0x00) && ((p)[15] == 0x0A))
-bool dim2_sysfs_get_state_cb(void)
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
bool state;
unsigned long flags;
@@ -127,9 +128,25 @@ bool dim2_sysfs_get_state_cb(void)
state = dim_get_lock_state();
spin_unlock_irqrestore(&dim_lock, flags);
- return state;
+ return sprintf(buf, "%s\n", state ? "locked" : "");
}
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *dim2_dev_attrs[] = {
+ &dev_attr_state.attr,
+ NULL,
+};
+
+static struct attribute_group dim2_attr_group = {
+ .attrs = dim2_dev_attrs,
+};
+
+static const struct attribute_group *dim2_attr_groups[] = {
+ &dim2_attr_group,
+ NULL,
+};
+
/**
* dimcb_on_error - callback from HAL to report miscommunication between
* HDM and HAL
@@ -874,16 +891,8 @@ static int dim2_probe(struct platform_device *pdev)
goto err_stop_thread;
}
- ret = dim2_sysfs_probe(&dev->dev);
- if (ret) {
- dev_err(&pdev->dev, "failed to create sysfs attribute\n");
- goto err_unreg_iface;
- }
-
return 0;
-err_unreg_iface:
- most_deregister_interface(&dev->most_iface);
err_stop_thread:
kthread_stop(dev->netinfo_task);
err_shutdown_dim:
@@ -906,7 +915,6 @@ static int dim2_remove(struct platform_device *pdev)
struct dim2_hdm *dev = platform_get_drvdata(pdev);
unsigned long flags;
- dim2_sysfs_destroy(&dev->dev);
most_deregister_interface(&dev->most_iface);
kthread_stop(dev->netinfo_task);
@@ -1100,6 +1108,7 @@ static struct platform_driver dim2_driver = {
.driver = {
.name = "hdm_dim2",
.of_match_table = dim2_of_match,
+ .dev_groups = dim2_attr_groups,
},
};
diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
deleted file mode 100644
index c85b2cdcdca3..000000000000
--- a/drivers/staging/most/dim2/sysfs.c
+++ /dev/null
@@ -1,49 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * sysfs.c - MediaLB sysfs information
- *
- * Copyright (C) 2015, Microchip Technology Germany II GmbH & Co. KG
- */
-
-/* Author: Andrey Shvetsov <[email protected]> */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/kernel.h>
-#include "sysfs.h"
-#include <linux/device.h>
-
-static ssize_t state_show(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- bool state = dim2_sysfs_get_state_cb();
-
- return sprintf(buf, "%s\n", state ? "locked" : "");
-}
-
-static DEVICE_ATTR_RO(state);
-
-static struct attribute *dev_attrs[] = {
- &dev_attr_state.attr,
- NULL,
-};
-
-static struct attribute_group dev_attr_group = {
- .attrs = dev_attrs,
-};
-
-static const struct attribute_group *dev_attr_groups[] = {
- &dev_attr_group,
- NULL,
-};
-
-int dim2_sysfs_probe(struct device *dev)
-{
- dev->groups = dev_attr_groups;
- return device_register(dev);
-}
-
-void dim2_sysfs_destroy(struct device *dev)
-{
- device_unregister(dev);
-}
diff --git a/drivers/staging/most/dim2/sysfs.h b/drivers/staging/most/dim2/sysfs.h
index 24277a17cff3..09115cf4ed00 100644
--- a/drivers/staging/most/dim2/sysfs.h
+++ b/drivers/staging/most/dim2/sysfs.h
@@ -16,15 +16,4 @@ struct medialb_bus {
struct kobject kobj_group;
};
-struct device;
-
-int dim2_sysfs_probe(struct device *dev);
-void dim2_sysfs_destroy(struct device *dev);
-
-/*
- * callback,
- * must deliver MediaLB state as true if locked or false if unlocked
- */
-bool dim2_sysfs_get_state_cb(void);
-
#endif /* DIM2_SYSFS_H */
--
2.30.2
Commit 723de0f9171e ("staging: most: remove device from interface
structure") moved registration of driver-provided struct device to
the most subsystem. This updated dim2 driver as well.
However, struct device passed to register_device() becomes refcounted,
and must not be explicitly deallocated, but must provide release method
instead. Which is incompatible with managing it via devres.
This patch makes the device structure allocated without devres, adds
device release method, and moves device destruction there.
Fixes: 723de0f9171e ("staging: most: remove device from interface structure")
Signed-off-by: Nikita Yushchenko <[email protected]>
---
drivers/staging/most/dim2/dim2.c | 55 +++++++++++++++++---------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index bb6dd508e531..4c0c3bc91ac0 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -734,6 +734,23 @@ static int get_dim2_clk_speed(const char *clock_speed, u8 *val)
return -EINVAL;
}
+static void dim2_release(struct device *d)
+{
+ struct dim2_hdm *dev = container_of(d, struct dim2_hdm, dev);
+ unsigned long flags;
+
+ kthread_stop(dev->netinfo_task);
+
+ spin_lock_irqsave(&dim_lock, flags);
+ dim_shutdown();
+ spin_unlock_irqrestore(&dim_lock, flags);
+
+ if (dev->disable_platform)
+ dev->disable_platform(to_platform_device(d->parent));
+
+ kfree(dev);
+}
+
/*
* dim2_probe - dim2 probe handler
* @pdev: platform device structure
@@ -755,7 +772,7 @@ static int dim2_probe(struct platform_device *pdev)
enum { MLB_INT_IDX, AHB0_INT_IDX };
- dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
@@ -767,19 +784,21 @@ static int dim2_probe(struct platform_device *pdev)
"microchip,clock-speed", &clock_speed);
if (ret) {
dev_err(&pdev->dev, "missing dt property clock-speed\n");
- return ret;
+ goto err_free_dev;
}
ret = get_dim2_clk_speed(clock_speed, &dev->clk_speed);
if (ret) {
dev_err(&pdev->dev, "bad dt property clock-speed\n");
- return ret;
+ goto err_free_dev;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->io_base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(dev->io_base))
- return PTR_ERR(dev->io_base);
+ if (IS_ERR(dev->io_base)) {
+ ret = PTR_ERR(dev->io_base);
+ goto err_free_dev;
+ }
of_id = of_match_node(dim2_of_match, pdev->dev.of_node);
pdata = of_id->data;
@@ -787,7 +806,7 @@ static int dim2_probe(struct platform_device *pdev)
if (pdata->enable) {
ret = pdata->enable(pdev);
if (ret)
- return ret;
+ goto err_free_dev;
}
dev->disable_platform = pdata->disable;
if (pdata->fcnt)
@@ -882,24 +901,19 @@ static int dim2_probe(struct platform_device *pdev)
dev->most_iface.request_netinfo = request_netinfo;
dev->most_iface.driver_dev = &pdev->dev;
dev->most_iface.dev = &dev->dev;
- dev->dev.init_name = "dim2_state";
+ dev->dev.init_name = dev->name;
dev->dev.parent = &pdev->dev;
+ dev->dev.release = dim2_release;
- ret = most_register_interface(&dev->most_iface);
- if (ret) {
- dev_err(&pdev->dev, "failed to register MOST interface\n");
- goto err_stop_thread;
- }
-
- return 0;
+ return most_register_interface(&dev->most_iface);
-err_stop_thread:
- kthread_stop(dev->netinfo_task);
err_shutdown_dim:
dim_shutdown();
err_disable_platform:
if (dev->disable_platform)
dev->disable_platform(pdev);
+err_free_dev:
+ kfree(dev);
return ret;
}
@@ -913,17 +927,8 @@ static int dim2_probe(struct platform_device *pdev)
static int dim2_remove(struct platform_device *pdev)
{
struct dim2_hdm *dev = platform_get_drvdata(pdev);
- unsigned long flags;
most_deregister_interface(&dev->most_iface);
- kthread_stop(dev->netinfo_task);
-
- spin_lock_irqsave(&dim_lock, flags);
- dim_shutdown();
- spin_unlock_irqrestore(&dim_lock, flags);
-
- if (dev->disable_platform)
- dev->disable_platform(pdev);
return 0;
}
--
2.30.2
On Tue, Oct 05, 2021 at 05:34:48PM +0300, Nikita Yushchenko wrote:
> Commit 723de0f9171e ("staging: most: remove device from interface
> structure") moved registration of driver-provided struct device to
> the most subsystem.
>
> Dim2 used to register the same struct device to provide a custom device
> attribute. This causes double-registration of the same struct device.
>
> Fix that by moving the custom attribute to driver's dev_groups.
> This moves attribute to the platform_device object, which is a better
> location for platform-specific attributes anyway.
>
> Fixes: 723de0f9171e ("staging: most: remove device from interface structure")
> Signed-off-by: Nikita Yushchenko <[email protected]>
> ---
> drivers/staging/most/dim2/Makefile | 2 +-
> drivers/staging/most/dim2/dim2.c | 31 ++++++++++++-------
> drivers/staging/most/dim2/sysfs.c | 49 ------------------------------
> drivers/staging/most/dim2/sysfs.h | 11 -------
> 4 files changed, 21 insertions(+), 72 deletions(-)
> delete mode 100644 drivers/staging/most/dim2/sysfs.c
>
> diff --git a/drivers/staging/most/dim2/Makefile b/drivers/staging/most/dim2/Makefile
> index 861adacf6c72..5f9612af3fa3 100644
> --- a/drivers/staging/most/dim2/Makefile
> +++ b/drivers/staging/most/dim2/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_MOST_DIM2) += most_dim2.o
>
> -most_dim2-objs := dim2.o hal.o sysfs.o
> +most_dim2-objs := dim2.o hal.o
> diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> index e8b03fa90e80..bb6dd508e531 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -118,7 +118,8 @@ struct dim2_platform_data {
> (((p)[1] == 0x18) && ((p)[2] == 0x05) && ((p)[3] == 0x0C) && \
> ((p)[13] == 0x3C) && ((p)[14] == 0x00) && ((p)[15] == 0x0A))
>
> -bool dim2_sysfs_get_state_cb(void)
> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> {
> bool state;
> unsigned long flags;
> @@ -127,9 +128,25 @@ bool dim2_sysfs_get_state_cb(void)
> state = dim_get_lock_state();
> spin_unlock_irqrestore(&dim_lock, flags);
>
> - return state;
> + return sprintf(buf, "%s\n", state ? "locked" : "");
sysfs_emit()?
> }
>
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *dim2_dev_attrs[] = {
> + &dev_attr_state.attr,
> + NULL,
> +};
> +
> +static struct attribute_group dim2_attr_group = {
> + .attrs = dim2_dev_attrs,
> +};
> +
> +static const struct attribute_group *dim2_attr_groups[] = {
> + &dim2_attr_group,
> + NULL,
> +};
ATTRIBUTE_GROUPS()?
Other than these minor things, looks good, thanks for doing this!
greg k-h
Commit 723de0f9171e ("staging: most: remove device from interface
structure") moved registration of driver-provided struct device to
the most subsystem.
Dim2 used to register the same struct device to provide a custom device
attribute. This causes double-registration of the same struct device.
Fix that by moving the custom attribute to driver's dev_groups.
This moves attribute to the platform_device object, which is a better
location for platform-specific attributes anyway.
Fixes: 723de0f9171e ("staging: most: remove device from interface structure")
Signed-off-by: Nikita Yushchenko <[email protected]>
---
Changes from v1:
- use ATTRIBUTE_GROUPS()
- use sysfs_emit()
drivers/staging/most/dim2/Makefile | 2 +-
drivers/staging/most/dim2/dim2.c | 24 ++++++++-------
drivers/staging/most/dim2/sysfs.c | 49 ------------------------------
drivers/staging/most/dim2/sysfs.h | 11 -------
4 files changed, 14 insertions(+), 72 deletions(-)
delete mode 100644 drivers/staging/most/dim2/sysfs.c
diff --git a/drivers/staging/most/dim2/Makefile b/drivers/staging/most/dim2/Makefile
index 861adacf6c72..5f9612af3fa3 100644
--- a/drivers/staging/most/dim2/Makefile
+++ b/drivers/staging/most/dim2/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_MOST_DIM2) += most_dim2.o
-most_dim2-objs := dim2.o hal.o sysfs.o
+most_dim2-objs := dim2.o hal.o
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index e8b03fa90e80..96cb5280a385 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -118,7 +118,8 @@ struct dim2_platform_data {
(((p)[1] == 0x18) && ((p)[2] == 0x05) && ((p)[3] == 0x0C) && \
((p)[13] == 0x3C) && ((p)[14] == 0x00) && ((p)[15] == 0x0A))
-bool dim2_sysfs_get_state_cb(void)
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
bool state;
unsigned long flags;
@@ -127,9 +128,18 @@ bool dim2_sysfs_get_state_cb(void)
state = dim_get_lock_state();
spin_unlock_irqrestore(&dim_lock, flags);
- return state;
+ return sysfs_emit(buf, "%s\n", state ? "locked" : "");
}
+static DEVICE_ATTR_RO(state);
+
+static struct attribute *dim2_attrs[] = {
+ &dev_attr_state.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(dim2);
+
/**
* dimcb_on_error - callback from HAL to report miscommunication between
* HDM and HAL
@@ -874,16 +884,8 @@ static int dim2_probe(struct platform_device *pdev)
goto err_stop_thread;
}
- ret = dim2_sysfs_probe(&dev->dev);
- if (ret) {
- dev_err(&pdev->dev, "failed to create sysfs attribute\n");
- goto err_unreg_iface;
- }
-
return 0;
-err_unreg_iface:
- most_deregister_interface(&dev->most_iface);
err_stop_thread:
kthread_stop(dev->netinfo_task);
err_shutdown_dim:
@@ -906,7 +908,6 @@ static int dim2_remove(struct platform_device *pdev)
struct dim2_hdm *dev = platform_get_drvdata(pdev);
unsigned long flags;
- dim2_sysfs_destroy(&dev->dev);
most_deregister_interface(&dev->most_iface);
kthread_stop(dev->netinfo_task);
@@ -1100,6 +1101,7 @@ static struct platform_driver dim2_driver = {
.driver = {
.name = "hdm_dim2",
.of_match_table = dim2_of_match,
+ .dev_groups = dim2_groups,
},
};
diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
deleted file mode 100644
index c85b2cdcdca3..000000000000
--- a/drivers/staging/most/dim2/sysfs.c
+++ /dev/null
@@ -1,49 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * sysfs.c - MediaLB sysfs information
- *
- * Copyright (C) 2015, Microchip Technology Germany II GmbH & Co. KG
- */
-
-/* Author: Andrey Shvetsov <[email protected]> */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/kernel.h>
-#include "sysfs.h"
-#include <linux/device.h>
-
-static ssize_t state_show(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- bool state = dim2_sysfs_get_state_cb();
-
- return sprintf(buf, "%s\n", state ? "locked" : "");
-}
-
-static DEVICE_ATTR_RO(state);
-
-static struct attribute *dev_attrs[] = {
- &dev_attr_state.attr,
- NULL,
-};
-
-static struct attribute_group dev_attr_group = {
- .attrs = dev_attrs,
-};
-
-static const struct attribute_group *dev_attr_groups[] = {
- &dev_attr_group,
- NULL,
-};
-
-int dim2_sysfs_probe(struct device *dev)
-{
- dev->groups = dev_attr_groups;
- return device_register(dev);
-}
-
-void dim2_sysfs_destroy(struct device *dev)
-{
- device_unregister(dev);
-}
diff --git a/drivers/staging/most/dim2/sysfs.h b/drivers/staging/most/dim2/sysfs.h
index 24277a17cff3..09115cf4ed00 100644
--- a/drivers/staging/most/dim2/sysfs.h
+++ b/drivers/staging/most/dim2/sysfs.h
@@ -16,15 +16,4 @@ struct medialb_bus {
struct kobject kobj_group;
};
-struct device;
-
-int dim2_sysfs_probe(struct device *dev);
-void dim2_sysfs_destroy(struct device *dev);
-
-/*
- * callback,
- * must deliver MediaLB state as true if locked or false if unlocked
- */
-bool dim2_sysfs_get_state_cb(void);
-
#endif /* DIM2_SYSFS_H */
--
2.30.2
On Mon, 2021-10-11 at 09:11 +0300, Nikita Yushchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Commit 723de0f9171e ("staging: most: remove device from interface
> structure") moved registration of driver-provided struct device to
> the most subsystem.
>
> Dim2 used to register the same struct device to provide a custom
> device
> attribute. This causes double-registration of the same struct device.
>
> Fix that by moving the custom attribute to driver's dev_groups.
> This moves attribute to the platform_device object, which is a better
> location for platform-specific attributes anyway.
>
> Fixes: 723de0f9171e ("staging: most: remove device from interface
> structure")
> Signed-off-by: Nikita Yushchenko <[email protected]>
> ---
> Changes from v1:
> - use ATTRIBUTE_GROUPS()
> - use sysfs_emit()
>
> drivers/staging/most/dim2/Makefile | 2 +-
> drivers/staging/most/dim2/dim2.c | 24 ++++++++-------
> drivers/staging/most/dim2/sysfs.c | 49 ----------------------------
> --
> drivers/staging/most/dim2/sysfs.h | 11 -------
> 4 files changed, 14 insertions(+), 72 deletions(-)
> delete mode 100644 drivers/staging/most/dim2/sysfs.c
>
> diff --git a/drivers/staging/most/dim2/Makefile
> b/drivers/staging/most/dim2/Makefile
> index 861adacf6c72..5f9612af3fa3 100644
> --- a/drivers/staging/most/dim2/Makefile
> +++ b/drivers/staging/most/dim2/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_MOST_DIM2) += most_dim2.o
>
> -most_dim2-objs := dim2.o hal.o sysfs.o
> +most_dim2-objs := dim2.o hal.o
> diff --git a/drivers/staging/most/dim2/dim2.c
> b/drivers/staging/most/dim2/dim2.c
> index e8b03fa90e80..96cb5280a385 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -118,7 +118,8 @@ struct dim2_platform_data {
> (((p)[1] == 0x18) && ((p)[2] == 0x05) && ((p)[3] == 0x0C) &&
> \
> ((p)[13] == 0x3C) && ((p)[14] == 0x00) && ((p)[15] == 0x0A))
>
> -bool dim2_sysfs_get_state_cb(void)
> +static ssize_t state_show(struct device *dev, struct
> device_attribute *attr,
> + char *buf)
> {
> bool state;
> unsigned long flags;
> @@ -127,9 +128,18 @@ bool dim2_sysfs_get_state_cb(void)
> state = dim_get_lock_state();
> spin_unlock_irqrestore(&dim_lock, flags);
>
> - return state;
> + return sysfs_emit(buf, "%s\n", state ? "locked" : "");
> }
>
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *dim2_attrs[] = {
> + &dev_attr_state.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dim2);
> +
> /**
> * dimcb_on_error - callback from HAL to report miscommunication
> between
> * HDM and HAL
> @@ -874,16 +884,8 @@ static int dim2_probe(struct platform_device
> *pdev)
> goto err_stop_thread;
> }
>
> - ret = dim2_sysfs_probe(&dev->dev);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to create sysfs
> attribute\n");
> - goto err_unreg_iface;
> - }
> -
> return 0;
>
> -err_unreg_iface:
> - most_deregister_interface(&dev->most_iface);
> err_stop_thread:
> kthread_stop(dev->netinfo_task);
> err_shutdown_dim:
> @@ -906,7 +908,6 @@ static int dim2_remove(struct platform_device
> *pdev)
> struct dim2_hdm *dev = platform_get_drvdata(pdev);
> unsigned long flags;
>
> - dim2_sysfs_destroy(&dev->dev);
> most_deregister_interface(&dev->most_iface);
> kthread_stop(dev->netinfo_task);
>
> @@ -1100,6 +1101,7 @@ static struct platform_driver dim2_driver = {
> .driver = {
> .name = "hdm_dim2",
> .of_match_table = dim2_of_match,
> + .dev_groups = dim2_groups,
> },
> };
>
> diff --git a/drivers/staging/most/dim2/sysfs.c
> b/drivers/staging/most/dim2/sysfs.c
> deleted file mode 100644
> index c85b2cdcdca3..000000000000
> --- a/drivers/staging/most/dim2/sysfs.c
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * sysfs.c - MediaLB sysfs information
> - *
> - * Copyright (C) 2015, Microchip Technology Germany II GmbH & Co. KG
> - */
> -
> -/* Author: Andrey Shvetsov <[email protected]> */
> -
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> -#include <linux/kernel.h>
> -#include "sysfs.h"
> -#include <linux/device.h>
> -
> -static ssize_t state_show(struct device *dev, struct
> device_attribute *attr,
> - char *buf)
> -{
> - bool state = dim2_sysfs_get_state_cb();
> -
> - return sprintf(buf, "%s\n", state ? "locked" : "");
> -}
> -
> -static DEVICE_ATTR_RO(state);
> -
> -static struct attribute *dev_attrs[] = {
> - &dev_attr_state.attr,
> - NULL,
> -};
> -
> -static struct attribute_group dev_attr_group = {
> - .attrs = dev_attrs,
> -};
> -
> -static const struct attribute_group *dev_attr_groups[] = {
> - &dev_attr_group,
> - NULL,
> -};
> -
> -int dim2_sysfs_probe(struct device *dev)
> -{
> - dev->groups = dev_attr_groups;
> - return device_register(dev);
> -}
> -
> -void dim2_sysfs_destroy(struct device *dev)
> -{
> - device_unregister(dev);
> -}
> diff --git a/drivers/staging/most/dim2/sysfs.h
> b/drivers/staging/most/dim2/sysfs.h
> index 24277a17cff3..09115cf4ed00 100644
> --- a/drivers/staging/most/dim2/sysfs.h
> +++ b/drivers/staging/most/dim2/sysfs.h
> @@ -16,15 +16,4 @@ struct medialb_bus {
> struct kobject kobj_group;
> };
>
> -struct device;
> -
> -int dim2_sysfs_probe(struct device *dev);
> -void dim2_sysfs_destroy(struct device *dev);
> -
> -/*
> - * callback,
> - * must deliver MediaLB state as true if locked or false if unlocked
> - */
> -bool dim2_sysfs_get_state_cb(void);
> -
> #endif /* DIM2_SYSFS_H */
> --
> 2.30.2
>
>
>
Acked-by: Christian Gromm <[email protected]>
thanks,
Chris