diff mbox series

[29/29] arm64: dts: qcom: Harmonize DWC USB3 DT nodes name

Message ID 20201020115959.2658-30-Sergey.Semin@baikalelectronics.ru
State Superseded
Headers show
Series dt-bindings: usb: Harmonize xHCI/EHCI/OHCI/DWC3 nodes name | expand

Commit Message

Serge Semin Oct. 20, 2020, 11:59 a.m. UTC
In accordance with the DWC USB3 bindings the corresponding node
name is suppose to comply with the Generic USB HCD DT schema, which
requires the USB nodes to have the name acceptable by the regexp:
"^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
named.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 4 ++--
 arch/arm64/boot/dts/qcom/ipq8074.dtsi        | 4 ++--
 arch/arm64/boot/dts/qcom/msm8996.dtsi        | 4 ++--
 arch/arm64/boot/dts/qcom/msm8998.dtsi        | 2 +-
 arch/arm64/boot/dts/qcom/qcs404-evb.dtsi     | 2 +-
 arch/arm64/boot/dts/qcom/qcs404.dtsi         | 4 ++--
 arch/arm64/boot/dts/qcom/sc7180.dtsi         | 2 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi         | 4 ++--
 arch/arm64/boot/dts/qcom/sm8150.dtsi         | 2 +-
 9 files changed, 14 insertions(+), 14 deletions(-)

Comments

Jun Li Nov. 2, 2020, 7:34 a.m. UTC | #1
Serge Semin <Sergey.Semin@baikalelectronics.ru> 于2020年10月20日周二 下午8:04写道:
>
> In accordance with the DWC USB3 bindings the corresponding node
> name is suppose to comply with the Generic USB HCD DT schema, which
> requires the USB nodes to have the name acceptable by the regexp:
> "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> named.

This need a counterpart driver change:
drivers/usb/dwc3/dwc3-qcom.c
dwc3_np = of_get_child_by_name(np, "dwc3");

Li Jun
John Stultz July 14, 2021, 12:07 a.m. UTC | #2
On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> In accordance with the DWC USB3 bindings the corresponding node
> name is suppose to comply with the Generic USB HCD DT schema, which
> requires the USB nodes to have the name acceptable by the regexp:
> "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> named.
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

I know folks like to ignore this, but this patch breaks AOSP on db845c. :(

In the exact same way an earlier patch broke HiKey960:
  https://lore.kernel.org/lkml/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/

(which I still have to carry a revert for).

I get that this change is useful so more dynamic userland can find
devices using consistent naming with future kernels (but doesn't the
dynamic userland have to handle the case for older kernels as well?)
But for userland that uses static configs, its painful as updating
userland to use the new node ids then causes older kernels to fail.

I'm looking into how we might be able to probe and set the property
dynamically, but AOSP's init system is far more aligned to static
configs.

This will probably be ignored again, but it would be nice if we could
have a release where DTS changes don't break userland for one of my
boards. As it feels like its been awhile.

thanks
-john
Bjorn Andersson July 14, 2021, 2:27 a.m. UTC | #3
On Tue 13 Jul 19:07 CDT 2021, John Stultz wrote:

> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > In accordance with the DWC USB3 bindings the corresponding node
> > name is suppose to comply with the Generic USB HCD DT schema, which
> > requires the USB nodes to have the name acceptable by the regexp:
> > "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> > named.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> I know folks like to ignore this, but this patch breaks AOSP on db845c. :(
> 

Sorry, I totally forgot that the name of that node is part of the USB
gadget configfs interface.

> In the exact same way an earlier patch broke HiKey960:
>   https://lore.kernel.org/lkml/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/
> 
> (which I still have to carry a revert for).
> 
> I get that this change is useful so more dynamic userland can find
> devices using consistent naming with future kernels (but doesn't the
> dynamic userland have to handle the case for older kernels as well?)
> But for userland that uses static configs, its painful as updating
> userland to use the new node ids then causes older kernels to fail.
> 

It won't help you, but having a mechanism to provide user friendly names
would certainly be welcome. I always struggle with the question of what
"6a00000.dwc3" (now .usb) actually is...

> I'm looking into how we might be able to probe and set the property
> dynamically, but AOSP's init system is far more aligned to static
> configs.
> 
> This will probably be ignored again, but it would be nice if we could
> have a release where DTS changes don't break userland for one of my
> boards. As it feels like its been awhile.
> 

I don't have any preference to this being names "dwc3" or "usb" and we
could back out the change in time for v5.14. But you will still have the
problem for Hikey iiuc and the dts would then violate the binding - so
we need to fix that, and all the other Qualcomm boards defined by the
same binding.

Regards,
Bjorn
Serge Semin July 14, 2021, 12:48 p.m. UTC | #4
Hello John,

On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:
> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin

> <Sergey.Semin@baikalelectronics.ru> wrote:

> >

> > In accordance with the DWC USB3 bindings the corresponding node

> > name is suppose to comply with the Generic USB HCD DT schema, which

> > requires the USB nodes to have the name acceptable by the regexp:

> > "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly

> > named.

> >

> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

> 


> I know folks like to ignore this, but this patch breaks AOSP on db845c. :(


Sorry to hear that. Alas there is no much can be done about it.
DT-nodes name is a subject of DT-schema convention and as we've finally
unified USB-controller nodes it shouldn't be reverted back. You can
find the final USB-controller bindings in:
Documentation/devicetree/bindings/usb/usb.yaml
It strictly defines to have USB-nodes with names like "usb(@.*)".
Reverting this patch will cause the DT-bindings check procedure
failure. You can also find the naming convention defined in the
latest DT spec:
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3

See also device-tree bindings requirements listed in the file:
Documentation/devicetree/bindings/writing-bindings.rst
It says: "DO use node names matching the class of the device. Many
standard names are defined in the DT Spec. If there isn't one,
consider adding it."

> 

> In the exact same way an earlier patch broke HiKey960:

>   https://lore.kernel.org/lkml/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/

> 

> (which I still have to carry a revert for).

> 

> I get that this change is useful so more dynamic userland can find

> devices using consistent naming with future kernels (but doesn't the

> dynamic userland have to handle the case for older kernels as well?)

> But for userland that uses static configs, its painful as updating

> userland to use the new node ids then causes older kernels to fail.

> 

> I'm looking into how we might be able to probe and set the property

> dynamically, but AOSP's init system is far more aligned to static

> configs.

> 


As Krzysztof said in
https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
and Bjorn noted in his response to your email, the only way to solve
the problem is to fix the user-land app so one would be able to deal
with both old and new DT-nodes name. Alternatively you can just
replace the dts with older one, where the name still have
the "dwc3"-prefix.

-Sergey

> This will probably be ignored again, but it would be nice if we could

> have a release where DTS changes don't break userland for one of my

> boards. As it feels like its been awhile.

> 

> thanks

> -john
Bjorn Andersson July 14, 2021, 2:59 p.m. UTC | #5
On Wed 14 Jul 07:48 CDT 2021, Serge Semin wrote:
[..]
> As Krzysztof said in

> https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/

> and Bjorn noted in his response to your email, the only way to solve

> the problem is to fix the user-land app so one would be able to deal

> with both old and new DT-nodes name.


How do you suggest this to be done? The userspace ABI in question is
used to bind a USB gadgets to the particular USB controller, using the
device name of the USB controller.

> Alternatively you can just

> replace the dts with older one, where the name still have

> the "dwc3"-prefix.

> 


This is exactly what Linus has mind when he tells us not to break
user space.

Regards,
Bjorn
Greg KH July 21, 2021, 7:37 a.m. UTC | #6
On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:
> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > In accordance with the DWC USB3 bindings the corresponding node
> > name is suppose to comply with the Generic USB HCD DT schema, which
> > requires the USB nodes to have the name acceptable by the regexp:
> > "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> > named.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> I know folks like to ignore this, but this patch breaks AOSP on db845c. :(
> 
> In the exact same way an earlier patch broke HiKey960:
>   https://lore.kernel.org/lkml/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/
> 
> (which I still have to carry a revert for).

This is not ok, I'll go revert it and push it to Linus soon.

thanks,

greg k-h
Greg KH July 21, 2021, 7:39 a.m. UTC | #7
On Tue, Jul 13, 2021 at 09:27:39PM -0500, Bjorn Andersson wrote:
> On Tue 13 Jul 19:07 CDT 2021, John Stultz wrote:
> 
> > On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> > >
> > > In accordance with the DWC USB3 bindings the corresponding node
> > > name is suppose to comply with the Generic USB HCD DT schema, which
> > > requires the USB nodes to have the name acceptable by the regexp:
> > > "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> > > named.
> > >
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > I know folks like to ignore this, but this patch breaks AOSP on db845c. :(
> > 
> 
> Sorry, I totally forgot that the name of that node is part of the USB
> gadget configfs interface.

Yes, and as such, it's a user-visible change, so I will go revert this.

thanks,

greg k-h
Greg KH July 21, 2021, 11:02 a.m. UTC | #8
On Wed, Jul 21, 2021 at 12:45:32PM +0200, Krzysztof Kozlowski wrote:
> On 21/07/2021 12:29, Greg Kroah-Hartman wrote:
> > On Wed, Jul 21, 2021 at 01:02:20PM +0300, Serge Semin wrote:
> >> Hi Greg,
> >> @Krzysztof, @Rob, please join the discussion so to finally get done
> >> with the concerned issue.
> >>
> >> On Wed, Jul 21, 2021 at 09:38:54AM +0200, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 14, 2021 at 03:48:07PM +0300, Serge Semin wrote:
> >>>> Hello John,
> >>>>
> >>>> On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:
> >>>>> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> >>>>> <Sergey.Semin@baikalelectronics.ru> wrote:
> >>>>>>
> >>>>>> In accordance with the DWC USB3 bindings the corresponding node
> >>>>>> name is suppose to comply with the Generic USB HCD DT schema, which
> >>>>>> requires the USB nodes to have the name acceptable by the regexp:
> >>>>>> "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> >>>>>> named.
> >>>>>>
> >>>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>>>
> >>>>
> >>>>> I know folks like to ignore this, but this patch breaks AOSP on db845c. :(
> >>>>
> >>>> Sorry to hear that. Alas there is no much can be done about it.
> >>>
> >>> Yes there is, we can revert the change.  We do not break existing
> >>> configurations, sorry.
> >>
> >> By reverting this patch we'll get back to the broken dt-bindings
> >> since it won't comply to the current USB DT-nodes requirements
> >> which at this state well describe the latest DT spec:
> >> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
> >> Thus the dtbs_check will fail for these nodes.
> >>
> >> Originally this whole patchset was connected with finally getting the
> >> DT-node names in order to comply with the standard requirement and it
> >> was successful mostly except a few patches which still haven't been
> >> merged in.
> >>
> >> Anyway @Krzysztof has already responded to the complain regarding this
> >> issue here:
> >> https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> >> but noone cared to respond on his reasonable questions in order to
> >> get to a suitable solution for everyone. Instead we are
> >> getting another email with the same request to revert the changes.
> >> Here is the quote from the Krzysztof email so we could continue the
> >> discussion:
> >>
> >> On Mon, 21 Dec 2020 13:04:27 -0800 (PST), Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> On Mon, Dec 21, 2020 at 12:24:11PM -0800, John Stultz wrote:
> >>>> On Sat, Dec 19, 2020 at 3:06 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>> ...
> >>>>>
> >>>>> The node names are not part of an ABI, are they? I expect only
> >>>>> compatibles and properties to be stable. If user-space looks for
> >>>>> something by name, it's a user-space's mistake.  Not mentioning that you
> >>>>> also look for specific address... Imagine remapping of addresses with
> >>>>> ranges (for whatever reason) - AOSP also would be broken? Addresses are
> >>>>> definitely not an ABI.
> >>>>
> >>>> Though that is how it's exported through sysfs.
> >>>
> >>> The ABI is the format of sysfs file for example in /sys/devices. However
> >>> the ABI is not the exact address or node name of each device.
> >>>
> >>>> In AOSP it is then used to setup the configfs gadget by writing that
> >>>> value into /config/usb_gadget/g1/UDC.
> >>>>
> >>>> Given there may be multiple controllers on a device, or even if its
> >>>> just one and the dummy hcd driver is enabled, I'm not sure how folks
> >>>> reference the "right" one without the node name?
> >>>
> >>> I think it is the same type of problem as for all other subsystems, e.g.
> >>> mmc, hwmon/iio.  They usually solve it either with aliases or with
> >>> special property with the name/label.
> >>>
> >>>> I understand the fuzziness with sysfs ABI, and I get that having
> >>>> consistent naming is important, but like the eth0 -> enp3s0 changes,
> >>>> it seems like this is going to break things.
> >>>
> >>> One could argue whether interface name is or is not ABI. But please tell
> >>> me how the address of a device in one's representation (for example DT)
> >>> is a part of a stable interface?
> >>>
> >>>> Greg? Is there some better way AOSP should be doing this?
> >>>
> >>> If you need to find specific device, maybe go through the given bus and
> >>> check compatibles?
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>
> >> So the main question is how is the DT-node really connected with ABI
> >> and is supposed to be stable in that concern?
> >>
> >> As I see it even if it affects the configfs node name, then we may
> >> either need to break that connection and somehow deliver DT-node-name
> >> independent interface to the user-space or we have no choice but to
> >> export the node with an updated name and ask of user-space to deal
> >> with it. In both suggested cases the DT-node name will still conform
> >> to the USB-node name DT spec. Currently we are at the second one.
> > 
> > I really do not care what you all decide on, but you CAN NOT break
> > existing working systems, sorry.  That is why I have reverted this
> > change in my tree and will send it to Linus soon.
> 
> I had impression that kernel defines interfaces which should be used and
> are stable (e.g. syscalls, sysfs and so on). This case is example of
> user-space relying on something not being marked as part of ABI. Instead
> they found something working for them and now it is being used in "we
> cannot break existing systems". Basically, AOSP unilaterally created a
> stable ABI and now kernel has to stick to it.

Since when are configfs names NOT a user-visable api?

Why would you not depend on them?

> Really, all normal systems depend on aliases or names and here we have
> dependency on device address. I proposed way how AOSP should be fixed.
> Anything happened? Nope.

Please work with the Android developers to fix this in their tree.  I
know they take patches quite easily if sent to them.  If this gets fixed
in their tree I will gladly revert this change.

> The device address can change. The node name can change. Reverting such
> changes is incorrect but my arguments why we can break existing systems
> who use weird, incorrect and not stable interfaces were not accepted and
> I do not have anything new in this matter.
> 
> Greg,
> You also did not join the discussion but use simple revert. It's not
> cooperative... what next? Serge sends the same patch to SoC tree and it
> gets merged and then you revert it again?

Yup, I can do this all day :)

Again, do NOT break working systems please, that's pretty much the ONLY
rule we have in kernel development.  It's not that complex of a rule...

thanks,

greg k-h
Serge Semin July 21, 2021, 11:25 a.m. UTC | #9
On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:
> On 21/07/2021 13:02, Greg Kroah-Hartman wrote:
> > On Wed, Jul 21, 2021 at 12:45:32PM +0200, Krzysztof Kozlowski wrote:
> >> On 21/07/2021 12:29, Greg Kroah-Hartman wrote:
> >>> On Wed, Jul 21, 2021 at 01:02:20PM +0300, Serge Semin wrote:
> >>>> Hi Greg,
> >>>> @Krzysztof, @Rob, please join the discussion so to finally get done
> >>>> with the concerned issue.
> >>>>
> >>>> On Wed, Jul 21, 2021 at 09:38:54AM +0200, Greg Kroah-Hartman wrote:
> >>>>> On Wed, Jul 14, 2021 at 03:48:07PM +0300, Serge Semin wrote:
> >>>>>> Hello John,
> >>>>>>
> >>>>>> On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:
> >>>>>>> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> >>>>>>> <Sergey.Semin@baikalelectronics.ru> wrote:
> >>>>>>>>
> >>>>>>>> In accordance with the DWC USB3 bindings the corresponding node
> >>>>>>>> name is suppose to comply with the Generic USB HCD DT schema, which
> >>>>>>>> requires the USB nodes to have the name acceptable by the regexp:
> >>>>>>>> "^usb(@.*)?" . Make sure the "snps,dwc3"-compatible nodes are correctly
> >>>>>>>> named.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>>>>>
> >>>>>>
> >>>>>>> I know folks like to ignore this, but this patch breaks AOSP on db845c. :(
> >>>>>>
> >>>>>> Sorry to hear that. Alas there is no much can be done about it.
> >>>>>
> >>>>> Yes there is, we can revert the change.  We do not break existing
> >>>>> configurations, sorry.
> >>>>
> >>>> By reverting this patch we'll get back to the broken dt-bindings
> >>>> since it won't comply to the current USB DT-nodes requirements
> >>>> which at this state well describe the latest DT spec:
> >>>> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
> >>>> Thus the dtbs_check will fail for these nodes.
> >>>>
> >>>> Originally this whole patchset was connected with finally getting the
> >>>> DT-node names in order to comply with the standard requirement and it
> >>>> was successful mostly except a few patches which still haven't been
> >>>> merged in.
> >>>>
> >>>> Anyway @Krzysztof has already responded to the complain regarding this
> >>>> issue here:
> >>>> https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> >>>> but noone cared to respond on his reasonable questions in order to
> >>>> get to a suitable solution for everyone. Instead we are
> >>>> getting another email with the same request to revert the changes.
> >>>> Here is the quote from the Krzysztof email so we could continue the
> >>>> discussion:
> >>>>
> >>>> On Mon, 21 Dec 2020 13:04:27 -0800 (PST), Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>> On Mon, Dec 21, 2020 at 12:24:11PM -0800, John Stultz wrote:
> >>>>>> On Sat, Dec 19, 2020 at 3:06 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>>>> ...
> >>>>>>>
> >>>>>>> The node names are not part of an ABI, are they? I expect only
> >>>>>>> compatibles and properties to be stable. If user-space looks for
> >>>>>>> something by name, it's a user-space's mistake.  Not mentioning that you
> >>>>>>> also look for specific address... Imagine remapping of addresses with
> >>>>>>> ranges (for whatever reason) - AOSP also would be broken? Addresses are
> >>>>>>> definitely not an ABI.
> >>>>>>
> >>>>>> Though that is how it's exported through sysfs.
> >>>>>
> >>>>> The ABI is the format of sysfs file for example in /sys/devices. However
> >>>>> the ABI is not the exact address or node name of each device.
> >>>>>
> >>>>>> In AOSP it is then used to setup the configfs gadget by writing that
> >>>>>> value into /config/usb_gadget/g1/UDC.
> >>>>>>
> >>>>>> Given there may be multiple controllers on a device, or even if its
> >>>>>> just one and the dummy hcd driver is enabled, I'm not sure how folks
> >>>>>> reference the "right" one without the node name?
> >>>>>
> >>>>> I think it is the same type of problem as for all other subsystems, e.g.
> >>>>> mmc, hwmon/iio.  They usually solve it either with aliases or with
> >>>>> special property with the name/label.
> >>>>>
> >>>>>> I understand the fuzziness with sysfs ABI, and I get that having
> >>>>>> consistent naming is important, but like the eth0 -> enp3s0 changes,
> >>>>>> it seems like this is going to break things.
> >>>>>
> >>>>> One could argue whether interface name is or is not ABI. But please tell
> >>>>> me how the address of a device in one's representation (for example DT)
> >>>>> is a part of a stable interface?
> >>>>>
> >>>>>> Greg? Is there some better way AOSP should be doing this?
> >>>>>
> >>>>> If you need to find specific device, maybe go through the given bus and
> >>>>> check compatibles?
> >>>>>
> >>>>> Best regards,
> >>>>> Krzysztof
> >>>>
> >>>> So the main question is how is the DT-node really connected with ABI
> >>>> and is supposed to be stable in that concern?
> >>>>
> >>>> As I see it even if it affects the configfs node name, then we may
> >>>> either need to break that connection and somehow deliver DT-node-name
> >>>> independent interface to the user-space or we have no choice but to
> >>>> export the node with an updated name and ask of user-space to deal
> >>>> with it. In both suggested cases the DT-node name will still conform
> >>>> to the USB-node name DT spec. Currently we are at the second one.
> >>>
> >>> I really do not care what you all decide on, but you CAN NOT break
> >>> existing working systems, sorry.  That is why I have reverted this
> >>> change in my tree and will send it to Linus soon.
> >>
> >> I had impression that kernel defines interfaces which should be used and
> >> are stable (e.g. syscalls, sysfs and so on). This case is example of
> >> user-space relying on something not being marked as part of ABI. Instead
> >> they found something working for them and now it is being used in "we
> >> cannot break existing systems". Basically, AOSP unilaterally created a
> >> stable ABI and now kernel has to stick to it.
> > 
> > Since when are configfs names NOT a user-visable api?
> > 
> > Why would you not depend on them?
> 

> It's not good example. The configfs entries (file names) are
> user-visible however the USB gadget exposes specific value for specific
> one device. It encodes device specific DT node name and HW address and
> gives it to user-space. It is valid only on this one HW, all other
> devices will have different values.
> 
> User-space has hard-coded this value (DT node name and hardware
> address). This value was never part of configfs ABI, maybe except of its
> format "[a-z]+\.[0-9a-f]+". Format is not broken. Just the value changes
> for a specific device/hardware.
> 
> It's like you depend that lsusb will always report:
>   Bus 003 Device 008: ID 046d:c52b Logitech, Inc. Unifying Receiver
> and then probing order changed and this Logitech ends as Device 009.
> Then AOSP guys come, wait, we hard-coded that Logitech on our device
> will be always Device 008, not 009. Please revert it, we depend on
> specific value of Device number. It must be always 009...
> 
> For the record - the change discussed here it's nothing like USB VID/PID. :)

Right I was wrong referring to the configfs names in this context.
That must have mislead Greg.

Getting back to the topic AFAICS from what John said in here
https://lore.kernel.org/lkml/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/

AOSP developers somehow hardcoded a USB-controller UDC name in the
internal property called "sys.usb.controller" with a value
"ff100000.dwc3". That value is generated by the kernel based on the
corresponding DT-node name. The property is then used to
pre-initialize the system like it's done here:
https://android.googlesource.com/platform/system/core/+/master/rootdir/init.usb.configfs.rc

Since we changed the DT-node name in the recent kernel, we thus changed the
UDC controller name so AOSP init procedure now fails to bring up the Linux
USB-gadget using on the older UDC name. UDC is supposed to be ff100000.usb now
(after this patch has been merged in).

What problems I see here:
1) the AOSP developers shouldn't have hard-coded the value but read
from the /sys/class/udc/* directory and then decided which controller
to use. As it's described for instance here:
https://www.kernel.org/doc/Documentation/usb/gadget_configfs.txt
2) even if they hard-coded the value, then they should have used an
older dts file for their platform, since DTS is more
platform-specific, but not the kernel one. Even if a dts-file is
supplied in the kernel it isn't supposed to have the node names
unchanged from release to release.

Regards,
-Sergey

> 
> Best regards,
> Krzysztof
John Stultz July 22, 2021, 8:09 p.m. UTC | #10
On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> > On Jul 21, 2021, 1:45 PM +0200, Krzysztof Kozlowski wrote:
> > > I had impression that kernel defines interfaces which should be used and
> > > are stable (e.g. syscalls, sysfs and so on). This case is example of
> > > user-space relying on something not being marked as part of ABI. Instead
> > > they found something working for them and now it is being used in "we
> > > cannot break existing systems". Basically, AOSP unilaterally created a
> > > stable ABI and now kernel has to stick to it.
> > >
> > > Really, all normal systems depend on aliases or names and here we have
> > > dependency on device address. I proposed way how AOSP should be fixed.
> > > Anything happened? Nope.
> >
> > First time he sent a possible solution for the problem:
> > https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> >
> > To sum up you could have used one of the more portable approaches
> > 1. add an udc alias to the controller and use it then to refer to the
> > corresponding USB controller
>
> Is there such a thing as "UDC alias"? Or are you suggesting that we
> should add such feature?
>
> I think it would be wonderful if we could identify the UDCs on our
> boards as "USB1" and "USB2", or "the one and only USB-C connector". But
> unless that will fall back to the existing naming it would break John's
> _existing_ userspace.

Well, I'd not hold up the existing userspace I'm using as sacrosanct
(AOSP devices still usually don't have to work cross-kernel versions -
devboards being the main exception). I'm fine if we can rework
userland as proposed, so that the issues can be avoided, but I
honestly don't have enough context to really understand what that
looks like (no idea what udc aliases are).

And whatever we do, the main constraint is that userland has to be
able to work with existing LTS kernels and newer kernels.

> > 2. search through DT-nodes looking for a specific compatible/reg
> > DT-properties.
> >
>
> We could define that this is the way, but again we didn't yesterday so
> your proposal is not backwards compatible.

It may be backwards compatible, but I'm still not clear exactly how it
would work.

I guess if we look through all
sys/bus/platform/devices/*/of_node/compatbile strings for the desired
"snps,dwc3", then find which of the directories have the desired
address in the string? (The suggestion for looking at reg seems
better, but I don't get a char value out of the dwc3 of_node/reg
file).

It seems much more straightforward to do an `ls /sys/class/udc/$GADGET_ADDR.*`

But again, if folks decide the names can be rearranged to usb.<addr>
in the future, that would break too.

thanks
-john
Serge Semin July 22, 2021, 10:09 p.m. UTC | #11
On Thu, Jul 22, 2021 at 01:09:05PM -0700, John Stultz wrote:
> On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > > On Jul 21, 2021, 1:45 PM +0200, Krzysztof Kozlowski wrote:
> > > > I had impression that kernel defines interfaces which should be used and
> > > > are stable (e.g. syscalls, sysfs and so on). This case is example of
> > > > user-space relying on something not being marked as part of ABI. Instead
> > > > they found something working for them and now it is being used in "we
> > > > cannot break existing systems". Basically, AOSP unilaterally created a
> > > > stable ABI and now kernel has to stick to it.
> > > >
> > > > Really, all normal systems depend on aliases or names and here we have
> > > > dependency on device address. I proposed way how AOSP should be fixed.
> > > > Anything happened? Nope.
> > >
> > > First time he sent a possible solution for the problem:
> > > https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> > >
> > > To sum up you could have used one of the more portable approaches
> > > 1. add an udc alias to the controller and use it then to refer to the
> > > corresponding USB controller
> >
> > Is there such a thing as "UDC alias"? Or are you suggesting that we
> > should add such feature?
> >
> > I think it would be wonderful if we could identify the UDCs on our
> > boards as "USB1" and "USB2", or "the one and only USB-C connector". But
> > unless that will fall back to the existing naming it would break John's
> > _existing_ userspace.
> 

> Well, I'd not hold up the existing userspace I'm using as sacrosanct
> (AOSP devices still usually don't have to work cross-kernel versions -
> devboards being the main exception). I'm fine if we can rework
> userland as proposed, so that the issues can be avoided, but I
> honestly don't have enough context to really understand what that
> looks like (no idea what udc aliases are).
> 
> And whatever we do, the main constraint is that userland has to be
> able to work with existing LTS kernels and newer kernels.

As I said in my response to Bjorn even if it is added to the kernel it
won't get to the official LTSes as it would be a new kernel feature.
New features aren't normally backported to the older kernels.

> 
> > > 2. search through DT-nodes looking for a specific compatible/reg
> > > DT-properties.
> > >
> >
> > We could define that this is the way, but again we didn't yesterday so
> > your proposal is not backwards compatible.
> 

> It may be backwards compatible, but I'm still not clear exactly how it
> would work.
> 
> I guess if we look through all
> sys/bus/platform/devices/*/of_node/compatbile strings for the desired
> "snps,dwc3", then find which of the directories have the desired
> address in the string? (The suggestion for looking at reg seems
> better, but I don't get a char value out of the dwc3 of_node/reg
> file).

The algorithm is simple:
1) You know what USB controllers you have on your platform. They are
supposed to be compatible with snps,dwc3 string and have some pre-defined
base address.
2) Find all the files in the directory /sys/class/udc/.
3) Walk through all the directories in /sys/bus/platform/devices/ with
names found in 2) and stop on the device with matching compatible/base
address defined in 1).

In my case the strings could be retrieved like this:
USB_NAME_COMPAT=$(/sys/bus/platform/devices/1f100000.usb/of_node/compatible | tr '\0' '\t' | cut -f1)
USB_DEVICE_ADDR="$(head -c 4 /sys/bus/platform/devices/1f100000.usb/of_node/reg | hexdump -ve '/1 "%02x"' | sed -e 's/^0*//g')"

Regards,
-Sergey

> 
> It seems much more straightforward to do an `ls /sys/class/udc/$GADGET_ADDR.*`
> 
> But again, if folks decide the names can be rearranged to usb.<addr>
> in the future, that would break too.
> 
> thanks
> -john
Greg KH July 23, 2021, 8:17 a.m. UTC | #12
On Fri, Jul 23, 2021 at 12:54:51AM +0300, Serge Semin wrote:
> I always thought that ABI is supposed to be something what is
> thoroughly documented and firmly declared to be so. It isn't something
> claimed to be on a random nature but defined to be one when it's
> more-or-less standardized. Thus the Linux kernel developers decide not
> to change something unless it went through the series of iterations like
> testing, stable, obsolete, remove. As I see it the rule-of-thumb is
> supposed to be as "nothing is ABI unless it's declared as such".

Not true at all.  Again, if something works in an older kernel version,
and you upgrade to a new kernel version and it breaks, that is a
regression and must be fixed/reverted.

Lack of documentation does not mean an ABI can be changed.

greg k-h
Greg KH July 24, 2021, 7:50 a.m. UTC | #13
On Fri, Jul 23, 2021 at 02:54:54PM -0500, Bjorn Andersson wrote:
> On Fri 23 Jul 10:54 CDT 2021, Greg Kroah-Hartman wrote:
> 
> > On Fri, Jul 23, 2021 at 09:34:20AM -0500, Bjorn Andersson wrote:
> > > On Fri 23 Jul 03:18 CDT 2021, Greg Kroah-Hartman wrote:
> > > 
> > > > On Wed, Jul 21, 2021 at 03:09:37PM -0500, Bjorn Andersson wrote:
> > > > > Which tree did you revert this in? 5.13.stable?)
> > > > 
> > > > My usb-linus branch which will go to Linus later today.  Then we can
> > > > backport the revert to older kernels as needed.
> > > > 
> > > 
> > > I'm not worried about the backports, I'm worried about conflicts you're
> > > causing because you're taking a non-usb patch through the usb tree.
> > > 
> > > I was about to push a revert (to this and the other Qualcomm platforms),
> > > but as you're taking some set of reverts through the usb tree we're just
> > > in for a bunch of merge conflicts.
> > 
> > It shouldn't be a merge conflict as you can apply the same revert to
> > your tree now and keep on merging.  When you pick up 5.14-rc3 from Linus
> > it should merge "correctly", right?
> > 
> 
> I typically don't merge back the -rcs into my -next branch, is that
> common practice?

I do it when Linus takes patches from my -linus branch in order to
ensure they end up in my -next branch for testing and merge issues.

> But I still don't understand why you insist on driving this through your
> tree. I've asked you several times to show me on the patch so I at least
> can Ack it. I made a mistake, but why do you insist on keeping me - the
> maintainer - out of the loop?

I had already done the revert, I wasn't trying to keep anyone out of the
loop here, sorry if it came across that way.  I just wanted to ensure
this got resolved quickly so I could move on to other issues.

This is now 1f958f3dff42 ("Revert "arm64: dts: qcom: Harmonize DWC USB3
DT nodes name"") in Linus's tree if you wish to cherry-pick it into your
tree to resolve merge issues, sorry for the confusion.

thanks,

greg k-h
John Stultz Aug. 14, 2021, 1:06 a.m. UTC | #14
On Thu, Jul 22, 2021 at 3:09 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Thu, Jul 22, 2021 at 01:09:05PM -0700, John Stultz wrote:
> > On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > > > On Jul 21, 2021, 1:45 PM +0200, Krzysztof Kozlowski wrote:
> > > > > I had impression that kernel defines interfaces which should be used and
> > > > > are stable (e.g. syscalls, sysfs and so on). This case is example of
> > > > > user-space relying on something not being marked as part of ABI. Instead
> > > > > they found something working for them and now it is being used in "we
> > > > > cannot break existing systems". Basically, AOSP unilaterally created a
> > > > > stable ABI and now kernel has to stick to it.
> > > > >
> > > > > Really, all normal systems depend on aliases or names and here we have
> > > > > dependency on device address. I proposed way how AOSP should be fixed.
> > > > > Anything happened? Nope.
> > > >
> > > > First time he sent a possible solution for the problem:
> > > > https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> > > >
> > > > To sum up you could have used one of the more portable approaches
> > > > 1. add an udc alias to the controller and use it then to refer to the
> > > > corresponding USB controller
> > >
> > > Is there such a thing as "UDC alias"? Or are you suggesting that we
> > > should add such feature?
> > >
> > > I think it would be wonderful if we could identify the UDCs on our
> > > boards as "USB1" and "USB2", or "the one and only USB-C connector". But
> > > unless that will fall back to the existing naming it would break John's
> > > _existing_ userspace.
> >
>
> > Well, I'd not hold up the existing userspace I'm using as sacrosanct
> > (AOSP devices still usually don't have to work cross-kernel versions -
> > devboards being the main exception). I'm fine if we can rework
> > userland as proposed, so that the issues can be avoided, but I
> > honestly don't have enough context to really understand what that
> > looks like (no idea what udc aliases are).
> >
> > And whatever we do, the main constraint is that userland has to be
> > able to work with existing LTS kernels and newer kernels.
>
> As I said in my response to Bjorn even if it is added to the kernel it
> won't get to the official LTSes as it would be a new kernel feature.
> New features aren't normally backported to the older kernels.
>
> >
> > > > 2. search through DT-nodes looking for a specific compatible/reg
> > > > DT-properties.
> > > >
> > >
> > > We could define that this is the way, but again we didn't yesterday so
> > > your proposal is not backwards compatible.
> >
>
> > It may be backwards compatible, but I'm still not clear exactly how it
> > would work.
> >
> > I guess if we look through all
> > sys/bus/platform/devices/*/of_node/compatbile strings for the desired
> > "snps,dwc3", then find which of the directories have the desired
> > address in the string? (The suggestion for looking at reg seems
> > better, but I don't get a char value out of the dwc3 of_node/reg
> > file).
>
> The algorithm is simple:
> 1) You know what USB controllers you have on your platform. They are
> supposed to be compatible with snps,dwc3 string and have some pre-defined
> base address.
> 2) Find all the files in the directory /sys/class/udc/.
> 3) Walk through all the directories in /sys/bus/platform/devices/ with
> names found in 2) and stop on the device with matching compatible/base
> address defined in 1).
>
> In my case the strings could be retrieved like this:
> USB_NAME_COMPAT=$(/sys/bus/platform/devices/1f100000.usb/of_node/compatible | tr '\0' '\t' | cut -f1)
> USB_DEVICE_ADDR="$(head -c 4 /sys/bus/platform/devices/1f100000.usb/of_node/reg | hexdump -ve '/1 "%02x"' | sed -e 's/^0*//g')"


Hey Serge,
   I just wanted to follow up here.  Amit has reworked the db845c AOSP
userland so that it no longer uses the fixed node name, but instead
probes for it:
  https://android-review.googlesource.com/c/device/linaro/dragonboard/+/1774872

Admittedly, it does take a short-cut.  As your algorithm above,
digging up the devices and finding the sys/bus path to read the reg
value and pipe through hexdump (which android doesn't have) seemed
overly obtuse when the address is in the node name itself (while the
only way to be sure, one normally doesn't use spectroscopy to
determine the value of a coin when you can read what's printed on it
:).  But, should the node naming be further changed at least the
infrastructure we have can be reworked fairly easily to adapt now.

In any case, as we can handle the name change now, if you want to
resubmit your patch, we would no longer object (but can't promise no
one else might be bitten).  Sorry for the delay this caused, and we
appreciate you working with us to find a solution.

thanks
-john
Bjorn Andersson Aug. 18, 2021, 3:44 a.m. UTC | #15
On Sun 15 Aug 14:46 CDT 2021, Serge Semin wrote:

> Hello John
> 
> On Fri, Aug 13, 2021 at 06:06:24PM -0700, John Stultz wrote:
> > On Thu, Jul 22, 2021 at 3:09 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Thu, Jul 22, 2021 at 01:09:05PM -0700, John Stultz wrote:
> > > > On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > > > > On Jul 21, 2021, 1:45 PM +0200, Krzysztof Kozlowski wrote:
> > > > > > > I had impression that kernel defines interfaces which should be used and
> > > > > > > are stable (e.g. syscalls, sysfs and so on). This case is example of
> > > > > > > user-space relying on something not being marked as part of ABI. Instead
> > > > > > > they found something working for them and now it is being used in "we
> > > > > > > cannot break existing systems". Basically, AOSP unilaterally created a
> > > > > > > stable ABI and now kernel has to stick to it.
> > > > > > >
> > > > > > > Really, all normal systems depend on aliases or names and here we have
> > > > > > > dependency on device address. I proposed way how AOSP should be fixed.
> > > > > > > Anything happened? Nope.
> > > > > >
> > > > > > First time he sent a possible solution for the problem:
> > > > > > https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/
> > > > > >
> > > > > > To sum up you could have used one of the more portable approaches
> > > > > > 1. add an udc alias to the controller and use it then to refer to the
> > > > > > corresponding USB controller
> > > > >
> > > > > Is there such a thing as "UDC alias"? Or are you suggesting that we
> > > > > should add such feature?
> > > > >
> > > > > I think it would be wonderful if we could identify the UDCs on our
> > > > > boards as "USB1" and "USB2", or "the one and only USB-C connector". But
> > > > > unless that will fall back to the existing naming it would break John's
> > > > > _existing_ userspace.
> > > >
> > >
> > > > Well, I'd not hold up the existing userspace I'm using as sacrosanct
> > > > (AOSP devices still usually don't have to work cross-kernel versions -
> > > > devboards being the main exception). I'm fine if we can rework
> > > > userland as proposed, so that the issues can be avoided, but I
> > > > honestly don't have enough context to really understand what that
> > > > looks like (no idea what udc aliases are).
> > > >
> > > > And whatever we do, the main constraint is that userland has to be
> > > > able to work with existing LTS kernels and newer kernels.
> > >
> > > As I said in my response to Bjorn even if it is added to the kernel it
> > > won't get to the official LTSes as it would be a new kernel feature.
> > > New features aren't normally backported to the older kernels.
> > >
> > > >
> > > > > > 2. search through DT-nodes looking for a specific compatible/reg
> > > > > > DT-properties.
> > > > > >
> > > > >
> > > > > We could define that this is the way, but again we didn't yesterday so
> > > > > your proposal is not backwards compatible.
> > > >
> > >
> > > > It may be backwards compatible, but I'm still not clear exactly how it
> > > > would work.
> > > >
> > > > I guess if we look through all
> > > > sys/bus/platform/devices/*/of_node/compatbile strings for the desired
> > > > "snps,dwc3", then find which of the directories have the desired
> > > > address in the string? (The suggestion for looking at reg seems
> > > > better, but I don't get a char value out of the dwc3 of_node/reg
> > > > file).
> > >
> > > The algorithm is simple:
> > > 1) You know what USB controllers you have on your platform. They are
> > > supposed to be compatible with snps,dwc3 string and have some pre-defined
> > > base address.
> > > 2) Find all the files in the directory /sys/class/udc/.
> > > 3) Walk through all the directories in /sys/bus/platform/devices/ with
> > > names found in 2) and stop on the device with matching compatible/base
> > > address defined in 1).
> > >
> > > In my case the strings could be retrieved like this:
> > > USB_NAME_COMPAT=$(/sys/bus/platform/devices/1f100000.usb/of_node/compatible | tr '\0' '\t' | cut -f1)
> > > USB_DEVICE_ADDR="$(head -c 4 /sys/bus/platform/devices/1f100000.usb/of_node/reg | hexdump -ve '/1 "%02x"' | sed -e 's/^0*//g')"
> > 
> > 
> 
> > Hey Serge,
> >    I just wanted to follow up here.  Amit has reworked the db845c AOSP
> > userland so that it no longer uses the fixed node name, but instead
> > probes for it:
> >   https://android-review.googlesource.com/c/device/linaro/dragonboard/+/1774872
> > 
> > Admittedly, it does take a short-cut.  As your algorithm above,
> > digging up the devices and finding the sys/bus path to read the reg
> > value and pipe through hexdump (which android doesn't have) seemed
> > overly obtuse when the address is in the node name itself (while the
> > only way to be sure, one normally doesn't use spectroscopy to
> > determine the value of a coin when you can read what's printed on it
> > :).  But, should the node naming be further changed at least the
> > infrastructure we have can be reworked fairly easily to adapt now.
> > 
> > In any case, as we can handle the name change now, if you want to
> > resubmit your patch, we would no longer object (but can't promise no
> > one else might be bitten).  Sorry for the delay this caused, and we
> > appreciate you working with us to find a solution.
> 
> Great! Thanks for sending the notification. I'll resend the patch with a
> reference to your report and to the update made to AOSP, as soon as I
> am done with my current task.
> 

Amit's change makes future versions of AOSP able to cope with changes in
the UDC name, unfortunately it doesn't change the fact that renaming the
node breaks compatibility in non-Android user space (or any existing
branches/tags of AOSP).

Regards,
Bjorn
Serge Semin Aug. 19, 2021, 11:03 a.m. UTC | #16
On Tue, Aug 17, 2021 at 10:44:23PM -0500, Bjorn Andersson wrote:
> On Sun 15 Aug 14:46 CDT 2021, Serge Semin wrote:

> 

> > Hello John

> > 

> > On Fri, Aug 13, 2021 at 06:06:24PM -0700, John Stultz wrote:

> > > On Thu, Jul 22, 2021 at 3:09 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> > > > On Thu, Jul 22, 2021 at 01:09:05PM -0700, John Stultz wrote:

> > > > > On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson

> > > > > <bjorn.andersson@linaro.org> wrote:

> > > > > > > On Jul 21, 2021, 1:45 PM +0200, Krzysztof Kozlowski wrote:

> > > > > > > > I had impression that kernel defines interfaces which should be used and

> > > > > > > > are stable (e.g. syscalls, sysfs and so on). This case is example of

> > > > > > > > user-space relying on something not being marked as part of ABI. Instead

> > > > > > > > they found something working for them and now it is being used in "we

> > > > > > > > cannot break existing systems". Basically, AOSP unilaterally created a

> > > > > > > > stable ABI and now kernel has to stick to it.

> > > > > > > >

> > > > > > > > Really, all normal systems depend on aliases or names and here we have

> > > > > > > > dependency on device address. I proposed way how AOSP should be fixed.

> > > > > > > > Anything happened? Nope.

> > > > > > >

> > > > > > > First time he sent a possible solution for the problem:

> > > > > > > https://lore.kernel.org/lkml/20201221210423.GA2504@kozik-lap/

> > > > > > >

> > > > > > > To sum up you could have used one of the more portable approaches

> > > > > > > 1. add an udc alias to the controller and use it then to refer to the

> > > > > > > corresponding USB controller

> > > > > >

> > > > > > Is there such a thing as "UDC alias"? Or are you suggesting that we

> > > > > > should add such feature?

> > > > > >

> > > > > > I think it would be wonderful if we could identify the UDCs on our

> > > > > > boards as "USB1" and "USB2", or "the one and only USB-C connector". But

> > > > > > unless that will fall back to the existing naming it would break John's

> > > > > > _existing_ userspace.

> > > > >

> > > >

> > > > > Well, I'd not hold up the existing userspace I'm using as sacrosanct

> > > > > (AOSP devices still usually don't have to work cross-kernel versions -

> > > > > devboards being the main exception). I'm fine if we can rework

> > > > > userland as proposed, so that the issues can be avoided, but I

> > > > > honestly don't have enough context to really understand what that

> > > > > looks like (no idea what udc aliases are).

> > > > >

> > > > > And whatever we do, the main constraint is that userland has to be

> > > > > able to work with existing LTS kernels and newer kernels.

> > > >

> > > > As I said in my response to Bjorn even if it is added to the kernel it

> > > > won't get to the official LTSes as it would be a new kernel feature.

> > > > New features aren't normally backported to the older kernels.

> > > >

> > > > >

> > > > > > > 2. search through DT-nodes looking for a specific compatible/reg

> > > > > > > DT-properties.

> > > > > > >

> > > > > >

> > > > > > We could define that this is the way, but again we didn't yesterday so

> > > > > > your proposal is not backwards compatible.

> > > > >

> > > >

> > > > > It may be backwards compatible, but I'm still not clear exactly how it

> > > > > would work.

> > > > >

> > > > > I guess if we look through all

> > > > > sys/bus/platform/devices/*/of_node/compatbile strings for the desired

> > > > > "snps,dwc3", then find which of the directories have the desired

> > > > > address in the string? (The suggestion for looking at reg seems

> > > > > better, but I don't get a char value out of the dwc3 of_node/reg

> > > > > file).

> > > >

> > > > The algorithm is simple:

> > > > 1) You know what USB controllers you have on your platform. They are

> > > > supposed to be compatible with snps,dwc3 string and have some pre-defined

> > > > base address.

> > > > 2) Find all the files in the directory /sys/class/udc/.

> > > > 3) Walk through all the directories in /sys/bus/platform/devices/ with

> > > > names found in 2) and stop on the device with matching compatible/base

> > > > address defined in 1).

> > > >

> > > > In my case the strings could be retrieved like this:

> > > > USB_NAME_COMPAT=$(/sys/bus/platform/devices/1f100000.usb/of_node/compatible | tr '\0' '\t' | cut -f1)

> > > > USB_DEVICE_ADDR="$(head -c 4 /sys/bus/platform/devices/1f100000.usb/of_node/reg | hexdump -ve '/1 "%02x"' | sed -e 's/^0*//g')"

> > > 

> > > 

> > 

> > > Hey Serge,

> > >    I just wanted to follow up here.  Amit has reworked the db845c AOSP

> > > userland so that it no longer uses the fixed node name, but instead

> > > probes for it:

> > >   https://android-review.googlesource.com/c/device/linaro/dragonboard/+/1774872

> > > 

> > > Admittedly, it does take a short-cut.  As your algorithm above,

> > > digging up the devices and finding the sys/bus path to read the reg

> > > value and pipe through hexdump (which android doesn't have) seemed

> > > overly obtuse when the address is in the node name itself (while the

> > > only way to be sure, one normally doesn't use spectroscopy to

> > > determine the value of a coin when you can read what's printed on it

> > > :).  But, should the node naming be further changed at least the

> > > infrastructure we have can be reworked fairly easily to adapt now.

> > > 

> > > In any case, as we can handle the name change now, if you want to

> > > resubmit your patch, we would no longer object (but can't promise no

> > > one else might be bitten).  Sorry for the delay this caused, and we

> > > appreciate you working with us to find a solution.

> > 

> > Great! Thanks for sending the notification. I'll resend the patch with a

> > reference to your report and to the update made to AOSP, as soon as I

> > am done with my current task.

> > 

> 


> Amit's change makes future versions of AOSP able to cope with changes in

> the UDC name, unfortunately it doesn't change the fact that renaming the

> node breaks compatibility in non-Android user space (or any existing

> branches/tags of AOSP).


After looking over the whole discussion, as I see it there is no
compatibility breakage in this case. But there is an improper UDC
interface usage, which has been fixed by Amit and could be ported to
another AOSP branches/tags if needed. As [1] says user-space is
able to create a USB gadget only with controllers listed in the
directory /sys/class/udc/*. That ABI doesn't change. Note the ABI
doesn't say that if someone found a file there in some kernel
version it will be available there in the in a next version with the
same name. A developer just need to search for the UDC controllers ach
time when a UDC gadget needs to be created.

Anyway as I see it the UDC gadget creation ABI IS indeed abided by
most of the developers since we haven't heard much complains so far
except from John. In case of AOSP the problem has been fixed so we can
get back the modification and proceed with the rest of the patches
review.

[1] Documentation/usb/gadget_configfs.rst

-Sergey

> 

> Regards,

> Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index defcbd15edf9..34e97da98270 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -1064,7 +1064,7 @@  &usb2 {
 	status = "okay";
 	extcon = <&usb2_id>;
 
-	dwc3@7600000 {
+	usb@7600000 {
 		extcon = <&usb2_id>;
 		dr_mode = "otg";
 		maximum-speed = "high-speed";
@@ -1075,7 +1075,7 @@  &usb3 {
 	status = "okay";
 	extcon = <&usb3_id>;
 
-	dwc3@6a00000 {
+	usb@6a00000 {
 		extcon = <&usb3_id>;
 		dr_mode = "otg";
 	};
diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 96a5ec89b5f0..1129062a4ca1 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -427,7 +427,7 @@  usb_0: usb@8af8800 {
 			resets = <&gcc GCC_USB0_BCR>;
 			status = "disabled";
 
-			dwc_0: dwc3@8a00000 {
+			dwc_0: usb@8a00000 {
 				compatible = "snps,dwc3";
 				reg = <0x8a00000 0xcd00>;
 				interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
@@ -468,7 +468,7 @@  usb_1: usb@8cf8800 {
 			resets = <&gcc GCC_USB1_BCR>;
 			status = "disabled";
 
-			dwc_1: dwc3@8c00000 {
+			dwc_1: usb@8c00000 {
 				compatible = "snps,dwc3";
 				reg = <0x8c00000 0xcd00>;
 				interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 9951286db775..66b6d2f0a093 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -1767,7 +1767,7 @@  usb3: usb@6af8800 {
 			power-domains = <&gcc USB30_GDSC>;
 			status = "disabled";
 
-			dwc3@6a00000 {
+			usb@6a00000 {
 				compatible = "snps,dwc3";
 				reg = <0x06a00000 0xcc00>;
 				interrupts = <0 131 IRQ_TYPE_LEVEL_HIGH>;
@@ -1978,7 +1978,7 @@  usb2: usb@76f8800 {
 			power-domains = <&gcc USB30_GDSC>;
 			status = "disabled";
 
-			dwc3@7600000 {
+			usb@7600000 {
 				compatible = "snps,dwc3";
 				reg = <0x07600000 0xcc00>;
 				interrupts = <0 138 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index c45870600909..7cc7897e7b83 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -1678,7 +1678,7 @@  usb3: usb@a8f8800 {
 
 			resets = <&gcc GCC_USB_30_BCR>;
 
-			usb3_dwc3: dwc3@a800000 {
+			usb3_dwc3: usb@a800000 {
 				compatible = "snps,dwc3";
 				reg = <0x0a800000 0xcd00>;
 				interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
index 6422cf9d5855..88d7b7a53743 100644
--- a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
@@ -337,7 +337,7 @@  &usb2_phy_sec {
 &usb3 {
 	status = "okay";
 
-	dwc3@7580000 {
+	usb@7580000 {
 		dr_mode = "host";
 	};
 };
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index b654b802e95c..f6ef17553064 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -544,7 +544,7 @@  usb3: usb@7678800 {
 			assigned-clock-rates = <19200000>, <200000000>;
 			status = "disabled";
 
-			dwc3@7580000 {
+			usb@7580000 {
 				compatible = "snps,dwc3";
 				reg = <0x07580000 0xcd00>;
 				interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
@@ -573,7 +573,7 @@  usb2: usb@79b8800 {
 			assigned-clock-rates = <19200000>, <133333333>;
 			status = "disabled";
 
-			dwc3@78c0000 {
+			usb@78c0000 {
 				compatible = "snps,dwc3";
 				reg = <0x078c0000 0xcc00>;
 				interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index d46b3833e52f..bbc9a2b5c570 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -2673,7 +2673,7 @@  usb_1: usb@a6f8800 {
 					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3>;
 			interconnect-names = "usb-ddr", "apps-usb";
 
-			usb_1_dwc3: dwc3@a600000 {
+			usb_1_dwc3: usb@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xe000>;
 				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 2884577dcb77..ca20e4e91f61 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3573,7 +3573,7 @@  usb_1: usb@a6f8800 {
 					<&gladiator_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3_0>;
 			interconnect-names = "usb-ddr", "apps-usb";
 
-			usb_1_dwc3: dwc3@a600000 {
+			usb_1_dwc3: usb@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xcd00>;
 				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
@@ -3621,7 +3621,7 @@  usb_2: usb@a8f8800 {
 					<&gladiator_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3_1>;
 			interconnect-names = "usb-ddr", "apps-usb";
 
-			usb_2_dwc3: dwc3@a800000 {
+			usb_2_dwc3: usb@a800000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a800000 0 0xcd00>;
 				interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index b86a7ead3006..167d14dda974 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -846,7 +846,7 @@  usb_1: usb@a6f8800 {
 
 			resets = <&gcc GCC_USB30_PRIM_BCR>;
 
-			usb_1_dwc3: dwc3@a600000 {
+			usb_1_dwc3: usb@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xcd00>;
 				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;