| Message ID | 20191001090657.25721-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: Avoid calling spi_slave_abort() with kfreed spidev | expand |
Hi Lukasz, On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote: > Call spi_slave_abort() only when the spidev->spi is !NULL and the > structure hasn't already been kfreed. > > Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Lukasz Majewski <lukma@denx.de> Thanks for your patch! > --- a/drivers/spi/spidev.c > +++ b/drivers/spi/spidev.c > @@ -600,15 +600,16 @@ static int spidev_open(struct inode *inode, struct file *filp) > static int spidev_release(struct inode *inode, struct file *filp) > { > struct spidev_data *spidev; > + int dofree; bool? > > mutex_lock(&device_list_lock); > spidev = filp->private_data; > filp->private_data = NULL; > + dofree = 0; Why not initialize it at declaration time? > > /* last close? */ > spidev->users--; > if (!spidev->users) { > - int dofree; > > kfree(spidev->tx_buffer); > spidev->tx_buffer = NULL; > @@ -628,7 +629,8 @@ static int spidev_release(struct inode *inode, struct file *filp) > kfree(spidev); > } > #ifdef CONFIG_SPI_SLAVE > - spi_slave_abort(spidev->spi); > + if (!dofree) > + spi_slave_abort(spidev->spi); Can spidev->spi be NULL, if spidev->users != 0? > #endif > mutex_unlock(&device_list_lock); Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for a very prompt response. > Hi Lukasz, > > On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote: > > Call spi_slave_abort() only when the spidev->spi is !NULL and the > > structure hasn't already been kfreed. > > > > Reported-by: kbuild test robot <lkp@intel.com> > > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Thanks for your patch! > > > --- a/drivers/spi/spidev.c > > +++ b/drivers/spi/spidev.c > > @@ -600,15 +600,16 @@ static int spidev_open(struct inode *inode, > > struct file *filp) static int spidev_release(struct inode *inode, > > struct file *filp) { > > struct spidev_data *spidev; > > + int dofree; > > bool? It may be bool, yes - I took this "int" from the original code (further down in the patch), as I've moved it a bit up. > > > > > mutex_lock(&device_list_lock); > > spidev = filp->private_data; > > filp->private_data = NULL; > > + dofree = 0; > > Why not initialize it at declaration time? I wanted to have it protected by mutex_lock() above. However, this also shall work with the initialization at declaration time. > > > > > /* last close? */ > > spidev->users--; > > if (!spidev->users) { > > - int dofree; > > > > kfree(spidev->tx_buffer); > > spidev->tx_buffer = NULL; > > @@ -628,7 +629,8 @@ static int spidev_release(struct inode *inode, > > struct file *filp) kfree(spidev); > > } > > #ifdef CONFIG_SPI_SLAVE > > - spi_slave_abort(spidev->spi); > > + if (!dofree) > > + spi_slave_abort(spidev->spi); > > Can spidev->spi be NULL, if spidev->users != 0? No, it shouldn't be. The "dofree" is only set to true (the spidev->spi == NULL condition is checked) if there are no references (spidev->users == 0). The if (!dofree) prevents from calling spi_slave_abort() when spidev->spi == NULL and spidev is kfree'd. > > > #endif > > mutex_unlock(&device_list_lock); > > Gr{oetje,eeting}s, > > Geert > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, On Tue, Oct 1, 2019 at 11:34 AM Lukasz Majewski <lukma@denx.de> wrote: > > On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote: > > > Call spi_slave_abort() only when the spidev->spi is !NULL and the > > > structure hasn't already been kfreed. > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > --- a/drivers/spi/spidev.c > > > +++ b/drivers/spi/spidev.c > > > @@ -600,15 +600,16 @@ static int spidev_open(struct inode *inode, > > > struct file *filp) static int spidev_release(struct inode *inode, > > > struct file *filp) { > > > struct spidev_data *spidev; > > > + int dofree; > > > > > > mutex_lock(&device_list_lock); > > > spidev = filp->private_data; > > > filp->private_data = NULL; > > > + dofree = 0; > > > > > > /* last close? */ > > > spidev->users--; > > > if (!spidev->users) { > > > - int dofree; > > > > > > kfree(spidev->tx_buffer); > > > spidev->tx_buffer = NULL; > > > @@ -628,7 +629,8 @@ static int spidev_release(struct inode *inode, > > > struct file *filp) kfree(spidev); > > > } > > > #ifdef CONFIG_SPI_SLAVE > > > - spi_slave_abort(spidev->spi); > > > + if (!dofree) > > > + spi_slave_abort(spidev->spi); > > > > Can spidev->spi be NULL, if spidev->users != 0? > > No, it shouldn't be. > > The "dofree" is only set to true (the spidev->spi == NULL condition is > checked) if there are no references (spidev->users == 0). > > The if (!dofree) prevents from calling spi_slave_abort() when > spidev->spi == NULL and spidev is kfree'd. If spidev->users != 0, the block checking spidev->spi == NULL is never executed, and spi_slave_abort() will be called. I'm wondering if spidev->spi can be NULL if spidev->users is still positive. Gr{oetje,eeting}s, Geert
Hi Geert, > Hi Lukasz, > > On Tue, Oct 1, 2019 at 11:34 AM Lukasz Majewski <lukma@denx.de> wrote: > > > On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> > > > wrote: > > > > Call spi_slave_abort() only when the spidev->spi is !NULL and > > > > the structure hasn't already been kfreed. > > > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > > --- a/drivers/spi/spidev.c > > > > +++ b/drivers/spi/spidev.c > > > > @@ -600,15 +600,16 @@ static int spidev_open(struct inode > > > > *inode, struct file *filp) static int spidev_release(struct > > > > inode *inode, struct file *filp) { > > > > struct spidev_data *spidev; > > > > + int dofree; > > > > > > > > mutex_lock(&device_list_lock); > > > > spidev = filp->private_data; > > > > filp->private_data = NULL; > > > > + dofree = 0; > > > > > > > > /* last close? */ > > > > spidev->users--; > > > > if (!spidev->users) { > > > > - int dofree; > > > > > > > > kfree(spidev->tx_buffer); > > > > spidev->tx_buffer = NULL; > > > > @@ -628,7 +629,8 @@ static int spidev_release(struct inode > > > > *inode, struct file *filp) kfree(spidev); > > > > } > > > > #ifdef CONFIG_SPI_SLAVE > > > > - spi_slave_abort(spidev->spi); > > > > + if (!dofree) > > > > + spi_slave_abort(spidev->spi); > > > > > > Can spidev->spi be NULL, if spidev->users != 0? > > > > No, it shouldn't be. > > > > The "dofree" is only set to true (the spidev->spi == NULL condition > > is checked) if there are no references (spidev->users == 0). > > > > The if (!dofree) prevents from calling spi_slave_abort() when > > spidev->spi == NULL and spidev is kfree'd. > > If spidev->users != 0, the block checking spidev->spi == NULL is never > executed, and spi_slave_abort() will be called. Yes, this is correct. My other patch [1] clears the FIFOs in SPI IP block and ends (if there are any stalled) DMA transactions. > > I'm wondering if spidev->spi can be NULL if spidev->users is still > positive. I think that it cannot. From my tests [2] - when I do enter spi_slave_abort() function the state of spidev->users: 0 dofree: 0 spidev->spi: 0x51337072 So it is possible to call the spidev_release without previously setting spidev->spi to NULL (which is done in spidev_remove() function). IMHO the above behavior also seems to be correct, as during distortion the slave losts synchronization from master. The spidev_remove() callback is part of spi_device struct and is called when the device is removed (rmmod spi_fsl_dspi). From my tests the spidev_release() is NOT called after spidev_remove(), so the code in former seems to be a dead one. Or maybe there is an use case which causes calling spidev_release() after spidev_remove()? > > Gr{oetje,eeting}s, > > Geert > Note: [1] - https://lkml.org/lkml/2019/9/24/245 [2] - https://github.com/lmajewski/tests-spi/blob/master/tests/spi/spi_tests.sh HW setup: HW loopback with two /dev/spidevX.Y devices used Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Oct 01, 2019 at 12:00:07PM +0200, Geert Uytterhoeven wrote: > On Tue, Oct 1, 2019 at 11:34 AM Lukasz Majewski <lukma@denx.de> wrote: > > > On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote: > > The if (!dofree) prevents from calling spi_slave_abort() when > > spidev->spi == NULL and spidev is kfree'd. > If spidev->users != 0, the block checking spidev->spi == NULL is never > executed, and spi_slave_abort() will be called. > I'm wondering if spidev->spi can be NULL if spidev->users is still positive. It *shouldn't* be. I think we have other problems if it is.
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 3ea9d8a3e6e8..2c6d4dbeebac 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -600,15 +600,16 @@ static int spidev_open(struct inode *inode, struct file *filp) static int spidev_release(struct inode *inode, struct file *filp) { struct spidev_data *spidev; + int dofree; mutex_lock(&device_list_lock); spidev = filp->private_data; filp->private_data = NULL; + dofree = 0; /* last close? */ spidev->users--; if (!spidev->users) { - int dofree; kfree(spidev->tx_buffer); spidev->tx_buffer = NULL; @@ -628,7 +629,8 @@ static int spidev_release(struct inode *inode, struct file *filp) kfree(spidev); } #ifdef CONFIG_SPI_SLAVE - spi_slave_abort(spidev->spi); + if (!dofree) + spi_slave_abort(spidev->spi); #endif mutex_unlock(&device_list_lock);
Call spi_slave_abort() only when the spidev->spi is !NULL and the structure hasn't already been kfreed. Reported-by: kbuild test robot <lkp@intel.com> Reported-by: Julia Lawall <julia.lawall@lip6.fr> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Lukasz Majewski <lukma@denx.de> --- This fix applies on: repo: https://kernel.googlesource.com/pub/scm/linux/kernel/git/broonie/spi.git branch: for-5.4 SHA1: 6b04e47b73f2a0d2c330cecca99f8e2cb8f85b34 --- drivers/spi/spidev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)