Re: [PATCH 08/10] s390/dasd: Display FC Endpoint Security information via sysfs

On Fri,  2 Oct 2020 21:39:38 +0200
Stefan Haberland [off-list ref] wrote:

From: Jan Höppner <hoeppner@linux.ibm.com>

Add a new sysfs attribute (fc_security) per device and per operational
channel path. The information of the current FC Endpoint Security state
is received through the CIO layer.

The state of the FC Endpoint Security can be either "Unsupported",
"Authentication", or "Encryption".

For example:
$ cat /sys/bus/ccw/devices/0.0.c600/fc_security
Encryption

If any of the operational paths is in a state different from all
others, the device sysfs attribute will display the additional state
"Inconsistent".

The sysfs attributes per paths are organised in a new directory called
"paths_info" with subdirectories for each path.

/sys/bus/ccw/devices/0.0.c600/paths_info/
├── 0.38
│   └── fc_security
├── 0.39
│   └── fc_security
├── 0.3a
│   └── fc_security
└── 0.3b
    └── fc_security

Reference-ID: IO1812
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c | 105 +++++++++++++++++++++++++++++++
 drivers/s390/block/dasd_eckd.c   |  30 +++++++++
 drivers/s390/block/dasd_int.h    |  68 ++++++++++++++++++++
 3 files changed, 203 insertions(+)

(...)

+static struct kobj_type path_attr_type = {
+	.release	= dasd_path_release,

This function does nothing; I think there's something wrong with your
kobject handling?

+	.default_attrs	= paths_info_attrs,
+	.sysfs_ops	= &kobj_sysfs_ops,
+};
+
+static void dasd_path_init_kobj(struct dasd_device *device, int chp)
+{
+	device->path[chp].kobj.kset = device->paths_info;
+	kobject_init(&device->path[chp].kobj, &path_attr_type);

This inits a static kobject; as you never free it, doesn't the code
moan about state_initialized if you try to do that a second time?

+}
+
+void dasd_path_create_kobj(struct dasd_device *device, int chp)
+{
+	int rc;
+
+	if (test_bit(DASD_FLAG_OFFLINE, &device->flags))
+		return;
+	if (!device->paths_info) {
+		dev_warn(&device->cdev->dev, "Unable to create paths objects\n");

I guess this warns every time you come along here, is warning more than
once useful?

+		return;
+	}
+	if (device->path[chp].in_sysfs)
+		return;
+	if (!device->path[chp].conf_data)

Out of interest: Have you tried this with vfio-ccw under QEMU, where
some information is simply not available?

+		return;
+
+	dasd_path_init_kobj(device, chp);
+
+	rc = kobject_add(&device->path[chp].kobj, NULL, "%x.%02x",
+			 device->path[chp].cssid, device->path[chp].chpid);
+	if (rc)
+		kobject_put(&device->path[chp].kobj);

This will eventually lead to the nop release function, which doesn't
unset state_initialized (see above) -- but OTOH, it shouldn't muck
around with kobject internals anyway.

I think the kobjects really want to be dynamically allocated; instead
of going through a remove/add cycle, is there a way to make path
objects "invisible" instead? Or add an "available" attribute, and error
out reading any other attribute?

+	device->path[chp].in_sysfs = true;
+}
+EXPORT_SYMBOL(dasd_path_create_kobj);
+
+void dasd_path_create_kobjects(struct dasd_device *device)
+{
+	u8 lpm, opm;
+
+	opm = dasd_path_get_opm(device);
+	for (lpm = 0x80; lpm; lpm >>= 1) {
+		if (!(lpm & opm))
+			continue;

Any reason you do not simply create objects for _all_ paths, combined
with returning n/a or erroring out for paths where this does not apply?
(I might be missing something obvious.)

+		dasd_path_create_kobj(device, pathmask_to_pos(lpm));
+	}
+}
+EXPORT_SYMBOL(dasd_path_create_kobjects);
+
+void dasd_path_remove_kobj(struct dasd_device *device, int chp)
+{
+	if (device->path[chp].in_sysfs) {
+		kobject_put(&device->path[chp].kobj);
+		device->path[chp].in_sysfs = false;
+	}
+}
+EXPORT_SYMBOL(dasd_path_remove_kobj);

Also, how is userspace supposed to deal with changes here? Should there
be a uevent on the parent device to notify about changes?

 
 int dasd_add_sysfs_files(struct ccw_device *cdev)
 {

(...)

+static inline void dasd_path_release(struct kobject *kobj)
+{
+/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
+}
+

As already said, I don't think that's a correct way to implement this.

(...)