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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
