[PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[Devendra Naga]

the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
w1_remove_master.

when w1_alloc_dev fails the return should be -ENODEV as it does
device_register, and that is the last case where that function
will fail.

Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
 drivers/w1/w1_int.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 6828835..a3cf27d 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -100,6 +100,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 static void w1_free_dev(struct w1_master *dev)
 {
 	device_unregister(&dev->dev);
+	kfree(dev);
 }
 
 int w1_add_master_device(struct w1_bus_master *master)
@@ -148,7 +149,7 @@ int w1_add_master_device(struct w1_bus_master *master)
 		&w1_master_driver, &w1_master_device);
 	if (!dev) {
 		mutex_unlock(&w1_mlock);
-		return -ENOMEM;
+		return -ENODEV;
 	}
 
 	retval =  w1_create_master_attributes(dev);
-- 
1.7.9.5

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[Evgeniy Polyakov]

Hi

On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga (devendra.aaru@gmail.com) wrote:
> the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
> w1_remove_master.
> 
> when w1_alloc_dev fails the return should be -ENODEV as it does
> device_register, and that is the last case where that function
> will fail.
> 
> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>

Hmm, looks correct, but I wonder how whatever_free() function happend
not to free its arguments.

Looks like device_unregister() calls release callback, but we do not
provide one.

Greg, please pull it into your tree. Thank you.
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

-- 
	Evgeniy Polyakov

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[devendra.aaru]

Hi Evgeniy,
On Wed, Jun 20, 2012 at 1:36 PM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> Hi
>
> On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga (devendra.aaru@gmail.com) wrote:
>> the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
>> w1_remove_master.
>>
>> when w1_alloc_dev fails the return should be -ENODEV as it does
>> device_register, and that is the last case where that function
>> will fail.
>>
>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
>
> Hmm, looks correct, but I wonder how whatever_free() function happend
> not to free its arguments.
>
I think , it its good to have the kfree after calling the w1_free_dev
as this way looks so wierd calling of kfree.
> Looks like device_unregister() calls release callback, but we do not
> provide one.
>
vim -t device_unregister points me to drivers/base/core.c
device_unregister function, where we do a device_del, where we do a
dev_release_all where it calls  release_nodes which calls the release
callback. is that what you are telling?

if so actually we are passing the structure w1_master_device which is
of struct device. its having a release callback. and there we free the
master device.

please suggest me if i mistaken....
> Greg, please pull it into your tree. Thank you.
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
>
> --
>        Evgeniy Polyakov

Thanks,
Devendra.

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[Greg Kroah-Hartman]

On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga wrote:
> the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
> w1_remove_master.
> 
> when w1_alloc_dev fails the return should be -ENODEV as it does
> device_register, and that is the last case where that function
> will fail.
> 
> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
> ---
>  drivers/w1/w1_int.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
> index 6828835..a3cf27d 100644
> --- a/drivers/w1/w1_int.c
> +++ b/drivers/w1/w1_int.c
> @@ -100,6 +100,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
>  static void w1_free_dev(struct w1_master *dev)
>  {
>  	device_unregister(&dev->dev);
> +	kfree(dev);

No, this is wrong, the memory will be freed in w1_master_release(),
right?  It is not freed here, sorry, this patch is incorrect.

greg k-h

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[devendra.aaru]

Hi Greg,

On Thu, Jun 21, 2012 at 5:25 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga wrote:
>> the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
>> w1_remove_master.
>>
>> when w1_alloc_dev fails the return should be -ENODEV as it does
>> device_register, and that is the last case where that function
>> will fail.
>>
>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
>> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
>> ---
>>  drivers/w1/w1_int.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
>> index 6828835..a3cf27d 100644
>> --- a/drivers/w1/w1_int.c
>> +++ b/drivers/w1/w1_int.c
>> @@ -100,6 +100,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
>>  static void w1_free_dev(struct w1_master *dev)
>>  {
>>       device_unregister(&dev->dev);
>> +     kfree(dev);
>
> No, this is wrong, the memory will be freed in w1_master_release(),
> right?  It is not freed here, sorry, this patch is incorrect.
>
Yeah, correct but the following change is correct no?

 int w1_add_master_device(struct w1_bus_master *master)
@@ -148,7 +149,7 @@ int w1_add_master_device(struct w1_bus_master *master)
               &w1_master_driver, &w1_master_device);
       if (!dev) {
               mutex_unlock(&w1_mlock);
-               return -ENOMEM;
+               return -ENODEV;
       }


> greg k-h

Thanks,
Devendra.

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[Evgeniy Polyakov]

On Wed, Jun 20, 2012 at 04:55:03PM -0700, Greg Kroah-Hartman (gregkh@linuxfoundation.org) wrote:
> No, this is wrong, the memory will be freed in w1_master_release(),
> right?  It is not freed here, sorry, this patch is incorrect.

Yes, you are right.
It is exactly that ->release() callback I missed!

-- 
	Evgeniy Polyakov

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[Greg Kroah-Hartman]

On Thu, Jun 21, 2012 at 10:14:53AM +0530, devendra.aaru wrote:
> Hi Greg,
> 
> On Thu, Jun 21, 2012 at 5:25 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga wrote:
> >> the w1_master pointer is allced at the w1_alloc_master and is not freed when called with
> >> w1_remove_master.
> >>
> >> when w1_alloc_dev fails the return should be -ENODEV as it does
> >> device_register, and that is the last case where that function
> >> will fail.
> >>
> >> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
> >> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
> >> ---
> >>  drivers/w1/w1_int.c |    3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
> >> index 6828835..a3cf27d 100644
> >> --- a/drivers/w1/w1_int.c
> >> +++ b/drivers/w1/w1_int.c
> >> @@ -100,6 +100,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
> >>  static void w1_free_dev(struct w1_master *dev)
> >>  {
> >>       device_unregister(&dev->dev);
> >> +     kfree(dev);
> >
> > No, this is wrong, the memory will be freed in w1_master_release(),
> > right?  It is not freed here, sorry, this patch is incorrect.
> >
> Yeah, correct but the following change is correct no?
> 
>  int w1_add_master_device(struct w1_bus_master *master)
> @@ -148,7 +149,7 @@ int w1_add_master_device(struct w1_bus_master *master)
>                &w1_master_driver, &w1_master_device);
>        if (!dev) {
>                mutex_unlock(&w1_mlock);
> -               return -ENOMEM;
> +               return -ENODEV;

Possibly, care to resend it in a format that explains it and allows it
to be applied?

thanks,

greg k-h

Re: [PATCH] drivers/w1: free the w1_master at w1_free_dev and return a correct return value

[devendra.aaru]

Hi Greg,
On Thu, Jun 21, 2012 at 8:09 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 21, 2012 at 10:14:53AM +0530, devendra.aaru wrote:
>> Hi Greg,
>>
>> Yeah, correct but the following change is correct no?
>>
>>  int w1_add_master_device(struct w1_bus_master *master)
>> @@ -148,7 +149,7 @@ int w1_add_master_device(struct w1_bus_master *master)
>>                &w1_master_driver, &w1_master_device);
>>        if (!dev) {
>>                mutex_unlock(&w1_mlock);
>> -               return -ENOMEM;
>> +               return -ENODEV;
>
> Possibly, care to resend it in a format that explains it and allows it
> to be applied?
>
I think i need to go through the kernel doc, and figure out what
should be returned and why.
I think we need to send -EINVAL as most of the drivers does if their
registration fails.

It may take more time to send the patch out :(. sorry.
> thanks,
>
> greg k-h

Thanks,
Devendra.