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

From: Serge Semin <fancer.lancer@gmail.com>
To: John Stultz <john.stultz@linaro.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>, Felipe Balbi <balbi@kernel.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Linux USB List <linux-usb@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Amit Pundir <amit.pundir@linaro.org>
Subject: Re: [PATCH 29/29] arm64: dts: qcom: Harmonize DWC USB3 DT nodes name
Date: Thu, 22 Jul 2021 21:12:21 +0300	[thread overview]
Message-ID: <20210722181221.xh3r5kyu7zlcojjx@mobilestation> (raw)
In-Reply-To: <CALAqxLViEqSO17P3JGRGYJh-wDoHaJiQQV48zeoRgnar4Xd5Bg@mail.gmail.com>

On Wed, Jul 21, 2021 at 11:08:08AM -0700, John Stultz wrote:
> On Wed, Jul 21, 2021 at 4:25 AM Serge Semin <fancer.lancer@gmail.com> 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/CALAqxLWGujgR7p8Vb5S_RimRVYxwm5XF-c4NkKgMH-43wEBaWg@mail.gmail.com/
> >
> > AOSP developers somehow hardcoded a USB-controller UDC name in the
> > internal property called "sys.usb.controller" with a value
> > "ff100000.dwc3". That value is generated by the kernel based on the
> > corresponding DT-node name. The property is then used to
> > pre-initialize the system like it's done here:
> > https://android.googlesource.com/platform/system/core/+/master/rootdir/init.usb.configfs.rc
> >
> > Since we changed the DT-node name in the recent kernel, we thus changed the
> > UDC controller name so AOSP init procedure now fails to bring up the Linux
> > USB-gadget using on the older UDC name. UDC is supposed to be ff100000.usb now
> > (after this patch has been merged in).
> >
> > What problems I see here:
> > 1) the AOSP developers shouldn't have hard-coded the value but read
> > from the /sys/class/udc/* directory and then decided which controller
> > to use. As it's described for instance here:
> > https://www.kernel.org/doc/Documentation/usb/gadget_configfs.txt
> 

> 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