Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-11 Thread Lennart Poettering 
On Tue, 10.11.15 11:12, Jordan Hargrave (jhar...@gmail.com) wrote:

>  The patch will also decode SMBIOS slot number for NIC, and store in the 
>  variable
>  ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method for naming
>  ports on a multi-port card plugged into a slot.
> >>>
> >>> Again, I don't think systemd should carry an SMBIOS parser.
> >>>
> >>> Sorry,
> >>> Kay
> >>
> >> From a customer usability standpoint, having the slot numbers as part
> >> of systemd would be a very useful feature.
> >
> > Sure, but I think Kay's point was that the needed info should be
> > exposed from the kernel in a sysattr, not be parsed from udev. Any
> > reason this cannot be done that way?
> 
> The pci kernel maintainer was reluctant to put it in the kernel and
> said systemd should parse it.  So a bit of circular argument here.
> I agree, it would be better for it to be a kernel sysfs attr.

There's really no point in placing SMBIOS parsers to userspace *and*
kernelspace. There's one in the kernel already, and it is already used
to attach metadata to network devices. Hence, please, extend that
existing parser and add what you need there, and we can then hook up
whatever you parse out of that with udev.

The kernel will always be better in making sense of SMBIOS, hence it's
really the place this should be done.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-10 Thread Tom Gundersen 
On Tue, Nov 10, 2015 at 2:29 PM, Jordan Hargrave  wrote:
> On Tue, Nov 10, 2015 at 4:53 AM, Kay Sievers  wrote:
>> On Tue, Nov 10, 2015 at 5:49 AM, Jordan Hargrave  wrote:
>>> Cleaned up linux coding style
>>>
>>> This patch will integrate some of the features of biosdevname into systemd.
>>> The code detects the port and index for detecting NIC partitions. This 
>>> creates
>>> a new environment variable, ID_NET_NAME_PARTITION of the format
>>> _
>>>
>>> The patch will also decode SMBIOS slot number for NIC, and store in the 
>>> variable
>>> ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method for naming
>>> ports on a multi-port card plugged into a slot.
>>
>> Again, I don't think systemd should carry an SMBIOS parser.
>>
>> Sorry,
>> Kay
>
> From a customer usability standpoint, having the slot numbers as part
> of systemd would be a very useful feature.

Sure, but I think Kay's point was that the needed info should be
exposed from the kernel in a sysattr, not be parsed from udev. Any
reason this cannot be done that way?

>  The current method only
> works for single-port NICs in a slot.  Multi-port NICs, especially
> ones with SR-IOV or multiple partitions get garbled names like

Just to make sure we are on the same page, when you say "garbled" you
mean that the naming scheme is not the one you want, but there are no
bugs here, right?

> enp4s0
> enp4s1
> enp4s0d1
> enp4s0f1
> enp4s0f2
> enp4s0f3
> enp4s0f4
> enp4s0f5
> enp4s0f6
> enp4s0f7
> enp4s0f1d1
> enp4s0f2d1
> enp4s0f3d1
> enp4s0f4d1
> enp4s0f5d1
> enp4s0f6d1
> enp4s0f7d1
> enp4s1d1
> enp68s0f0
> enp68s0f1
> enp69s0f0
> enp69s0f1
>
> That's another annoying thing with systemd names, the bus number is
> *decimal*.  lspci is in hex, so the customer has to do a conversion to
> figure out even what PCI device that is.

I guess too late to change that now.

> All enp4 are a dual-port NIC in Slot 3 with 8 SR-IOV devices.

Hm, there are 17 devices listed, shouldn't there be 16 based on your
description?

> All enp68xx and enp69xxx are a single quad-port NIC in slot 2.
> Systemd breaks here if trying to name using slot numbers with the
> existing method.  As there are 4 devices under the slot with same
> device numbers, systemd would name them
> ens2f0
> ens2f1
> ens2f0
> ens2f1
>
> Which causes name collision.  I was able to verify this as either they
> got named:
> ens2f0
> ens2f1
> enp69s0f0
> enp69s0f1
>
> or
> enp68s0f0
> enp68s0f1
> ens2f0
> ens2f1
>
> at startup.
>
> That's the best feature of biosdevname, being able to tell which slot
> the NIC is located just from the name.  Systemd still has some
> limitations and/or bugs in this regard.

So how would your proposed naming scheme look in the examples you
gave? Is the information needed to generate the name taken from the
device in question or any of its parent devices (but never from its
siblings or other devices) and hence independent of probe-ordering?

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-10 Thread Jordan Hargrave 
On Tue, Nov 10, 2015 at 4:53 AM, Kay Sievers  wrote:
> On Tue, Nov 10, 2015 at 5:49 AM, Jordan Hargrave  wrote:
>> Cleaned up linux coding style
>>
>> This patch will integrate some of the features of biosdevname into systemd.
>> The code detects the port and index for detecting NIC partitions. This 
>> creates
>> a new environment variable, ID_NET_NAME_PARTITION of the format
>> _
>>
>> The patch will also decode SMBIOS slot number for NIC, and store in the 
>> variable
>> ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method for naming
>> ports on a multi-port card plugged into a slot.
>
> Again, I don't think systemd should carry an SMBIOS parser.
>
> Sorry,
> Kay

From a customer usability standpoint, having the slot numbers as part
of systemd would be a very useful feature.  The current method only
works for single-port NICs in a slot.  Multi-port NICs, especially
ones with SR-IOV or multiple partitions get garbled names like

enp4s0
enp4s1
enp4s0d1
enp4s0f1
enp4s0f2
enp4s0f3
enp4s0f4
enp4s0f5
enp4s0f6
enp4s0f7
enp4s0f1d1
enp4s0f2d1
enp4s0f3d1
enp4s0f4d1
enp4s0f5d1
enp4s0f6d1
enp4s0f7d1
enp4s1d1
enp68s0f0
enp68s0f1
enp69s0f0
enp69s0f1

That's another annoying thing with systemd names, the bus number is
*decimal*.  lspci is in hex, so the customer has to do a conversion to
figure out even what PCI device that is.

All enp4 are a dual-port NIC in Slot 3 with 8 SR-IOV devices.

All enp68xx and enp69xxx are a single quad-port NIC in slot 2.
Systemd breaks here if trying to name using slot numbers with the
existing method.  As there are 4 devices under the slot with same
device numbers, systemd would name them
ens2f0
ens2f1
ens2f0
ens2f1

Which causes name collision.  I was able to verify this as either they
got named:
ens2f0
ens2f1
enp69s0f0
enp69s0f1

or
enp68s0f0
enp68s0f1
ens2f0
ens2f1

at startup.

That's the best feature of biosdevname, being able to tell which slot
the NIC is located just from the name.  Systemd still has some
limitations and/or bugs in this regard.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-09 Thread Andrei Borzenkov 

10.11.2015 04:41, Jordan Hargrave пишет:



The patch will also decode SMBIOS slot number for NIC, and store in
the variable ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method
for naming ports on a multi-port card plugged into a slot.


Hmm, isn't this stuff the same as exported by the kernel as the
"index" field, i.e. SMBIOS Type 41? Can you elaborate on the relation
of that field and the stuff this patch adds?



  The index is exported for embedded devices, but not add-in cards.
acpi_index may be exported for add-in cards, but generally not very
useful as it has a number like 17 or 24... no relation to the actual
physical slot where a NIC is located.



Quoting SMBIOS spec for Type 9 "Slot ID":

--><--
On a system that supports ACPI, identifies the value returned in the 
_SUN object for this slot.


On a system that supports the PCI IRQ Routing Table Specification, 
identifies the value present
in the Slot Number field of the PCI Interrupt Routing table entry that 
is associated with this slot

--><--

Kernel already exports _SUN when system supports it; and other one does 
not sound very meaningful.


What do you have in /sys/bus/pci/slots?


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-09 Thread Jordan Hargrave 
On Mon, Nov 9, 2015 at 4:43 PM, Lennart Poettering
 wrote:
> On Mon, 09.11.15 12:42, Jordan Hargrave (jhar...@gmail.com) wrote:
>
>> From: Jordan Hargrave 
>>
>> This patch will integrate some of the features of biosdevname into systemd.
>> The code detects the port and index for detecting NIC partitions. This 
>> creates
>> a new environment variable, ID_NET_NAME_PARTITION of the format
>> _
>
> "partitions"? What's that supposed to be? SR-IOV?
>

Similar to SR-IOV but on Dell servers partitions actually show up as
PCI devices in the initial enumeration.  The PCI device vital product
data area has a map of which Bus:Dev:Func is which port and which port
instance.

So say we have a PCI device tree for a quad-port NIC, 4 physical ports
but 8 NICs.

04:00.0 port 1, instance 1
04:00.1 port 2, instance 1
04:00.2 port 3, instance 1
04:00.3 port 4, instance 1
04:00.4 port 1, instance 2
04:00.5 port 2, instance 2
04:00.6 port 3, instance 2
04:00.7 port 4, instance 2

We use this partition info to display if a bond is created that
contains the same physical nic port.

>> The patch will also decode SMBIOS slot number for NIC, and store in
>> the variable ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method
>> for naming ports on a multi-port card plugged into a slot.
>
> Hmm, isn't this stuff the same as exported by the kernel as the
> "index" field, i.e. SMBIOS Type 41? Can you elaborate on the relation
> of that field and the stuff this patch adds?
>

 The index is exported for embedded devices, but not add-in cards.
acpi_index may be exported for add-in cards, but generally not very
useful as it has a number like 17 or 24... no relation to the actual
physical slot where a NIC is located.

>> Signed-off-by: Jordan Hargrave 
>
> I didn't look too closely in the actual sources, but I did notice that
> it is line-broken, and doesn't follow CODING_STYLE in quite a few
> cases, regarding placement of brackets, or error handling for
> example.
>
> In order to avoid any confusion with line-broken patches, and to make
> review easier, please submit the patch as github PR, which is the way
> we generally prefer receiving patches these days!

Where do I submit this?

>
> Thanks,
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-09 Thread Jordan Hargrave 
Cleaned up linux coding style

This patch will integrate some of the features of biosdevname into systemd.
The code detects the port and index for detecting NIC partitions. This creates
a new environment variable, ID_NET_NAME_PARTITION of the format
_

The patch will also decode SMBIOS slot number for NIC, and store in the variable
ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method for naming
ports on a multi-port card plugged into a slot.

Signed-off-by: Jordan Hargrave 
---
 src/udev/udev-builtin-net_id.c | 206 +
 1 file changed, 206 insertions(+)

diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c
index bf5c9c6..04b08dd 100644
--- a/src/udev/udev-builtin-net_id.c
+++ b/src/udev/udev-builtin-net_id.c
@@ -119,16 +119,130 @@ struct netnames {
 bool mac_valid;

 struct udev_device *pcidev;
+struct udev_device *physdev;
 char pci_slot[IFNAMSIZ];
 char pci_path[IFNAMSIZ];
 char pci_onboard[IFNAMSIZ];
 const char *pci_onboard_label;
+int  npar_port;
+int  npar_pfi;
+int  smbios_slot;

 char usb_ports[IFNAMSIZ];
 char bcma_core[IFNAMSIZ];
 char ccw_group[IFNAMSIZ];
 };

+#define FLAG_IOV 0x80
+#define FLAG_NPAR 0x1000
+
+#define VPDI_TAG 0x82
+#define VPDR_TAG 0x90
+
+struct vpd_tag {
+char  cc[2];
+unsigned char len;
+char  data[1];
+};
+
+/* Read VPD tag ID */
+static int vpd_readtag(int fd, int *len)
+{
+unsigned char tag, tlen[2];
+
+if (read(fd, , 1) != 1)
+return -1;
+if (tag == 0x00 || tag == 0xFF || tag == 0x7F)
+return -1;
+if (tag & 0x80) {
+if (read(fd, tlen, 2) != 2)
+return -1;
+*len = tlen[0] + (tlen[1] << 8);
+return tag;
+}
+*len = (tag & 0x7);
+return (tag & ~0x7);
+}
+
+static void *vpd_findtag(void *buf, int len, const char *sig)
+{
+int off, siglen;
+struct vpd_tag *t;
+
+off = 0;
+siglen = strlen(sig);
+while (off < len) {
+t = (struct vpd_tag *)((unsigned char *)buf + off);
+if (!memcmp(t->data, sig, siglen))
+return t;
+off += (t->len + 3);
+}
+return NULL;
+}
+
+static void dev_pci_npar_dcm(struct udev_device *dev, struct netnames *names,
+ int len, const char *dcm,
+ const char *fmt, int step)
+{
+int domain, bus, slot, func, off, mydf;
+int port, df, pfi, flag;
+
+if (sscanf(udev_device_get_sysname(names->physdev), "%x:%x:%x.%u",
+   , , , ) != 4)
+return;
+mydf = (slot << 3) + func;
+for (off = 3; off < len; off += step) {
+if (sscanf(dcm+off, fmt, , , , ) != 4)
+continue;
+if ((flag & FLAG_NPAR) && mydf == df) {
+names->npar_port = port;
+names->npar_pfi = pfi;
+}
+}
+}
+
+static void dev_pci_npar(struct udev_device *dev, struct netnames *names)
+{
+const char *filename;
+int len, fd;
+struct vpd_tag *dcm;
+void *buf;
+
+/* Search for VPD or IOV VPD */
+filename = strjoina(udev_device_get_syspath(names->physdev), "/vpd");
+fd = open(filename, O_RDONLY);
+if (fd < 0)
+return;
+if (vpd_readtag(fd, ) != VPDI_TAG)
+goto done;
+lseek(fd, len, SEEK_CUR);
+
+/* Check VPD-R */
+if (vpd_readtag(fd, ) != VPDR_TAG)
+goto done;
+buf = alloca(len);
+if (read(fd, buf, len) != len)
+goto done;
+
+/* Check for DELL VPD tag */
+if (!vpd_findtag(buf, len, "DSV1028VPDR.VER"))
+goto done;
+
+/* Find DCM/DC2 tag */
+dcm = vpd_findtag(buf, len, "DCM");
+if (dcm != NULL)
+dev_pci_npar_dcm(dev, names, dcm->len, dcm->data,
+ "%1x%1x%2x%6x", 10);
+else {
+dcm = vpd_findtag(buf, len, "DC2");
+if (dcm != NULL)
+dev_pci_npar_dcm(dev, names, dcm->len, dcm->data,
+ "%1x%2x%2x%6x", 11);
+}
+done:
+close(fd);
+}
+
 /* retrieve on-board index number and label from firmware */
 static int dev_pci_onboard(struct udev_device *dev, struct netnames *names) {
 unsigned dev_port = 0;
@@ -187,6 +301,78 @@ static bool is_pci_multifunction(struct udev_device *dev) {
 return false;
 }

+enum {
+SLOT_USAGE_OTHER = 1,
+SLOT_USAGE_UNKNOWN,
+SLOT_USAGE_AVAILABLE,
+SLOT_USAGE_INUSE,
+};
+
+struct smbios_type9 {
+

Re: [systemd-devel] Fwd: [PATCH] Add support for detecting NIC partitions on Dell Servers

2015-11-09 Thread Lennart Poettering 
On Mon, 09.11.15 12:42, Jordan Hargrave (jhar...@gmail.com) wrote:

> From: Jordan Hargrave 
> 
> This patch will integrate some of the features of biosdevname into systemd.
> The code detects the port and index for detecting NIC partitions. This creates
> a new environment variable, ID_NET_NAME_PARTITION of the format
> _

"partitions"? What's that supposed to be? SR-IOV?

> The patch will also decode SMBIOS slot number for NIC, and store in
> the variable ID_NET_NAME_SMBIOS_SLOT.  Systemd does not have a method
> for naming ports on a multi-port card plugged into a slot.

Hmm, isn't this stuff the same as exported by the kernel as the
"index" field, i.e. SMBIOS Type 41? Can you elaborate on the relation
of that field and the stuff this patch adds?

> Signed-off-by: Jordan Hargrave 

I didn't look too closely in the actual sources, but I did notice that
it is line-broken, and doesn't follow CODING_STYLE in quite a few
cases, regarding placement of brackets, or error handling for
example. 

In order to avoid any confusion with line-broken patches, and to make
review easier, please submit the patch as github PR, which is the way
we generally prefer receiving patches these days!

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel