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

[Michal Prívozník]

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 at linux.ibm.com>
> ---
>  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/drivers/s390/cio/css.c#n433

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