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 <[email protected]>
---
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
Hi
On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga ([email protected]) 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 <[email protected]>
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 <[email protected]>
--
Evgeniy Polyakov
Hi Evgeniy,
On Wed, Jun 20, 2012 at 1:36 PM, Evgeniy Polyakov <[email protected]> wrote:
> Hi
>
> On Sun, Jun 17, 2012 at 09:47:59PM +0530, Devendra Naga ([email protected]) 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 <[email protected]>
>
> 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 <[email protected]>
>
> --
> ? ? ? ?Evgeniy Polyakov
Thanks,
Devendra.
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 <[email protected]>
> Acked-by: Evgeniy Polyakov <[email protected]>
> ---
> 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
Hi Greg,
On Thu, Jun 21, 2012 at 5:25 AM, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
>> Acked-by: Evgeniy Polyakov <[email protected]>
>> ---
>> ?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.
On Wed, Jun 20, 2012 at 04:55:03PM -0700, Greg Kroah-Hartman ([email protected]) 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
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
> <[email protected]> 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 <[email protected]>
> >> Acked-by: Evgeniy Polyakov <[email protected]>
> >> ---
> >> ?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
Hi Greg,
On Thu, Jun 21, 2012 at 8:09 PM, Greg Kroah-Hartman
<[email protected]> 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.