On 06/28/2014 07:39 AM, Ben Chan wrote:
> On Wed, Jun 25, 2014 at 5:11 AM, Michalis Pappas <mpap...@fastmail.fm> 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.
>
Hi Ben,
would you be interested to work on this driver together? My reviewing
process is a bit slow as this is the first driver I'm going through and
I would like to understand how everything works in detail. I was
planning to submit a series of patches once I finished my review and got
my hands on some hardware for testing.
On the other hand, if you would like to take over on your own, I am
happy to share my progress so far and switch to another driver (I have
the et131x in mind if it turns out that no one else is working on it).
Thanks,
Michalis
>>
>> 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
