USB hub_probe: rework ugly goto-into-compound-statement
PATCH v4 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 since v3: fix typo
Changes since v2: extract the code to static function Changes since v1: 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 int 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 0 ; + +/* Multiple endpoints? What kind of mutant ninja-hub is this? */ +if (desc->desc.bNumEndpoints !=1 ) +return 0 ; + +/* If it's not an interrupt in endpoint, we'd better punt! */ +return usb_endpoint_is_int_in (&desc->endpoint[0 ].desc); +} +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
Hi,
Quoted 21 lines · Eugene Korenevsky
Rework smelling code (goto inside compound statement). Perhaps this is legacy. Anyway such code is not appropriate for Linux kernel.
Changes since v3: fix typo
Changes since v2: extract the code to static function Changes since v1: fix spaces instead of tab, add missing 'Signed-off-by'Signed-off-by Eugene Korenevsky <[email protected]>--- drivers/usb/core/hub.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)drivers/usb/core/hub.c +1
-0@@ -1722,10 +1722,25 @@ static void hub_disconnect(struct usb_interface *intf)kref_put (&hub->kref, hub_release); } +static int hub_check_descriptor_sanity (struct usb_host_interface *desc)
drivers/usb/core/hub.c static void hub_disconnect(struct usb_interface *intf) · line 1735
Quoted 10 lines
drivers/usb/core/hub.c +10
-0+{ +/* 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 0 ; + +/* Multiple endpoints? What kind of mutant ninja-hub is this? */ +if (desc->desc.bNumEndpoints !=1 ) +return 0 ;
We usually return 0 as successful result, can you return kernel error number if we check the descriptor failed? Or change the return value of the function as bool type.
Quoted 56 lines
drivers/usb/core/hub.c +7
-18+ +/* If it's not an interrupt in endpoint, we'd better punt! */ +return usb_endpoint_is_int_in (&desc->endpoint[0 ].desc); +} +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); --2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Baolin.wang Best Regards
Greg Kroah-Hartman
maintainerQuoted 6 lines · Eugene Korenevsky
Rework smelling code (goto inside compound statement). Perhaps this is legacy. Anyway such code is not appropriate for Linux kernel.
Changes since v3: fix typo
Changes since v2: extract the code to static function Changes since v1: fix spaces instead of tab, add missing 'Signed-off-by'
changes go below the --- line, please read Documentation/SubmittingPatches for all of the details.
thanks,
greg k-h