On Wed, Jul 21, 2021 at 01:10:19PM +0200, Krzysztof Kozlowski wrote:

On 21/07/2021 13:02, Greg Kroah-Hartman wrote:

quoted

On Wed, Jul 21, 2021 at 12:45:32PM +0200, Krzysztof Kozlowski wrote:

quoted

On 21/07/2021 12:29, Greg Kroah-Hartman wrote:

quoted

On Wed, Jul 21, 2021 at 01:02:20PM +0300, Serge Semin wrote:

quoted

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:

quoted

On Wed, Jul 14, 2021 at 03:48:07PM +0300, Serge Semin wrote:

quoted

Hello John,

On Tue, Jul 13, 2021 at 05:07:00PM -0700, John Stultz wrote:

quoted

On Tue, Oct 20, 2020 at 5:10 AM Serge Semin
[off-list ref] wrote:

quoted

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

quoted

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/ (local)
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 [off-list ref] wrote:

quoted

On Mon, Dec 21, 2020 at 12:24:11PM -0800, John Stultz wrote:

quoted

On Sat, Dec 19, 2020 at 3:06 AM Krzysztof Kozlowski [off-list ref] wrote:

quoted

...

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.

quoted

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.

quoted

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?

quoted

Greg? Is there some better way AOSP should be doing this?

If you need to find specific device, maybe go through the given bus and
check compatibles?

Best regards,
Krzysztof

So the main question is how is the DT-node really connected with ABI
and is supposed to be stable in that concern?

As I see it even if it affects the configfs node name, then we may
either need to break that connection and somehow deliver DT-node-name
independent interface to the user-space or we have no choice but to
export the node with an updated name and ask of user-space to deal
with it. In both suggested cases the DT-node name will still conform
to the USB-node name DT spec. Currently we are at the second one.

I really do not care what you all decide on, but you CAN NOT break
existing working systems, sorry.  That is why I have reverted this
change in my tree and will send it to Linus soon.

I had impression that kernel defines interfaces which should be used and
are stable (e.g. syscalls, sysfs and so on). This case is example of
user-space relying on something not being marked as part of ABI. Instead
they found something working for them and now it is being used in "we
cannot break existing systems". Basically, AOSP unilaterally created a
stable ABI and now kernel has to stick to it.

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

Why would you not depend on them?

It's not good example. The configfs entries (file names) are
user-visible however the USB gadget exposes specific value for specific
one device. It encodes device specific DT node name and HW address and
gives it to user-space. It is valid only on this one HW, all other
devices will have different values.

User-space has hard-coded this value (DT node name and hardware
address). This value was never part of configfs ABI, maybe except of its
format "[a-z]+\.[0-9a-f]+". Format is not broken. Just the value changes
for a specific device/hardware.

It's like you depend that lsusb will always report:
  Bus 003 Device 008: ID 046d:c52b Logitech, Inc. Unifying Receiver
and then probing order changed and this Logitech ends as Device 009.
Then AOSP guys come, wait, we hard-coded that Logitech on our device
will be always Device 008, not 009. Please revert it, we depend on
specific value of Device number. It must be always 009...

For the record - the change discussed here it's nothing like USB VID/PID. :)

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

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel