Skip to contentSkip to conversation

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

PATCH In review Devendra Naga 3 participants 8 messages Jun 17, 2012 – Jun 23, 2012 W1 dallas's 1-wire bus

lore
Share & tools
lore

Devendra Naga

author
#0 lore raw

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

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(-)
drivers/w1/w1_int.c +2 -1
index 6828835..a3cf27d 100644
@@ -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);
Signature · 1 line
1.7.9.5
Acked-by on v1 1/1
#1 lore raw

Hi

Quoted 8 lines · 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 <[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]>
Signature · 1 line
	Evgeniy Polyakov
author
#2 lore raw

Hi Evgeniy,

Quoted 6 lines · Evgeniy Polyakov

Hi

Quoted 3 lines · Devendra Naga

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

Quoted 9 lines
Quoted 5 lines

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.

Quoted 3 lines

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....

Quoted 5 lines

Greg, please pull it into your tree. Thank you.

Acked-by Evgeniy Polyakov <[email protected]>

--
       Evgeniy Polyakov

Thanks,
Devendra.

#3 lore raw
In reply to Devendra Naga
Quoted 18 lines · 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 <[email protected]>
Acked-by Evgeniy Polyakov <[email protected]>
---
 drivers/w1/w1_int.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/w1/w1_int.c +0 -0
index 6828835..a3cf27d 100644

drivers/w1/w1_int.c static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl, · line 103

Quoted 4 lines
drivers/w1/w1_int.c +1 -0
@@ -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

author
#4 lore raw

Hi Greg,

Quoted 14 lines · Greg Kroah-Hartman
Quoted 13 lines · 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 <[email protected]>
Acked-by Evgeniy Polyakov <[email protected]>

---
 drivers/w1/w1_int.c |    3 ++-

 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 13 lines

drivers/w1/w1_int.c static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl, · line 103

Quoted 9 lines
drivers/w1/w1_int.c +1 -0
index 6828835..a3cf27d 100644
@@ -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;
       }
Quoted 1 line

greg k-h

Thanks,
Devendra.

#5 lore raw
Quoted 35 lines · devendra.aaru

Hi Greg,

Quoted 14 lines · Greg Kroah-Hartman
Quoted 13 lines · 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 <[email protected]>
Acked-by Evgeniy Polyakov <[email protected]>

---
 drivers/w1/w1_int.c |    3 ++-

 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 13 lines

drivers/w1/w1_int.c static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl, · line 103

Quoted 9 lines
drivers/w1/w1_int.c +1 -0
index 6828835..a3cf27d 100644
@@ -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)

drivers/w1/w1_int.c int w1_add_master_device(struct w1_bus_master *master) · line 152

Quoted 5 lines
drivers/w1/w1_int.c +1 -1
@@ -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

author
#6 lore raw

Hi Greg,

Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines · devendra.aaru

Hi Greg,

Yeah, correct but the following change is correct no?

Quoted 11 lines

drivers/w1/w1_int.c int w1_add_master_device(struct w1_bus_master *master) · line 152

Quoted 7 lines

 int w1_add_master_device(struct w1_bus_master *master)

drivers/w1/w1_int.c +1 -1
@@ -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.

Quoted 3 lines

thanks,

greg k-h

Thanks,
Devendra.

#7 lore raw
In reply to Greg Kroah-Hartman
Quoted 2 lines · Greg Kroah-Hartman

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!

Signature · 1 line
	Evgeniy Polyakov