[RFC][PATCH] dt-bindings: usb: add non-removable device property

[RFC][PATCH] dt-bindings: usb: add non-removable device property

[Mans Rullgard]

Add a boolean property indicating that a device is hardwired to the
upstream port.  Although hubs can provide this information, they are not
always configured correctly.  An alternate means of indicating this for
built-in USB devices is thus useful.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
I have a situation where userspace would like to know which USB devices
are built-in, but the on-board hub doesn't have the right setting.
Also, the hub itself can't be indicated as fixed in any other way that
I'm aware of.

In a way, adding this property seems a bit silly since dt can only
sensibly be used for hardwired devices in the first place.  Thus the
mere presence of a dt node could be taken to indicate the same thing.
On the other hand, it's conceivable that someone might dynamically
generate a devicetree based on what happens to be connected on boot or
something.  For that reason, and explicit property seems safer.
---
 Documentation/devicetree/bindings/usb/usb-device.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
index 036be172b1ae..903d39b7e415 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.txt
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -26,6 +26,10 @@ Required properties for device nodes:
 - reg: the number of the USB hub port or the USB host-controller port to which
   this device is attached. The range is 1-255.
 
+Optional properties for device nodes:
+- non-removable: boolean, if present the device is hardwired to the
+  upstream port.
+
 
 Required properties for device nodes with interface nodes:
 - #address-cells: shall be 2
@@ -55,6 +59,10 @@ Required properties for combined nodes:
 - reg: the number of the USB hub port or the USB host-controller port to which
   this device is attached. The range is 1-255.
 
+Optional properties for combined nodes:
+- non-removable: boolean, if present the device is hardwired to the
+  upstream port.
+
 
 Required properties for hub nodes with device nodes:
 - #address-cells: shall be 1
-- 
2.20.1

Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property

[Greg Kroah-Hartman]

On Thu, Feb 28, 2019 at 02:33:44PM +0000, Mans Rullgard wrote:
> Add a boolean property indicating that a device is hardwired to the
> upstream port.  Although hubs can provide this information, they are not
> always configured correctly.  An alternate means of indicating this for
> built-in USB devices is thus useful.
> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> I have a situation where userspace would like to know which USB devices
> are built-in, but the on-board hub doesn't have the right setting.
> Also, the hub itself can't be indicated as fixed in any other way that
> I'm aware of.

Then that's a firmware bug, right?  We have a way for firmware to export
this to the USB core, why not use that?  Your on-board hub should get a
firmware update with this information, let's not try to create
yet-another-way to define this type of information please.

> In a way, adding this property seems a bit silly since dt can only
> sensibly be used for hardwired devices in the first place.  Thus the
> mere presence of a dt node could be taken to indicate the same thing.
> On the other hand, it's conceivable that someone might dynamically
> generate a devicetree based on what happens to be connected on boot or
> something.  For that reason, and explicit property seems safer.
> ---
>  Documentation/devicetree/bindings/usb/usb-device.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)

Can you show some code actually using this?  Again, this should "just
work" for USB today unless your platform is really broken (and if it is,
go complain to the vendor...)

thanks,

greg k-h

Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property

[Måns Rullgård]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Feb 28, 2019 at 02:33:44PM +0000, Mans Rullgard wrote:
>> Add a boolean property indicating that a device is hardwired to the
>> upstream port.  Although hubs can provide this information, they are not
>> always configured correctly.  An alternate means of indicating this for
>> built-in USB devices is thus useful.
>> 
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>> I have a situation where userspace would like to know which USB devices
>> are built-in, but the on-board hub doesn't have the right setting.
>> Also, the hub itself can't be indicated as fixed in any other way that
>> I'm aware of.
>
> Then that's a firmware bug, right?  We have a way for firmware to export
> this to the USB core, why not use that?  Your on-board hub should get a
> firmware update with this information, let's not try to create
> yet-another-way to define this type of information please.

What firmware?  This is not an ACPI system, obviously, so DT _is_ the
firmware.

>> In a way, adding this property seems a bit silly since dt can only
>> sensibly be used for hardwired devices in the first place.  Thus the
>> mere presence of a dt node could be taken to indicate the same thing.
>> On the other hand, it's conceivable that someone might dynamically
>> generate a devicetree based on what happens to be connected on boot or
>> something.  For that reason, and explicit property seems safer.
>> ---
>>  Documentation/devicetree/bindings/usb/usb-device.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>
> Can you show some code actually using this?  Again, this should "just
> work" for USB today unless your platform is really broken (and if it is,
> go complain to the vendor...)

You know full well that complaining to the vendor is rarely something
that works.  Especially not when there are already thousands of the
devices in the field.

This is how I meant to use it:

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3adff4da2ee1..81ef3cb705b7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device *udev)
                break;
        }
 
+       /*
+        * Otherwise, check whether DT indicates this device is non-removable.
+        */
+       if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
+               udev->removable = USB_DEVICE_FIXED;
+               return;
+       }
+
        /*
         * Otherwise, check whether the hub knows whether a port is removable
         * or not


-- 
Måns Rullgård

Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property

[Greg Kroah-Hartman]

On Thu, Feb 28, 2019 at 03:22:24PM +0000, Måns Rullgård wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Thu, Feb 28, 2019 at 02:33:44PM +0000, Mans Rullgard wrote:
> >> Add a boolean property indicating that a device is hardwired to the
> >> upstream port.  Although hubs can provide this information, they are not
> >> always configured correctly.  An alternate means of indicating this for
> >> built-in USB devices is thus useful.
> >> 
> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> ---
> >> I have a situation where userspace would like to know which USB devices
> >> are built-in, but the on-board hub doesn't have the right setting.
> >> Also, the hub itself can't be indicated as fixed in any other way that
> >> I'm aware of.
> >
> > Then that's a firmware bug, right?  We have a way for firmware to export
> > this to the USB core, why not use that?  Your on-board hub should get a
> > firmware update with this information, let's not try to create
> > yet-another-way to define this type of information please.
> 
> What firmware?  This is not an ACPI system, obviously, so DT _is_ the
> firmware.

Firmware in your hub.  There's a whole crazy software stack in that
beast :)

> >> In a way, adding this property seems a bit silly since dt can only
> >> sensibly be used for hardwired devices in the first place.  Thus the
> >> mere presence of a dt node could be taken to indicate the same thing.
> >> On the other hand, it's conceivable that someone might dynamically
> >> generate a devicetree based on what happens to be connected on boot or
> >> something.  For that reason, and explicit property seems safer.
> >> ---
> >>  Documentation/devicetree/bindings/usb/usb-device.txt | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >
> > Can you show some code actually using this?  Again, this should "just
> > work" for USB today unless your platform is really broken (and if it is,
> > go complain to the vendor...)
> 
> You know full well that complaining to the vendor is rarely something
> that works.  Especially not when there are already thousands of the
> devices in the field.

Understood, but constantly working around broken hardware is annoying at
times.

> This is how I meant to use it:
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3adff4da2ee1..81ef3cb705b7 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device *udev)
>                 break;
>         }
>  
> +       /*
> +        * Otherwise, check whether DT indicates this device is non-removable.
> +        */
> +       if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
> +               udev->removable = USB_DEVICE_FIXED;
> +               return;
> +       }

Shouldn't this be an attribute of the USB hub's port, not the device
itself?  That's the way it works with ACPI, and odds are any description
of USB devices in DT is also going to look much like how ACPI describes
the devices, let's not try to diverge when it is not necessary.

thanks,

greg k-h

Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property

[Måns Rullgård]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Feb 28, 2019 at 03:22:24PM +0000, Måns Rullgård wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Thu, Feb 28, 2019 at 02:33:44PM +0000, Mans Rullgard wrote:
>> >> Add a boolean property indicating that a device is hardwired to the
>> >> upstream port.  Although hubs can provide this information, they are not
>> >> always configured correctly.  An alternate means of indicating this for
>> >> built-in USB devices is thus useful.
>> >> 
>> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> >> ---
>> >> I have a situation where userspace would like to know which USB devices
>> >> are built-in, but the on-board hub doesn't have the right setting.
>> >> Also, the hub itself can't be indicated as fixed in any other way that
>> >> I'm aware of.
>> >
>> > Then that's a firmware bug, right?  We have a way for firmware to export
>> > this to the USB core, why not use that?  Your on-board hub should get a
>> > firmware update with this information, let's not try to create
>> > yet-another-way to define this type of information please.
>> 
>> What firmware?  This is not an ACPI system, obviously, so DT _is_ the
>> firmware.
>
> Firmware in your hub.  There's a whole crazy software stack in that
> beast :)

The hub chip itself (SMSC/Microchip USB2512B in the case at hand) is
fine.  The problem is that whoever designed the PCB didn't add the
pull-ups marking the ports non-removable.  Besides, the hub can't
indicate that it itself is hardwired to the host port.  That information
needs to be supplied elsewhere.

>> >> In a way, adding this property seems a bit silly since dt can only
>> >> sensibly be used for hardwired devices in the first place.  Thus the
>> >> mere presence of a dt node could be taken to indicate the same thing.
>> >> On the other hand, it's conceivable that someone might dynamically
>> >> generate a devicetree based on what happens to be connected on boot or
>> >> something.  For that reason, and explicit property seems safer.
>> >> ---
>> >>  Documentation/devicetree/bindings/usb/usb-device.txt | 8 ++++++++
>> >>  1 file changed, 8 insertions(+)
>> >
>> > Can you show some code actually using this?  Again, this should "just
>> > work" for USB today unless your platform is really broken (and if it is,
>> > go complain to the vendor...)
>> 
>> You know full well that complaining to the vendor is rarely something
>> that works.  Especially not when there are already thousands of the
>> devices in the field.
>
> Understood, but constantly working around broken hardware is annoying at
> times.

It's annoying, sure.  It is also the reality, and we have to deal with it.
Ignoring such hardware won't make it go away.

>> This is how I meant to use it:
>> 
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 3adff4da2ee1..81ef3cb705b7 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device *udev)
>>                 break;
>>         }
>>  
>> +       /*
>> +        * Otherwise, check whether DT indicates this device is non-removable.
>> +        */
>> +       if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
>> +               udev->removable = USB_DEVICE_FIXED;
>> +               return;
>> +       }
>
> Shouldn't this be an attribute of the USB hub's port, not the device
> itself?  That's the way it works with ACPI, and odds are any description
> of USB devices in DT is also going to look much like how ACPI describes
> the devices, let's not try to diverge when it is not necessary.

Fine with me.  That's why I asked.

How about a non-removable-ports property in the hub node listing the
hardwired ports?

-- 
Måns Rullgård

Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property

[Greg Kroah-Hartman]

On Thu, Feb 28, 2019 at 05:34:25PM +0000, Måns Rullgård wrote:
> >> --- a/drivers/usb/core/hub.c
> >> +++ b/drivers/usb/core/hub.c
> >> @@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device *udev)
> >>                 break;
> >>         }
> >>  
> >> +       /*
> >> +        * Otherwise, check whether DT indicates this device is non-removable.
> >> +        */
> >> +       if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
> >> +               udev->removable = USB_DEVICE_FIXED;
> >> +               return;
> >> +       }
> >
> > Shouldn't this be an attribute of the USB hub's port, not the device
> > itself?  That's the way it works with ACPI, and odds are any description
> > of USB devices in DT is also going to look much like how ACPI describes
> > the devices, let's not try to diverge when it is not necessary.
> 
> Fine with me.  That's why I asked.
> 
> How about a non-removable-ports property in the hub node listing the
> hardwired ports?

Sounds reasonable to me as that should mirror what happens in ACPI, so
you should be able to just mirror what drivers/usb/core/usb-acpi.c does
for dt.

thanks,

greg k-h