USB hub_probe: rework ugly goto-into-compound-statement
PATCH v5 1/1 USB hub_probe: rework ugly goto-into-compound-statement
Rework smelling code (goto inside compound statement). Perhaps this is legacy. Anyway such code is not appropriate for Linux kernel.
---
Changes in v5: make bool a return type of hub_check_descriptor_sanity() Changes in v4: fix typo
Changes in v3: extract the code to static function
Changes in v2: fix spaces instead of tab, add missing 'Signed-off-by'
drivers/usb/core/hub.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)
drivers/usb/core/hub.c +18 -17
@@ -1722,10 +1722,25 @@ static void hub_disconnect(struct usb_interface *intf)kref_put (&hub->kref, hub_release); } +static bool hub_check_descriptor_sanity (struct usb_host_interface *desc) +{ +/* Some hubs have a subclass of 1, which AFAICT according to the */ +/* specs is not defined, but it works */ +if (desc->desc.bInterfaceSubClass !=0 && + desc->desc.bInterfaceSubClass !=1 ) +return false; + +/* Multiple endpoints? What kind of mutant ninja-hub is this? */ +if (desc->desc.bNumEndpoints !=1 ) +return false; + +/* If it's not an interrupt in endpoint, we'd better punt! */ +return usb_endpoint_is_int_in (&desc->endpoint[0 ].desc) !=0 ; +} +static int hub_probe (struct usb_interface *intf,const struct usb_device_id *id) {struct usb_host_interface *desc; -struct usb_endpoint_descriptor *endpoint;struct usb_device *hdev;struct usb_hub *hub;
@@ -1800,25 +1815,11 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id) }#endif -/* Some hubs have a subclass of 1, which AFAICT according to the */ -/* specs is not defined, but it works */ -if ((desc->desc.bInterfaceSubClass !=0 ) && - (desc->desc.bInterfaceSubClass !=1 )) { -descriptor_error: +if (!hub_check_descriptor_sanity (desc)) {dev_err (&intf->dev,"bad descriptor, ignoring hub\n" );return -EIO ; } -/* Multiple endpoints? What kind of mutant ninja-hub is this? */ -if (desc->desc.bNumEndpoints !=1 ) - goto descriptor_error; - - endpoint = &desc->endpoint[0 ].desc; - -/* If it's not an interrupt in endpoint, we'd better punt! */ -if (!usb_endpoint_is_int_in (endpoint)) - goto descriptor_error; -/* We found a hub */ dev_info (&intf->dev,"USB hub found\n" );
@@ -1845,7 +1846,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)if (id->driver_info &HUB_QUIRK_CHECK_PORT_AUTOSUSPEND ) hub->quirk_check_port_auto_suspend =1 ; -if (hub_configure (hub, endpoint) >=0 ) +if (hub_configure (hub, &desc->endpoint[0 ].desc) >=0 )return 0 ;hub_disconnect (intf);
Signature · 1 line
2.10.2
Greg Kroah-Hartman
maintainerQuoted 25 lines · Eugene Korenevsky
Rework smelling code (goto inside compound statement). Perhaps this is legacy. Anyway such code is not appropriate for Linux kernel.
Signed-off-by Eugene Korenevsky <[email protected]>---
Changes in v5: makeboola return type ofhub_check_descriptor_sanity()Changes in v4: fix typo
Changes in v3: extract the code to static function
Changes in v2: fix spaces instead of tab, add missing 'Signed-off-by'drivers/usb/core/hub.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)drivers/usb/core/hub.c +4
-0@@ -1722,10 +1722,25 @@ static void hub_disconnect(struct usb_interface *intf)kref_put (&hub->kref, hub_release); } +static bool hub_check_descriptor_sanity (struct usb_host_interface *desc) +{ +/* Some hubs have a subclass of 1, which AFAICT according to the */ +/* specs is not defined, but it works */
drivers/usb/core/hub.c static void hub_disconnect(struct usb_interface *intf) · line 1738
Quoted 10 lines
drivers/usb/core/hub.c +10
-0+if (desc->desc.bInterfaceSubClass !=0 && + desc->desc.bInterfaceSubClass !=1 ) +return false; + +/* Multiple endpoints? What kind of mutant ninja-hub is this? */ +if (desc->desc.bNumEndpoints !=1 ) +return false; + +/* If it's not an interrupt in endpoint, we'd better punt! */ +return usb_endpoint_is_int_in (&desc->endpoint[0 ].desc) !=0 ;
Ok, I'm going to be really pedantic here and ask that you spell this last statement out:
if (usb...) return true; return false;
thanks,
greg k-h
Greg Kroah-Hartman
maintainerQuoted 25 lines · Eugene Korenevsky
Rework smelling code (goto inside compound statement). Perhaps this is legacy. Anyway such code is not appropriate for Linux kernel.
Signed-off-by Eugene Korenevsky <[email protected]>---
Changes in v5: makeboola return type ofhub_check_descriptor_sanity()Changes in v4: fix typo
Changes in v3: extract the code to static function
Changes in v2: fix spaces instead of tab, add missing 'Signed-off-by'drivers/usb/core/hub.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)drivers/usb/core/hub.c +4
-0@@ -1722,10 +1722,25 @@ static void hub_disconnect(struct usb_interface *intf)kref_put (&hub->kref, hub_release); } +static bool hub_check_descriptor_sanity (struct usb_host_interface *desc) +{ +/* Some hubs have a subclass of 1, which AFAICT according to the */ +/* specs is not defined, but it works */
drivers/usb/core/hub.c static void hub_disconnect(struct usb_interface *intf) · line 1738
Quoted 10 lines
drivers/usb/core/hub.c +10
-0+if (desc->desc.bInterfaceSubClass !=0 && + desc->desc.bInterfaceSubClass !=1 ) +return false; + +/* Multiple endpoints? What kind of mutant ninja-hub is this? */ +if (desc->desc.bNumEndpoints !=1 ) +return false; + +/* If it's not an interrupt in endpoint, we'd better punt! */ +return usb_endpoint_is_int_in (&desc->endpoint[0 ].desc) !=0 ;
Also, the comment should say:
/* If the first endpoint is not interrupt IN, we... */
Quoted 11 lines
Ok, I'm going to be really pedantic here and ask that you spell this last statement out:
if (usb...) return true; return false;Also, the comment should say:
/* If the first endpoint is not interrupt IN, we... */
It's better to inverse the condition and return false:
```
if (!usb...)
return false;
return true;
```This is exactly what is said in the comment ("If the first endpoint... we'd better punt!"). And does not break the composition of the entire function (all if bodies return false, last statement is return true).
Signature · 1 line
Eugene