Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Matt Sickler <Matt.Sickler@daktronics.com>
To:     Nicholas Mc Guire <hofrat@osadl.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH RFC V2] staging: kpc2000: use int for
 wait_for_completion_interruptible
Thread-Topic: [PATCH RFC V2] staging: kpc2000: use int for
 wait_for_completion_interruptible
Thread-Index: AQHU/NprkXgP4dxHiEqrWO1iDMxlTqZTPadg
Date:   Mon, 29 Apr 2019 15:02:10 +0000
Message-ID: <SN6PR02MB4016A8EC6E8402A26BDE54CDEE390@SN6PR02MB4016.namprd02.prod.outlook.com>
References: <1556356474-8575-1-git-send-email-hofrat@osadl.org>
In-Reply-To: <1556356474-8575-1-git-send-email-hofrat@osadl.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: daktronics.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 333dc2d1-0ed9-47cf-2162-08d6ccb3a7f5
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Apr 2019 15:02:10.6714
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: be88af81-0945-42aa-a3d2-b122777351a2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR02MB4494
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

ACK.   However, that part isn't the only part of that function that uses "r=
eturn rv" though.
There's another part that does "rv =3D get_user_pages(...)" and get_user_pa=
ges() returns a long.
Does this same kind of change need to happen for that case?

>-----Original Message-----
>From: Nicholas Mc Guire <hofrat@osadl.org>
>Sent: Saturday, April 27, 2019 4:15 AM
>To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Matt Sickler <Matt.Sickler@daktronics.com>; devel@driverdev.osuosl.org=
;
>linux-kernel@vger.kernel.org; Nicholas Mc Guire <hofrat@osadl.org>
>Subject: [PATCH RFC V2] staging: kpc2000: use int for
>wait_for_completion_interruptible
>
>
>weit_for_completion_interruptible returns in (0 on completion and -
>ERESTARTSYS on interruption) - so use an int not long for API conformance
>and simplify the logic here a bit: need not check explicitly for =3D=3D 0 =
as this is
>either -ERESTARTSYS or 0.
>
>Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>---
>
>Problem located with experimental API conformance checking cocci script
>
>V2: kbuild reported a missing closing comment - seems that I managed
>    send out the the initial version before compile testing/checkpatching
>    sorry for the noise.
>
>Not sure if making such point-wise fixes makes much sense - this driver ha=
s a
>number of issues both style-wise and API compliance wise.
>
>Note that kpc_dma_transfer() returns int not long - currently rv (long) is=
 being
>returned in most places (some places do return int) - so the return handli=
ng
>here is a bit inconsistent.
>
>Patch was compile-tested with: x86_64_defconfig + KPC2000=3Dy,
>KPC2000_DMA=3Dy (with a number of unrelated sparse warnings about non-
>declared symbols, and  smatch warnings about overflowing constants as well
>as coccicheck warning  about usless casting)
>
>Patch is against 5.1-rc6 (localversion-next is next-20190426)
>
> drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index 5741d2b..66f0d5a 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -38,6 +38,7 @@ int  kpc_dma_transfer(struct dev_private_data *priv,
>struct kiocb *kcb, unsigned  {
>        unsigned int i =3D 0;
>        long rv =3D 0;
>+       int ret =3D 0;
>        struct kpc_dma_device *ldev;
>        struct aio_cb_data *acd;
>        DECLARE_COMPLETION_ONSTACK(done); @@ -180,16 +181,17 @@ int
>kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigne=
d
>
>        // If this is a synchronous kiocb, we need to put the calling proc=
ess to
>sleep until the transfer is complete
>        if (kcb =3D=3D NULL || is_sync_kiocb(kcb)){
>-               rv =3D wait_for_completion_interruptible(&done);
>-               // If the user aborted (rv =3D=3D -ERESTARTSYS), we're no =
longer
>responsible for cleaning up the acd
>-               if (rv =3D=3D -ERESTARTSYS){
>+               ret =3D wait_for_completion_interruptible(&done);
>+               /* If the user aborted (ret =3D=3D -ERESTARTSYS), we're
>+                * no longer responsible for cleaning up the acd
>+                */
>+               if (ret) {
>                        acd->cpl =3D NULL;
>-               }
>-               if (rv =3D=3D 0){
>-                       rv =3D acd->len;
>+               } else {
>+                       ret =3D acd->len;
>                        kfree(acd);
>                }
>-               return rv;
>+               return ret;
>        }
>
>        return -EIOCBQUEUED;
>--
>2.1.4