Messages in this thread | | | Subject | Re: [PATCH v2 16/23] counter: interrupt-cnt: Convert to new counter registration | From | Lars-Peter Clausen <> | Date | Mon, 27 Dec 2021 12:44:29 +0100 |
| |
On 12/27/21 12:34 PM, Greg Kroah-Hartman wrote: > On Mon, Dec 27, 2021 at 12:21:14PM +0100, Lars-Peter Clausen wrote: >> On 12/27/21 11:59 AM, Greg Kroah-Hartman wrote: >>> On Mon, Dec 27, 2021 at 10:45:19AM +0100, Uwe Kleine-König wrote: >>>> This fixes device lifetime issues where it was possible to free a live >>>> struct device. >>>> >>>> Fixes: a55ebd47f21f ("counter: add IRQ or GPIO based counter") >>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>>> --- >>>> drivers/counter/interrupt-cnt.c | 28 ++++++++++++++++------------ >>>> 1 file changed, 16 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c >>>> index 4bf706ef46e2..9e99702470c2 100644 >>>> --- a/drivers/counter/interrupt-cnt.c >>>> +++ b/drivers/counter/interrupt-cnt.c >>>> @@ -16,7 +16,6 @@ >>>> struct interrupt_cnt_priv { >>>> atomic_t count; >>>> - struct counter_device counter; >>>> struct gpio_desc *gpio; >>>> int irq; >>>> bool enabled; >>>> @@ -148,12 +147,14 @@ static const struct counter_ops interrupt_cnt_ops = { >>>> static int interrupt_cnt_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> + struct counter_device *counter; >>>> struct interrupt_cnt_priv *priv; >>>> int ret; >>>> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>> - if (!priv) >>>> + counter = devm_counter_alloc(dev, sizeof(*priv)); >>> I just picked one of these patches at random, nothing specific about >>> this driver... >>> >>> You can not have a 'struct device' in memory allocated by devm_*() >>> functions for the obvious reason that now that memory is being >>> controlled by a reference count that is OUTSIDE of the structure itself. >>> >>> So while your goal might be good here, this is not the correct solution >>> at all, sorry. >> Before this patch the memory for the struct device was devm_kzalloc'ed. >> Which as you point out is a bug. >> >> After this patch the memory is reference counted and will be freed when the >> last reference is dropped, in the release callback of the struct device. >> >> The alloc function is still a devm_ function, but on 'free' it will only >> drop the reference to the struct device that it holds. This is a very common >> pattern that is used by basically any driver subsystem in the kernel. > Then it is not a real devm_() call, let's not call it that please as it > is obviously very confusing :) > > Just call it counter_alloc(), or , counter_create(), or something a bit > more in line with the rest of all driver subsystems.
But all the other driver subsystems call this kind of function devm_... :)
Usually for everything you call in probe() you need a corresponding action in remove(). With the devm_... function is remove action will be automatically called.
| |