Re: [PATCH v2 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node

[Lu Baolu]

On 11/13/2015 11:28 PM, Alan Stern wrote:
> On Fri, 13 Nov 2015, Lu, Baolu wrote:
>
> > On 11/13/2015 12:20 AM, Alan Stern wrote:
> > > On Thu, 12 Nov 2015, Lu Baolu wrote:
> > >
> > > > Commit 655fe4effe0f ("usbcore: add sysfs support to xHCI usb3
> > > > hardware LPM") introduced usb3_hardware_lpm sysfs node. This
> > > > doesn't show the correct status of USB3 U1 and U2 LPM status.
> > > >
> > > > This patch fixes this by replacing usb3_hardware_lpm with two
> > > > nodes, usb3_hardware_lpm_u1 (for U1) and usb3_hardware_lpm_u2
> > > > (for U2), and recording the U1/U2 LPM status in right places.
> > > >
> > > > This patch should be back-ported to kernels as old as 4.3,
> > > > that contains Commit 655fe4effe0f ("usbcore: add sysfs support
> > > > to xHCI usb3 hardware LPM").
> > > >
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > ...
> > >
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -3875,17 +3875,23 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
> > > >    		return;
> > > >    	}
> > > >    
> > > > -	if (usb_set_lpm_timeout(udev, state, timeout))
> > > > +	ret = usb_set_lpm_timeout(udev, state, timeout);
> > > > +	if (ret)
> > > >    		/* If we can't set the parent hub U1/U2 timeout,
> > > >    		 * device-initiated LPM won't be allowed either, so let the xHCI
> > > >    		 * host know that this link state won't be enabled.
> > > >    		 */
> > > >    		hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
> > > > -
> > > >    	/* Only a configured device will accept the Set Feature U1/U2_ENABLE */
> > > >    	else if (udev->actconfig)
> > > >    		usb_set_device_initiated_lpm(udev, state, true);
> > > >    
> > > > +	if (!ret) {
> > > > +		if (state == USB3_LPM_U1)
> > > > +			udev->usb3_lpm_u1_enabled = 1;
> > > > +		else if (state == USB3_LPM_U2)
> > > > +			udev->usb3_lpm_u2_enabled = 1;
> > > > +	}
> > > This doesn't look right at all.  What happens if ret is 0 but the
> > > device isn't configured?  You'll set the usb3_lpm_u*_enabled flag even
> > > though LPM isn't really enabled.
> > >
> > > Don't you want to set these flags inside the
> > > usb_set_device_initiated_lpm() function, where you know whether the
> > > action succeeded?  And leave this routine unchanged?
> > My understand is that both hub and device can initiate LPM.
> > As soon as usb_set_lpm_timeout(valid_timeout_value)
> > returns 0, the hub-initiated LPM is enabled. Thus, LPM is
> > enabled no matter the result of usb_set_device_initiated_lpm().
> > The only difference is whether device is able to initiate LPM.
> >
> > On disable side, as soon as usb_set_lpm_timeout(0) return 0,
> > hub initiated LPM is disabled. Hub will disallows link to enter
> > U1/U2 as well, even device is initiating LPM. Hence LPM
> > is disabled as soon as hub LPM timeout set to 0, no matter
> > device-initiated LPM is disabled or not.
> Then maybe you can add a comment explaining this.

Yes, I will add comments for this.

> The patch still looks strange, though.  Your new code does this:
>
> 	ret = usb_set_lpm_timeout(...);
> 	if (ret)
> 		...
> 	else if (udev->actconfig)
> 		...
> 	if (!ret) {
> 		if (state == USB3_LPM_U1)
> 		...
> 	}
>
> It would be better to do this:
>
> 	if (usb_set_lpm_timeout(...)) {
> 		...
> 	} else {
> 		if (udev->actconfig)
> 			...
> 		if (state == USB3_LPM_U1)
> 		...
> 	}

Yes, this looks better. I will refactor this part of code.

> Alan Stern

Thank you.
-Baolu


--
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/