Re: How should dev_[gs]et_drvdata be used?

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/