From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FE13C388F3 for ; Tue, 1 Oct 2019 11:07:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 32A9D21920 for ; Tue, 1 Oct 2019 11:07:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731223AbfJALH0 (ORCPT ); Tue, 1 Oct 2019 07:07:26 -0400 Received: from mail-out.m-online.net ([212.18.0.10]:44334 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730506AbfJALH0 (ORCPT ); Tue, 1 Oct 2019 07:07:26 -0400 Received: from frontend01.mail.m-online.net (unknown [192.168.8.182]) by mail-out.m-online.net (Postfix) with ESMTP id 46jGgs6lMMz1rhsg; Tue, 1 Oct 2019 13:07:21 +0200 (CEST) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 46jGgs5ZDDz1qql3; Tue, 1 Oct 2019 13:07:21 +0200 (CEST) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from mail.mnet-online.de ([192.168.8.182]) by localhost (dynscan1.mail.m-online.net [192.168.6.70]) (amavisd-new, port 10024) with ESMTP id FEVJJvJNUS56; Tue, 1 Oct 2019 13:07:19 +0200 (CEST) X-Auth-Info: 93MR/orNrEe2jhKJjzkEZxd48FBIEA3YGLYZuH1w+pY= Received: from jawa (85-222-111-42.dynamic.chello.pl [85.222.111.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.mnet-online.de (Postfix) with ESMTPSA; Tue, 1 Oct 2019 13:07:19 +0200 (CEST) Date: Tue, 1 Oct 2019 13:07:13 +0200 From: Lukasz Majewski To: Geert Uytterhoeven Cc: Mark Brown , Colin Ian King , linux-spi , Krzysztof Kozlowski , Linux Kernel Mailing List , kbuild test robot , Julia Lawall , Dan Carpenter Subject: Re: [PATCH] spi: Avoid calling spi_slave_abort() with kfreed spidev Message-ID: <20191001130713.6eafb728@jawa> In-Reply-To: References: <20191001090657.25721-1-lukma@denx.de> <20191001113420.032dbfef@jawa> Organization: denx.de X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; boundary="Sig_/BmcV7SX+cE.Huey1lpa1dZL"; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Sig_/BmcV7SX+cE.Huey1lpa1dZL Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Hi Geert, > Hi Lukasz, >=20 > On Tue, Oct 1, 2019 at 11:34 AM Lukasz Majewski wrote: > > > On Tue, Oct 1, 2019 at 11:07 AM Lukasz Majewski > > > wrote: =20 > > > > Call spi_slave_abort() only when the spidev->spi is !NULL and > > > > the structure hasn't already been kfreed. > > > > > > > > Reported-by: kbuild test robot > > > > Reported-by: Julia Lawall > > > > Reported-by: Dan Carpenter > > > > Signed-off-by: Lukasz Majewski =20 >=20 > > > > --- 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 =3D filp->private_data; > > > > filp->private_data =3D NULL; > > > > + dofree =3D 0; > > > > > > > > /* last close? */ > > > > spidev->users--; > > > > if (!spidev->users) { > > > > - int dofree; > > > > > > > > kfree(spidev->tx_buffer); > > > > spidev->tx_buffer =3D 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); =20 > > > > > > Can spidev->spi be NULL, if spidev->users !=3D 0? =20 > > > > No, it shouldn't be. > > > > The "dofree" is only set to true (the spidev->spi =3D=3D NULL condition > > is checked) if there are no references (spidev->users =3D=3D 0). > > > > The if (!dofree) prevents from calling spi_slave_abort() when > > spidev->spi =3D=3D NULL and spidev is kfree'd. =20 >=20 > If spidev->users !=3D 0, the block checking spidev->spi =3D=3D NULL is ne= ver > 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. >=20 > I'm wondering if spidev->spi can be NULL if spidev->users is still > positive. I think that it cannot. =46rom 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). =46rom 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()? >=20 > Gr{oetje,eeting}s, >=20 > Geert >=20 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 --Sig_/BmcV7SX+cE.Huey1lpa1dZL Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEgAyFJ+N6uu6+XupJAR8vZIA0zr0FAl2TM2EACgkQAR8vZIA0 zr22NQf/R5mfFd/b6MTcfGxaoSiilulAObqBQ/0HBVTAQFbBK0EFU2/b3Jgoy0Nu NaGoOOT/hHUAmVf9oZR1h5FsdV0rzSG/jbSkOMA+uN+7QlhlwqaBg4BWanrPcSEi xpP7gsbTPj+yiDDtdRoUDxz5P+TMvaf/DrLPn6fkau+YpPAzGTy1AYkdXbDp19SY EoJjxBet0qDfK4/r7hssANNvT2iAeF4jInswxhtjFR8+WN8tT69kOP1n+skXN7eH hbNn7ghgjNsos+iT+kWLOvdr6vdjR6eh1fCEdZwYhKbyyoMHgnZAicC1tA5TDWqR 70fH/6TXos2MgYjiLYpq2GgzAQUgSg== =JhoH -----END PGP SIGNATURE----- --Sig_/BmcV7SX+cE.Huey1lpa1dZL--