Oliver Neukum <oneukum@xxxxxxxx> writes:
> This introduces the common parser for extra CDC headers now that it no longer
> depends on usbnet.
>
> Signed-off-by: Oliver Neukum <ONeukum@xxxxxxxx>
> ---
> drivers/usb/class/cdc-acm.c | 68 +++++++--------------------------------------
> 1 file changed, 10 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index def5a54..1620542 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1147,6 +1147,7 @@ static int acm_probe(struct usb_interface *intf,
> {
> struct usb_cdc_union_desc *union_header = NULL;
> struct usb_cdc_country_functional_desc *cfd = NULL;
> + struct usb_cdc_call_mgmt_descriptor *cmd = NULL;
> unsigned char *buffer = intf->altsetting->extra;
> int buflen = intf->altsetting->extralen;
> struct usb_interface *control_interface;
This patch looks good to me. Feel free to add
Reviewed-by: Bjørn Mork <bjorn@xxxxxxx>
if you want to.
But if you are spinning a new series, then I have one minor comment: Do
we really need all these additional descriptor pointers? Or at least,
do we need them in this scope? acm_probe() is very long and it is
difficult to keep track of all the variables. Moving some of the
variables to a more local scope would help.
BTW, "cmd" is often used for "command", so I'n not sure it's a good name
for this descriptor, as this grep shows:
bjorn@miraculix:/usr/local/src/git/linux$ git grep -n cmd drivers/usb/class/cdc-acm.c
drivers/usb/class/cdc-acm.c:1010: unsigned int cmd, unsigned long arg)
drivers/usb/class/cdc-acm.c:1015: switch (cmd) {
drivers/usb/class/cdc-acm.c:1150: struct usb_cdc_call_mgmt_descriptor *cmd = NULL;
drivers/usb/class/cdc-acm.c:1214: cmd = hdr.usb_cdc_call_mgmt_descriptor;
drivers/usb/class/cdc-acm.c:1215: if (cmd)
drivers/usb/class/cdc-acm.c:1216: call_interface_num = cmd->bDataInterface;
But as you also see, it is only used once to assign call_interface_num.
If the length of that assignment is the problem, then I suggest
a) renaming hdr => h saves you two characters everywhere the
descriptors are used and reducess
call_interface_num = h.usb_cdc_call_mgmt_descriptor->bDataInterface;
to 83 columns width.
b) or moving the variable into the if () scope:
if (hdr.usb_cdc_call_mgmt_descriptor) {
struct usb_cdc_call_mgmt_descriptor *cm;
cm = hdr.usb_cdc_call_mgmt_descriptor;
call_interface_num = cmd->cm;
}
Personally, I find b very awkward. IMHO, it would be better to either
break the 80 column rule or break the assigment into two lines. In the
longer run, I think we also should consider if the long field names in
struct usb_cdc_parsed_header helps on readability. It makes the names
self explanatory, but that does not help when you cannot use the field
name directly because it is so long...
Anyway, back to cdc-acm: "union_header" is also used only once, and
only inside an else scope:
bjorn@miraculix:/usr/local/src/git/linux$ git grep -n union_header drivers/usb/class/cdc-acm.c
drivers/usb/class/cdc-acm.c:1148: struct usb_cdc_union_desc *union_header = NULL;
drivers/usb/class/cdc-acm.c:1213: union_header = hdr.usb_cdc_union_desc;
drivers/usb/class/cdc-acm.c:1218: if (!union_header) {
drivers/usb/class/cdc-acm.c:1239: control_interface = usb_ifnum_to_if(usb_dev, union_header->bMasterInterface0);
drivers/usb/class/cdc-acm.c:1240: data_interface = usb_ifnum_to_if(usb_dev, (data_interface_num = union_header->bSlaveInterface0));
Those assignments are already breaking 80 columns. And that nested
assignment in there - I didn't notice that before. That's ugly. This
could just as well be rewritten to
if (!hdr.usb_cdc_union_desc) {
..
} else {
control_interface = usb_ifnum_to_if(usb_dev,
hdr.usb_cdc_union_desc->bMasterInterface0);
data_interface_num = hdr.usb_cdc_union_desc->->bSlaveInterface0;
data_interface = usb_ifnum_to_if(usb_dev, data_interface_num);
}
And then the last one. "cfd" is also used only in a single block. It's
used multiple times there, so a local variable makes sense though:
bjorn@miraculix:/usr/local/src/git/linux$ git grep -n cfd drivers/usb/class/cdc-acm.c
drivers/usb/class/cdc-acm.c:1149: struct usb_cdc_country_functional_desc *cfd = NULL;
drivers/usb/class/cdc-acm.c:1438: cfd = hdr.usb_cdc_country_functional_desc;
drivers/usb/class/cdc-acm.c:1439: if (cfd) { /* export the country data */
drivers/usb/class/cdc-acm.c:1440: acm->country_codes = kmalloc(cfd->bLength - 4, GFP_KERNEL);
drivers/usb/class/cdc-acm.c:1443: acm->country_code_size = cfd->bLength - 4;
drivers/usb/class/cdc-acm.c:1444: memcpy(acm->country_codes, (u8 *)&cfd->wCountyCode0,
drivers/usb/class/cdc-acm.c:1445: cfd->bLength - 4);
drivers/usb/class/cdc-acm.c:1446: acm->country_rel_date = cfd->iCountryCodeRelDate;
This could be
if (hdr.usb_cdc_country_functional_desc) { /* country data */
struct usb_cdc_country_functional_desc *cfd;
cfd = hdr.usb_cdc_country_functional_desc;
acm->country_codes = kmalloc(cfd->bLength - 4, GFP_KERNEL);
..
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
