On Fri, 2 Oct 2020 21:39:38 +0200
Stefan Haberland <sth@xxxxxxxxxxxxx> wrote:
> From: Jan Höppner <hoeppner@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Reviewed-by: Stefan Haberland <sth@xxxxxxxxxxxxx>
> Signed-off-by: Stefan Haberland <sth@xxxxxxxxxxxxx>
> ---
> 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.
(...)
