Re: [PATCH 16/17] nodedev: add optional device address of channel device to css device

On 5/13/22 12:31, Boris Fiuczynski wrote: > Add the new introduced sysfs attribute dev_busid which provides the address > of the device in the subchannel independent from the bound device driver. > It is added if available in the sysfs as optional channel_dev_addr element into > the css device capabilty providing the ccw deivce address attributes cssid, > ssid and devno. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > src/conf/node_device_conf.c | 26 ++++++++++++++++++++++++++ > src/conf/node_device_conf.h | 2 ++ > src/conf/schemas/nodedev.rng | 5 +++++ > src/node_device/node_device_udev.c | 8 ++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 5aafcbb838..d5ee8da3ee 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -645,6 +645,17 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf, > > virNodeDeviceCapCCWDefFormat(buf, data); > > + if (ccw_dev.channel_dev_addr) { > + virCCWDeviceAddress *ccw = ccw_dev.channel_dev_addr; > + virBufferAddLit(buf, "<channel_dev_addr>\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferAsprintf(buf, "<cssid>0x%x</cssid>\n", ccw->cssid); > + virBufferAsprintf(buf, "<ssid>0x%x</ssid>\n", ccw->ssid); > + virBufferAsprintf(buf, "<devno>0x%04x</devno>\n", ccw->devno); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</channel_dev_addr>\n"); > + } > + > if (ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV) > virNodeDeviceCapMdevTypesFormat(buf, > ccw_dev.mdev_types, > @@ -1257,6 +1268,8 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt, > g_autofree xmlNodePtr *nodes = NULL; > int n = 0; > size_t i = 0; > + xmlNodePtr channel_ddno = NULL; > + g_autofree virCCWDeviceAddress *channel_dev = NULL; > > ctxt->node = node; > > @@ -1271,6 +1284,19 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt, > return -1; > } > > + // channel_dev_addr is optional c89 style of comments please :-) > + if ((channel_ddno = virXPathNode("./channel_dev_addr[1]", ctxt))) { > + channel_dev = g_new0(virCCWDeviceAddress, 1); This variable is not used anywhere else but in this block. It may be worth declaring it within.

> + > + if (virNodeDevCCWDeviceAddressParseXML(ctxt, > + channel_ddno, > + def->name, > + channel_dev) < 0) > + return -1; > + > + ccw_dev->channel_dev_addr = g_steal_pointer(&channel_dev); So this steals allocation from passed variable. But corresponding free() call in virNodeDevCapsDefFree() is missing.

> + } > + > return 0; > } > > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index e4d1f67d53..d1751ed874 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -24,6 +24,7 @@ > > #include "internal.h" > #include "virbitmap.h" > +#include "virccw.h" > #include "virpcivpd.h" > #include "virscsihost.h" > #include "virpci.h" > @@ -279,6 +280,7 @@ struct _virNodeDevCapCCW { > unsigned int flags; /* enum virNodeDevCCWCapFlags */ > virMediatedDeviceType **mdev_types; > size_t nmdev_types; > + virCCWDeviceAddress *channel_dev_addr; > }; > > typedef struct _virNodeDevCapVDPA virNodeDevCapVDPA; > diff --git a/src/conf/schemas/nodedev.rng b/src/conf/schemas/nodedev.rng > index a9f83e048c..e40243e257 100644 > --- a/src/conf/schemas/nodedev.rng > +++ b/src/conf/schemas/nodedev.rng > @@ -682,6 +682,11 @@ > <value>css</value> > </attribute> > <ref name="capccwaddress"/> > + <optional> > + <element name="channel_dev_addr"> > + <ref name="capccwaddress"/> > + </element> > + </optional> > <optional> > <ref name="mdev_types"/> > </optional> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c > index 4bb841c66b..16314627ca 100644 > --- a/src/node_device/node_device_udev.c > +++ b/src/node_device/node_device_udev.c > @@ -1124,6 +1124,8 @@ static int > udevProcessCSS(struct udev_device *device, > virNodeDeviceDef *def) > { > + char *dev_busid; > + > /* only process IO subchannel and vfio-ccw devices to keep the list sane */ > if (!def->driver || > (STRNEQ(def->driver, "io_subchannel") && > @@ -1135,6 +1137,12 @@ udevProcessCSS(struct udev_device *device, > > udevGenerateDeviceName(device, def, NULL); > > + /* process optional channel devices information */ > + udevGetStringSysfsAttr(device, "dev_busid", &dev_busid); This allocates dev_busid, which is never freed. it's sufficient to use g_autofree for the variable. Looking into the kernel sources, we may either get proper address here or "none": https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/d... Is it worth to bother with "none" string here?

> + > + if (dev_busid != NULL) > + def->caps->data.ccw_dev.channel_dev_addr = virCCWDeviceAddressFromString(dev_busid); > + > if (virNodeDeviceGetCSSDynamicCaps(def->sysfs_path, &def->caps->data.ccw_dev) < 0) > return -1; > Michal