Re: [PATCH 2/2] usb: fail on usb_hub_create_port_device() errors

[Alan Stern]

On Wed, 21 Aug 2013, Krzysztof Mazur wrote:

> > We should never have maxchild < bNbrPorts (unless maxchild is 0).  But 
> > just in case we do, changing the code is a good idea.
> > 
> > Besides, "hub->maxchild" is shorter and easier to read than
> > "hub->descriptor->bNbrPorts".  :-)
> > 
> 
> It's "hub->hdev->maxchild", but it's still shorter and
> in many places it's just "hdev->maxchild" :)
> 
> I tested that with maxchild < bNbrPorts and there are no NULL pointer
> dereferences anymore.
> 
> Krzysiek
> -- >8 --
> Subject: [PATCH 3/2] usb: don't use bNbrPorts after initialization
> 
> After successful initialization hub->descriptor->bNbrPorts and
> hub->hdev->maxchild are equal, but using hub->hdev->maxchild is
> preferred because that value is explicitly used for initialization
> of hub->ports[].
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@xxxxxxxxxxxx>
> ---
>  drivers/usb/core/hub.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 292ffa8..86e353f 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -451,7 +451,7 @@ static void led_work (struct work_struct *work)
>  	if (hdev->state != USB_STATE_CONFIGURED || hub->quiescing)
>  		return;
>  
> -	for (i = 0; i < hub->descriptor->bNbrPorts; i++) {
> +	for (i = 0; i < hdev->maxchild; i++) {
>  		unsigned	selector, mode;
>  
>  		/* 30%-50% duty cycle */
> @@ -500,7 +500,7 @@ static void led_work (struct work_struct *work)
>  	}
>  	if (!changed && blinkenlights) {
>  		cursor++;
> -		cursor %= hub->descriptor->bNbrPorts;
> +		cursor %= hdev->maxchild;
>  		set_port_led(hub, cursor + 1, HUB_LED_GREEN);
>  		hub->indicator[cursor] = INDICATOR_CYCLE;
>  		changed++;
> @@ -826,7 +826,7 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
>  	else
>  		dev_dbg(hub->intfdev, "trying to enable port power on "
>  				"non-switchable hub\n");
> -	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> +	for (port1 = 1; port1 <= hub->hdev->maxchild; port1++)
>  		if (hub->ports[port1 - 1]->power_is_on)
>  			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
>  		else
> @@ -4655,9 +4655,7 @@ static void hub_events(void)
>  		hub_dev = hub->intfdev;
>  		intf = to_usb_interface(hub_dev);
>  		dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
> -				hdev->state, hub->descriptor
> -					? hub->descriptor->bNbrPorts
> -					: 0,
> +				hdev->state, hdev->maxchild,
>  				/* NOTE: expects max 15 ports... */
>  				(u16) hub->change_bits[0],
>  				(u16) hub->event_bits[0]);
> @@ -4702,7 +4700,7 @@ static void hub_events(void)
>  		}
>  
>  		/* deal with port status changes */
> -		for (i = 1; i <= hub->descriptor->bNbrPorts; i++) {
> +		for (i = 1; i <= hdev->maxchild; i++) {
>  			if (test_bit(i, hub->busy_bits))
>  				continue;
>  			connect_change = test_bit(i, hub->change_bits);
> 

This is good.  Combined with your other changes, it should fix all 
the problems you described.  You can add my Acked-by to these patches 
now.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/