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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
