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

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 <[e-mail]>
---
 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

Krzysztof Kozlowski Oct. 20, 2020, 12:44 p.m. UTC | #1

On Tue, Oct 20, 2020 at 02:59:59PM +0300, Serge Semin 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 <[e-mail]>
> ---
>  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(-)
> 

Acked-by: Krzysztof Kozlowski <[e-mail]>

Best regards,
Krzysztof

Jun Li Nov. 2, 2020, 7:34 a.m. UTC | #2

Serge Semin <[e-mail]> 于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

Bjorn Andersson Nov. 3, 2020, 11:23 p.m. UTC | #3

On Mon 02 Nov 01:34 CST 2020, Jun Li wrote:

> Serge Semin <[e-mail]> ???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");
> 

Thanks for catching this Jun. The code certainly needs to be updated to
look for the new child node, while falling back to the old name, before
I can merge this change.

Regards,
Bjorn

Serge Semin Nov. 10, 2020, 12:12 p.m. UTC | #4

Hello Jun and Bjorn.

On Tue, Nov 03, 2020 at 05:23:47PM -0600, Bjorn Andersson wrote:
> On Mon 02 Nov 01:34 CST 2020, Jun Li wrote:
> 
> > Serge Semin <[e-mail]> ???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");
> > 
> 
> Thanks for catching this Jun. The code certainly needs to be updated to
> look for the new child node, while falling back to the old name, before
> I can merge this change.

Thanks for looking into this. I'll add a patch, which fixes that into
the next series, but with no tested status guarantee, since I haven't
got a corresponding hardware.

-Sergey

> 
> Regards,
> Bjorn

John Stultz July 14, 2021, 12:07 a.m. UTC | #5

On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
<[e-mail]> 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 <[e-mail]>

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/[e-mail]/

(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 | #6

On Tue 13 Jul 19:07 CDT 2021, John Stultz wrote:

> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> <[e-mail]> 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 <[e-mail]>
> 
> 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/[e-mail]/
> 
> (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 | #7

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
> <[e-mail]> 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 <[e-mail]>
> 

> 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/[e-mail]/
> 
> (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 | #8

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 | #9

On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:
> On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
> <[e-mail]> 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 <[e-mail]>
> 
> 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/[e-mail]/
> 
> (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:38 a.m. UTC | #10

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
> > <[e-mail]> 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 <[e-mail]>
> > 
> 
> > 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.

thanks,

greg k-h

Greg KH July 21, 2021, 7:39 a.m. UTC | #11

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
> > <[e-mail]> 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 <[e-mail]>
> > 
> > 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

Serge Semin July 21, 2021, 10:02 a.m. UTC | #12

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
> > > <[e-mail]> 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 <[e-mail]>
> > > 
> > 
> > > 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 <[e-mail]> 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 <[e-mail]> 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.

Regards,
-Sergey

> 
> thanks,
> 
> greg k-h

Greg KH July 21, 2021, 10:29 a.m. UTC | #13

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
> > > > <[e-mail]> 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 <[e-mail]>
> > > > 
> > > 
> > > > 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 <[e-mail]> 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 <[e-mail]> 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.

thanks,

greg k-h

Krzysztof Kozlowski July 21, 2021, 10:45 a.m. UTC | #14

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
>>>>> <[e-mail]> 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 <[e-mail]>
>>>>>
>>>>
>>>>> 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 <[e-mail]> 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 <[e-mail]> 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.

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.

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?

Best regards,
Krzysztof

Greg KH July 21, 2021, 11:02 a.m. UTC | #15

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
> >>>>> <[e-mail]> 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 <[e-mail]>
> >>>>>
> >>>>
> >>>>> 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 <[e-mail]> 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 <[e-mail]> 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

Krzysztof Kozlowski July 21, 2021, 11:10 a.m. UTC | #16

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
>>>>>>> <[e-mail]> 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 <[e-mail]>
>>>>>>>
>>>>>>
>>>>>>> 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 <[e-mail]> 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 <[e-mail]> 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. :)

Best regards,
Krzysztof

Serge Semin July 21, 2021, 11:25 a.m. UTC | #17

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
> >>>>>>> <[e-mail]> 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 <[e-mail]>
> >>>>>>>
> >>>>>>
> >>>>>>> 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 <[e-mail]> 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 <[e-mail]> 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/[e-mail]/

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 21, 2021, 6:08 p.m. UTC | #18

On Wed, Jul 21, 2021 at 4:25 AM Serge Semin <[e-mail]> wrote:
> On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:
> > 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/[e-mail]/
>
> 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

The problem with this is there may be multiple USB controllers on a
system (not all exposed outside the case - and also the dummy
controller is often present). How can we configure the system to know
which controller is which?

The only name we have for distinguishing the controllers is the DTS
node. So it seems inherent that changes to that name will break the
config.

That said, this issue reminded me of the /dev/hda -> /dev/sda device
name or the eth0 -> enp3s0 switch which both also had the potential to
break configurations or scripts.  I get that having a standard naming
scheme is important (I'm very sympathetic to this point)! I can
imagine UI trying to show possible controllers for a user to select
needs a simple way to determine if a device is a usb controller - but
again this just shows that the node names are an ABI.

So I'm not the one to judge if this change is useful enough to push
through the pain, but it did seem to be done a bit casually.

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

DTS changes are a constant source of regressions in my experience. We
mostly just have to roll with it, but it feels never ending. :)  I'd
personally rather folks in general be more thoughtful about what DTS
changes they make and accept, understanding that they do have impact
on userland.  And I'd imagine If updates to linux-firmware broke the
most recent LTS kernel, that would be seen as a regression too, and
folks wouldn't be told to just keep the old firmware.  But all the
same, I'd also be happy for suggestions to remove any such
dependencies userland has on specific dts naming, where possible, to
make the constant pain go away. :)

thanks
-john

Bjorn Andersson July 21, 2021, 8:09 p.m. UTC | #19

On Wed 21 Jul 05:29 CDT 2021, 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
> > > > > <[e-mail]> 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 <[e-mail]>
> > > > > 
> > > > 
> > > > > 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 <[e-mail]> 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 <[e-mail]> 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.
> 

Which tree did you revert this in? 5.13.stable?)

I'm onboard with us reverting this, but for any 5.14-rc and 5.15 this
will conflict badly with the qcom tree, so I much rather take the revert
in my tree - than have Linus run into this mess down the road.

For stable, I don't mind if you merge something...Perhaps you can point
me to your revert and I can pick it up in my tree?

Regards,
Bjorn

Serge Semin July 22, 2021, 6:12 p.m. UTC | #20

On Wed, Jul 21, 2021 at 11:08:08AM -0700, John Stultz wrote:
> On Wed, Jul 21, 2021 at 4:25 AM Serge Semin <[e-mail]> wrote:
> > On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:
> > > 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/[e-mail]/
> >
> > 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
> 

> The problem with this is there may be multiple USB controllers on a
> system (not all exposed outside the case - and also the dummy
> controller is often present). How can we configure the system to know
> which controller is which?

How did you distinguish them before this change? It has nothing really
related with the patch in topic.

> 
> The only name we have for distinguishing the controllers is the DTS
> node. So it seems inherent that changes to that name will break the
> config.

No, it's not the only way you have. I say it was the easiest way for
you to find a corresponding device. The DT-node name never was a part
of ABI for at least up until we finally get the DT-node names
consistent with DT spec. Only then it would be possible to consider
them as such. One more time I'll quote what @Krzisztof has already
told you twice:

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
2. search through DT-nodes looking for a specific compatible/reg
DT-properties.

Of course it was easier to revert the patch. But if we always chose
such approach, the kernel would have been filled with tons of
workarounds and legacy parts without possibility to move forward to
having more unified interfaces.

> 
> That said, this issue reminded me of the /dev/hda -> /dev/sda device
> name or the eth0 -> enp3s0 switch which both also had the potential to
> break configurations or scripts.  I get that having a standard naming
> scheme is important (I'm very sympathetic to this point)! I can
> imagine UI trying to show possible controllers for a user to select
> needs a simple way to determine if a device is a usb controller - but
> again this just shows that the node names are an ABI.
> 
> So I'm not the one to judge if this change is useful enough to push
> through the pain, but it did seem to be done a bit casually.
> 
> > 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.
> 
> DTS changes are a constant source of regressions in my experience. We
> mostly just have to roll with it, but it feels never ending. :)  I'd
> personally rather folks in general be more thoughtful about what DTS
> changes they make and accept, understanding that they do have impact
> on userland.  And I'd imagine If updates to linux-firmware broke the
> most recent LTS kernel, that would be seen as a regression too, and
> folks wouldn't be told to just keep the old firmware.

> But all the
> same, I'd also be happy for suggestions to remove any such
> dependencies userland has on specific dts naming, where possible, to
> make the constant pain go away. :)

Well, @Krzisztof has already given you such suggestions regarding this
issue not once. But you tend to ignore them.

Anyway this patch doesn't break LTS kernel and doesn't break the
linux-firmware either. It changes DT-node name, which happens to
change the device name, which isn't guaranteed to be stable as it's
not part of the kernel ABI. If you think otherwise please provide some
proves to that. I didn't find any in the official Linux ABI docs.

-Sergey

> 
> thanks
> -john

Bjorn Andersson July 22, 2021, 7:17 p.m. UTC | #21

On Thu 22 Jul 13:12 CDT 2021, Serge Semin wrote:

> On Wed, Jul 21, 2021 at 11:08:08AM -0700, John Stultz wrote:
> > On Wed, Jul 21, 2021 at 4:25 AM Serge Semin <[e-mail]> wrote:
> > > On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:
> > > > 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/[e-mail]/
> > >
> > > 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
> > 
> 
> > The problem with this is there may be multiple USB controllers on a
> > system (not all exposed outside the case - and also the dummy
> > controller is often present). How can we configure the system to know
> > which controller is which?
> 
> How did you distinguish them before this change? It has nothing really
> related with the patch in topic.
> 
> > 
> > The only name we have for distinguishing the controllers is the DTS
> > node. So it seems inherent that changes to that name will break the
> > config.
> 
> No, it's not the only way you have. I say it was the easiest way for
> you to find a corresponding device. The DT-node name never was a part
> of ABI for at least up until we finally get the DT-node names
> consistent with DT spec. Only then it would be possible to consider
> them as such. One more time I'll quote what @Krzisztof has already
> told you twice:
> 

The requirement to keep a stable userspace ABI does not consider the
fact that the kernel made up part of that ABI based on things it found
in e.g. DT.

The name of the UDC devices has been and is simply the
dev_name(dwc3-device) and this is directly based on the DT node.

So changing DT indirectly changed the user space ABI and anyone who is
using USB Gadget Configfs is directly affected by this.

Regards,
Bjorn

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

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

> Of course it was easier to revert the patch. But if we always chose
> such approach, the kernel would have been filled with tons of
> workarounds and legacy parts without possibility to move forward to
> having more unified interfaces.
> 

Yes, and that's exactly where we are heading with the ongoing DT
validation work. But "don't break your users" isn't excused because the
ABI is derived from some third party.

> > 
> > That said, this issue reminded me of the /dev/hda -> /dev/sda device
> > name or the eth0 -> enp3s0 switch which both also had the potential to
> > break configurations or scripts.  I get that having a standard naming
> > scheme is important (I'm very sympathetic to this point)! I can
> > imagine UI trying to show possible controllers for a user to select
> > needs a simple way to determine if a device is a usb controller - but
> > again this just shows that the node names are an ABI.
> > 
> > So I'm not the one to judge if this change is useful enough to push
> > through the pain, but it did seem to be done a bit casually.
> > 
> > > 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.
> > 
> > DTS changes are a constant source of regressions in my experience. We
> > mostly just have to roll with it, but it feels never ending. :)  I'd
> > personally rather folks in general be more thoughtful about what DTS
> > changes they make and accept, understanding that they do have impact
> > on userland.  And I'd imagine If updates to linux-firmware broke the
> > most recent LTS kernel, that would be seen as a regression too, and
> > folks wouldn't be told to just keep the old firmware.
> 
> > But all the
> > same, I'd also be happy for suggestions to remove any such
> > dependencies userland has on specific dts naming, where possible, to
> > make the constant pain go away. :)
> 
> Well, @Krzisztof has already given you such suggestions regarding this
> issue not once. But you tend to ignore them.
> 

Those are good suggestions, but they require changes in userspace.

> Anyway this patch doesn't break LTS kernel and doesn't break the
> linux-firmware either. It changes DT-node name, which happens to
> change the device name, which isn't guaranteed to be stable as it's
> not part of the kernel ABI. If you think otherwise please provide some
> proves to that. I didn't find any in the official Linux ABI docs.
> 

I think it's a gray area, at least I do want it to be a gray area,
because I don't want device names to be part of the ABI.


The problem is that someone decided to use the device name in the USB
gadget configfs interface and as such made it part of the ABI.

Regards,
Bjorn

John Stultz July 22, 2021, 8:09 p.m. UTC | #22

On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
<[e-mail]> 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, 9:54 p.m. UTC | #23

On Thu, Jul 22, 2021 at 02:17:15PM -0500, Bjorn Andersson wrote:
> On Thu 22 Jul 13:12 CDT 2021, Serge Semin wrote:
> > On Wed, Jul 21, 2021 at 11:08:08AM -0700, John Stultz wrote:
> > > On Wed, Jul 21, 2021 at 4:25 AM Serge Semin <[e-mail]> wrote:
> > > > On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:
> > > > > 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/[e-mail]/
> > > >
> > > > 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
> > > 
> > 
> > > The problem with this is there may be multiple USB controllers on a
> > > system (not all exposed outside the case - and also the dummy
> > > controller is often present). How can we configure the system to know
> > > which controller is which?
> > 
> > How did you distinguish them before this change? It has nothing really
> > related with the patch in topic.
> > 
> > > 
> > > The only name we have for distinguishing the controllers is the DTS
> > > node. So it seems inherent that changes to that name will break the
> > > config.
> > 
> > No, it's not the only way you have. I say it was the easiest way for
> > you to find a corresponding device. The DT-node name never was a part
> > of ABI for at least up until we finally get the DT-node names
> > consistent with DT spec. Only then it would be possible to consider
> > them as such. One more time I'll quote what @Krzisztof has already
> > told you twice:
> > 
> 

> The requirement to keep a stable userspace ABI does not consider the
> fact that the kernel made up part of that ABI based on things it found
> in e.g. DT.
> 
> The name of the UDC devices has been and is simply the
> dev_name(dwc3-device) and this is directly based on the DT node.
> 
> So changing DT indirectly changed the user space ABI and anyone who is
> using USB Gadget Configfs is directly affected by this.

Here we get to the main question again: is the device-name or an
acceptable values of the UDC configfs node are a part of the kernel
ABI? Could you provide a link to where the Linux ABI docs state that?
I failed to find it. The only part what concerns UDC is described in
Documentation/ABI/stable/sysfs-class-udc. But it doesn't guarantee any
device name, DT-node name or so.

> 
> Regards,
> Bjorn
> 
> > 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?

There isn't at the moment. But it could be added.

> 
> 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, even if we implemented that feature, then I don't see why John
or someone responsible for AOSP support would have wanted to change
the user-space part to comply with it seeing nobody decided to follow
the 2nd case suggested by @Krzysztof (see my next comment). Especially
seeing it won't be backported to the LTS kernels as it would be a new
feature.

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

Hmm, what isn't backwards compatible in this solution? What I can see
is that it wasn't indeed implemented in AOSP.

Anyway looking at this discussion in the whole the logic seems to get
to a deadlock. John said there is no a solution. We suggest two of
them. One of them involves kernel changes (alias-related) and thus
user-space modifications, another one - only the user-space changes.
You say the second one isn't appropriate as it won't be backwards
compatible. The first one, which involves having both part modified,
won't be also backwards compatible and thus won't be suitable either.
Therefore the only solution you accept is reverting the patch just
because nobody really wants to provide the AOSP modification instead.

> 
> > Of course it was easier to revert the patch. But if we always chose
> > such approach, the kernel would have been filled with tons of
> > workarounds and legacy parts without possibility to move forward to
> > having more unified interfaces.
> > 
> 

> Yes, and that's exactly where we are heading with the ongoing DT
> validation work. But "don't break your users" isn't excused because the
> ABI is derived from some third party.

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".

> 
> > > 
> > > That said, this issue reminded me of the /dev/hda -> /dev/sda device
> > > name or the eth0 -> enp3s0 switch which both also had the potential to
> > > break configurations or scripts.  I get that having a standard naming
> > > scheme is important (I'm very sympathetic to this point)! I can
> > > imagine UI trying to show possible controllers for a user to select
> > > needs a simple way to determine if a device is a usb controller - but
> > > again this just shows that the node names are an ABI.
> > > 
> > > So I'm not the one to judge if this change is useful enough to push
> > > through the pain, but it did seem to be done a bit casually.
> > > 
> > > > 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.
> > > 
> > > DTS changes are a constant source of regressions in my experience. We
> > > mostly just have to roll with it, but it feels never ending. :)  I'd
> > > personally rather folks in general be more thoughtful about what DTS
> > > changes they make and accept, understanding that they do have impact
> > > on userland.  And I'd imagine If updates to linux-firmware broke the
> > > most recent LTS kernel, that would be seen as a regression too, and
> > > folks wouldn't be told to just keep the old firmware.
> > 
> > > But all the
> > > same, I'd also be happy for suggestions to remove any such
> > > dependencies userland has on specific dts naming, where possible, to
> > > make the constant pain go away. :)
> > 
> > Well, @Krzisztof has already given you such suggestions regarding this
> > issue not once. But you tend to ignore them.
> > 
> 

> Those are good suggestions, but they require changes in userspace.

So what I can infer from all of that is we aren't supposed to change
in the kernel anything what may cause user-space changes. Right? If
so the kernel will end up in a dead-end very soon.

> 
> > Anyway this patch doesn't break LTS kernel and doesn't break the
> > linux-firmware either. It changes DT-node name, which happens to
> > change the device name, which isn't guaranteed to be stable as it's
> > not part of the kernel ABI. If you think otherwise please provide some
> > proves to that. I didn't find any in the official Linux ABI docs.
> > 
> 

> I think it's a gray area, at least I do want it to be a gray area,
> because I don't want device names to be part of the ABI.
> 
> 
> The problem is that someone decided to use the device name in the USB
> gadget configfs interface and as such made it part of the ABI.

I can agree with that only if any value acceptable by configfs
nodes implicitly gets to be ABI. But I failed to find any proof of
that. As I said above the only UDC-related ABI I found was described
in Documentation/ABI/stable/sysfs-class-udc. In addition to that there
is a currently testing USB-gadget configfs interface
Documentation/ABI/testing/configfs-usb-gadget
but it also claims that the user-space applications are supposed to use the
USB-controller names found in the /sys/class/udc/* directory.

Regards,
-Sergey

> 
> Regards,
> Bjorn

Serge Semin July 22, 2021, 10:09 p.m. UTC | #24

On Thu, Jul 22, 2021 at 01:09:05PM -0700, John Stultz wrote:
> On Thu, Jul 22, 2021 at 12:17 PM Bjorn Andersson
> <[e-mail]> 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 | #25

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 23, 2021, 8:18 a.m. UTC | #26

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.

thanks,

greg k-h

Bjorn Andersson July 23, 2021, 2:34 p.m. UTC | #27

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.

Regards,
Bjorn

Greg KH July 23, 2021, 3:54 p.m. UTC | #28

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?

thanks,

greg k-h

Bjorn Andersson July 23, 2021, 7:54 p.m. UTC | #29

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?


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?

Regards,
Bjorn

Greg KH July 24, 2021, 7:50 a.m. UTC | #30

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 | #31

On Thu, Jul 22, 2021 at 3:09 PM Serge Semin <[e-mail]> 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
> > <[e-mail]> 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

Serge Semin Aug. 15, 2021, 7:46 p.m. UTC | #32

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 <[e-mail]> 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
> > > <[e-mail]> 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.

Regards
-Sergey

> 
> thanks
> -john

Bjorn Andersson Aug. 18, 2021, 3:44 a.m. UTC | #33

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 <[e-mail]> 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
> > > > <[e-mail]> 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 | #34

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 <[e-mail]> 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
> > > > > <[e-mail]> 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

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