On Tue, 2022-02-15 at 18:51 +0100, Steffen Maier wrote:
> On 2/14/22 20:19, Martin Wilck wrote:
> >
> > Is it certain that this condition can't cause a valid ccw device
> > (where
> > the driver attribute isn't required) to be skipped with the
> > "continue"
> > statement? Even if the answer is "yes", I'd prefer self-explanatory
>
> I had the same thought, but it does work. Apparently, the device node
> we're
> interested in for ccw-attached FCP devices has both driver and
> subsystem
> attributes that exist and both with a non-empty value. So we're good,
> even if
> the preceding "early continue" skipped an uninteresting parent.
> However,
> proving generality is beyond my capabilities, as I'm not even sure
> libudev
> works on the udev database or sysfs directly. For instance,
>
> # udevadm info -a /sys/class/scsi_host/host2
>
> shows SUBSYSTEM and DRIVER property for each part of the ancestor
> chain, though
> sometimes with empty string values which would not be a problem,
> whereas
>
> # dir=$(readlink -e /sys/class/scsi_host/host2/); while [ -n "$dir"
> ]; do echo
> $dir; ls -laF $dir/driver $dir/subsystem; dir=${dir%/*}; done
>
> shows some ancestors completely lacking "driver" and some also
> lacking "subsystem".
>
> > code here, because not all of us are s390 / ccw experts.
>
> I don't think there is anything specific to architecture or bus type.
I was thinking about something like this (untested):
for (parent = udev_device_get_parent(hostdev); parent;
parent = udev_device_get_parent(parent)) {
driver_name = udev_device_get_driver(parent);
if (driver_name && !strcmp(driver_name, "pcieport"))
break;
subsystem_name = udev_device_get_subsystem(parent);
if (subsystem_name && !strcmp(subsystem_name, "ccw"))
break;
}
if (!parent) {
udev_device_unref(hostdev);
return 1;
}
...
At least this would make it clear to the reader that we check for both
ccw and pcie on each level of the hierarchy. Functionally, it would be
equivalent.
>
> In fact, I was surprised to see this code here to match for driver
> "pcieport"
> [also "pci**e**port" sounds like PCI-Express, so what about HBAs
> attached to
> the old parallel PCI instead of PCIe?], because udev-builtin-
> path_id.c looks
> very consistent and similar between pci and ccw to me:
>
> static int builtin_path_id(sd_device *dev, sd_netlink **rtnl, int
> argc, char
> *argv[], bool test) {
>
> /* walk up the chain of devices and compose path */
> parent = dev;
> while (parent) {
> const char *subsys, *sysname;
>
> if (sd_device_get_subsystem(parent, &subsys) < 0 ||
> sd_device_get_sysname(parent, &sysname) < 0) {
> ;
>
> } else if (streq(subsys, "pci")) {
> path_prepend(&path, "pci-%s", sysname);
> if (compat_path)
> path_prepend(&compat_path, "pci-%s",
> sysname);
> parent = skip_subsystem(parent, "pci");
> supported_parent = true;
>
> } else if (streq(subsys, "ccw")) {
> path_prepend(&path, "ccw-%s", sysname);
> if (compat_path)
> path_prepend(&compat_path, "ccw-%s",
> sysname);
> parent = skip_subsystem(parent, "ccw");
> supported_transport = true;
> supported_parent = true;
>
> However, the details inside sd_device_get_subsystem() and
> sd_device_get_sysname() are beyond my powers to understand, so there
> might be
> differences hidden in there.
>
> That said, I don't want to touch the PCI part here. I don't even have
> something
> to test that.
I didn't mean you should. I was also wondering about "pcieport", too.
It dates back to a28e61e ("Crafted ordering of child paths for round
robin path selector"), which was merged before I started working on
multipath-tools.
Indeed for this FC adapter:
/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.7/driver ->
../../../../bus/pci/drivers/qla2xxx
the "%a" wildcard returns '0000:00:01.0', which is the pcieport
upstream of the FC adapter, but arguably not the "host adapter" itself.
This feels wrong.
@Ben, what's your take on this?
I suppose it may be related to the purpose of a28e61e which had nothing
to do with human-readable output. Rather, the patch attempted to
distribute IO evenly over paths, and apparently used the PCIe port to
identify the PCI-Express part of the "path". The %a wildcard was added
later.
See
https://listman.redhat.com/archives/dm-devel/2014-February/msg00104.html
You may want to double-check if your CCW implementation meets the
purpose of commit a28e61e wrt distributing load evenly over adapters.
Thanks,
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
