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 == 0 as
this is either -ERESTARTSYS or 0.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
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 has
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
handling here is a bit inconsistent.
Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y
(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 = 0;
long rv = 0;
+ int ret = 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, unsigned
// If this is a synchronous kiocb, we need to put the calling process to sleep until the transfer is complete
if (kcb == NULL || is_sync_kiocb(kcb)){
- rv = wait_for_completion_interruptible(&done);
- // If the user aborted (rv == -ERESTARTSYS), we're no longer responsible for cleaning up the acd
- if (rv == -ERESTARTSYS){
+ ret = wait_for_completion_interruptible(&done);
+ /* If the user aborted (ret == -ERESTARTSYS), we're
+ * no longer responsible for cleaning up the acd
+ */
+ if (ret) {
acd->cpl = NULL;
- }
- if (rv == 0){
- rv = acd->len;
+ } else {
+ ret = acd->len;
kfree(acd);
}
- return rv;
+ return ret;
}
return -EIOCBQUEUED;
--
2.1.4
ACK. However, that part isn't the only part of that function that uses "return rv" though.
There's another part that does "rv = get_user_pages(...)" and get_user_pages() returns a long.
Does this same kind of change need to happen for that case?
>-----Original Message-----
>From: Nicholas Mc Guire <[email protected]>
>Sent: Saturday, April 27, 2019 4:15 AM
>To: Greg Kroah-Hartman <[email protected]>
>Cc: Matt Sickler <[email protected]>; [email protected];
>[email protected]; Nicholas Mc Guire <[email protected]>
>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 == 0 as this is
>either -ERESTARTSYS or 0.
>
>Signed-off-by: Nicholas Mc Guire <[email protected]>
>---
>
>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 has 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 handling
>here is a bit inconsistent.
>
>Patch was compile-tested with: x86_64_defconfig + KPC2000=y,
>KPC2000_DMA=y (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 = 0;
> long rv = 0;
>+ int ret = 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, unsigned
>
> // If this is a synchronous kiocb, we need to put the calling process to
>sleep until the transfer is complete
> if (kcb == NULL || is_sync_kiocb(kcb)){
>- rv = wait_for_completion_interruptible(&done);
>- // If the user aborted (rv == -ERESTARTSYS), we're no longer
>responsible for cleaning up the acd
>- if (rv == -ERESTARTSYS){
>+ ret = wait_for_completion_interruptible(&done);
>+ /* If the user aborted (ret == -ERESTARTSYS), we're
>+ * no longer responsible for cleaning up the acd
>+ */
>+ if (ret) {
> acd->cpl = NULL;
>- }
>- if (rv == 0){
>- rv = acd->len;
>+ } else {
>+ ret = acd->len;
> kfree(acd);
> }
>- return rv;
>+ return ret;
> }
>
> return -EIOCBQUEUED;
>--
>2.1.4
On Sat, Apr 27, 2019 at 11:14:34AM +0200, Nicholas Mc Guire wrote:
> weit_for_completion_interruptible returns in (0 on completion and
^
wait_for_completion_interruptible
> -ERESTARTSYS on interruption) - so use an int not long for API conformance
> and simplify the logic here a bit: need not check explicitly for == 0 as
> this is either -ERESTARTSYS or 0.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
>
> 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 has
> 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
> handling here is a bit inconsistent.
>
> Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y
> (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 = 0;
> long rv = 0;
> + int ret = 0;
This assignment is never used. It just turns static checking for
uninitialized variable bugs.
> 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, unsigned
>
> // If this is a synchronous kiocb, we need to put the calling process to sleep until the transfer is complete
> if (kcb == NULL || is_sync_kiocb(kcb)){
> - rv = wait_for_completion_interruptible(&done);
> - // If the user aborted (rv == -ERESTARTSYS), we're no longer responsible for cleaning up the acd
> - if (rv == -ERESTARTSYS){
> + ret = wait_for_completion_interruptible(&done);
> + /* If the user aborted (ret == -ERESTARTSYS), we're
> + * no longer responsible for cleaning up the acd
> + */
> + if (ret) {
> acd->cpl = NULL;
> - }
> - if (rv == 0){
> - rv = acd->len;
> + } else {
> + ret = acd->len;
"acd->len" is a size_t (unsigned long) so probably we should just
change the type of kpc_dma_transfer to ssize_t actually.
regards,
dan carpenter