Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
	linux-renesas-soc@vger.kernel.org,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Hyun Kwon" <hyunk@xilinx.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>
Subject: Re: [PATCH v8 02/13] squash! max9286: convert probe kzalloc
Date: Thu, 23 Apr 2020 10:38:41 +0300	[thread overview]
Message-ID: <20200423073841.GJ5381@paasikivi.fi.intel.com> (raw)
In-Reply-To: <20200410111519.GA4751@pendragon.ideasonboard.com>

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.

-- 
Regards,

Sakari Ailus