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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hi Jean,

On Fri, Nov 28, 2014 at 02:48:13PM +0100, Jean Delvare wrote:
> On Tue, 25 Nov 2014 22:14:32 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 08, 2014 at 02:28:49PM +0100, Jean Delvare wrote:
> > > Having looked at the code in deeper detail, I think I understand what
> > > is going on. The problem is with:
> > > 
> > > 	i2c_set_adapdata(&priv->adapter, priv);
> > > 
> > > at the beginning of i801_probe(). It triggers the allocation of dev->p
> > > by the driver core. If we bail out at any point before i2c_add_adapter
> > > (and subsequently device_register) is called, then that memory is never
> > > freed.
> > > 
> > > Unfortunately it is not possible to move the i2c_set_adapdata() call
> > > after i2c_add_adapter(), because the data pointer is needed by code
> > > which runs as part of i2c_add_adapter().
> > > 
> > > We could move it right before the call to i2c_add_adapter(), to make
> > > the problem window smaller, but this wouldn't solve the problem
> > > completely, as i2c_add_adapter() itself can fail before
> > > device_register() is called.
> > > 
> > > The only solution I can think of at this point is to stop using
> > > i2c_set_adapdata() altogether, and use i2c_adapter.algo_data instead:
> > > 
> > > From: Jean Delvare <khali@linux-fr.org>
> > > Subject: i2c-i801: Use i2c_adapter.algo_data
> > > 
> > > Use i2c_adapter.algo_data instead of i2c_set/get_adapdata(). The
> > > latter makes use of the driver core's private data mechanism, which
> > > allocates memory. That memory is never released if an error happens
> > > between the call to i2c_set_adapdata() and the actual i2c_adapter
> > > registration.
> >
> > Since commit 1bb6c08abfb6 (which makes the driver core use a pointer in
> > struct device again for dev_set_drvdata; went into v3.16-rc1) this patch
> > is obsolete, right?
> 
> Correct. It was never applied upstream anyway, which is good as I never
> liked it.
I came back to it as it was still in the linux-i2c patchwork queue.
It is set to be superseeded now.

> > (Still there might be the opportunity for a few patches converting all
> > driver to i2c_set_adapdata and then drop adapter.algo_data.)
> 
> That's at least 35 bus drivers that would have to be converted then.
> But you should first check if it is possible to get rid of
> i2c_adapter.algo_data without breaking i2c-algo-bit and i2c-mux. If not
> then converting the bus drivers would really only be a minor cleanup
> with no real benefit (not saying it's not worth it though.)
Yeah. I'm not sure I will address this cleanup, but will keep your hint
on my radar.

Best regards and thanks for your reply,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |