Re: [PATCH] spi/mxs: Fix device remove function

[Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Dear Guenter Roeck,

> On Fri, Aug 24, 2012 at 10:10:12PM +0200, Marek Vasut wrote:
> > Dear Guenter Roeck,
> > 
> > > The call sequence
> > > spi_alloc_master/spi_register_master/spi_unregister_master is complete;
> > > it reduces the device reference count to zero, which results in device
> > > memory being freed. The remove function accesses the freed memory
> > > after the call to spi_unregister_master(), _and_ it calls
> > > spi_master_put on the freed memory.
> > > 
> > > Acquire a reference to the SPI master device and release it after
> > > cleanup is complete (with the existing spi_master_put) to solve the
> > > problem.
> > > 
> > > Also, the device subsystem ensures that the remove function is only
> > > called once, and resets device driver data to NULL. Remove the
> > > unnecessaary calls to platform_set_drvdata().
> > > 
> > > Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > 
> > Damn, I thought this was fixed, apparently not. Thanks!
> 
> For some reason most spi drivers have similar problems.

That's their problem, but I think the problem here is in the submitter, aka. me, 
being a lazy flunk.

> I learned it the
> hard way when I tested my own driver, and decided to fix as many drivers
> as I can. Which is why you see all those patches from me lately.

I'm very grateful for these. This yet again proves how important it is to push 
code mainline.

> On a side note, at least some of the spi bitbang drivers have a similar
> problem. Unfortunately, I can not fix it right now because I have no means
> to test it, and have no idea what exactly is _supposed_ to happen.

Which ones?

> > Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > 
> > > ---
> > > Applies to -next (as of 8/24/12).
> > > 
> > >  drivers/spi/spi-mxs.c |    5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> > > index 130a436..cb189b7 100644
> > > --- a/drivers/spi/spi-mxs.c
> > > +++ b/drivers/spi/spi-mxs.c
> > > @@ -582,7 +582,6 @@ static int __devinit mxs_spi_probe(struct
> > > platform_device *pdev) return 0;
> > > 
> > >  out_free_dma:
> > > -	platform_set_drvdata(pdev, NULL);
> > > 
> > >  	dma_release_channel(ssp->dmach);
> > >  	clk_disable_unprepare(ssp->clk);
> > >  
> > >  out_master_free:
> > > @@ -596,14 +595,12 @@ static int __devexit mxs_spi_remove(struct
> > > platform_device *pdev) struct mxs_spi *spi;
> > > 
> > >  	struct mxs_ssp *ssp;
> > > 
> > > -	master = platform_get_drvdata(pdev);
> > > +	master = spi_master_get(platform_get_drvdata(pdev));
> > 
> > What happens if platform_get_drvdata() returns NULL ? (is it possible?)
> 
> The driver subsystem locks the device during remove, clears drvdata,
> and only releases the lock after it is done. The code ensures that the
> remove function is not called again. See
> drivers/base/dd.c:__device_release_driver(). The same happens during
> probe; see drivers/base/dd.c:really_probe().
> 
> Thanks,
> Guenter

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/