On Wed, Jun 25, 2014 at 5:11 AM, Michalis Pappas <mpappas@xxxxxxxxxxx> wrote:
>
>
> 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< ---
>
Thanks Michalis and Dan. I've submitted the patches to address these issues.
>
> 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.
I'm not familiar with this part and need to do some homework first.
Suggestions are welcome.
>
> 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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
