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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello Jean,

[not stripping the quotes as the thread is already old]

On Wed, Jan 08, 2014 at 02:28:49PM +0100, Jean Delvare wrote:
> On Tue, 24 Dec 2013 01:18:03 +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?
> 
> FWIW I don't think this is supposed to happen.
> 
> > > > Or inside the probe function, but not for the device that is being
> > > > probed (such as is the case with the i801 i2c driver)?
> 
> This OTOH does happen. Is suspect any driver which instantiates class
> devices will do exactly that. It's nothing i2c specific. For example
> media drivers call video_set_drvdata() in their probe functions.
> 
> > (...)
> > 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.
> 
> What makes you think so? It's as standard as I can imagine.
> 
> > 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)?
> 
> There are two pieces of data to consider here. The data structure which
> is pointed to by dev_get_drvdata (or i2c_get_adapdata) is allocated
> explicitly by the driver and must be freed explicitly by the driver
> (unless devm_kzalloc is used, in which case the cleanup is automatic).
> 
> The data structure used internally by the driver core (dev->p) is
> allocated transparently and is thus the core's responsibility to free.
> I remember looking into this some time ago after someone reported a
> possible memleak to me, and was not fully convinced that the core was
> properly releasing dev->p in all cases. This may be the same problem
> you are looking at right now.
> 
> I do not understand your claim that i2c_adapter class devices are not
> registered with device_register. I2c bus drivers such as i2c-i801 call
> i2c_add_adapter(), which calls i2c_register_adapter(), which calls
> device_register().
> 
> 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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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?

(Still there might be the opportunity for a few patches converting all
driver to i2c_set_adapdata and then drop adapter.algo_data.)

Best regards
Uwe

> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Peter Wu <lekensteyn-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-i801.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -671,7 +671,7 @@ static s32 i801_access(struct i2c_adapte
>  	int hwpec;
>  	int block = 0;
>  	int ret, xact = 0;
> -	struct i801_priv *priv = i2c_get_adapdata(adap);
> +	struct i801_priv *priv = adap->algo_data;
>  
>  	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
>  		&& size != I2C_SMBUS_QUICK
> @@ -774,7 +774,7 @@ static s32 i801_access(struct i2c_adapte
>  
>  static u32 i801_func(struct i2c_adapter *adapter)
>  {
> -	struct i801_priv *priv = i2c_get_adapdata(adapter);
> +	struct i801_priv *priv = adapter->algo_data;
>  
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> @@ -1117,7 +1117,7 @@ static int i801_probe(struct pci_dev *de
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	i2c_set_adapdata(&priv->adapter, priv);
> +	priv->adapter.algo_data = priv;
>  	priv->adapter.owner = THIS_MODULE;
>  	priv->adapter.class = i801_get_adapter_class(priv);
>  	priv->adapter.algo = &smbus_algorithm;
> 
> I can't think of any drawback, other than the feeling that switching
> from a generic implementation to a specific one is a move in the wrong
> direction.
> 
> If the above is the proper fix then we may consider just changing the
> implementation of i2c_set/get_adapdata to use adapter.algo_data instead
> of calling dev_set/get_drvdata(). This would let us fix all the drivers
> at once.
> 
> I'll bring the topic upstream for discussion.
> 
> -- 
> Jean Delvare

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