| Message ID | 20211227094526.698714-14-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2,01/23] counter: Use container_of instead of drvdata to track counter_device | expand |
On 12/27/21 10:45 AM, Uwe Kleine-König wrote: > The current implementation gets device lifetime tracking wrong. The > problem is that allocation of struct counter_device is controlled by the > individual drivers but this structure contains a struct device that > might have to live longer than a driver is bound. As a result a command > sequence like: > > { sleep 5; echo bang; } > /dev/counter0 & > sleep 1; > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > can keep a reference to the struct device and unbinding results in > freeing the memory occupied by this device resulting in an oops. > > This commit provides two new functions (plus some helpers): > - counter_alloc() to allocate a struct counter_device that is > automatically freed once the embedded struct device is released > - counter_add() to register such a device. > > Note that this commit doesn't fix any issues, all drivers have to be > converted to these new functions to correct the lifetime problems. Nice work! > [...] > @@ -24,6 +25,11 @@ > /* Provides a unique ID for each counter device */ > static DEFINE_IDA(counter_ida); > > +struct counter_device_allochelper { > + struct counter_device counter; > + unsigned long privdata[]; The normal k*alloc functions guarantee that the allocation is cacheline aligned. Without that for example the ____cacheline_aligned attribute will not work correctly. And while it is unlikely that a counter driver will need for example DMA memory I think it is still good practice to guarantee this here to avoid bad surprises. There are two ways of doing this. Make privdata ____cacheline_aligned like in the memstick framework[1]. The downside is that this will reserve padding for the allocation, even if no private data is allocated. The alternative is to do something similar to IIO[2] which only allocates the padding if there actually is any private data requested. The disadvantage of that is that the code is a bit more cumbersome. And most drivers will want to have some private data anyway so it might not be worth it. [1] https://elixir.bootlin.com/linux/v5.16-rc7/source/include/linux/memstick.h#L292 [2] https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/iio/industrialio-core.c#L1638 > [...] > +struct counter_device *counter_alloc(size_t sizeof_priv) I'd add a parent parameter here since with the devm_ variant we have to pass it anyway and this allows to slightly reduce the boilerplate code in the driver where the parent field of the counter has to be initialized. > +{ > + struct counter_device_allochelper *ch; > + struct counter_device *counter; > + struct device *dev; > + int id, err; > + > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > + if (!ch) { > + err = -ENOMEM; > + goto err_alloc_ch; > + } > + > + counter = &ch->counter; > + dev = &counter->dev; > + > + /* Acquire unique ID */ > + err = ida_alloc(&counter_ida, GFP_KERNEL); > + if (err < 0) { > + goto err_ida_alloc; > + } There is a inconsistency whether braces are used for single statement `if`s in this patch. > + dev->id = err; > + > + err = counter_chrdev_add(counter); > + if (err < 0) > + goto err_chrdev_add; > + > + device_initialize(dev); > + /* Configure device structure for Counter */ > + dev->type = &counter_device_type; > + dev->bus = &counter_bus_type; > + dev->devt = MKDEV(MAJOR(counter_devt), id); > + > + mutex_init(&counter->ops_exist_lock); > + > + return counter; > + > +err_chrdev_add: > + > + ida_free(&counter_ida, dev->id); > +err_ida_alloc: > + > + kfree(ch); > +err_alloc_ch: > + > + return ERR_PTR(err); > +} > +EXPORT_SYMBOL_GPL(counter_alloc); > + > +void counter_put(struct counter_device *counter) > +{ > + put_device(&counter->dev); > +} > + > +/** > + * counter_add - complete registration of a counter > + * @counter: the counter to add > + * > + * This is part two of counter registration. > + * > + * If this succeeds, call counter_unregister() to get rid of the counter_device again. > + */ > +int counter_add(struct counter_device *counter) > +{ > + int err; > + struct device *dev = &counter->dev; > + > + get_device(&counter->dev); This get_device() is not needed. device_add() will also add a reference, so you increment the reference count by 2 when calling counter_add(). It is only needed to balance the put_device() in counter_unregister(). I'd add a `counter->legacy_device` around that put_device() and then remove it in the last patch. The extra get_device() here is also leaked on the error paths, so removing it will allow to keep the errors paths as they are. > + > + if (counter->parent) { > + dev->parent = counter->parent; > + dev->of_node = counter->parent->of_node; > + } > + > + err = counter_sysfs_add(counter); > + if (err < 0) > + return err; > + > + /* implies device_add(dev) */ > + err = cdev_device_add(&counter->chrdev, dev); > + > + return err; return cdev_device_add(...). > +struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv) API Documentation? > +{ > + struct counter_device *counter; > + int err; > + > + counter = counter_alloc(sizeof_priv); > + if (IS_ERR(counter)) > + return counter; > + > + err = devm_add_action_or_reset(dev, devm_counter_put, counter); > + if (err < 0) > + return ERR_PTR(err); > + > + return counter; > +} > +EXPORT_SYMBOL_GPL(devm_counter_alloc); > + > +int devm_counter_add(struct device *dev, > + struct counter_device *const counter) > +{ API Documentation? > + int err; > + > + err = counter_add(counter); > + if (err < 0) > + return err; > + > + return devm_add_action_or_reset(dev, devm_counter_release, counter); > +} > +EXPORT_SYMBOL_GPL(devm_counter_add); > + > #define COUNTER_DEV_MAX 256 >
Hi "Uwe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on a7904a538933c525096ca2ccde1e60d0ee62c08e]
url: https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/counter-cleanups-and-device-lifetime-fixes/20211227-174815
base: a7904a538933c525096ca2ccde1e60d0ee62c08e
config: i386-randconfig-r006-20211227 (https://download.01.org/0day-ci/archive/20211227/202112272054.WsrEZBF1-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 511726c64d3b6cca66f7c54d457d586aa3129f67)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/250e0e3d91caea9f7c61652a7a9a8c006b2464be
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Uwe-Kleine-K-nig/counter-cleanups-and-device-lifetime-fixes/20211227-174815
git checkout 250e0e3d91caea9f7c61652a7a9a8c006b2464be
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/counter/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/counter/counter-core.c:175:41: warning: variable 'id' is uninitialized when used here [-Wuninitialized]
dev->devt = MKDEV(MAJOR(counter_devt), id);
^~
include/linux/kdev_t.h:12:46: note: expanded from macro 'MKDEV'
#define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
^~
drivers/counter/counter-core.c:149:8: note: initialize the variable 'id' to silence this warning
int id, err;
^
= 0
1 warning generated.
vim +/id +175 drivers/counter/counter-core.c
134
135 /**
136 * counter_alloc - allocate a counter_device
137 * @sizeof_priv: size of the driver private data
138 *
139 * This is part one of counter registration. The structure is allocated
140 * dynamically to ensure the right lifetime for the embedded struct device.
141 *
142 * If this succeeds, call counter_put() to get rid of the counter_device again.
143 */
144 struct counter_device *counter_alloc(size_t sizeof_priv)
145 {
146 struct counter_device_allochelper *ch;
147 struct counter_device *counter;
148 struct device *dev;
149 int id, err;
150
151 ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL);
152 if (!ch) {
153 err = -ENOMEM;
154 goto err_alloc_ch;
155 }
156
157 counter = &ch->counter;
158 dev = &counter->dev;
159
160 /* Acquire unique ID */
161 err = ida_alloc(&counter_ida, GFP_KERNEL);
162 if (err < 0) {
163 goto err_ida_alloc;
164 }
165 dev->id = err;
166
167 err = counter_chrdev_add(counter);
168 if (err < 0)
169 goto err_chrdev_add;
170
171 device_initialize(dev);
172 /* Configure device structure for Counter */
173 dev->type = &counter_device_type;
174 dev->bus = &counter_bus_type;
> 175 dev->devt = MKDEV(MAJOR(counter_devt), id);
176
177 mutex_init(&counter->ops_exist_lock);
178
179 return counter;
180
181 err_chrdev_add:
182
183 ida_free(&counter_ida, dev->id);
184 err_ida_alloc:
185
186 kfree(ch);
187 err_alloc_ch:
188
189 return ERR_PTR(err);
190 }
191 EXPORT_SYMBOL_GPL(counter_alloc);
192
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello, On Mon, Dec 27, 2021 at 01:16:56PM +0100, Lars-Peter Clausen wrote: > On 12/27/21 10:45 AM, Uwe Kleine-König wrote: > > The current implementation gets device lifetime tracking wrong. The > > problem is that allocation of struct counter_device is controlled by the > > individual drivers but this structure contains a struct device that > > might have to live longer than a driver is bound. As a result a command > > sequence like: > > > > { sleep 5; echo bang; } > /dev/counter0 & > > sleep 1; > > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > > > can keep a reference to the struct device and unbinding results in > > freeing the memory occupied by this device resulting in an oops. > > > > This commit provides two new functions (plus some helpers): > > - counter_alloc() to allocate a struct counter_device that is > > automatically freed once the embedded struct device is released > > - counter_add() to register such a device. > > > > Note that this commit doesn't fix any issues, all drivers have to be > > converted to these new functions to correct the lifetime problems. > Nice work! \o/ > > [...] > > @@ -24,6 +25,11 @@ > > /* Provides a unique ID for each counter device */ > > static DEFINE_IDA(counter_ida); > > +struct counter_device_allochelper { > > + struct counter_device counter; > > + unsigned long privdata[]; > > The normal k*alloc functions guarantee that the allocation is cacheline > aligned. Without that for example the ____cacheline_aligned attribute will > not work correctly. And while it is unlikely that a counter driver will need > for example DMA memory I think it is still good practice to guarantee this > here to avoid bad surprises. There are two ways of doing this. Yeah, I thought to add more alignment once the need arises. My preference would be to got for the ____cacheline_aligned approach. > > [...] > > +struct counter_device *counter_alloc(size_t sizeof_priv) > I'd add a parent parameter here since with the devm_ variant we have to pass > it anyway and this allows to slightly reduce the boilerplate code in the > driver where the parent field of the counter has to be initialized. I don't feel strong here, can do that. > > +{ > > + struct counter_device_allochelper *ch; > > + struct counter_device *counter; > > + struct device *dev; > > + int id, err; > > + > > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > > + if (!ch) { > > + err = -ENOMEM; > > + goto err_alloc_ch; > > + } > > + > > + counter = &ch->counter; > > + dev = &counter->dev; > > + > > + /* Acquire unique ID */ > > + err = ida_alloc(&counter_ida, GFP_KERNEL); > > + if (err < 0) { > > + goto err_ida_alloc; > > + } > There is a inconsistency whether braces are used for single statement `if`s > in this patch. Oh, indeed. > > + dev->id = err; > > + > > + err = counter_chrdev_add(counter); > > + if (err < 0) > > + goto err_chrdev_add; > > + > > + device_initialize(dev); > > + /* Configure device structure for Counter */ > > + dev->type = &counter_device_type; > > + dev->bus = &counter_bus_type; > > + dev->devt = MKDEV(MAJOR(counter_devt), id); > > + > > + mutex_init(&counter->ops_exist_lock); > > + > > + return counter; > > + > > +err_chrdev_add: > > + > > + ida_free(&counter_ida, dev->id); > > +err_ida_alloc: > > + > > + kfree(ch); > > +err_alloc_ch: > > + > > + return ERR_PTR(err); > > +} > > +EXPORT_SYMBOL_GPL(counter_alloc); > > + > > +void counter_put(struct counter_device *counter) > > +{ > > + put_device(&counter->dev); > > +} > > + > > +/** > > + * counter_add - complete registration of a counter > > + * @counter: the counter to add > > + * > > + * This is part two of counter registration. > > + * > > + * If this succeeds, call counter_unregister() to get rid of the counter_device again. > > + */ > > +int counter_add(struct counter_device *counter) > > +{ > > + int err; > > + struct device *dev = &counter->dev; > > + > > + get_device(&counter->dev); > > This get_device() is not needed. device_add() will also add a reference, so > you increment the reference count by 2 when calling counter_add(). > > It is only needed to balance the put_device() in counter_unregister(). I'd > add a `counter->legacy_device` around that put_device() and then remove it > in the last patch. Ah indeed. I added that to balance the call of put_device() by devm_counter_alloc()'s release function (devm_counter_put()). But on a quick check you're right. I will think about it a bit more and test. Best regards Uwe
On Mon, 27 Dec 2021 10:45:16 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The current implementation gets device lifetime tracking wrong. The > problem is that allocation of struct counter_device is controlled by the > individual drivers but this structure contains a struct device that > might have to live longer than a driver is bound. As a result a command > sequence like: > > { sleep 5; echo bang; } > /dev/counter0 & > sleep 1; > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > can keep a reference to the struct device and unbinding results in > freeing the memory occupied by this device resulting in an oops. > > This commit provides two new functions (plus some helpers): > - counter_alloc() to allocate a struct counter_device that is > automatically freed once the embedded struct device is released > - counter_add() to register such a device. > > Note that this commit doesn't fix any issues, all drivers have to be > converted to these new functions to correct the lifetime problems. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> In general looks very nice to me. Few minor things inline. > --- > drivers/counter/counter-core.c | 149 ++++++++++++++++++++++++++++++++- > include/linux/counter.h | 15 ++++ > 2 files changed, 163 insertions(+), 1 deletion(-) > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > index 00c41f28c101..8261567b6272 100644 > --- a/drivers/counter/counter-core.c > +++ b/drivers/counter/counter-core.c > @@ -15,6 +15,7 @@ > #include <linux/kdev_t.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/slab.h> > #include <linux/types.h> > #include <linux/wait.h> > > @@ -24,6 +25,11 @@ > /* Provides a unique ID for each counter device */ > static DEFINE_IDA(counter_ida); > > +struct counter_device_allochelper { > + struct counter_device counter; > + unsigned long privdata[]; Nice to keep this opaque. We danced around how to do this in IIO for a while and never got to a solution we all liked. Slight disadvantage is this might matter in hot paths where the compiler doesn't have visibility to know it can very cheaply access the privdata. I guess that can be looked at if anyone ever measures it as an important element (they probably never will!) > +}; > + > static void counter_device_release(struct device *dev) > { > struct counter_device *const counter = > @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev) > > counter_chrdev_remove(counter); > ida_free(&counter_ida, dev->id); > + > + if (!counter->legacy_device) > + kfree(container_of(counter, struct counter_device_allochelper, counter)); > } > > static struct device_type counter_device_type = { > @@ -53,7 +62,14 @@ static dev_t counter_devt; > */ > void *counter_priv(const struct counter_device *const counter) > { > - return counter->priv; > + if (counter->legacy_device) { > + return counter->priv; > + } else { > + struct counter_device_allochelper *ch = > + container_of(counter, struct counter_device_allochelper, counter); > + > + return &ch->privdata; > + } > } > EXPORT_SYMBOL_GPL(counter_priv); > > @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter) > int id; > int err; > > + counter->legacy_device = true; > + > /* Acquire unique ID */ > id = ida_alloc(&counter_ida, GFP_KERNEL); > if (id < 0) > @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter) > } > EXPORT_SYMBOL_GPL(counter_register); > > +/** > + * counter_alloc - allocate a counter_device > + * @sizeof_priv: size of the driver private data > + * > + * This is part one of counter registration. The structure is allocated > + * dynamically to ensure the right lifetime for the embedded struct device. > + * > + * If this succeeds, call counter_put() to get rid of the counter_device again. > + */ > +struct counter_device *counter_alloc(size_t sizeof_priv) > +{ > + struct counter_device_allochelper *ch; > + struct counter_device *counter; > + struct device *dev; > + int id, err; > + > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > + if (!ch) { > + err = -ENOMEM; > + goto err_alloc_ch; > + } > + > + counter = &ch->counter; > + dev = &counter->dev; > + > + /* Acquire unique ID */ > + err = ida_alloc(&counter_ida, GFP_KERNEL); > + if (err < 0) { > + goto err_ida_alloc; > + } > + dev->id = err; > + > + err = counter_chrdev_add(counter); Should think about renaming counter_chrdev_add() given it does init and allocation stuff but no adding. Personal inclination here would be to keep the elements in here in the same order as in counter_register() where it doesn't matter in the interests of slightly easier comparison of the code. > + if (err < 0) > + goto err_chrdev_add; > + > + device_initialize(dev); > + /* Configure device structure for Counter */ > + dev->type = &counter_device_type; > + dev->bus = &counter_bus_type; > + dev->devt = MKDEV(MAJOR(counter_devt), id); As 0-day pointed out id not initialized. > + > + mutex_init(&counter->ops_exist_lock); > + > + return counter; > + > +err_chrdev_add: > + > + ida_free(&counter_ida, dev->id); > +err_ida_alloc: > + > + kfree(ch); > +err_alloc_ch: > + > + return ERR_PTR(err); > +} > +EXPORT_SYMBOL_GPL(counter_alloc); > + > +void counter_put(struct counter_device *counter) > +{ > + put_device(&counter->dev); > +} > + > +/** > + * counter_add - complete registration of a counter > + * @counter: the counter to add > + * > + * This is part two of counter registration. > + * > + * If this succeeds, call counter_unregister() to get rid of the counter_device again. > + */ > +int counter_add(struct counter_device *counter) > +{ > + int err; > + struct device *dev = &counter->dev; > + > + get_device(&counter->dev); > + > + if (counter->parent) { > + dev->parent = counter->parent; > + dev->of_node = counter->parent->of_node; > + } > + > + err = counter_sysfs_add(counter); > + if (err < 0) > + return err; > + > + /* implies device_add(dev) */ > + err = cdev_device_add(&counter->chrdev, dev); > + > + return err; return cdev_device_add(&counter->chrdev, dev); Lars got some of these points as well (maybe all of them but I'm too lazy to filter them for you ;) > +} > +EXPORT_SYMBOL_GPL(counter_add); > + > /** > * counter_unregister - unregister Counter from the system > * @counter: pointer to Counter to unregister > @@ -168,6 +280,41 @@ int devm_counter_register(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_counter_register); > > +static void devm_counter_put(void *counter) > +{ > + counter_put(counter); > +} > + > +struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv) > +{ > + struct counter_device *counter; > + int err; > + > + counter = counter_alloc(sizeof_priv); > + if (IS_ERR(counter)) > + return counter; > + > + err = devm_add_action_or_reset(dev, devm_counter_put, counter); > + if (err < 0) > + return ERR_PTR(err); > + > + return counter; > +} > +EXPORT_SYMBOL_GPL(devm_counter_alloc); > + > +int devm_counter_add(struct device *dev, > + struct counter_device *const counter) > +{ > + int err; > + > + err = counter_add(counter); > + if (err < 0) > + return err; > + > + return devm_add_action_or_reset(dev, devm_counter_release, counter); > +} > +EXPORT_SYMBOL_GPL(devm_counter_add); > + > #define COUNTER_DEV_MAX 256 > > static int __init counter_init(void) > diff --git a/include/linux/counter.h b/include/linux/counter.h > index 8daaa38c71d8..f1350a43cd48 100644 > --- a/include/linux/counter.h > +++ b/include/linux/counter.h > @@ -327,14 +327,29 @@ struct counter_device { > spinlock_t events_in_lock; > struct mutex events_out_lock; > struct mutex ops_exist_lock; > + > + /* > + * This can go away once all drivers are converted to > + * counter_alloc()/counter_add(). > + */ > + bool legacy_device; > }; > > void *counter_priv(const struct counter_device *const counter); > > int counter_register(struct counter_device *const counter); > + > +struct counter_device *counter_alloc(size_t sizeof_priv); > +void counter_put(struct counter_device *const counter); > +int counter_add(struct counter_device *const counter); > + > void counter_unregister(struct counter_device *const counter); > int devm_counter_register(struct device *dev, > struct counter_device *const counter); > +struct counter_device *devm_counter_alloc(struct device *dev, > + size_t sizeof_priv); > +int devm_counter_add(struct device *dev, > + struct counter_device *const counter); > void counter_push_event(struct counter_device *const counter, const u8 event, > const u8 channel); >
On Mon, Dec 27, 2021 at 10:45:16AM +0100, Uwe Kleine-König wrote: > The current implementation gets device lifetime tracking wrong. The > problem is that allocation of struct counter_device is controlled by the > individual drivers but this structure contains a struct device that > might have to live longer than a driver is bound. As a result a command > sequence like: > > { sleep 5; echo bang; } > /dev/counter0 & > sleep 1; > echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind > > can keep a reference to the struct device and unbinding results in > freeing the memory occupied by this device resulting in an oops. > > This commit provides two new functions (plus some helpers): > - counter_alloc() to allocate a struct counter_device that is > automatically freed once the embedded struct device is released > - counter_add() to register such a device. > > Note that this commit doesn't fix any issues, all drivers have to be > converted to these new functions to correct the lifetime problems. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I agree with the approach taken in this patch, and I don't have much to add after the suggestions Lars-Peter and Jonathan have already given. So assuming those are addressed in the next version I expect to Ack this patch as well. > --- > drivers/counter/counter-core.c | 149 ++++++++++++++++++++++++++++++++- > include/linux/counter.h | 15 ++++ > 2 files changed, 163 insertions(+), 1 deletion(-) > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > index 00c41f28c101..8261567b6272 100644 > --- a/drivers/counter/counter-core.c > +++ b/drivers/counter/counter-core.c > @@ -15,6 +15,7 @@ > #include <linux/kdev_t.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/slab.h> > #include <linux/types.h> > #include <linux/wait.h> > > @@ -24,6 +25,11 @@ > /* Provides a unique ID for each counter device */ > static DEFINE_IDA(counter_ida); > > +struct counter_device_allochelper { > + struct counter_device counter; > + unsigned long privdata[]; > +}; > + > static void counter_device_release(struct device *dev) > { > struct counter_device *const counter = > @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev) > > counter_chrdev_remove(counter); > ida_free(&counter_ida, dev->id); > + > + if (!counter->legacy_device) > + kfree(container_of(counter, struct counter_device_allochelper, counter)); > } > > static struct device_type counter_device_type = { > @@ -53,7 +62,14 @@ static dev_t counter_devt; > */ > void *counter_priv(const struct counter_device *const counter) > { > - return counter->priv; > + if (counter->legacy_device) { > + return counter->priv; > + } else { > + struct counter_device_allochelper *ch = > + container_of(counter, struct counter_device_allochelper, counter); > + > + return &ch->privdata; > + } > } > EXPORT_SYMBOL_GPL(counter_priv); > > @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter) > int id; > int err; > > + counter->legacy_device = true; > + > /* Acquire unique ID */ > id = ida_alloc(&counter_ida, GFP_KERNEL); > if (id < 0) > @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter) > } > EXPORT_SYMBOL_GPL(counter_register); > > +/** > + * counter_alloc - allocate a counter_device > + * @sizeof_priv: size of the driver private data > + * > + * This is part one of counter registration. The structure is allocated > + * dynamically to ensure the right lifetime for the embedded struct device. > + * > + * If this succeeds, call counter_put() to get rid of the counter_device again. > + */ > +struct counter_device *counter_alloc(size_t sizeof_priv) > +{ > + struct counter_device_allochelper *ch; > + struct counter_device *counter; > + struct device *dev; > + int id, err; > + > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > + if (!ch) { > + err = -ENOMEM; > + goto err_alloc_ch; > + } > + > + counter = &ch->counter; > + dev = &counter->dev; > + > + /* Acquire unique ID */ > + err = ida_alloc(&counter_ida, GFP_KERNEL); > + if (err < 0) { > + goto err_ida_alloc; > + } > + dev->id = err; > + > + err = counter_chrdev_add(counter); > + if (err < 0) > + goto err_chrdev_add; > + > + device_initialize(dev); > + /* Configure device structure for Counter */ > + dev->type = &counter_device_type; > + dev->bus = &counter_bus_type; > + dev->devt = MKDEV(MAJOR(counter_devt), id); > + > + mutex_init(&counter->ops_exist_lock); > + > + return counter; > + > +err_chrdev_add: > + > + ida_free(&counter_ida, dev->id); > +err_ida_alloc: > + > + kfree(ch); > +err_alloc_ch: > + > + return ERR_PTR(err); > +} > +EXPORT_SYMBOL_GPL(counter_alloc); > + > +void counter_put(struct counter_device *counter) > +{ > + put_device(&counter->dev); > +} > + > +/** > + * counter_add - complete registration of a counter > + * @counter: the counter to add > + * > + * This is part two of counter registration. > + * > + * If this succeeds, call counter_unregister() to get rid of the counter_device again. > + */ > +int counter_add(struct counter_device *counter) > +{ > + int err; > + struct device *dev = &counter->dev; > + > + get_device(&counter->dev); > + > + if (counter->parent) { > + dev->parent = counter->parent; > + dev->of_node = counter->parent->of_node; > + } > + > + err = counter_sysfs_add(counter); > + if (err < 0) > + return err; > + > + /* implies device_add(dev) */ > + err = cdev_device_add(&counter->chrdev, dev); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(counter_add); > + > /** > * counter_unregister - unregister Counter from the system > * @counter: pointer to Counter to unregister > @@ -168,6 +280,41 @@ int devm_counter_register(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_counter_register); > > +static void devm_counter_put(void *counter) > +{ > + counter_put(counter); > +} > + > +struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv) > +{ > + struct counter_device *counter; > + int err; > + > + counter = counter_alloc(sizeof_priv); > + if (IS_ERR(counter)) > + return counter; > + > + err = devm_add_action_or_reset(dev, devm_counter_put, counter); > + if (err < 0) > + return ERR_PTR(err); > + > + return counter; > +} > +EXPORT_SYMBOL_GPL(devm_counter_alloc); > + > +int devm_counter_add(struct device *dev, > + struct counter_device *const counter) > +{ > + int err; > + > + err = counter_add(counter); > + if (err < 0) > + return err; > + > + return devm_add_action_or_reset(dev, devm_counter_release, counter); > +} > +EXPORT_SYMBOL_GPL(devm_counter_add); > + > #define COUNTER_DEV_MAX 256 > > static int __init counter_init(void) > diff --git a/include/linux/counter.h b/include/linux/counter.h > index 8daaa38c71d8..f1350a43cd48 100644 > --- a/include/linux/counter.h > +++ b/include/linux/counter.h > @@ -327,14 +327,29 @@ struct counter_device { > spinlock_t events_in_lock; > struct mutex events_out_lock; > struct mutex ops_exist_lock; > + > + /* > + * This can go away once all drivers are converted to > + * counter_alloc()/counter_add(). > + */ > + bool legacy_device; > }; > > void *counter_priv(const struct counter_device *const counter); > > int counter_register(struct counter_device *const counter); > + > +struct counter_device *counter_alloc(size_t sizeof_priv); > +void counter_put(struct counter_device *const counter); > +int counter_add(struct counter_device *const counter); > + > void counter_unregister(struct counter_device *const counter); > int devm_counter_register(struct device *dev, > struct counter_device *const counter); > +struct counter_device *devm_counter_alloc(struct device *dev, > + size_t sizeof_priv); > +int devm_counter_add(struct device *dev, > + struct counter_device *const counter); > void counter_push_event(struct counter_device *const counter, const u8 event, > const u8 channel); > > -- > 2.33.0 >
Hello, On Tue, Dec 28, 2021 at 06:00:17PM +0000, Jonathan Cameron wrote: > On Mon, 27 Dec 2021 10:45:16 +0100 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > +struct counter_device_allochelper { > > + struct counter_device counter; > > + unsigned long privdata[]; > Nice to keep this opaque. We danced around how to do this in IIO for > a while and never got to a solution we all liked. Slight disadvantage > is this might matter in hot paths where the compiler doesn't have visibility > to know it can very cheaply access the privdata. > > I guess that can be looked at if anyone ever measures it as an important > element (they probably never will!) *nod* > > +}; > > + > > static void counter_device_release(struct device *dev) > > { > > struct counter_device *const counter = > > @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev) > > > > counter_chrdev_remove(counter); > > ida_free(&counter_ida, dev->id); > > + > > + if (!counter->legacy_device) > > + kfree(container_of(counter, struct counter_device_allochelper, counter)); > > } > > > > static struct device_type counter_device_type = { > > @@ -53,7 +62,14 @@ static dev_t counter_devt; > > */ > > void *counter_priv(const struct counter_device *const counter) > > { > > - return counter->priv; > > + if (counter->legacy_device) { > > + return counter->priv; > > + } else { > > + struct counter_device_allochelper *ch = > > + container_of(counter, struct counter_device_allochelper, counter); > > + > > + return &ch->privdata; > > + } > > } > > EXPORT_SYMBOL_GPL(counter_priv); > > > > @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter) > > int id; > > int err; > > > > + counter->legacy_device = true; > > + > > /* Acquire unique ID */ > > id = ida_alloc(&counter_ida, GFP_KERNEL); > > if (id < 0) > > @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter) > > } > > EXPORT_SYMBOL_GPL(counter_register); > > > > +/** > > + * counter_alloc - allocate a counter_device > > + * @sizeof_priv: size of the driver private data > > + * > > + * This is part one of counter registration. The structure is allocated > > + * dynamically to ensure the right lifetime for the embedded struct device. > > + * > > + * If this succeeds, call counter_put() to get rid of the counter_device again. > > + */ > > +struct counter_device *counter_alloc(size_t sizeof_priv) > > +{ > > + struct counter_device_allochelper *ch; > > + struct counter_device *counter; > > + struct device *dev; > > + int id, err; > > + > > + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > > + if (!ch) { > > + err = -ENOMEM; > > + goto err_alloc_ch; > > + } > > + > > + counter = &ch->counter; > > + dev = &counter->dev; > > + > > + /* Acquire unique ID */ > > + err = ida_alloc(&counter_ida, GFP_KERNEL); > > + if (err < 0) { > > + goto err_ida_alloc; > > + } > > + dev->id = err; > > + > > + err = counter_chrdev_add(counter); > > Should think about renaming counter_chrdev_add() given it > does init and allocation stuff but no adding. This is orthogonal to this series though. So I won't address this in the context of the lifetime fixes here. > Personal inclination here would be to keep the elements in here > in the same order as in counter_register() where it doesn't matter > in the interests of slightly easier comparison of the code. I reordered a bit because counter_register fails to undo ida_alloc() in the error path. However I might have missed that some initialisation has to be done before calling counter_chrdev_add(). Will check in detail for v3. Best regards Uwe
Hello, On Wed, Dec 29, 2021 at 12:27:32PM +0100, Uwe Kleine-König wrote: > > Personal inclination here would be to keep the elements in here > > in the same order as in counter_register() where it doesn't matter > > in the interests of slightly easier comparison of the code. > > I reordered a bit because counter_register fails to undo ida_alloc() in > the error path. However I might have missed that some initialisation has > to be done before calling counter_chrdev_add(). Another issue in counter_register() is: If counter_sysfs_add() fails, put_device() is called which eventually calls counter_device_release() -> counter_chrdev_remove() -> kfifo_free(). I think it's not a real problem, but it's ugly because because counter_chrdev_add() -> kfifo_alloc() wasn't called yet. Best regards Uwe
On 12/28/21 20:00, Jonathan Cameron wrote: > On Mon, 27 Dec 2021 10:45:16 +0100 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: >> + if (err < 0) >> + goto err_chrdev_add; >> + >> + device_initialize(dev); >> + /* Configure device structure for Counter */ >> + dev->type = &counter_device_type; >> + dev->bus = &counter_bus_type; >> + dev->devt = MKDEV(MAJOR(counter_devt), id); > > As 0-day pointed out id not initialized. > This was the reason why second counter instance initialization failed for me when testing the patch 17. I fixed it locally by changing the line a few rows above the MKDEV(): - dev->id = err; + dev->id = id = err;
On Wed, Dec 29, 2021 at 03:49:29PM +0200, Jarkko Nikula wrote: > On 12/28/21 20:00, Jonathan Cameron wrote: > > On Mon, 27 Dec 2021 10:45:16 +0100 > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > + if (err < 0) > > > + goto err_chrdev_add; > > > + > > > + device_initialize(dev); > > > + /* Configure device structure for Counter */ > > > + dev->type = &counter_device_type; > > > + dev->bus = &counter_bus_type; > > > + dev->devt = MKDEV(MAJOR(counter_devt), id); > > > > As 0-day pointed out id not initialized. > > > This was the reason why second counter instance initialization failed for me > when testing the patch 17. I fixed it locally by changing the line a few > rows above the MKDEV(): > > - dev->id = err; > + dev->id = id = err; Instead I dropped id for v3. I failed to check this mailthread again before sending it out. So I missed your feedback. I will go through it again and comment later. Best regards Uwe
Hi "Uwe,
url: https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/counter-cleanups-and-device-lifetime-fixes/20211227-174815
base: a7904a538933c525096ca2ccde1e60d0ee62c08e
config: i386-randconfig-m021-20211227 (https://download.01.org/0day-ci/archive/20211229/202112290340.PMwi47LV-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/counter/counter-core.c:175 counter_alloc() error: uninitialized symbol 'id'.
vim +/id +175 drivers/counter/counter-core.c
250e0e3d91caea Uwe Kleine-König 2021-12-27 144 struct counter_device *counter_alloc(size_t sizeof_priv)
250e0e3d91caea Uwe Kleine-König 2021-12-27 145 {
250e0e3d91caea Uwe Kleine-König 2021-12-27 146 struct counter_device_allochelper *ch;
250e0e3d91caea Uwe Kleine-König 2021-12-27 147 struct counter_device *counter;
250e0e3d91caea Uwe Kleine-König 2021-12-27 148 struct device *dev;
250e0e3d91caea Uwe Kleine-König 2021-12-27 149 int id, err;
250e0e3d91caea Uwe Kleine-König 2021-12-27 150
250e0e3d91caea Uwe Kleine-König 2021-12-27 151 ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL);
250e0e3d91caea Uwe Kleine-König 2021-12-27 152 if (!ch) {
250e0e3d91caea Uwe Kleine-König 2021-12-27 153 err = -ENOMEM;
250e0e3d91caea Uwe Kleine-König 2021-12-27 154 goto err_alloc_ch;
250e0e3d91caea Uwe Kleine-König 2021-12-27 155 }
250e0e3d91caea Uwe Kleine-König 2021-12-27 156
250e0e3d91caea Uwe Kleine-König 2021-12-27 157 counter = &ch->counter;
250e0e3d91caea Uwe Kleine-König 2021-12-27 158 dev = &counter->dev;
250e0e3d91caea Uwe Kleine-König 2021-12-27 159
250e0e3d91caea Uwe Kleine-König 2021-12-27 160 /* Acquire unique ID */
250e0e3d91caea Uwe Kleine-König 2021-12-27 161 err = ida_alloc(&counter_ida, GFP_KERNEL);
250e0e3d91caea Uwe Kleine-König 2021-12-27 162 if (err < 0) {
250e0e3d91caea Uwe Kleine-König 2021-12-27 163 goto err_ida_alloc;
250e0e3d91caea Uwe Kleine-König 2021-12-27 164 }
250e0e3d91caea Uwe Kleine-König 2021-12-27 165 dev->id = err;
250e0e3d91caea Uwe Kleine-König 2021-12-27 166
250e0e3d91caea Uwe Kleine-König 2021-12-27 167 err = counter_chrdev_add(counter);
250e0e3d91caea Uwe Kleine-König 2021-12-27 168 if (err < 0)
250e0e3d91caea Uwe Kleine-König 2021-12-27 169 goto err_chrdev_add;
250e0e3d91caea Uwe Kleine-König 2021-12-27 170
250e0e3d91caea Uwe Kleine-König 2021-12-27 171 device_initialize(dev);
250e0e3d91caea Uwe Kleine-König 2021-12-27 172 /* Configure device structure for Counter */
250e0e3d91caea Uwe Kleine-König 2021-12-27 173 dev->type = &counter_device_type;
250e0e3d91caea Uwe Kleine-König 2021-12-27 174 dev->bus = &counter_bus_type;
250e0e3d91caea Uwe Kleine-König 2021-12-27 @175 dev->devt = MKDEV(MAJOR(counter_devt), id);
"id" is uninitialized. Should this be "dev->id"?
250e0e3d91caea Uwe Kleine-König 2021-12-27 176
250e0e3d91caea Uwe Kleine-König 2021-12-27 177 mutex_init(&counter->ops_exist_lock);
250e0e3d91caea Uwe Kleine-König 2021-12-27 178
250e0e3d91caea Uwe Kleine-König 2021-12-27 179 return counter;
250e0e3d91caea Uwe Kleine-König 2021-12-27 180
250e0e3d91caea Uwe Kleine-König 2021-12-27 181 err_chrdev_add:
250e0e3d91caea Uwe Kleine-König 2021-12-27 182
250e0e3d91caea Uwe Kleine-König 2021-12-27 183 ida_free(&counter_ida, dev->id);
250e0e3d91caea Uwe Kleine-König 2021-12-27 184 err_ida_alloc:
250e0e3d91caea Uwe Kleine-König 2021-12-27 185
250e0e3d91caea Uwe Kleine-König 2021-12-27 186 kfree(ch);
250e0e3d91caea Uwe Kleine-König 2021-12-27 187 err_alloc_ch:
250e0e3d91caea Uwe Kleine-König 2021-12-27 188
250e0e3d91caea Uwe Kleine-König 2021-12-27 189 return ERR_PTR(err);
250e0e3d91caea Uwe Kleine-König 2021-12-27 190 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c index 00c41f28c101..8261567b6272 100644 --- a/drivers/counter/counter-core.c +++ b/drivers/counter/counter-core.c @@ -15,6 +15,7 @@ #include <linux/kdev_t.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/slab.h> #include <linux/types.h> #include <linux/wait.h> @@ -24,6 +25,11 @@ /* Provides a unique ID for each counter device */ static DEFINE_IDA(counter_ida); +struct counter_device_allochelper { + struct counter_device counter; + unsigned long privdata[]; +}; + static void counter_device_release(struct device *dev) { struct counter_device *const counter = @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev) counter_chrdev_remove(counter); ida_free(&counter_ida, dev->id); + + if (!counter->legacy_device) + kfree(container_of(counter, struct counter_device_allochelper, counter)); } static struct device_type counter_device_type = { @@ -53,7 +62,14 @@ static dev_t counter_devt; */ void *counter_priv(const struct counter_device *const counter) { - return counter->priv; + if (counter->legacy_device) { + return counter->priv; + } else { + struct counter_device_allochelper *ch = + container_of(counter, struct counter_device_allochelper, counter); + + return &ch->privdata; + } } EXPORT_SYMBOL_GPL(counter_priv); @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter) int id; int err; + counter->legacy_device = true; + /* Acquire unique ID */ id = ida_alloc(&counter_ida, GFP_KERNEL); if (id < 0) @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter) } EXPORT_SYMBOL_GPL(counter_register); +/** + * counter_alloc - allocate a counter_device + * @sizeof_priv: size of the driver private data + * + * This is part one of counter registration. The structure is allocated + * dynamically to ensure the right lifetime for the embedded struct device. + * + * If this succeeds, call counter_put() to get rid of the counter_device again. + */ +struct counter_device *counter_alloc(size_t sizeof_priv) +{ + struct counter_device_allochelper *ch; + struct counter_device *counter; + struct device *dev; + int id, err; + + ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); + if (!ch) { + err = -ENOMEM; + goto err_alloc_ch; + } + + counter = &ch->counter; + dev = &counter->dev; + + /* Acquire unique ID */ + err = ida_alloc(&counter_ida, GFP_KERNEL); + if (err < 0) { + goto err_ida_alloc; + } + dev->id = err; + + err = counter_chrdev_add(counter); + if (err < 0) + goto err_chrdev_add; + + device_initialize(dev); + /* Configure device structure for Counter */ + dev->type = &counter_device_type; + dev->bus = &counter_bus_type; + dev->devt = MKDEV(MAJOR(counter_devt), id); + + mutex_init(&counter->ops_exist_lock); + + return counter; + +err_chrdev_add: + + ida_free(&counter_ida, dev->id); +err_ida_alloc: + + kfree(ch); +err_alloc_ch: + + return ERR_PTR(err); +} +EXPORT_SYMBOL_GPL(counter_alloc); + +void counter_put(struct counter_device *counter) +{ + put_device(&counter->dev); +} + +/** + * counter_add - complete registration of a counter + * @counter: the counter to add + * + * This is part two of counter registration. + * + * If this succeeds, call counter_unregister() to get rid of the counter_device again. + */ +int counter_add(struct counter_device *counter) +{ + int err; + struct device *dev = &counter->dev; + + get_device(&counter->dev); + + if (counter->parent) { + dev->parent = counter->parent; + dev->of_node = counter->parent->of_node; + } + + err = counter_sysfs_add(counter); + if (err < 0) + return err; + + /* implies device_add(dev) */ + err = cdev_device_add(&counter->chrdev, dev); + + return err; +} +EXPORT_SYMBOL_GPL(counter_add); + /** * counter_unregister - unregister Counter from the system * @counter: pointer to Counter to unregister @@ -168,6 +280,41 @@ int devm_counter_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_counter_register); +static void devm_counter_put(void *counter) +{ + counter_put(counter); +} + +struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv) +{ + struct counter_device *counter; + int err; + + counter = counter_alloc(sizeof_priv); + if (IS_ERR(counter)) + return counter; + + err = devm_add_action_or_reset(dev, devm_counter_put, counter); + if (err < 0) + return ERR_PTR(err); + + return counter; +} +EXPORT_SYMBOL_GPL(devm_counter_alloc); + +int devm_counter_add(struct device *dev, + struct counter_device *const counter) +{ + int err; + + err = counter_add(counter); + if (err < 0) + return err; + + return devm_add_action_or_reset(dev, devm_counter_release, counter); +} +EXPORT_SYMBOL_GPL(devm_counter_add); + #define COUNTER_DEV_MAX 256 static int __init counter_init(void) diff --git a/include/linux/counter.h b/include/linux/counter.h index 8daaa38c71d8..f1350a43cd48 100644 --- a/include/linux/counter.h +++ b/include/linux/counter.h @@ -327,14 +327,29 @@ struct counter_device { spinlock_t events_in_lock; struct mutex events_out_lock; struct mutex ops_exist_lock; + + /* + * This can go away once all drivers are converted to + * counter_alloc()/counter_add(). + */ + bool legacy_device; }; void *counter_priv(const struct counter_device *const counter); int counter_register(struct counter_device *const counter); + +struct counter_device *counter_alloc(size_t sizeof_priv); +void counter_put(struct counter_device *const counter); +int counter_add(struct counter_device *const counter); + void counter_unregister(struct counter_device *const counter); int devm_counter_register(struct device *dev, struct counter_device *const counter); +struct counter_device *devm_counter_alloc(struct device *dev, + size_t sizeof_priv); +int devm_counter_add(struct device *dev, + struct counter_device *const counter); void counter_push_event(struct counter_device *const counter, const u8 event, const u8 channel);
The current implementation gets device lifetime tracking wrong. The problem is that allocation of struct counter_device is controlled by the individual drivers but this structure contains a struct device that might have to live longer than a driver is bound. As a result a command sequence like: { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo 40000000.timer:counter > /sys/bus/platform/drivers/stm32-timer-counter/unbind can keep a reference to the struct device and unbinding results in freeing the memory occupied by this device resulting in an oops. This commit provides two new functions (plus some helpers): - counter_alloc() to allocate a struct counter_device that is automatically freed once the embedded struct device is released - counter_add() to register such a device. Note that this commit doesn't fix any issues, all drivers have to be converted to these new functions to correct the lifetime problems. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/counter/counter-core.c | 149 ++++++++++++++++++++++++++++++++- include/linux/counter.h | 15 ++++ 2 files changed, 163 insertions(+), 1 deletion(-)