| Message ID | 1387791578-1372-1-git-send-email-lekensteyn@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Nevermind this patch, it does not really fix the memleak because i2c_set_adapdata() calls dev_set_drvdata() which allocates memory. (I must have ran kmemleak too early, right after boot it did not give any warnings, now it does). RFC: what about dropping i2c_set_adapdata() from the probe function and replacing i2c_get_adapdata(adapter) by pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not sure what the purpose is for i2c_set_adapdata, hence this question. Regards, Peter On Monday 23 December 2013 10:39:38 Peter Wu wrote: > The driver-specific data for i801 was only set for the device on > success, that led to a memory leak on error paths (for instance, when > there is a resource conflict with ACPI). (The driver core clears the > driver data (if set) if the probe routine fails). > > Fix it by setting the driver data right after successful memory > allocation, before reaching any error paths. > > References: http://lkml.org/lkml/2013/1/23/191 > Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz> > Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path] > Signed-off-by: Peter Wu <lekensteyn@gmail.com> > --- > Hi Jean, > > This memleak issue is still present in v3.13-rc4-256-gb7000ad. > From kmemleak: > > unreferenced object 0xffff88022f501a00 (size 256): > comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s) > hex dump (first 32 bytes): > 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N.......... > ff ff ff ff ff ff ff ff f4 e2 53 82 ff ff ff ff ..........S..... > backtrace: > [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0 > [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0 > [<ffffffff813efc63>] device_private_init+0x23/0x80 > [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50 > [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801] > [<ffffffff81332d95>] local_pci_probe+0x45/0xa0 > [<ffffffff81333be9>] pci_device_probe+0xd9/0x130 > [<ffffffff813f30e7>] driver_probe_device+0x87/0x390 > [<ffffffff813f34c3>] __driver_attach+0x93/0xa0 > [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0 > [<ffffffff813f2b0e>] driver_attach+0x1e/0x20 > [<ffffffff813f26e8>] bus_add_driver+0x188/0x260 > [<ffffffff813f3b04>] driver_register+0x64/0xf0 > [<ffffffff81332930>] __pci_register_driver+0x60/0x70 > [<ffffffffa02990af>] 0xffffffffa02990af > [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0 > > The dmesg for this laptop also contains a resource conflict message, > just like the reporter (Martin Mokrejs): > > [ 15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251) > [ 15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver > > With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad), > the memleak is gone. > > Regards, > Peter > --- > drivers/i2c/busses/i2c-i801.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 737e298..a7096bf 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > if (!priv) > return -ENOMEM; > > + pci_set_drvdata(dev, priv); > i2c_set_adapdata(&priv->adapter, priv); > priv->adapter.owner = THIS_MODULE; > priv->adapter.class = i801_get_adapter_class(priv); > @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > /* We ignore errors - multiplexing is optional */ > i801_add_mux(priv); > > - pci_set_drvdata(dev, priv); > - > return 0; > > exit_free_irq: > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the note, was just compiling a new 3.10.24 kernel to test it. ;-) So far just booted an old 3.9 kernel and after plugging in an external USB3 drive I got the message, just to be sure I am still able to reproduce the error and that I have the right .config in the running kernel. Will wait for another fix instead. Martin Peter Wu wrote: > Nevermind this patch, it does not really fix the memleak because > i2c_set_adapdata() calls dev_set_drvdata() which allocates memory. > (I must have ran kmemleak too early, right after boot it did not > give any warnings, now it does). > > RFC: what about dropping i2c_set_adapdata() from the probe function > and replacing i2c_get_adapdata(adapter) by > pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not > sure what the purpose is for i2c_set_adapdata, hence this question. > > Regards, > Peter > > On Monday 23 December 2013 10:39:38 Peter Wu wrote: >> The driver-specific data for i801 was only set for the device on >> success, that led to a memory leak on error paths (for instance, when >> there is a resource conflict with ACPI). (The driver core clears the >> driver data (if set) if the probe routine fails). >> >> Fix it by setting the driver data right after successful memory >> allocation, before reaching any error paths. >> >> References: http://lkml.org/lkml/2013/1/23/191 >> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz> >> Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path] >> Signed-off-by: Peter Wu <lekensteyn@gmail.com> >> --- >> Hi Jean, >> >> This memleak issue is still present in v3.13-rc4-256-gb7000ad. >> From kmemleak: >> >> unreferenced object 0xffff88022f501a00 (size 256): >> comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s) >> hex dump (first 32 bytes): >> 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N.......... >> ff ff ff ff ff ff ff ff f4 e2 53 82 ff ff ff ff ..........S..... >> backtrace: >> [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0 >> [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0 >> [<ffffffff813efc63>] device_private_init+0x23/0x80 >> [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50 >> [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801] >> [<ffffffff81332d95>] local_pci_probe+0x45/0xa0 >> [<ffffffff81333be9>] pci_device_probe+0xd9/0x130 >> [<ffffffff813f30e7>] driver_probe_device+0x87/0x390 >> [<ffffffff813f34c3>] __driver_attach+0x93/0xa0 >> [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0 >> [<ffffffff813f2b0e>] driver_attach+0x1e/0x20 >> [<ffffffff813f26e8>] bus_add_driver+0x188/0x260 >> [<ffffffff813f3b04>] driver_register+0x64/0xf0 >> [<ffffffff81332930>] __pci_register_driver+0x60/0x70 >> [<ffffffffa02990af>] 0xffffffffa02990af >> [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0 >> >> The dmesg for this laptop also contains a resource conflict message, >> just like the reporter (Martin Mokrejs): >> >> [ 15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251) >> [ 15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver >> >> With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad), >> the memleak is gone. >> >> Regards, >> Peter >> --- >> drivers/i2c/busses/i2c-i801.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index 737e298..a7096bf 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> if (!priv) >> return -ENOMEM; >> >> + pci_set_drvdata(dev, priv); >> i2c_set_adapdata(&priv->adapter, priv); >> priv->adapter.owner = THIS_MODULE; >> priv->adapter.class = i801_get_adapter_class(priv); >> @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> /* We ignore errors - multiplexing is optional */ >> i801_add_mux(priv); >> >> - pci_set_drvdata(dev, priv); >> - >> return 0; >> >> exit_free_irq: >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 December 2013 11:51:12 Martin Mokrejs wrote: > Thanks for the note, was just compiling a new 3.10.24 kernel to test it. > ;-) > > So far just booted an old 3.9 kernel and after plugging in an external > USB3 drive I got the message, just to be sure I am still able to reproduce > the error and that I have the right .config in the running kernel. > > Will wait for another fix instead. > Martin There is still one thing I do not fully understand, how should dev_set_drvdata and dev_get_drvdata be used? For the devices passed to probe functions, the core takes care of setting to NULL on error. Then device_unregister frees the memory, right? Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata, i2c_set_adapinfo, etc.) are manually called outside probe functions? Or inside the probe function, but not for the device that is being probed (such as is the case with the i801 i2c driver)? The VFIO driver also does something odd, it clears the driver data, but the device holding it is freed using kfree(): static void vfio_device_release(struct kref *kref) { struct vfio_device *device = container_of(kref, struct vfio_device, kref); struct vfio_group *group = device->group; list_del(&device->group_next); mutex_unlock(&group->device_lock); dev_set_drvdata(device->dev, NULL); kfree(device); Is a memory leak also present here since dev_set_drvdata() always tries to allocate memory? Regards, Peter > Peter Wu wrote: > > Nevermind this patch, it does not really fix the memleak because > > i2c_set_adapdata() calls dev_set_drvdata() which allocates memory. > > (I must have ran kmemleak too early, right after boot it did not > > give any warnings, now it does). > > > > RFC: what about dropping i2c_set_adapdata() from the probe function > > and replacing i2c_get_adapdata(adapter) by > > pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not > > sure what the purpose is for i2c_set_adapdata, hence this question. > > > > Regards, > > Peter > > > > On Monday 23 December 2013 10:39:38 Peter Wu wrote: > >> The driver-specific data for i801 was only set for the device on > >> success, that led to a memory leak on error paths (for instance, when > >> there is a resource conflict with ACPI). (The driver core clears the > >> driver data (if set) if the probe routine fails). > >> > >> Fix it by setting the driver data right after successful memory > >> allocation, before reaching any error paths. > >> > >> References: http://lkml.org/lkml/2013/1/23/191 > >> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz> > >> Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path] > >> Signed-off-by: Peter Wu <lekensteyn@gmail.com> > >> --- > >> Hi Jean, > >> > >> This memleak issue is still present in v3.13-rc4-256-gb7000ad. > >> From kmemleak: > >> > >> unreferenced object 0xffff88022f501a00 (size 256): > >> comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s) > >> hex dump (first 32 bytes): > >> 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N.......... > >> ff ff ff ff ff ff ff ff f4 e2 53 82 ff ff ff ff ..........S..... > >> backtrace: > >> [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0 > >> [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0 > >> [<ffffffff813efc63>] device_private_init+0x23/0x80 > >> [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50 > >> [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801] > >> [<ffffffff81332d95>] local_pci_probe+0x45/0xa0 > >> [<ffffffff81333be9>] pci_device_probe+0xd9/0x130 > >> [<ffffffff813f30e7>] driver_probe_device+0x87/0x390 > >> [<ffffffff813f34c3>] __driver_attach+0x93/0xa0 > >> [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0 > >> [<ffffffff813f2b0e>] driver_attach+0x1e/0x20 > >> [<ffffffff813f26e8>] bus_add_driver+0x188/0x260 > >> [<ffffffff813f3b04>] driver_register+0x64/0xf0 > >> [<ffffffff81332930>] __pci_register_driver+0x60/0x70 > >> [<ffffffffa02990af>] 0xffffffffa02990af > >> [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0 > >> > >> The dmesg for this laptop also contains a resource conflict message, > >> just like the reporter (Martin Mokrejs): > >> > >> [ 15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251) > >> [ 15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver > >> > >> With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad), > >> the memleak is gone. > >> > >> Regards, > >> Peter > >> --- > >> drivers/i2c/busses/i2c-i801.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > >> index 737e298..a7096bf 100644 > >> --- a/drivers/i2c/busses/i2c-i801.c > >> +++ b/drivers/i2c/busses/i2c-i801.c > >> @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > >> if (!priv) > >> return -ENOMEM; > >> > >> + pci_set_drvdata(dev, priv); > >> i2c_set_adapdata(&priv->adapter, priv); > >> priv->adapter.owner = THIS_MODULE; > >> priv->adapter.class = i801_get_adapter_class(priv); > >> @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > >> /* We ignore errors - multiplexing is optional */ > >> i801_add_mux(priv); > >> > >> - pci_set_drvdata(dev, priv); > >> - > >> return 0; > >> > >> exit_free_irq: > >> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote: > On Monday 23 December 2013 11:51:12 Martin Mokrejs wrote: > > Thanks for the note, was just compiling a new 3.10.24 kernel to test it. > > ;-) > > > > So far just booted an old 3.9 kernel and after plugging in an external > > USB3 drive I got the message, just to be sure I am still able to reproduce > > the error and that I have the right .config in the running kernel. > > > > Will wait for another fix instead. > > Martin > > There is still one thing I do not fully understand, how should > dev_set_drvdata and dev_get_drvdata be used? For the devices passed > to probe functions, the core takes care of setting to NULL on error. > Then device_unregister frees the memory, right? > > Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata, > i2c_set_adapinfo, etc.) are manually called outside probe functions? > Or inside the probe function, but not for the device that is being > probed (such as is the case with the i801 i2c driver)? > > The VFIO driver also does something odd, it clears the driver data, > but the device holding it is freed using kfree(): > > static void vfio_device_release(struct kref *kref) { > struct vfio_device *device = container_of(kref, > struct vfio_device, kref); > struct vfio_group *group = device->group; > > list_del(&device->group_next); > mutex_unlock(&group->device_lock); > > dev_set_drvdata(device->dev, NULL); > > kfree(device); > > Is a memory leak also present here since dev_set_drvdata() always tries to > allocate memory? But it doesn't: int dev_set_drvdata(struct device *dev, void *data) { int error; if (!dev->p) { error = device_private_init(dev); if (error) return error; } dev->p->driver_data = data; return 0; } Also, the code referenced is kfree'ing a struct vfio_device, not the struct device. VFIO uses the drvdata to provide a back pointer to the vfio specific structure, which also includes a pointer to the struct device. We obviously want to clear drvdata when the vfio specific structure is being released. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 December 2013 10:37:21 Alex Williamson wrote: > On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote: [..] > > > > There is still one thing I do not fully understand, how should > > dev_set_drvdata and dev_get_drvdata be used? For the devices passed > > to probe functions, the core takes care of setting to NULL on error. > > Then device_unregister frees the memory, right? > > > > Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata, > > i2c_set_adapinfo, etc.) are manually called outside probe functions? > > Or inside the probe function, but not for the device that is being > > probed (such as is the case with the i801 i2c driver)? > > > > The VFIO driver also does something odd, it clears the driver data, > > but the device holding it is freed using kfree(): > > > > static void vfio_device_release(struct kref *kref) { > > struct vfio_device *device = container_of(kref, > > struct vfio_device, kref); > > struct vfio_group *group = device->group; > > > > list_del(&device->group_next); > > mutex_unlock(&group->device_lock); > > > > dev_set_drvdata(device->dev, NULL); > > > > kfree(device); > > > > Is a memory leak also present here since dev_set_drvdata() always tries to > > allocate memory? > > But it doesn't: > > int dev_set_drvdata(struct device *dev, void *data) > { > int error; > > if (!dev->p) { > error = device_private_init(dev); > if (error) > return error; > } > dev->p->driver_data = data; > return 0; > } It does: int device_private_init(struct device *dev) { dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL); if (!dev->p) return -ENOMEM; dev->p->device = dev; klist_init(&dev->p->klist_children, klist_children_get, klist_children_put); INIT_LIST_HEAD(&dev->p->deferred_probe); return 0; } and if it doesn't, then I must be missing something in this non-obvious code. I scanned the interwebs and Documentation/, but could not really find a great example on how this is supposed to work. The dev_set_drvdata function existed since Linus moved to git. > Also, the code referenced is kfree'ing a struct vfio_device, not the > struct device. VFIO uses the drvdata to provide a back pointer to the > vfio specific structure, which also includes a pointer to the struct > device. We obviously want to clear drvdata when the vfio specific > structure is being released. Ah, I see. "device->dev" is not freed, but "device". And the data is cleared for "device->dev". Thanks for correcting. Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is still wanted. I stepped in it yesterday, i2c seems to have its own way to register new devices. More specifically, how can the memory associated with dev_set_drvdata be free'd on error paths if the device is not registered with device_register (as is done in the probe function of the i801 i2c driver)? Regards, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-12-24 at 01:18 +0100, Peter Wu wrote: > On Monday 23 December 2013 10:37:21 Alex Williamson wrote: > > On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote: > [..] > > > > > > There is still one thing I do not fully understand, how should > > > dev_set_drvdata and dev_get_drvdata be used? For the devices passed > > > to probe functions, the core takes care of setting to NULL on error. > > > Then device_unregister frees the memory, right? > > > > > > Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata, > > > i2c_set_adapinfo, etc.) are manually called outside probe functions? > > > Or inside the probe function, but not for the device that is being > > > probed (such as is the case with the i801 i2c driver)? > > > > > > The VFIO driver also does something odd, it clears the driver data, > > > but the device holding it is freed using kfree(): > > > > > > static void vfio_device_release(struct kref *kref) { > > > struct vfio_device *device = container_of(kref, > > > struct vfio_device, kref); > > > struct vfio_group *group = device->group; > > > > > > list_del(&device->group_next); > > > mutex_unlock(&group->device_lock); > > > > > > dev_set_drvdata(device->dev, NULL); > > > > > > kfree(device); > > > > > > Is a memory leak also present here since dev_set_drvdata() always tries to > > > allocate memory? > > > > But it doesn't: > > > > int dev_set_drvdata(struct device *dev, void *data) > > { > > int error; > > > > if (!dev->p) { > > error = device_private_init(dev); > > if (error) > > return error; > > } > > dev->p->driver_data = data; > > return 0; > > } > > It does: > > int device_private_init(struct device *dev) > { > dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL); > if (!dev->p) > return -ENOMEM; > dev->p->device = dev; > klist_init(&dev->p->klist_children, klist_children_get, > klist_children_put); > INIT_LIST_HEAD(&dev->p->deferred_probe); > return 0; > } > > and if it doesn't, then I must be missing something in this non-obvious > code. I scanned the interwebs and Documentation/, but could not really > find a great example on how this is supposed to work. The dev_set_drvdata > function existed since Linus moved to git. You're missing that device_private_init() is only called if (!dev->p). It's a one time initializer and after that we only set the driver_data. > > Also, the code referenced is kfree'ing a struct vfio_device, not the > > struct device. VFIO uses the drvdata to provide a back pointer to the > > vfio specific structure, which also includes a pointer to the struct > > device. We obviously want to clear drvdata when the vfio specific > > structure is being released. > > Ah, I see. "device->dev" is not freed, but "device". And the data is > cleared for "device->dev". Thanks for correcting. > > Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is > still wanted. I stepped in it yesterday, i2c seems to have its own > way to register new devices. More specifically, how can the memory > associated with dev_set_drvdata be free'd on error paths if the > device is not registered with device_register (as is done in the > probe function of the i801 i2c driver)? > > Regards, > Peter > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 December 2013 18:51:09 Alex Williamson wrote: > > It does: > > > > int device_private_init(struct device *dev) > > { > > dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL); > > if (!dev->p) > > return -ENOMEM; > > dev->p->device = dev; > > klist_init(&dev->p->klist_children, klist_children_get, > > klist_children_put); > > INIT_LIST_HEAD(&dev->p->deferred_probe); > > return 0; > > } > > > > and if it doesn't, then I must be missing something in this non-obvious > > code. I scanned the interwebs and Documentation/, but could not really > > find a great example on how this is supposed to work. The dev_set_drvdata > > function existed since Linus moved to git. > > You're missing that device_private_init() is only called if (!dev->p). > It's a one time initializer and after that we only set the driver_data. Yes I am aware of that, the question is about the memory that got allocated in vfio_group_create_device(). I think I have found the answer. The "dev" device is passed from the PCI layer, that will probably take care of freeing the resources when the device is gone. Is this view correct? Thanks for your replies! Peter > > > Also, the code referenced is kfree'ing a struct vfio_device, not the > > > struct device. VFIO uses the drvdata to provide a back pointer to the > > > vfio specific structure, which also includes a pointer to the struct > > > device. We obviously want to clear drvdata when the vfio specific > > > structure is being released. > > > > Ah, I see. "device->dev" is not freed, but "device". And the data is > > cleared for "device->dev". Thanks for correcting. > > > > Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is > > still wanted. I stepped in it yesterday, i2c seems to have its own > > way to register new devices. More specifically, how can the memory > > associated with dev_set_drvdata be free'd on error paths if the > > device is not registered with device_register (as is done in the > > probe function of the i801 i2c driver)? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, Joining the discussion late as I was on vacation... On Mon, 23 Dec 2013 11:43:21 +0100, Peter Wu wrote: > Nevermind this patch, it does not really fix the memleak because > i2c_set_adapdata() calls dev_set_drvdata() which allocates memory. > (I must have ran kmemleak too early, right after boot it did not > give any warnings, now it does). I can confirm that your proposed patch doesn't fix anything. It makes no difference if pci_set_drvdata(dev, priv) is called earlier or later. > RFC: what about dropping i2c_set_adapdata() from the probe function > and replacing i2c_get_adapdata(adapter) by > pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not > sure what the purpose is for i2c_set_adapdata, hence this question. The purpose of i2c_set_adapdata is to attach a data structure to a specific i2c_adapter device. In the case of the i2c-i801 driver, there's only one such (class) device per PCI device so we could indeed reuse the PCI device data pointer as you suggest above. But in the general case there can be several i2c_adapter devices and each needs its own data. Also for performance reasons we don't want to do that because pci_get_drvdata(adapter->pci_dev) is slower than i2c_get_adapdata(adapter) (due to the extra pointer deference.) As this happens repeatedly at run-time (in function i801_access) we want it to be as fast as possible. Note that we could (ab)use i2c_adapter.algo_data to store the i2c_adapter device-specific data. Some drivers are doing exactly that, for example i2c-nforce2. I suspect this legacy field is now somewhat redundant with i2c_set_adapdata() as I couldn't find any i2c bus driver using both.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 737e298..a7096bf 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) if (!priv) return -ENOMEM; + pci_set_drvdata(dev, priv); i2c_set_adapdata(&priv->adapter, priv); priv->adapter.owner = THIS_MODULE; priv->adapter.class = i801_get_adapter_class(priv); @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) /* We ignore errors - multiplexing is optional */ i801_add_mux(priv); - pci_set_drvdata(dev, priv); - return 0; exit_free_irq: