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@xxxxxxxxxxxxxxxx/ > > > > > > >>> + > > > >>> +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. -- Regards, Laurent Pinchart
