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@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. 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
