| Message ID | 20231024211818.365844-3-nfraprado@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add test to verify probe of devices from discoverable busses on DT platforms | expand |
On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote: > Add the list of devices expected to be probed from the USB and PCI > busses on the google,spherion machine. The USB host controller at > 11200000 is shared between two busses, for USB2 and USB3, so an > additional match is used to select the USB2 bus. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > > tools/testing/selftests/devices/boards/google,spherion | 3 +++ > 1 file changed, 3 insertions(+) > create mode 100644 tools/testing/selftests/devices/boards/google,spherion > > diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion > new file mode 100644 > index 000000000000..ba86ffcfe43c > --- /dev/null > +++ b/tools/testing/selftests/devices/boards/google,spherion > @@ -0,0 +1,3 @@ > +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 > +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 > +pci wifi 11230000 0.0/0.0 USB busses (and PCI ids) are not determinisitic and can, and will, change values randomly. So while it is nice to test "did the devices show up properly", you can not do that based on bus ids at all, sorry. Unless I'm reading these values wrong? What are the fields representing? Perhaps a comment at the top to describe them so that we know how to parse them? thanks, greg k-h
On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote: > On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote: > > Add the list of devices expected to be probed from the USB and PCI > > busses on the google,spherion machine. The USB host controller at > > 11200000 is shared between two busses, for USB2 and USB3, so an > > additional match is used to select the USB2 bus. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > > > tools/testing/selftests/devices/boards/google,spherion | 3 +++ > > 1 file changed, 3 insertions(+) > > create mode 100644 tools/testing/selftests/devices/boards/google,spherion > > > > diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion > > new file mode 100644 > > index 000000000000..ba86ffcfe43c > > --- /dev/null > > +++ b/tools/testing/selftests/devices/boards/google,spherion > > @@ -0,0 +1,3 @@ > > +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 > > +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 > > +pci wifi 11230000 0.0/0.0 > > USB busses (and PCI ids) are not determinisitic and can, and will, > change values randomly. So while it is nice to test "did the devices > show up properly", you can not do that based on bus ids at all, sorry. > > Unless I'm reading these values wrong? What are the fields > representing? Perhaps a comment at the top to describe them so that we > know how to parse them? Hi Greg, I have described the fields in the commit message of patch 1. Here they are: usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces> pci <test_name> <controller_address> <device-function_pairs_path> I'm aware that bus IDs are assigned at runtime, and that's exactly why I've avoided those in the test definitions, instead describing the hardware topology, which won't ever change. And just to be extra clear, by hardware topology I mean: For USB, we find the USB bus based on the address of its controller (and optionally its productID if two busses share the same controller for USB2 and USB3), and then find the device by following the ports at each hub. The configuration number and interfaces then describe what interfaces to check for presence and driver binding. For PCI, we find the controller again based on its address, and follow the device-function pairs at each level in the topology until we arrive at the desired device. We don't rely on the USB bus number, nor on the PCI domain and bus number, since these are all assigned at runtime. Thanks, Nícolas
On Wed, Oct 25, 2023 at 08:32:42AM -0400, Nícolas F. R. A. Prado wrote: > On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote: > > > Add the list of devices expected to be probed from the USB and PCI > > > busses on the google,spherion machine. The USB host controller at > > > 11200000 is shared between two busses, for USB2 and USB3, so an > > > additional match is used to select the USB2 bus. > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > --- > > > > > > tools/testing/selftests/devices/boards/google,spherion | 3 +++ > > > 1 file changed, 3 insertions(+) > > > create mode 100644 tools/testing/selftests/devices/boards/google,spherion > > > > > > diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion > > > new file mode 100644 > > > index 000000000000..ba86ffcfe43c > > > --- /dev/null > > > +++ b/tools/testing/selftests/devices/boards/google,spherion > > > @@ -0,0 +1,3 @@ > > > +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 > > > +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 > > > +pci wifi 11230000 0.0/0.0 > > > > USB busses (and PCI ids) are not determinisitic and can, and will, > > change values randomly. So while it is nice to test "did the devices > > show up properly", you can not do that based on bus ids at all, sorry. > > > > Unless I'm reading these values wrong? What are the fields > > representing? Perhaps a comment at the top to describe them so that we > > know how to parse them? > > Hi Greg, > > I have described the fields in the commit message of patch 1. Here they are: > > usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces> > > pci <test_name> <controller_address> <device-function_pairs_path> That's not a good place to document them, we'll never find them, and I obviously missed it as well. Please put it in a comment at the top of this file _AND_ in a comment in the script that parses these files. > I'm aware that bus IDs are assigned at runtime, and that's exactly why I've > avoided those in the test definitions, instead describing the hardware topology, > which won't ever change. > > And just to be extra clear, by hardware topology I mean: > > For USB, we find the USB bus based on the address of its controller (and > optionally its productID if two busses share the same controller for USB2 and > USB3), What exactly do you mean by "address" of a controller? That will be unique per bus-type that the controller lives on, right? > and then find the device by following the ports at each hub. The > configuration number and interfaces then describe what interfaces to check for > presence and driver binding. Ok, good, hub and port locations _should_ be stable, but note they have changed at times so you will have to deal with that for the next 20+ years, are you ok with that? > For PCI, we find the controller again based on its address, and follow the > device-function pairs at each level in the topology until we arrive at the > desired device. "address"? What exactly do you mean by this value? For PCI, that will change. > We don't rely on the USB bus number, nor on the PCI domain and bus number, since > these are all assigned at runtime. You are relying on a specific sysfs tree as well, are you able to handle it when new devices get added to the middle of a device path? That sometimes happens in sysfs too. thanks, greg k-h
On Fri, Oct 27, 2023 at 12:48:35PM +0200, Greg Kroah-Hartman wrote: > On Wed, Oct 25, 2023 at 08:32:42AM -0400, Nícolas F. R. A. Prado wrote: > > On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote: > > > On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote: > > > > Add the list of devices expected to be probed from the USB and PCI > > > > busses on the google,spherion machine. The USB host controller at > > > > 11200000 is shared between two busses, for USB2 and USB3, so an > > > > additional match is used to select the USB2 bus. > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > --- > > > > > > > > tools/testing/selftests/devices/boards/google,spherion | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > create mode 100644 tools/testing/selftests/devices/boards/google,spherion > > > > > > > > diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion > > > > new file mode 100644 > > > > index 000000000000..ba86ffcfe43c > > > > --- /dev/null > > > > +++ b/tools/testing/selftests/devices/boards/google,spherion > > > > @@ -0,0 +1,3 @@ > > > > +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 > > > > +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 > > > > +pci wifi 11230000 0.0/0.0 > > > > > > USB busses (and PCI ids) are not determinisitic and can, and will, > > > change values randomly. So while it is nice to test "did the devices > > > show up properly", you can not do that based on bus ids at all, sorry. > > > > > > Unless I'm reading these values wrong? What are the fields > > > representing? Perhaps a comment at the top to describe them so that we > > > know how to parse them? > > > > Hi Greg, > > > > I have described the fields in the commit message of patch 1. Here they are: > > > > usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces> > > > > pci <test_name> <controller_address> <device-function_pairs_path> > > That's not a good place to document them, we'll never find them, and I > obviously missed it as well. > > Please put it in a comment at the top of this file _AND_ in a comment in > the script that parses these files. Fair enough, will do so in the next version. > > > I'm aware that bus IDs are assigned at runtime, and that's exactly why I've > > avoided those in the test definitions, instead describing the hardware topology, > > which won't ever change. > > > > And just to be extra clear, by hardware topology I mean: > > > > For USB, we find the USB bus based on the address of its controller (and > > optionally its productID if two busses share the same controller for USB2 and > > USB3), > > What exactly do you mean by "address" of a controller? That will be > unique per bus-type that the controller lives on, right? Well, in its current form the test is targetted at DT-based platforms, so by address I mean the MMIO address of the host controller, as defined in the controller's node in the DT. In order to expand the coverage of the test to non-DT platforms as well, yes, we'll need to allow describing controllers that are under some bus, rather than at a fixed memory address. I'm already planning to do that as a next step. > > > and then find the device by following the ports at each hub. The > > configuration number and interfaces then describe what interfaces to check for > > presence and driver binding. > > Ok, good, hub and port locations _should_ be stable, but note they have > changed at times so you will have to deal with that for the next 20+ > years, are you ok with that? If the locations changed it means we have a new hardware revision. I'd expect most of those occurences to result in new compatible strings or DMI identifiers, which we could then use to run a different set of tests, using the new hub and port locations, since this is effectively a different piece of hardware. > > > For PCI, we find the controller again based on its address, and follow the > > device-function pairs at each level in the topology until we arrive at the > > desired device. > > "address"? What exactly do you mean by this value? For PCI, that will > change. Same as for USB. > > > We don't rely on the USB bus number, nor on the PCI domain and bus number, since > > these are all assigned at runtime. > > You are relying on a specific sysfs tree as well, are you able to handle > it when new devices get added to the middle of a device path? That > sometimes happens in sysfs too. Right, I'm relying on the fact that the PCI topology is reflected into the sysfs topology, meaning the sysfs directories for devices on a PCI bus are direct descendants of the directory for the PCI bridge exposing that bus. That's the only place I'm aware of which exposes the PCI topology. In any case, although simplistic, this approach has worked well so far on the platforms I've tested on. Thanks, Nícolas
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion