| Message ID | 20200409121202.11130-3-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286 v8 - modifications | expand |
Hi Kieran, Thank you for the patch. On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: > v8: > - Convert probe kzalloc usage to devm_ variant This isn't worse than the existing code, but are you aware that devm_* should not be used in this case ? The memory should be allocated with kzalloc() and freed in the .release() operation. > --- > drivers/media/i2c/max9286.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index b84d2daa6561..0a43137b8112 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client) > unsigned int i; > int ret; > > - priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client) > max9286_configure_i2c(priv, false); > err_free: > max9286_cleanup_dt(priv); > - kfree(priv); > > return ret; > } > @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client) > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); > > - kfree(priv); > - > return 0; > } >
Hi Laurent, (+Sakari) On 09/04/2020 17:33, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: >> v8: >> - Convert probe kzalloc usage to devm_ variant > > This isn't worse than the existing code, but are you aware that devm_* > should not be used in this case ? The memory should be allocated with > kzalloc() and freed in the .release() operation. This change was at the request of a review comment from Sakari. From: https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ > >>> + >>> +static int max9286_probe(struct i2c_client *client) >>> +{ >>> + struct max9286_priv *priv; >>> + unsigned int i; >>> + int ret; >>> + >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >> >> You won't lose anything by using the devm_ variant here. > > Indeed, > >> >>> + >>> + priv->client = client; >>> + i2c_set_clientdata(client, priv); >>> + >>> + for (i = 0; i < MAX9286_N_SINKS; i++) >>> + max9286_init_format(&priv->fmt[i]); >>> + >>> + ret = max9286_parse_dt(priv); >>> + if (ret) >>> + return ret; >> >> But you can avoid accidental memory leaks for nothing. :-) > > It would be good not to leak indeed! I understand that there are lifetime issues in V4L2 - but in my opinion that needs to be handled by core V4l2 (and or support from driver core framework). Also - isn't it highly unlikely to affect the max9286? Isn't the lifetime issue that the device can be unplugged while the file handle is open? I don't think anyone is going to 'unplug' the max9286 while it's active :-) >> --- >> drivers/media/i2c/max9286.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >> index b84d2daa6561..0a43137b8112 100644 >> --- a/drivers/media/i2c/max9286.c >> +++ b/drivers/media/i2c/max9286.c >> @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client) >> unsigned int i; >> int ret; >> >> - priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); >> if (!priv) >> return -ENOMEM; >> >> @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client) >> max9286_configure_i2c(priv, false); >> err_free: >> max9286_cleanup_dt(priv); >> - kfree(priv); >> >> return ret; >> } >> @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client) >> >> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); >> >> - kfree(priv); >> - >> return 0; >> } >> >
Hi Kieran, On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote: > On 09/04/2020 17:33, Laurent Pinchart wrote: > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: > >> v8: > >> - Convert probe kzalloc usage to devm_ variant > > > > This isn't worse than the existing code, but are you aware that devm_* > > should not be used in this case ? The memory should be allocated with > > kzalloc() and freed in the .release() operation. > > This change was at the request of a review comment from Sakari. > > From: > https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ > > >>> + > >>> +static int max9286_probe(struct i2c_client *client) > >>> +{ > >>> + struct max9286_priv *priv; > >>> + unsigned int i; > >>> + int ret; > >>> + > >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >>> + if (!priv) > >>> + return -ENOMEM; > >> > >> You won't lose anything by using the devm_ variant here. > > > > Indeed, > > > >>> + > >>> + priv->client = client; > >>> + i2c_set_clientdata(client, priv); > >>> + > >>> + for (i = 0; i < MAX9286_N_SINKS; i++) > >>> + max9286_init_format(&priv->fmt[i]); > >>> + > >>> + ret = max9286_parse_dt(priv); > >>> + if (ret) > >>> + return ret; > >> > >> But you can avoid accidental memory leaks for nothing. :-) > > > > It would be good not to leak indeed! > > I understand that there are lifetime issues in V4L2 - but in my opinion > that needs to be handled by core V4l2 (and or support from driver core > framework). I'm afraid that's not possible. The V4L2 core can't delay remove(). There are helpers we could create to simplify correct memory management for drivers, but in any case, devm_kzalloc() isn't correct. There are also issues in the core that would make unbinding unsafe even if correctly implemented in this driver, but a correct implementation in drivers will be required in any case. As I said before this patch isn't a regression as memory allocation is already broken here, but it doesn't go in the right direction either. > Also - isn't it highly unlikely to affect the max9286? Isn't the > lifetime issue that the device can be unplugged while the file handle is > open? > > I don't think anyone is going to 'unplug' the max9286 while it's active :-) No, but someone could unbind it through sysfs. In any case it's not an excuse to not implement memory allocation correctly :-) > >> --- > >> drivers/media/i2c/max9286.c | 5 +---- > >> 1 file changed, 1 insertion(+), 4 deletions(-) > >> > >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > >> index b84d2daa6561..0a43137b8112 100644 > >> --- a/drivers/media/i2c/max9286.c > >> +++ b/drivers/media/i2c/max9286.c > >> @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client) > >> unsigned int i; > >> int ret; > >> > >> - priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); > >> if (!priv) > >> return -ENOMEM; > >> > >> @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client) > >> max9286_configure_i2c(priv, false); > >> err_free: > >> max9286_cleanup_dt(priv); > >> - kfree(priv); > >> > >> return ret; > >> } > >> @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client) > >> > >> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); > >> > >> - kfree(priv); > >> - > >> return 0; > >> } > >>
Hi Laurent, Kieran, On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote: > Hi Kieran, > > On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote: > > On 09/04/2020 17:33, Laurent Pinchart wrote: > > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: > > >> v8: > > >> - Convert probe kzalloc usage to devm_ variant > > > > > > This isn't worse than the existing code, but are you aware that devm_* > > > should not be used in this case ? The memory should be allocated with > > > kzalloc() and freed in the .release() operation. > > > > This change was at the request of a review comment from Sakari. > > > > From: > > https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ > > > > >>> + > > >>> +static int max9286_probe(struct i2c_client *client) > > >>> +{ > > >>> + struct max9286_priv *priv; > > >>> + unsigned int i; > > >>> + int ret; > > >>> + > > >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > >>> + if (!priv) > > >>> + return -ENOMEM; > > >> > > >> You won't lose anything by using the devm_ variant here. > > > > > > Indeed, > > > > > >>> + > > >>> + priv->client = client; > > >>> + i2c_set_clientdata(client, priv); > > >>> + > > >>> + for (i = 0; i < MAX9286_N_SINKS; i++) > > >>> + max9286_init_format(&priv->fmt[i]); > > >>> + > > >>> + ret = max9286_parse_dt(priv); > > >>> + if (ret) > > >>> + return ret; > > >> > > >> But you can avoid accidental memory leaks for nothing. :-) > > > > > > It would be good not to leak indeed! > > > > I understand that there are lifetime issues in V4L2 - but in my opinion > > that needs to be handled by core V4l2 (and or support from driver core > > framework). > > I'm afraid that's not possible. The V4L2 core can't delay remove(). > There are helpers we could create to simplify correct memory management > for drivers, but in any case, devm_kzalloc() isn't correct. > > There are also issues in the core that would make unbinding unsafe even > if correctly implemented in this driver, but a correct implementation in > drivers will be required in any case. > > As I said before this patch isn't a regression as memory allocation is > already broken here, but it doesn't go in the right direction either. > > > Also - isn't it highly unlikely to affect the max9286? Isn't the > > lifetime issue that the device can be unplugged while the file handle is > > open? > > > > I don't think anyone is going to 'unplug' the max9286 while it's active :-) > > No, but someone could unbind it through sysfs. In any case it's not an > excuse to not implement memory allocation correctly :-) Properly refcounting the objects and being unbind-safe would require rewriting the memory allocation in drivers. This can't be done as the framework is broken. Whether you use devm_* variant here makes no difference from that point of view. But it makes it easier to find out driver does not do its object refcounting properly later on when (or if) the framework is fixed.
Hi Sakari, On Thu, Apr 23, 2020 at 10:38:41AM +0300, Sakari Ailus wrote: > On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote: > > On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote: > > > On 09/04/2020 17:33, Laurent Pinchart wrote: > > > > On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: > > > >> v8: > > > >> - Convert probe kzalloc usage to devm_ variant > > > > > > > > This isn't worse than the existing code, but are you aware that devm_* > > > > should not be used in this case ? The memory should be allocated with > > > > kzalloc() and freed in the .release() operation. > > > > > > This change was at the request of a review comment from Sakari. > > > > > > From: > > > https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ > > > > > > >>> + > > > >>> +static int max9286_probe(struct i2c_client *client) > > > >>> +{ > > > >>> + struct max9286_priv *priv; > > > >>> + unsigned int i; > > > >>> + int ret; > > > >>> + > > > >>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > > >>> + if (!priv) > > > >>> + return -ENOMEM; > > > >> > > > >> You won't lose anything by using the devm_ variant here. > > > > > > > > Indeed, > > > > > > > >>> + > > > >>> + priv->client = client; > > > >>> + i2c_set_clientdata(client, priv); > > > >>> + > > > >>> + for (i = 0; i < MAX9286_N_SINKS; i++) > > > >>> + max9286_init_format(&priv->fmt[i]); > > > >>> + > > > >>> + ret = max9286_parse_dt(priv); > > > >>> + if (ret) > > > >>> + return ret; > > > >> > > > >> But you can avoid accidental memory leaks for nothing. :-) > > > > > > > > It would be good not to leak indeed! > > > > > > I understand that there are lifetime issues in V4L2 - but in my opinion > > > that needs to be handled by core V4l2 (and or support from driver core > > > framework). > > > > I'm afraid that's not possible. The V4L2 core can't delay remove(). > > There are helpers we could create to simplify correct memory management > > for drivers, but in any case, devm_kzalloc() isn't correct. > > > > There are also issues in the core that would make unbinding unsafe even > > if correctly implemented in this driver, but a correct implementation in > > drivers will be required in any case. > > > > As I said before this patch isn't a regression as memory allocation is > > already broken here, but it doesn't go in the right direction either. > > > > > Also - isn't it highly unlikely to affect the max9286? Isn't the > > > lifetime issue that the device can be unplugged while the file handle is > > > open? > > > > > > I don't think anyone is going to 'unplug' the max9286 while it's active :-) > > > > No, but someone could unbind it through sysfs. In any case it's not an > > excuse to not implement memory allocation correctly :-) > > Properly refcounting the objects and being unbind-safe would require > rewriting the memory allocation in drivers. This can't be done as the > framework is broken. > > Whether you use devm_* variant here makes no difference from that point of > view. But it makes it easier to find out driver does not do its object > refcounting properly later on when (or if) the framework is fixed. It's an interesting point, "as it's broken, make it very visible" :-) I'm fine with that.
Hi Sakari, On 23/04/2020 08:38, Sakari Ailus wrote: > Hi Laurent, Kieran, > > On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote: >> Hi Kieran, >> >> On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote: >>> On 09/04/2020 17:33, Laurent Pinchart wrote: >>>> On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: >>>>> v8: >>>>> - Convert probe kzalloc usage to devm_ variant >>>> >>>> This isn't worse than the existing code, but are you aware that devm_* >>>> should not be used in this case ? The memory should be allocated with >>>> kzalloc() and freed in the .release() operation. >>> >>> This change was at the request of a review comment from Sakari. >>> >>> From: >>> https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ >>> >>>>>> + >>>>>> +static int max9286_probe(struct i2c_client *client) >>>>>> +{ >>>>>> + struct max9286_priv *priv; >>>>>> + unsigned int i; >>>>>> + int ret; >>>>>> + >>>>>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >>>>>> + if (!priv) >>>>>> + return -ENOMEM; >>>>> >>>>> You won't lose anything by using the devm_ variant here. >>>> >>>> Indeed, >>>> >>>>>> + >>>>>> + priv->client = client; >>>>>> + i2c_set_clientdata(client, priv); >>>>>> + >>>>>> + for (i = 0; i < MAX9286_N_SINKS; i++) >>>>>> + max9286_init_format(&priv->fmt[i]); >>>>>> + >>>>>> + ret = max9286_parse_dt(priv); >>>>>> + if (ret) >>>>>> + return ret; >>>>> >>>>> But you can avoid accidental memory leaks for nothing. :-) >>>> >>>> It would be good not to leak indeed! >>> >>> I understand that there are lifetime issues in V4L2 - but in my opinion >>> that needs to be handled by core V4l2 (and or support from driver core >>> framework). >> >> I'm afraid that's not possible. The V4L2 core can't delay remove(). >> There are helpers we could create to simplify correct memory management >> for drivers, but in any case, devm_kzalloc() isn't correct. >> >> There are also issues in the core that would make unbinding unsafe even >> if correctly implemented in this driver, but a correct implementation in >> drivers will be required in any case. >> >> As I said before this patch isn't a regression as memory allocation is >> already broken here, but it doesn't go in the right direction either. >> >>> Also - isn't it highly unlikely to affect the max9286? Isn't the >>> lifetime issue that the device can be unplugged while the file handle is >>> open? >>> >>> I don't think anyone is going to 'unplug' the max9286 while it's active :-) >> >> No, but someone could unbind it through sysfs. In any case it's not an >> excuse to not implement memory allocation correctly :-) > > Properly refcounting the objects and being unbind-safe would require > rewriting the memory allocation in drivers. This can't be done as the > framework is broken. Which framework is broken? > Whether you use devm_* variant here makes no difference from that point of > view. But it makes it easier to find out driver does not do its object > refcounting properly later on when (or if) the framework is fixed. Is there anything *preventing* us from adding refcnting to the devm_ system so that we can take a reference on the struct device, thus postponing all devm_ release actions while a file handle is open? Then the reference handling can be done by V4l2 core - and we get a whole bunch of drivers fixed ? As it sounds like this is something which affects all subsystems which are capable of exposing a device which gets opened (so presumably all char/block devices?) this seems like it really needs to be fixed as low as possible. -- Kieran
Hi Kieran, On Thu, Apr 23, 2020 at 11:55:47AM +0100, Kieran Bingham wrote: > Hi Sakari, > > On 23/04/2020 08:38, Sakari Ailus wrote: > > Hi Laurent, Kieran, > > > > On Fri, Apr 10, 2020 at 02:15:19PM +0300, Laurent Pinchart wrote: > >> Hi Kieran, > >> > >> On Fri, Apr 10, 2020 at 09:20:25AM +0100, Kieran Bingham wrote: > >>> On 09/04/2020 17:33, Laurent Pinchart wrote: > >>>> On Thu, Apr 09, 2020 at 01:11:51PM +0100, Kieran Bingham wrote: > >>>>> v8: > >>>>> - Convert probe kzalloc usage to devm_ variant > >>>> > >>>> This isn't worse than the existing code, but are you aware that devm_* > >>>> should not be used in this case ? The memory should be allocated with > >>>> kzalloc() and freed in the .release() operation. > >>> > >>> This change was at the request of a review comment from Sakari. > >>> > >>> From: > >>> https://lore.kernel.org/linux-media/4139f241-2fde-26ad-fe55-dcaeb76ad6cc@ideasonboard.com/ > >>> > >>>>>> + > >>>>>> +static int max9286_probe(struct i2c_client *client) > >>>>>> +{ > >>>>>> + struct max9286_priv *priv; > >>>>>> + unsigned int i; > >>>>>> + int ret; > >>>>>> + > >>>>>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > >>>>>> + if (!priv) > >>>>>> + return -ENOMEM; > >>>>> > >>>>> You won't lose anything by using the devm_ variant here. > >>>> > >>>> Indeed, > >>>> > >>>>>> + > >>>>>> + priv->client = client; > >>>>>> + i2c_set_clientdata(client, priv); > >>>>>> + > >>>>>> + for (i = 0; i < MAX9286_N_SINKS; i++) > >>>>>> + max9286_init_format(&priv->fmt[i]); > >>>>>> + > >>>>>> + ret = max9286_parse_dt(priv); > >>>>>> + if (ret) > >>>>>> + return ret; > >>>>> > >>>>> But you can avoid accidental memory leaks for nothing. :-) > >>>> > >>>> It would be good not to leak indeed! > >>> > >>> I understand that there are lifetime issues in V4L2 - but in my opinion > >>> that needs to be handled by core V4l2 (and or support from driver core > >>> framework). > >> > >> I'm afraid that's not possible. The V4L2 core can't delay remove(). > >> There are helpers we could create to simplify correct memory management > >> for drivers, but in any case, devm_kzalloc() isn't correct. > >> > >> There are also issues in the core that would make unbinding unsafe even > >> if correctly implemented in this driver, but a correct implementation in > >> drivers will be required in any case. > >> > >> As I said before this patch isn't a regression as memory allocation is > >> already broken here, but it doesn't go in the right direction either. > >> > >>> Also - isn't it highly unlikely to affect the max9286? Isn't the > >>> lifetime issue that the device can be unplugged while the file handle is > >>> open? > >>> > >>> I don't think anyone is going to 'unplug' the max9286 while it's active :-) > >> > >> No, but someone could unbind it through sysfs. In any case it's not an > >> excuse to not implement memory allocation correctly :-) > > > > Properly refcounting the objects and being unbind-safe would require > > rewriting the memory allocation in drivers. This can't be done as the > > framework is broken. > > > Which framework is broken? V4L2 and MC. There is nothing that prevents unbinding a driver, and thus freeing the resources related to it --- while a device is in use. This can be done with V4L2 video nodes but not with subdev nodes nor MC. The same applies all the graph objects in the same scope. The original assumption was more or less that it's all mounted on the same PCB, so why would it go away? Well, it turns out this is not so easy to change afterwards. > > > > Whether you use devm_* variant here makes no difference from that point of > > view. But it makes it easier to find out driver does not do its object > > refcounting properly later on when (or if) the framework is fixed. > > Is there anything *preventing* us from adding refcnting to the devm_ > system so that we can take a reference on the struct device, thus > postponing all devm_ release actions while a file handle is open? That doesn't help really, they'll be freed at driver remove time AFAIR as well. The effect is no different than freeing them in the remove callback yourself. > > Then the reference handling can be done by V4l2 core - and we get a > whole bunch of drivers fixed ? To address this, you'd need to keep reference to the relevant objects, so the memory is eventually released. > > As it sounds like this is something which affects all subsystems which > are capable of exposing a device which gets opened (so presumably all > char/block devices?) this seems like it really needs to be fixed as low > as possible. I think it's really everything that exposes some sort of interface, whether it's kernel or user. I haven't studied other parts of the kernel this in mind, but my understanding is that this is generally working as it should. We just happen to have complex dependencies between the devices and thus also drivers so the problem area is wider than elsewhere. I wonder how ALSA has addressed similar issues.
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index b84d2daa6561..0a43137b8112 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -1155,7 +1155,7 @@ static int max9286_probe(struct i2c_client *client) unsigned int i; int ret; - priv = kzalloc(sizeof(*priv), GFP_KERNEL); + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -1232,7 +1232,6 @@ static int max9286_probe(struct i2c_client *client) max9286_configure_i2c(priv, false); err_free: max9286_cleanup_dt(priv); - kfree(priv); return ret; } @@ -1253,8 +1252,6 @@ static int max9286_remove(struct i2c_client *client) gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); - kfree(priv); - return 0; }