[1/2] mtd: use put_device() if device_register fail

Message ID 39c4af723ea37cd05976a17cb9c1fbc975496ffd.1520592440.git.arvind.yadav.cs@gmail.com
State Changes Requested
Delegated to: Boris Brezillon
Headers show
Series
  • mtd: use put_device() if device_register fail
Related show

Commit Message

Arvind Yadav March 9, 2018, 10:50 a.m.
if device_register() returned an error! Always use put_device()
to give up the reference initialized.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/mtd/mtdcore.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Boris Brezillon March 14, 2018, 2:36 p.m. | #1
On Fri,  9 Mar 2018 16:20:48 +0530
Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:

> if device_register() returned an error! Always use put_device()
> to give up the reference initialized.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  drivers/mtd/mtdcore.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 28553c8..4d77ca2 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd)
>  	return 0;
>  
>  fail_added:
> +	put_device(&mtd->dev);

Not sure this is a good idea: the put_device() call will trigger
an mtd_devtype->release(), which will in turn call device_destroy() on
something that does not exist yet. Not sure if this is a real problem,
but it does not look like the right thing to do.

>  	of_node_put(mtd_get_of_node(mtd));

You're referencing an object that is supposed to have been
freed/released by the put_device() call. Again, it's not really a
problem because in our case ->release() does not free the mtd object
(as is usually done in other parts of the kernel), but it still looks
wrong. It's probably better to move the of_node_put() and the below
idr_remove() call in the ->release() hook if you want to use
put_device().

>  	idr_remove(&mtd_idr, i);



>  fail_locked:
Arvind Yadav March 17, 2018, 9:45 a.m. | #2
On Wednesday 14 March 2018 08:06 PM, Boris Brezillon wrote:
> On Fri,  9 Mar 2018 16:20:48 +0530
> Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
>
>> if device_register() returned an error! Always use put_device()
>> to give up the reference initialized.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>>   drivers/mtd/mtdcore.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index 28553c8..4d77ca2 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd)
>>   	return 0;
>>   
>>   fail_added:
>> +	put_device(&mtd->dev);
> Not sure this is a good idea: the put_device() call will trigger
> an mtd_devtype->release(), which will in turn call device_destroy() on
> something that does not exist yet. Not sure if this is a real problem,
> but it does not look like the right thing to do.
>
yes, you are correct. No need to call put_device().
which can cause other problem.

>>   	of_node_put(mtd_get_of_node(mtd));
> You're referencing an object that is supposed to have been
> freed/released by the put_device() call. Again, it's not really a
> problem because in our case ->release() does not free the mtd object
> (as is usually done in other parts of the kernel), but it still looks
> wrong. It's probably better to move the of_node_put() and the below
> idr_remove() call in the ->release() hook if you want to use
> put_device().
>
>>   	idr_remove(&mtd_idr, i);
Sure, we can move put_device() below this. But need to check
how we can add hook in release.
>
>>   fail_locked:
>
>
~arvind
Martin Habets March 19, 2018, 10:43 a.m. | #3
On 17/03/18 09:45, arvindY wrote:
>>>       of_node_put(mtd_get_of_node(mtd));
>> You're referencing an object that is supposed to have been
>> freed/released by the put_device() call. Again, it's not really a
>> problem because in our case ->release() does not free the mtd object
>> (as is usually done in other parts of the kernel), but it still looks
>> wrong. It's probably better to move the of_node_put() and the below
>> idr_remove() call in the ->release() hook if you want to use
>> put_device().
>>
>>>       idr_remove(&mtd_idr, i);
> Sure, we can move put_device() below this. But need to check
> how we can add hook in release.

My guess is that you would need this:
http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html

Martin

>>>   fail_locked:
>>
>>
> ~arvind
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Martin Habets March 21, 2018, 10:08 a.m. | #4
Hi Arvind,

On 19/03/18 17:56, arvindY wrote:
> 
> 
> On Monday 19 March 2018 04:13 PM, Martin Habets wrote:
>> On 17/03/18 09:45, arvindY wrote:
>>>>>       of_node_put(mtd_get_of_node(mtd));
>>>> You're referencing an object that is supposed to have been
>>>> freed/released by the put_device() call. Again, it's not really a
>>>> problem because in our case ->release() does not free the mtd object
>>>> (as is usually done in other parts of the kernel), but it still looks
>>>> wrong. It's probably better to move the of_node_put() and the below
>>>> idr_remove() call in the ->release() hook if you want to use
>>>> put_device().
>>>>
>>>>>       idr_remove(&mtd_idr, i);
>>> Sure, we can move put_device() below this. But need to check
>>> how we can add hook in release.
>> My guess is that you would need this:
>> http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html
> 
> we should not removes(device_destroy) a MTD device in
> release. MTD device should be removes when
> deleting(unregister) a MTD device.

No, deleting an MTD device should only decrement a refcounter.
At this point there can still be other processes with a /dev/mtd*
device open.
When there are no more users release gets called to remove it.

> MTD device should decrement refcount of a node and
> Remove MTD from IDR in dev->release().

You could be right about this, I'm not sure.

My patch allows the caller to free the mtd_info memory. This is needed since
the caller allocated the memory in the first place, and because the caller has
no other of knowing that the last MTD user is gone.

Martin

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 28553c8..4d77ca2 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -586,6 +586,7 @@  int add_mtd_device(struct mtd_info *mtd)
 	return 0;
 
 fail_added:
+	put_device(&mtd->dev);
 	of_node_put(mtd_get_of_node(mtd));
 	idr_remove(&mtd_idr, i);
 fail_locked: