Re: [PATCH 0/3] spi: core: Introduce devm_spi_alloc_master

From: Maxime Ripard
Date: Mon Feb 03 2014 - 03:30:26 EST


Hi,

On Sat, Feb 01, 2014 at 05:38:41PM +0000, Mark Brown wrote:
> On Fri, Jan 31, 2014 at 02:31:11PM +0100, Maxime Ripard wrote:
> > On Fri, Jan 31, 2014 at 12:12:15PM +0000, Mark Brown wrote:
>
> > > This seems confusing - the idea here is that if we've handed the
> > > device off to the managed function then the managed function
> > > deals with destroying it. Note that spi_alloc_master() says
> > > that the put is only required after errors adding the device
> > > (which would be the expected behaviour if you look at other
> > > APIs). Looking at the code I think there is an issue here but
> > > I'm not at all clear that this is the best fix.
>
> > Ah, right, spi_master_put doesn't free the memory either...
>
> The memory is freed by the driver core calling the
> spi_master_release() callback when it's safe to do so (after the
> last reference to the device has gone away).
>
> > I guess we have a few choices here, either:
> > - Add a devm_kzalloc to spi_alloc_master, since most of the drivers
> > I've been looking at fail to free the memory, this would be the
> > least intrusive solution. We'd still have to remove all the kfree
> > calls in the driver that rightfully free the memory.
> > - Make devm_unregister_master also call kfree on the master
> > - Add a kfree to my devm_put_master so that the memory is reclaimed,
> > which isn't the case for now.
>
> > I don't have a strong preference here, maybe for the third one, since
> > it makes obvious that it's managed and you don't have to do anything
> > about it, while the other do not.
>
> None of the above, definitely nothing to do with calling kfree() once
> the device is registered. We already have that free, it's not the
> issue. The issue is making sure that we hold references when we need
> them and drop them when we don't. I (or someone) needs to sit down and
> think it through since things are a bit confused in the code.

Ok. I'll drop the patches and repost the A31 SPI patches.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachment: signature.asc
Description: Digital signature