Subject: Re: [PATCH v2 16/23] counter: interrupt-cnt: Convert to new counter
 registration
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     William Breathitt Gray <vilhelm.gray@gmail.com>,
        kernel@pengutronix.de,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ahmad Fatoum <a.fatoum@pengutronix.de>,
        Oleksij Rempel <linux@rempel-privat.de>
References: <20211227094526.698714-1-u.kleine-koenig@pengutronix.de>
 <20211227094526.698714-17-u.kleine-koenig@pengutronix.de>
 <YcmcdftX3YO06nD1@kroah.com>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <7e3a4a67-4e97-8660-9437-0250588d5e5e@metafoo.de>
Date:   Mon, 27 Dec 2021 12:21:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YcmcdftX3YO06nD1@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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.

This is the correct solution to the problem.