get rid of 'return -1'

[selltc]

Source: Dan Carpenter dan.carpenter@oracle.com Mon 5/2/2016 5:53 AM

Still a lot of return -1; grep "return -1;" drivers/staging/unisys/ -R

See KanBoard-992

[selltc]

Does Dan just want "-1" to be replaced by a define value, like "-EINVAL"?

[selltc]

I suspect I need this to join you guys into the thread... @arfvidsonUIS @davidker

[arfvidsonUIS]

drivers/staging/unisys/visorbus/iovmcall_gnuc.h:25: return -1; drivers/staging/unisys/visorbus/iovmcall_gnuc.h:43: return -1; drivers/staging/unisys/visorbus/visorchipset.c:1622: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:879: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:882: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:898: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:901: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:918: return -1; drivers/staging/unisys/visorbus/visorbus_main.c:921: return -1; drivers/staging/unisys/visorhba/visorhba_main.c:205: return -1; drivers/staging/unisys/visorinput/visorinput.c:513: return -1;

All -1 returns

[selltc]

(Erik, regarding our brief discussion in the mail room...)

__unisys_vmcall_gnuc

from drivers/staging/unisys/visorbus/iovmcall_gnuc.h

15 /* Linux GCC Version (32-bit and 64-bit) */
16 static inline unsigned long
17 __unisys_vmcall_gnuc(unsigned long tuple, unsigned long reg_ebx,
18              unsigned long reg_ecx)
19 {
20     unsigned long result = 0;
21     unsigned int cpuid_eax, cpuid_ebx, cpuid_ecx, cpuid_edx;
22 
23     cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
24     if (!(cpuid_ecx & 0x80000000))
25         return -1;
26 
27     __asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
28         "a"(tuple), "b"(reg_ebx), "c"(reg_ecx));
29     return result;
30 }
31 
32 static inline unsigned long
33 __unisys_extended_vmcall_gnuc(unsigned long long tuple,
34                   unsigned long long reg_ebx,
35                   unsigned long long reg_ecx,
36                   unsigned long long reg_edx)
37 {
38     unsigned long result = 0;
39     unsigned int cpuid_eax, cpuid_ebx, cpuid_ecx, cpuid_edx;
40 
41     cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
42     if (!(cpuid_ecx & 0x80000000))
43         return -1;
44 
45     __asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
46         "a"(tuple), "b"(reg_ebx), "c"(reg_ecx), "d"(reg_edx));
47     return result;
48 }

These appear to be used in these macros in drivers/staging/unisys/visorbus/vmcallinterface.h:

#define unisys_vmcall(tuple, reg_ebx, reg_ecx) \
    __unisys_vmcall_gnuc(tuple, reg_ebx, reg_ecx)
#define unisys_extended_vmcall(tuple, reg_ebx, reg_ecx, reg_edx) \
    __unisys_extended_vmcall_gnuc(tuple, reg_ebx, reg_ecx, reg_edx)

ultimately by:

#define ISSUE_IO_VMCALL(method, param, result) \
    (result = unisys_vmcall(method, (param) & 0xFFFFFFFF,   \
                (param) >> 32))
#define ISSUE_IO_EXTENDED_VMCALL(method, param1, param2, param3) \
    unisys_extended_vmcall(method, param1, param2, param3)

#define ISSUE_IO_VMCALL_POSTCODE_SEVERITY(postcode, severity)       \
    ISSUE_IO_EXTENDED_VMCALL(VMCALL_POST_CODE_LOGEVENT, severity,   \
                 MDS_APPOS, postcode)

These are used in the Linux guest to issue vmcalls for such things as to obtain the controlvm channel address, and to log postcodes.

The "return -1" cases would basically be hit in cases where you were running the code when NOT in a guest environment, e.g., like maybe you tried to use it while running the OS on metal. "-EPERM" (operation not supported) might be a good choice as an alternative to -1, as the return is basically telling the caller that the processor doesn't support vmcall operations.

[arfvidsonUIS]

I just pushed this branch to githubissue#34. @selltc @davidker

[selltc]

Comment from Neil Horman nhorman@redhat.com 6/1:

@@ -1613,7 +1613,7 @@ parahotplug_request_complete(int id, u16 active)
     }

     spin_unlock(&parahotplug_request_list_lock);
-    return -1;
+    return -EINVAL;
 }

 /*

Why return anything here, every caller of this function ignores the return code entirely. Alternatively, passing the EINVAL back from the caller seems reasonable.

[selltc]

Comment from Neil Horman nhorman@redhat.com 6/1:

@@ -915,10 +915,10 @@ write_vbus_dev_info(struct visorchannel *chan,
         (hdr_info->device_info_struct_bytes * devix);

     if (hdr_info->dev_info_offset == 0)
-        return -1;
+        return -EFAULT;

     if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-        return -1;
+        return -EFAULT;
     return 0;
 }

This seems fine, but why are you bothering to return anything at all, since the return code is ignored at all the call sites. Or more directly, why aren't you checking these return codes, and acting appropriately if a fault is returned?

David K is correcting this in the v3 patchset with this (commit bca81def251c14e86afa744eae416c3d767acad9):

--- a/drivers/staging/unisys/visorbus/visorbus_main.c
+++ b/drivers/staging/unisys/visorbus/visorbus_main.c
@@ -866,9 +866,12 @@ get_vbus_header_info(struct visorchannel *chan,

 /* Write the contents of <info> to the struct
  * spar_vbus_channel_protocol.chp_info.
+ *
+ * Returns void since this is debug information and not needed for
+ * device functionality.
  */

-static int
+static void
 write_vbus_chp_info(struct visorchannel *chan,
            struct spar_vbus_headerinfo *hdr_info,
            struct ultra_vbus_deviceinfo *info)
@@ -876,18 +879,19 @@ write_vbus_chp_info(struct visorchannel *chan,
    int off = sizeof(struct channel_header) + hdr_info->chp_info_offset;

    if (hdr_info->chp_info_offset == 0)
-       return -1;
+       return;

-   if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-       return -1;
-   return 0;
+   visorchannel_write(chan, off, info, sizeof(*info));
 }

 /* Write the contents of <info> to the struct
  * spar_vbus_channel_protocol.bus_info.
+ *
+ * Returns void since this is debug information and not needed for
+ * device functionality.
  */

-static int
+static void
 write_vbus_bus_info(struct visorchannel *chan,
            struct spar_vbus_headerinfo *hdr_info,
            struct ultra_vbus_deviceinfo *info)
@@ -895,17 +899,18 @@ write_vbus_bus_info(struct visorchannel *chan,
    int off = sizeof(struct channel_header) + hdr_info->bus_info_offset;

    if (hdr_info->bus_info_offset == 0)
-       return -1;
+       return;

-   if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-       return -1;
-   return 0;
+   visorchannel_write(chan, off, info, sizeof(*info));
 }

 /* Write the contents of <info> to the
  * struct spar_vbus_channel_protocol.dev_info[<devix>].
+ *
+ * Returns void since this is debug information and not needed for
+ * device functionality.
  */
-static int
+static void
 write_vbus_dev_info(struct visorchannel *chan,
            struct spar_vbus_headerinfo *hdr_info,
            struct ultra_vbus_deviceinfo *info, int devix)
@@ -915,11 +920,9 @@ write_vbus_dev_info(struct visorchannel *chan,
        (hdr_info->device_info_struct_bytes * devix);

    if (hdr_info->dev_info_offset == 0)
-       return -1;
+       return;

-   if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
-       return -1;
-   return 0;
+   visorchannel_write(chan, off, info, sizeof(*info));
 }

 /* For a child device just created on a client bus, fill in

[selltc]

Comment from Neil Horman nhorman@redhat.com 6/1:

@@ -40,7 +40,7 @@ __unisys_extended_vmcall_gnuc(unsigned long long tuple,

     cpuid(0x00000001, &cpuid_eax, &cpuid_ebx, &cpuid_ecx, &cpuid_edx);
     if (!(cpuid_ecx & 0x80000000))
-        return -1;
+        return -EPERM;

     __asm__ __volatile__(".byte 0x00f, 0x001, 0x0c1" : "=a"(result) :
         "a"(tuple), "b"(reg_ebx), "c"(reg_ecx), "d"(reg_edx));

This gets properly checked as far as return codes go for the most part, which is good, but in the case of an issuing of the VMCALL_QUERY_GUEST_VIRTUAL_TIME_OFFSET ioctl, the return code is simply copied into a user space buffer. That might be expected if -1 is used as an error condition marker, but I would check before you assume that a user space caller will handle -EINVAL properly there.

[selltc]

6/7 - Greg applied Erik's commits to staging-testing and staging-next:

• commit 2efffad3148838842ce4ddbcb719bd1d07905d0c - staging: unisys: visorinput change -1 return value
• commit ba78c4707c8757ef8aacee7f26002c38c49caa8f - staging: unisys: visorhba change -1 return value
• commit c294ea31aa785afe2144fc933fd59106506c1c53 - staging: unisys: visorbus change -1 return values
• commit 119296eaa8a5dd8c3dbd5051202a06ae9a1d04fc - staging: unisys: visorchipset change -1 return value
• commit 35b214155601cdbb97bc2d97524b5f0df176348d - staging: unisys: iovmcall_gnuc.h change -1 return values

We still need to leave this github issue open, though, because the patch to address Neil Horman's comments are still in the queue.

[selltc]

8/15 - Greg applied the fix to address one of Neil's comments to staging-next in:

• commit ff13cf37fac6961c1c53886eb47753c82cc16963 - staging: unisys: visorbus: remove return values for write_vbus functions

[selltc]

8/15 - Greg applied this patch to staging-next, which APPEARS to be related to this github issue, but I cannot find conclusive evidence to prove it:

• commit 80224f064ead5e73d1cf7aacd20a61b5999dfdd6 - staging: unisys: visorbus: check parahotplug_request_complete_result