Anybody working on gdm72xx?

[Michalis Pappas]

On 06/24/2014 08:12 AM, Ben Chan wrote:
> Thanks Kristina.  checkpatch reports 1 errors and 17 warnings on the
> current driver.
> 
> I guess I need to first submit patches to staging-next to address
> these issues, and then submit another patch to move it out of staging,
> right?
>

Hi everyone,

I've been working on a driver review for gdm72xx and I think there are some more issues to be fixed except from stuff reported by checkpatch.pl. Here's what I've come up with so far:

First of all, during a patch review, Dan Carpenter (cced) mentioned some issues. I quote his message:

--- 8< --- snip snip --- 8< ---

Also why is returning a u32?  Kernel style is int.  But this doesn't
return errors and the callers don't check so it should be void.  This
driver suffers from prefering u32 over int in many places.

...

In gdm_usb_send(), the "BUG_ON(len > TX_BUF_SIZE - padding - 1);" should
be proper error handling."

--- 8< --- snip snip --- 8< ---

Additionally, wm_ioctl.h defines its own netlink address. FWIK netlink addresses should be defined in uapi/linux/netlink.h and given the scarcity of netlink addresses, I think netlink_generic should be used instead. Please correct me if I'm wrong.

Now regarding some of the checkpatch errors:

> WARNING: unchecked sscanf return value
> #2662: FILE: drivers/net/wimax/gdm72xx/gdm_wimax.c:294:
> +               sscanf(e->dev->name, "wm%d", &idx);
> 

I know Ben has submitted a patch for this already, but from my understanding this check is not really needed. The value stored in e->dev>name is generated by __dev_alloc_name which does all the necessary checks for user supplied input etc, so it should be considered as a trusted value.

> ERROR: Macros with complex values should be enclosed in parenthesis
> #4429: FILE: drivers/net/wimax/gdm72xx/usb_ids.h:34:
> +#define USB_DEVICE_BOOTLOADER(vid, pid)        \
> +       {USB_DEVICE((vid), ((pid)&BL_PID_MASK)|B_DOWNLOAD)},    \
> +       {USB_DEVICE((vid), ((pid)&BL_PID_MASK)|B_DOWNLOAD|B_DIFF_DL_DRV)}

This a false positive indeed