[PATCH 0/2] usb: override hub device bInterval with device node

[PATCH 0/2] usb: override hub device bInterval with device node

[Ikjoon Jang]

This patchset enables hard wired hub device to use different bInterval
from its descriptor when the hub has a combined device node.

When we know the specific hard wired hub supports changing its polling
interval, we can adjust hub's interval to reduce the time of waking up
from autosuspend or connect detection of HIDs.

Ikjoon Jang (2):
  dt-bindings: usb: add "hub,interval" property
  usb: overridable hub bInterval by device node

 Documentation/devicetree/bindings/usb/usb-device.txt | 4 ++++
 drivers/usb/core/config.c                            | 6 ++++++
 2 files changed, 10 insertions(+)

-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Greg Kroah-Hartman]

On Sun, Nov 17, 2019 at 11:31:49AM +0800, Ikjoon Jang wrote:
> This patchset enables hard wired hub device to use different bInterval
> from its descriptor when the hub has a combined device node.

If it is a hard-wired hub, why can't you change that interval in the
firmware for that hub as you (as a platform owner) have control over
that?

thanks,

greg k-h

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Alan Stern]

On Sun, 17 Nov 2019, Ikjoon Jang wrote:

> This patchset enables hard wired hub device to use different bInterval
> from its descriptor when the hub has a combined device node.
> 
> When we know the specific hard wired hub supports changing its polling
> interval, we can adjust hub's interval to reduce the time of waking up
> from autosuspend or connect detection of HIDs.

In fact, _all_ hubs support changing the polling interval.  The value 
given in the USB spec is just an upper limit; any smaller value is 
equally acceptable.

So why are you doing this only for hard-wired hubs?  Why not for all 
hubs?

And is 250 ms really too long to wait for remote wakeup or connect 
detection?  What's the real motivation behind this change?

Alan Stern

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Ikjoon Jang]

On Sun, Nov 17, 2019 at 3:14 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Nov 17, 2019 at 11:31:49AM +0800, Ikjoon Jang wrote:
> > This patchset enables hard wired hub device to use different bInterval
> > from its descriptor when the hub has a combined device node.
>
> If it is a hard-wired hub, why can't you change that interval in the
> firmware for that hub as you (as a platform owner) have control over
> that?

yes, actually that's the best option, but the hub devices were shipped with
mask ROM we can't change the descriptors.

>
> thanks,
>
> greg k-h

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Ikjoon Jang]

On Sun, Nov 17, 2019 at 11:46 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sun, 17 Nov 2019, Ikjoon Jang wrote:
>
> > This patchset enables hard wired hub device to use different bInterval
> > from its descriptor when the hub has a combined device node.
> >
> > When we know the specific hard wired hub supports changing its polling
> > interval, we can adjust hub's interval to reduce the time of waking up
> > from autosuspend or connect detection of HIDs.
>
> In fact, _all_ hubs support changing the polling interval.  The value
> given in the USB spec is just an upper limit; any smaller value is
> equally acceptable.
>
> So why are you doing this only for hard-wired hubs?  Why not for all
> hubs?

Because we only want to apply it to a specific device instance under
our control.
We apply autosuspend to built-in touchpad device for power savings,

Users can attach external hub devices with same VID:PID that we don't want to
change the behavior. Maybe disabling autosuspend for external HIDs
can be more reasonable for that case?

>
> And is 250 ms really too long to wait for remote wakeup or connect
> detection?  What's the real motivation behind this change?

When a user starts to move the cursor while touchpad is in autosuspend state,
It takes more than >250ms (worst case can be >500ms) to wake up and response.
That makes the cursor stuck for a while and warp to another location suddenly.

Thanks.
>
> Alan Stern
>

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Alan Stern]

On Tue, 19 Nov 2019, Ikjoon Jang wrote:

> On Sun, Nov 17, 2019 at 11:46 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Sun, 17 Nov 2019, Ikjoon Jang wrote:
> >
> > > This patchset enables hard wired hub device to use different bInterval
> > > from its descriptor when the hub has a combined device node.
> > >
> > > When we know the specific hard wired hub supports changing its polling
> > > interval, we can adjust hub's interval to reduce the time of waking up
> > > from autosuspend or connect detection of HIDs.
> >
> > In fact, _all_ hubs support changing the polling interval.  The value
> > given in the USB spec is just an upper limit; any smaller value is
> > equally acceptable.
> >
> > So why are you doing this only for hard-wired hubs?  Why not for all
> > hubs?
> 
> Because we only want to apply it to a specific device instance under
> our control.

Why?  What's so special about that device instance?

For example, why not instead have a poll_interval sysfs attribute for
all hubs that can be written from userspace?  Then people could reduce
the autoresume latency for any device they want.

> We apply autosuspend to built-in touchpad device for power savings,
> 
> Users can attach external hub devices with same VID:PID that we don't want to
> change the behavior.

Why don't you want to change the behavior?  Or allow the user to change 
the behavior?

>  Maybe disabling autosuspend for external HIDs
> can be more reasonable for that case?

If it makes sense to to save power for your built-in touchpad device, 
why doesn't it also make sense to save power for other external HIDs?

> > And is 250 ms really too long to wait for remote wakeup or connect
> > detection?  What's the real motivation behind this change?
> 
> When a user starts to move the cursor while touchpad is in autosuspend state,
> It takes more than >250ms (worst case can be >500ms) to wake up and response.
> That makes the cursor stuck for a while and warp to another location suddenly.

All right, that's a good reason.  But doesn't it apply just as well to 
other devices, not only your built-in touchpad?

Alan Stern

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Ikjoon Jang]

On Tue, Nov 19, 2019 at 11:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, 19 Nov 2019, Ikjoon Jang wrote:
>
> > On Sun, Nov 17, 2019 at 11:46 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Sun, 17 Nov 2019, Ikjoon Jang wrote:
> > >
> > > > This patchset enables hard wired hub device to use different bInterval
> > > > from its descriptor when the hub has a combined device node.
> > > >
> > > > When we know the specific hard wired hub supports changing its polling
> > > > interval, we can adjust hub's interval to reduce the time of waking up
> > > > from autosuspend or connect detection of HIDs.
> > >
> > > In fact, _all_ hubs support changing the polling interval.  The value
> > > given in the USB spec is just an upper limit; any smaller value is
> > > equally acceptable.
> > >
> > > So why are you doing this only for hard-wired hubs?  Why not for all
> > > hubs?
> >
> > Because we only want to apply it to a specific device instance under
> > our control.
>
> Why?  What's so special about that device instance?
>
> For example, why not instead have a poll_interval sysfs attribute for
> all hubs that can be written from userspace?  Then people could reduce
> the autoresume latency for any device they want.

Changing its INT interval during runtime seems not so easy, there's no device
drivers doing this to my knowledge. At least xhci needs to restart
endpoint to change
the interval. So I think patching ep descriptor at enumeration stage
is more convincing.

>
> > We apply autosuspend to built-in touchpad device for power savings,
> >
> > Users can attach external hub devices with same VID:PID that we don't want to
> > change the behavior.
>
> Why don't you want to change the behavior?  Or allow the user to change
> the behavior?
>

Yes, that's a difficult question here too, when the hub is external device,
it can't be fully controlled by here. Even though it's the same
VID:PID hub chip,
that's not the 100% same device. We don't know how much this will
impact to the other
external hub devices regarding power consumption and compatibility.

> >  Maybe disabling autosuspend for external HIDs
> > can be more reasonable for that case?
>
> If it makes sense to to save power for your built-in touchpad device,
> why doesn't it also make sense to save power for other external HIDs?
>
> > > And is 250 ms really too long to wait for remote wakeup or connect
> > > detection?  What's the real motivation behind this change?
> >
> > When a user starts to move the cursor while touchpad is in autosuspend state,
> > It takes more than >250ms (worst case can be >500ms) to wake up and response.
> > That makes the cursor stuck for a while and warp to another location suddenly.
>
> All right, that's a good reason.  But doesn't it apply just as well to
> other devices, not only your built-in touchpad?

Actually the hub is the one to be applied, I don't care much about the
rare case that
a user connects an additional external hub with same PID and connect
external HID
under that hub.

We could reduce autosuspend delay for built-in touchpad when we know
that's better
for power savings only if response time of wake up is good enough. but
we don't know
the optimal values for external HIDs. So we could use the default long
delay for external
devices, or just disable autosuspend for all external HIDs,
so user might experience much less cursor lags even with that rare case.

>
> Alan Stern
>

Re: [PATCH 0/2] usb: override hub device bInterval with device node

[Alan Stern]

On Wed, 20 Nov 2019, Ikjoon Jang wrote:

> On Tue, Nov 19, 2019 at 11:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Tue, 19 Nov 2019, Ikjoon Jang wrote:
> >
> > > On Sun, Nov 17, 2019 at 11:46 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > >
> > > > On Sun, 17 Nov 2019, Ikjoon Jang wrote:
> > > >
> > > > > This patchset enables hard wired hub device to use different bInterval
> > > > > from its descriptor when the hub has a combined device node.
> > > > >
> > > > > When we know the specific hard wired hub supports changing its polling
> > > > > interval, we can adjust hub's interval to reduce the time of waking up
> > > > > from autosuspend or connect detection of HIDs.
> > > >
> > > > In fact, _all_ hubs support changing the polling interval.  The value
> > > > given in the USB spec is just an upper limit; any smaller value is
> > > > equally acceptable.
> > > >
> > > > So why are you doing this only for hard-wired hubs?  Why not for all
> > > > hubs?
> > >
> > > Because we only want to apply it to a specific device instance under
> > > our control.
> >
> > Why?  What's so special about that device instance?
> >
> > For example, why not instead have a poll_interval sysfs attribute for
> > all hubs that can be written from userspace?  Then people could reduce
> > the autoresume latency for any device they want.
> 
> Changing its INT interval during runtime seems not so easy, there's no device
> drivers doing this to my knowledge. At least xhci needs to restart
> endpoint to change
> the interval. So I think patching ep descriptor at enumeration stage
> is more convincing.

That's a good point.  It would be necessary to send a Set-Interface 
request at the very least if you wanted to change the interval 
on-the-fly.

> > > We apply autosuspend to built-in touchpad device for power savings,
> > >
> > > Users can attach external hub devices with same VID:PID that we don't want to
> > > change the behavior.
> >
> > Why don't you want to change the behavior?  Or allow the user to change
> > the behavior?
> >
> 
> Yes, that's a difficult question here too, when the hub is external device,
> it can't be fully controlled by here. Even though it's the same
> VID:PID hub chip,
> that's not the 100% same device. We don't know how much this will
> impact to the other
> external hub devices regarding power consumption and compatibility.
> 
> > >  Maybe disabling autosuspend for external HIDs
> > > can be more reasonable for that case?
> >
> > If it makes sense to to save power for your built-in touchpad device,
> > why doesn't it also make sense to save power for other external HIDs?
> >
> > > > And is 250 ms really too long to wait for remote wakeup or connect
> > > > detection?  What's the real motivation behind this change?
> > >
> > > When a user starts to move the cursor while touchpad is in autosuspend state,
> > > It takes more than >250ms (worst case can be >500ms) to wake up and response.
> > > That makes the cursor stuck for a while and warp to another location suddenly.
> >
> > All right, that's a good reason.  But doesn't it apply just as well to
> > other devices, not only your built-in touchpad?
> 
> Actually the hub is the one to be applied, I don't care much about the
> rare case that
> a user connects an additional external hub with same PID and connect
> external HID
> under that hub.
> 
> We could reduce autosuspend delay for built-in touchpad when we know
> that's better
> for power savings only if response time of wake up is good enough. but
> we don't know
> the optimal values for external HIDs. So we could use the default long
> delay for external
> devices, or just disable autosuspend for all external HIDs,
> so user might experience much less cursor lags even with that rare case.

All right.  Can you resubmit the patch with this explanation added to
the Changelog?

Alan Stern