Patchwork How should dev_[gs]et_drvdata be used?

login
register
mail settings
Submitter Jean Delvare
Date Jan. 8, 2014, 1:28 p.m.
Message ID <20140108142849.3993341c@endymion.delvare>
Download mbox | patch
Permalink /patch/308250/
State Superseded
Headers show

Comments

Jean Delvare - Jan. 8, 2014, 1:28 p.m.
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@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.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Peter Wu <lekensteyn@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


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.
Uwe Kleine-König - Nov. 25, 2014, 9:14 p.m.
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@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?

(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@linux-fr.org>
> Cc: Peter Wu <lekensteyn@gmail.com>
> ---
>  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
Jean Delvare - Nov. 28, 2014, 1:48 p.m.
Hi Uwe,

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.

> (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.)

> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Peter Wu <lekensteyn@gmail.com>
> > ---
> >  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.
Uwe Kleine-König - Nov. 28, 2014, 2:04 p.m.
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

Patch

--- 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;