| Message ID | 20140108142849.3993341c@endymion.delvare |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
--- 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;