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

Commit Message

Stefan Haberland Oct. 2, 2020, 7:39 p.m. UTC

From: Jan Höppner <[e-mail]>

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 <[e-mail]>
Reviewed-by: Stefan Haberland <[e-mail]>
Signed-off-by: Stefan Haberland <[e-mail]>
---
 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(+)

Comments

Cornelia Huck Oct. 6, 2020, 10:26 a.m. UTC | #1

On Fri,  2 Oct 2020 21:39:38 +0200
Stefan Haberland <[e-mail]> wrote:

> From: Jan Höppner <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Stefan Haberland <[e-mail]>
> Signed-off-by: Stefan Haberland <[e-mail]>
> ---
>  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.

(...)

Jan Höppner Oct. 6, 2020, 4:57 p.m. UTC | #2

On 10/6/20 12:26 PM, Cornelia Huck wrote:
> On Fri,  2 Oct 2020 21:39:38 +0200
> Stefan Haberland <[e-mail]> wrote:
> 
>> From: Jan Höppner <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Stefan Haberland <[e-mail]>
>> Signed-off-by: Stefan Haberland <[e-mail]>
>> ---
>>  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?

Explanation below.

> 
>> +	.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

kobject_put() frees the kobject data.

> moan about state_initialized if you try to do that a second time?

No, because we check whether we have this kobject already present
in sysfs before we try to initialize it (we have in_sysfs per path
object for this).

> 
>> +}
>> +
>> +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?
> 

paths_info is a kset created during the device initialization. Do you mean,
in case the kset creation fails, this check here should only warn once?
I'm not sure about that, hm.

>> +		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?

I did not, sorry.

> 
>> +		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.

The release function is supposed to free memory of the structure where
the kobject lies in (our release function is explained below).
The rest is taking care of by the kobject library.

> 
> 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.)

Because we likely don't have all information required to create the kobject
for other paths, e.g. the cssid and chpid (which are required for the
proper name).

> 
>> +		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?

I can't think of a user just yet. I'll keep this in mind for further
improvements. I certainly won't hurt to create uevents here.

> 
>>  
>>  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.
> 

As you correctly pointed out, our release function doesn't do anything.
This is because our path data is a (static) part of our device.
This data is critical to keep our devices operational.
We can't simply rely on allocated memory if systems are under stress. 

Having this data dynamically allocated involves a lot of rework of our
path handling as well. There are a few things that are subject to improvement
and evaluating whether our dasd_path structures can be dynamic is one of
these things. However, even then, the above concern persists and I
highly doubt that dynamic dasd_paths objects are doable for us at this
moment.

I do understand the concerns, however, we release the memory for dasd_path
structures eventually when dasd_free_device() is called. Until that point,
the data has to be kept alive. The rest is taking care of by the kobject
library.
In our path handling we also make sure that we can always verify/validate
paths information even if a system is under high memory pressure. Another
reason why it would contradictory for dasd_path objects to be dynamic.

I hope this explains the reasoning behind the release function.

so long,
Jan

Cornelia Huck Oct. 7, 2020, 9:49 a.m. UTC | #3

On Tue, 6 Oct 2020 18:57:46 +0200
Jan Höppner <[e-mail]> wrote:

> On 10/6/20 12:26 PM, Cornelia Huck wrote:
> > On Fri,  2 Oct 2020 21:39:38 +0200
> > Stefan Haberland <[e-mail]> wrote:
> >   
> >> From: Jan Höppner <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Reviewed-by: Stefan Haberland <[e-mail]>
> >> Signed-off-by: Stefan Haberland <[e-mail]>
> >> ---
> >>  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?  
> 
> Explanation below.
> 
> >   
> >> +	.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  
> 
> kobject_put() frees the kobject data.

Not quite; if the last ref is put, it invokes the provided ->release
callback and frees the allocated name. If the ->release callback does
not free the object embedding the kobject, only the name is freed
AFAICS.

> 
> > moan about state_initialized if you try to do that a second time?  
> 
> No, because we check whether we have this kobject already present
> in sysfs before we try to initialize it (we have in_sysfs per path
> object for this).

I might be confused by the path revalidation logic; but don't you unset
in_sysfs when you remove the path object? What happens when you readd it?

> 
> >   
> >> +}
> >> +
> >> +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?
> >   
> 
> paths_info is a kset created during the device initialization. Do you mean,
> in case the kset creation fails, this check here should only warn once?
> I'm not sure about that, hm.

If the kset could not be set up during init, you'll hit this for every
path object you want to add afterwards -- one warning per device of
that should be enough, I guess :)

> 
> >> +		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?  
> 
> I did not, sorry.
> 
> >   
> >> +		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.  
> 
> The release function is supposed to free memory of the structure where
> the kobject lies in (our release function is explained below).
> The rest is taking care of by the kobject library.

Yes, but the kobject code does not unset state_initialized.
> 
> > 
> > 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?

> > (...)
> >   
> >> +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.
> >   
> 
> As you correctly pointed out, our release function doesn't do anything.
> This is because our path data is a (static) part of our device.
> This data is critical to keep our devices operational.
> We can't simply rely on allocated memory if systems are under stress. 

Yes, avoiding freeing and reallocating memory certainly makes sense.

> 
> Having this data dynamically allocated involves a lot of rework of our
> path handling as well. There are a few things that are subject to improvement
> and evaluating whether our dasd_path structures can be dynamic is one of
> these things. However, even then, the above concern persists and I
> highly doubt that dynamic dasd_paths objects are doable for us at this
> moment.
> 
> I do understand the concerns, however, we release the memory for dasd_path
> structures eventually when dasd_free_device() is called. Until that point,
> the data has to be kept alive. The rest is taking care of by the kobject
> library.

Yes, there doesn't seem to be any memory leakage.

> In our path handling we also make sure that we can always verify/validate
> paths information even if a system is under high memory pressure. Another
> reason why it would contradictory for dasd_path objects to be dynamic.
> 
> I hope this explains the reasoning behind the release function.

I understand where you're coming from.

However, "static" kobjects (in the sense of "we may re-register the
same kobject") are still problematic. Is there any way to simply
"disappear" path objects that are not valid at the moment, or mark them
as not valid?

Also, the simple act of registering/unregistering a kobject already
creates stress from its sysfs interactions... it seems you should try
to avoid that as well?

Jan Höppner Oct. 7, 2020, 2:33 p.m. UTC | #4

... snip ...
>>>   
>>>> +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?  
>>
>> Explanation below.
>>
>>>   
>>>> +	.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  
>>
>> kobject_put() frees the kobject data.
> 
> Not quite; if the last ref is put, it invokes the provided ->release
> callback and frees the allocated name. If the ->release callback does
> not free the object embedding the kobject, only the name is freed
> AFAICS.
> 

True, the rest is freed when the device is being destroyed with
dasd_free_device().

>>
>>> moan about state_initialized if you try to do that a second time?  
>>
>> No, because we check whether we have this kobject already present
>> in sysfs before we try to initialize it (we have in_sysfs per path
>> object for this).
> 
> I might be confused by the path revalidation logic; but don't you unset
> in_sysfs when you remove the path object? What happens when you readd it?

We set it to true again (See dasd_path_create_kobj()). (More details below)

> 
>>
>>>   
>>>> +}
>>>> +
>>>> +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?
>>>   
>>
>> paths_info is a kset created during the device initialization. Do you mean,
>> in case the kset creation fails, this check here should only warn once?
>> I'm not sure about that, hm.
> 
> If the kset could not be set up during init, you'll hit this for every
> path object you want to add afterwards -- one warning per device of
> that should be enough, I guess :)

I think this could be changed to one warning.

> 
>>
>>>> +		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?  
>>
>> I did not, sorry.
>>
>>>   
>>>> +		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.  
>>
>> The release function is supposed to free memory of the structure where
>> the kobject lies in (our release function is explained below).
>> The rest is taking care of by the kobject library.
> 
> Yes, but the kobject code does not unset state_initialized.
>>
>>>
>>> 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?
> 
>>> (...)
>>>   
>>>> +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.
>>>   
>>
>> As you correctly pointed out, our release function doesn't do anything.
>> This is because our path data is a (static) part of our device.
>> This data is critical to keep our devices operational.
>> We can't simply rely on allocated memory if systems are under stress. 
> 
> Yes, avoiding freeing and reallocating memory certainly makes sense.
> 
>>
>> Having this data dynamically allocated involves a lot of rework of our
>> path handling as well. There are a few things that are subject to improvement
>> and evaluating whether our dasd_path structures can be dynamic is one of
>> these things. However, even then, the above concern persists and I
>> highly doubt that dynamic dasd_paths objects are doable for us at this
>> moment.
>>
>> I do understand the concerns, however, we release the memory for dasd_path
>> structures eventually when dasd_free_device() is called. Until that point,
>> the data has to be kept alive. The rest is taking care of by the kobject
>> library.
> 
> Yes, there doesn't seem to be any memory leakage.
> 
>> In our path handling we also make sure that we can always verify/validate
>> paths information even if a system is under high memory pressure. Another
>> reason why it would contradictory for dasd_path objects to be dynamic.
>>
>> I hope this explains the reasoning behind the release function.
> 
> I understand where you're coming from.
> 
> However, "static" kobjects (in the sense of "we may re-register the
> same kobject") are still problematic. Is there any way to simply
> "disappear" path objects that are not valid at the moment, or mark them
> as not valid?

You could use kobject_del(), but it is rather intended to be used for
a two-stage removal of the kobject.

> 
> Also, the simple act of registering/unregistering a kobject already
> creates stress from its sysfs interactions... it seems you should try
> to avoid that as well?
> 

We don't re-register kobjects over and over again. The kobjects are
infact initialized and created only _once_. This is done either during
device initialization (after dasd_eckd_read_conf() in
dasd_eckd_check_characteristics()) or when a path is newly added
(in the path event handler).
The kobject will stay until the memory for the whole device is being
freed. This is also the reason why the kobject can stay initialized and
we track ourselves whether we did the initialization/creation already
(which we check e.g. when a path is removed and added again).
So, instead of the release function freeing the kobject data,
it is done by our dasd_free_device() (same thing, different function IMHO).

I think the concerns would be more worrisome if we'd remove/add
the kobjects every time. And then I agree, we'd run into trouble.

Cornelia Huck Oct. 7, 2020, 4:40 p.m. UTC | #5

On Wed, 7 Oct 2020 16:33:37 +0200
Jan Höppner <[e-mail]> wrote:

> >>>> +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.
> >>>     
> >>
> >> As you correctly pointed out, our release function doesn't do anything.
> >> This is because our path data is a (static) part of our device.
> >> This data is critical to keep our devices operational.
> >> We can't simply rely on allocated memory if systems are under stress.   
> > 
> > Yes, avoiding freeing and reallocating memory certainly makes sense.
> >   
> >>
> >> Having this data dynamically allocated involves a lot of rework of our
> >> path handling as well. There are a few things that are subject to improvement
> >> and evaluating whether our dasd_path structures can be dynamic is one of
> >> these things. However, even then, the above concern persists and I
> >> highly doubt that dynamic dasd_paths objects are doable for us at this
> >> moment.
> >>
> >> I do understand the concerns, however, we release the memory for dasd_path
> >> structures eventually when dasd_free_device() is called. Until that point,
> >> the data has to be kept alive. The rest is taking care of by the kobject
> >> library.  
> > 
> > Yes, there doesn't seem to be any memory leakage.
> >   
> >> In our path handling we also make sure that we can always verify/validate
> >> paths information even if a system is under high memory pressure. Another
> >> reason why it would contradictory for dasd_path objects to be dynamic.
> >>
> >> I hope this explains the reasoning behind the release function.  
> > 
> > I understand where you're coming from.
> > 
> > However, "static" kobjects (in the sense of "we may re-register the
> > same kobject") are still problematic. Is there any way to simply
> > "disappear" path objects that are not valid at the moment, or mark them
> > as not valid?  
> 
> You could use kobject_del(), but it is rather intended to be used for
> a two-stage removal of the kobject.
> 
> > 
> > Also, the simple act of registering/unregistering a kobject already
> > creates stress from its sysfs interactions... it seems you should try
> > to avoid that as well?
> >   
> 
> We don't re-register kobjects over and over again. The kobjects are
> infact initialized and created only _once_. This is done either during
> device initialization (after dasd_eckd_read_conf() in
> dasd_eckd_check_characteristics()) or when a path is newly added
> (in the path event handler).
> The kobject will stay until the memory for the whole device is being
> freed. This is also the reason why the kobject can stay initialized and
> we track ourselves whether we did the initialization/creation already
> (which we check e.g. when a path is removed and added again).
> So, instead of the release function freeing the kobject data,
> it is done by our dasd_free_device() (same thing, different function IMHO).
> 
> I think the concerns would be more worrisome if we'd remove/add
> the kobjects every time. And then I agree, we'd run into trouble.
> 

The thing that tripped me is

+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;
+	}
+}

As an exported function, it is not clear where this may be called from.
Given your explanation above (and some more code reading on my side),
the code looks ok in its current incarnation (but non-idiomatic).

Is there a way to check that indeed nobody re-adds a previously removed
path object due to a (future) programming error? And maybe add a
comment that you must never re-register a path? "The path is gone,
let's remove the object" looks quite tempting.

Jan Höppner Oct. 7, 2020, 8:10 p.m. UTC | #6

On 10/7/20 6:40 PM, Cornelia Huck wrote:
> On Wed, 7 Oct 2020 16:33:37 +0200
> Jan Höppner <[e-mail]> wrote:
> 
>>>>>> +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.
>>>>>     
>>>>
>>>> As you correctly pointed out, our release function doesn't do anything.
>>>> This is because our path data is a (static) part of our device.
>>>> This data is critical to keep our devices operational.
>>>> We can't simply rely on allocated memory if systems are under stress.   
>>>
>>> Yes, avoiding freeing and reallocating memory certainly makes sense.
>>>   
>>>>
>>>> Having this data dynamically allocated involves a lot of rework of our
>>>> path handling as well. There are a few things that are subject to improvement
>>>> and evaluating whether our dasd_path structures can be dynamic is one of
>>>> these things. However, even then, the above concern persists and I
>>>> highly doubt that dynamic dasd_paths objects are doable for us at this
>>>> moment.
>>>>
>>>> I do understand the concerns, however, we release the memory for dasd_path
>>>> structures eventually when dasd_free_device() is called. Until that point,
>>>> the data has to be kept alive. The rest is taking care of by the kobject
>>>> library.  
>>>
>>> Yes, there doesn't seem to be any memory leakage.
>>>   
>>>> In our path handling we also make sure that we can always verify/validate
>>>> paths information even if a system is under high memory pressure. Another
>>>> reason why it would contradictory for dasd_path objects to be dynamic.
>>>>
>>>> I hope this explains the reasoning behind the release function.  
>>>
>>> I understand where you're coming from.
>>>
>>> However, "static" kobjects (in the sense of "we may re-register the
>>> same kobject") are still problematic. Is there any way to simply
>>> "disappear" path objects that are not valid at the moment, or mark them
>>> as not valid?  
>>
>> You could use kobject_del(), but it is rather intended to be used for
>> a two-stage removal of the kobject.
>>
>>>
>>> Also, the simple act of registering/unregistering a kobject already
>>> creates stress from its sysfs interactions... it seems you should try
>>> to avoid that as well?
>>>   
>>
>> We don't re-register kobjects over and over again. The kobjects are
>> infact initialized and created only _once_. This is done either during
>> device initialization (after dasd_eckd_read_conf() in
>> dasd_eckd_check_characteristics()) or when a path is newly added
>> (in the path event handler).
>> The kobject will stay until the memory for the whole device is being
>> freed. This is also the reason why the kobject can stay initialized and
>> we track ourselves whether we did the initialization/creation already
>> (which we check e.g. when a path is removed and added again).
>> So, instead of the release function freeing the kobject data,
>> it is done by our dasd_free_device() (same thing, different function IMHO).
>>
>> I think the concerns would be more worrisome if we'd remove/add
>> the kobjects every time. And then I agree, we'd run into trouble.
>>
> 
> The thing that tripped me is
> 
> +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;
> +	}
> +}
> 
> As an exported function, it is not clear where this may be called from.
> Given your explanation above (and some more code reading on my side),
> the code looks ok in its current incarnation (but non-idiomatic).
> 
> Is there a way to check that indeed nobody re-adds a previously removed
> path object due to a (future) programming error? And maybe add a
> comment that you must never re-register a path? "The path is gone,
> let's remove the object" looks quite tempting.
> 

A comment is the minimum I can think of at the moment and
I'll prepare a fixup patch or a new version of this patch that adds
a proper comment for this function.
Other ways to protect the usage must be investigated. 
I have to discuss with Stefan what the best approach would be as the patchset
is supposed to be ready for upstream integration.

I'd prefer a fixup patch that we could send with at least one more fixup patch
that we have in the pipe already. Let's see. I hope that's fine with you
(and Jens obviously) so far.

regards,
Jan

Cornelia Huck Oct. 8, 2020, 7:03 a.m. UTC | #7

On Wed, 7 Oct 2020 22:10:11 +0200
Jan Höppner <[e-mail]> wrote:

> On 10/7/20 6:40 PM, Cornelia Huck wrote:
> > On Wed, 7 Oct 2020 16:33:37 +0200
> > Jan Höppner <[e-mail]> wrote:
> >   
> >>>>>> +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.
> >>>>>       
> >>>>
> >>>> As you correctly pointed out, our release function doesn't do anything.
> >>>> This is because our path data is a (static) part of our device.
> >>>> This data is critical to keep our devices operational.
> >>>> We can't simply rely on allocated memory if systems are under stress.     
> >>>
> >>> Yes, avoiding freeing and reallocating memory certainly makes sense.
> >>>     
> >>>>
> >>>> Having this data dynamically allocated involves a lot of rework of our
> >>>> path handling as well. There are a few things that are subject to improvement
> >>>> and evaluating whether our dasd_path structures can be dynamic is one of
> >>>> these things. However, even then, the above concern persists and I
> >>>> highly doubt that dynamic dasd_paths objects are doable for us at this
> >>>> moment.
> >>>>
> >>>> I do understand the concerns, however, we release the memory for dasd_path
> >>>> structures eventually when dasd_free_device() is called. Until that point,
> >>>> the data has to be kept alive. The rest is taking care of by the kobject
> >>>> library.    
> >>>
> >>> Yes, there doesn't seem to be any memory leakage.
> >>>     
> >>>> In our path handling we also make sure that we can always verify/validate
> >>>> paths information even if a system is under high memory pressure. Another
> >>>> reason why it would contradictory for dasd_path objects to be dynamic.
> >>>>
> >>>> I hope this explains the reasoning behind the release function.    
> >>>
> >>> I understand where you're coming from.
> >>>
> >>> However, "static" kobjects (in the sense of "we may re-register the
> >>> same kobject") are still problematic. Is there any way to simply
> >>> "disappear" path objects that are not valid at the moment, or mark them
> >>> as not valid?    
> >>
> >> You could use kobject_del(), but it is rather intended to be used for
> >> a two-stage removal of the kobject.
> >>  
> >>>
> >>> Also, the simple act of registering/unregistering a kobject already
> >>> creates stress from its sysfs interactions... it seems you should try
> >>> to avoid that as well?
> >>>     
> >>
> >> We don't re-register kobjects over and over again. The kobjects are
> >> infact initialized and created only _once_. This is done either during
> >> device initialization (after dasd_eckd_read_conf() in
> >> dasd_eckd_check_characteristics()) or when a path is newly added
> >> (in the path event handler).
> >> The kobject will stay until the memory for the whole device is being
> >> freed. This is also the reason why the kobject can stay initialized and
> >> we track ourselves whether we did the initialization/creation already
> >> (which we check e.g. when a path is removed and added again).
> >> So, instead of the release function freeing the kobject data,
> >> it is done by our dasd_free_device() (same thing, different function IMHO).
> >>
> >> I think the concerns would be more worrisome if we'd remove/add
> >> the kobjects every time. And then I agree, we'd run into trouble.
> >>  
> > 
> > The thing that tripped me is
> > 
> > +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;
> > +	}
> > +}
> > 
> > As an exported function, it is not clear where this may be called from.
> > Given your explanation above (and some more code reading on my side),
> > the code looks ok in its current incarnation (but non-idiomatic).
> > 
> > Is there a way to check that indeed nobody re-adds a previously removed
> > path object due to a (future) programming error? And maybe add a
> > comment that you must never re-register a path? "The path is gone,
> > let's remove the object" looks quite tempting.
> >   
> 
> A comment is the minimum I can think of at the moment and
> I'll prepare a fixup patch or a new version of this patch that adds
> a proper comment for this function.
> Other ways to protect the usage must be investigated. 
> I have to discuss with Stefan what the best approach would be as the patchset
> is supposed to be ready for upstream integration.
> 
> I'd prefer a fixup patch that we could send with at least one more fixup patch
> that we have in the pipe already. Let's see. I hope that's fine with you
> (and Jens obviously) so far.

Fine with me. I don't really have a horse in that race; I just wanted
to look at this from a vfio-ccw perspective and then stumbled over the
kobject handling...

Stefan Haberland Oct. 8, 2020, 11:04 a.m. UTC | #8

Am 08.10.20 um 09:03 schrieb Cornelia Huck:
> On Wed, 7 Oct 2020 22:10:11 +0200
> Jan Höppner <[e-mail]> wrote:
>
>> On 10/7/20 6:40 PM, Cornelia Huck wrote:
>>> On Wed, 7 Oct 2020 16:33:37 +0200
>>> Jan Höppner <[e-mail]> wrote:
>>>   
>>>>>>>> +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.
>>>>>>>       
>>>>>> As you correctly pointed out, our release function doesn't do anything.
>>>>>> This is because our path data is a (static) part of our device.
>>>>>> This data is critical to keep our devices operational.
>>>>>> We can't simply rely on allocated memory if systems are under stress.     
>>>>> Yes, avoiding freeing and reallocating memory certainly makes sense.
>>>>>     
>>>>>> Having this data dynamically allocated involves a lot of rework of our
>>>>>> path handling as well. There are a few things that are subject to improvement
>>>>>> and evaluating whether our dasd_path structures can be dynamic is one of
>>>>>> these things. However, even then, the above concern persists and I
>>>>>> highly doubt that dynamic dasd_paths objects are doable for us at this
>>>>>> moment.
>>>>>>
>>>>>> I do understand the concerns, however, we release the memory for dasd_path
>>>>>> structures eventually when dasd_free_device() is called. Until that point,
>>>>>> the data has to be kept alive. The rest is taking care of by the kobject
>>>>>> library.    
>>>>> Yes, there doesn't seem to be any memory leakage.
>>>>>     
>>>>>> In our path handling we also make sure that we can always verify/validate
>>>>>> paths information even if a system is under high memory pressure. Another
>>>>>> reason why it would contradictory for dasd_path objects to be dynamic.
>>>>>>
>>>>>> I hope this explains the reasoning behind the release function.    
>>>>> I understand where you're coming from.
>>>>>
>>>>> However, "static" kobjects (in the sense of "we may re-register the
>>>>> same kobject") are still problematic. Is there any way to simply
>>>>> "disappear" path objects that are not valid at the moment, or mark them
>>>>> as not valid?    
>>>> You could use kobject_del(), but it is rather intended to be used for
>>>> a two-stage removal of the kobject.
>>>>  
>>>>> Also, the simple act of registering/unregistering a kobject already
>>>>> creates stress from its sysfs interactions... it seems you should try
>>>>> to avoid that as well?
>>>>>     
>>>> We don't re-register kobjects over and over again. The kobjects are
>>>> infact initialized and created only _once_. This is done either during
>>>> device initialization (after dasd_eckd_read_conf() in
>>>> dasd_eckd_check_characteristics()) or when a path is newly added
>>>> (in the path event handler).
>>>> The kobject will stay until the memory for the whole device is being
>>>> freed. This is also the reason why the kobject can stay initialized and
>>>> we track ourselves whether we did the initialization/creation already
>>>> (which we check e.g. when a path is removed and added again).
>>>> So, instead of the release function freeing the kobject data,
>>>> it is done by our dasd_free_device() (same thing, different function IMHO).
>>>>
>>>> I think the concerns would be more worrisome if we'd remove/add
>>>> the kobjects every time. And then I agree, we'd run into trouble.
>>>>  
>>> The thing that tripped me is
>>>
>>> +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;
>>> +	}
>>> +}
>>>
>>> As an exported function, it is not clear where this may be called from.
>>> Given your explanation above (and some more code reading on my side),
>>> the code looks ok in its current incarnation (but non-idiomatic).
>>>
>>> Is there a way to check that indeed nobody re-adds a previously removed
>>> path object due to a (future) programming error? And maybe add a
>>> comment that you must never re-register a path? "The path is gone,
>>> let's remove the object" looks quite tempting.
>>>   
>> A comment is the minimum I can think of at the moment and
>> I'll prepare a fixup patch or a new version of this patch that adds
>> a proper comment for this function.
>> Other ways to protect the usage must be investigated. 
>> I have to discuss with Stefan what the best approach would be as the patchset
>> is supposed to be ready for upstream integration.
>>
>> I'd prefer a fixup patch that we could send with at least one more fixup patch
>> that we have in the pipe already. Let's see. I hope that's fine with you
>> (and Jens obviously) so far.
> Fine with me. I don't really have a horse in that race; I just wanted
> to look at this from a vfio-ccw perspective and then stumbled over the
> kobject handling...
>

Thanks for this. I will send a v2 shortly.

Patch

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 32fc51341d99..d4af087ecfe7 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -576,6 +576,11 @@  dasd_create_device(struct ccw_device *cdev)
 	dev_set_drvdata(&cdev->dev, device);
 	spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
 
+	device->paths_info = kset_create_and_add("paths_info", NULL,
+						 &device->cdev->dev.kobj);
+	if (!device->paths_info)
+		dev_warn(&cdev->dev, "Could not create paths_info kset\n");
+
 	return device;
 }
 
@@ -622,6 +627,9 @@  dasd_delete_device(struct dasd_device *device)
 	wait_event(dasd_delete_wq, atomic_read(&device->ref_count) == 0);
 
 	dasd_generic_free_discipline(device);
+
+	kset_unregister(device->paths_info);
+
 	/* Disconnect dasd_device structure from ccw_device structure. */
 	cdev = device->cdev;
 	device->cdev = NULL;
@@ -1641,6 +1649,39 @@  dasd_path_interval_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(path_interval, 0644, dasd_path_interval_show,
 		   dasd_path_interval_store);
 
+static ssize_t
+dasd_device_fcs_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct dasd_device *device;
+	int fc_sec;
+	int rc;
+
+	device = dasd_device_from_cdev(to_ccwdev(dev));
+	if (IS_ERR(device))
+		return -ENODEV;
+	fc_sec = dasd_path_get_fcs_device(device);
+	if (fc_sec == -EINVAL)
+		rc = snprintf(buf, PAGE_SIZE, "Inconsistent\n");
+	else
+		rc = snprintf(buf, PAGE_SIZE, "%s\n", dasd_path_get_fcs_str(fc_sec));
+	dasd_put_device(device);
+
+	return rc;
+}
+static DEVICE_ATTR(fc_security, 0444, dasd_device_fcs_show, NULL);
+
+static ssize_t
+dasd_path_fcs_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct dasd_path *path = to_dasd_path(kobj);
+	unsigned int fc_sec = path->fc_security;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", dasd_path_get_fcs_str(fc_sec));
+}
+
+static struct kobj_attribute path_fcs_attribute =
+	__ATTR(fc_security, 0444, dasd_path_fcs_show, NULL);
 
 #define DASD_DEFINE_ATTR(_name, _func)					\
 static ssize_t dasd_##_name##_show(struct device *dev,			\
@@ -1697,6 +1738,7 @@  static struct attribute * dasd_attrs[] = {
 	&dev_attr_path_reset.attr,
 	&dev_attr_hpf.attr,
 	&dev_attr_ese.attr,
+	&dev_attr_fc_security.attr,
 	NULL,
 };
 
@@ -1777,6 +1819,69 @@  dasd_set_feature(struct ccw_device *cdev, int feature, int flag)
 }
 EXPORT_SYMBOL(dasd_set_feature);
 
+static struct attribute *paths_info_attrs[] = {
+	&path_fcs_attribute.attr,
+	NULL,
+};
+
+static struct kobj_type path_attr_type = {
+	.release	= dasd_path_release,
+	.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);
+}
+
+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");
+		return;
+	}
+	if (device->path[chp].in_sysfs)
+		return;
+	if (!device->path[chp].conf_data)
+		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);
+	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;
+		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);
 
 int dasd_add_sysfs_files(struct ccw_device *cdev)
 {
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 271768e4606c..63061ff5ecab 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1030,6 +1030,30 @@  static void dasd_eckd_clear_conf_data(struct dasd_device *device)
 		device->path[i].ssid = 0;
 		device->path[i].chpid = 0;
 		dasd_path_notoper(device, i);
+		dasd_path_remove_kobj(device, i);
+	}
+}
+
+static void dasd_eckd_read_fc_security(struct dasd_device *device)
+{
+	struct dasd_eckd_private *private = device->private;
+	u8 esm_valid;
+	u8 esm[8];
+	int chp;
+	int rc;
+
+	rc = chsc_scud(private->uid.ssid, (u64 *)esm, &esm_valid);
+	if (rc) {
+		for (chp = 0; chp < 8; chp++)
+			device->path[chp].fc_security = 0;
+		return;
+	}
+
+	for (chp = 0; chp < 8; chp++) {
+		if (esm_valid & (0x80 >> chp))
+			device->path[chp].fc_security = esm[chp];
+		else
+			device->path[chp].fc_security = 0;
 	}
 }
 
@@ -1159,6 +1183,8 @@  static int dasd_eckd_read_conf(struct dasd_device *device)
 		}
 	}
 
+	dasd_eckd_read_fc_security(device);
+
 	return path_err;
 }
 
@@ -1425,6 +1451,8 @@  static void do_path_verification_work(struct work_struct *work)
 		dasd_path_add_cablepm(device, cablepm);
 		dasd_path_add_nohpfpm(device, hpfpm);
 		spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
+
+		dasd_path_create_kobj(device, pos);
 	}
 	clear_bit(DASD_FLAG_PATH_VERIFY, &device->flags);
 	dasd_put_device(device);
@@ -2064,6 +2092,8 @@  dasd_eckd_check_characteristics(struct dasd_device *device)
 	if (rc)
 		goto out_err3;
 
+	dasd_path_create_kobjects(device);
+
 	/* Read Feature Codes */
 	dasd_eckd_read_features(device);
 
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 6dce9d4c55ce..93d9cc938924 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -426,6 +426,35 @@  extern struct dasd_discipline *dasd_diag_discipline_pointer;
 #define DASD_THRHLD_MAX		4294967295U
 #define DASD_INTERVAL_MAX	4294967295U
 
+/* FC Endpoint Security Capabilities */
+#define DASD_FC_SECURITY_UNSUP		0
+#define DASD_FC_SECURITY_AUTH		1
+#define DASD_FC_SECURITY_ENC_FCSP2	2
+#define DASD_FC_SECURITY_ENC_ERAS	3
+
+#define DASD_FC_SECURITY_ENC_STR	"Encryption"
+static const struct {
+	u8 value;
+	char *name;
+} dasd_path_fcs_mnemonics[] = {
+	{ DASD_FC_SECURITY_UNSUP,	"Unsupported" },
+	{ DASD_FC_SECURITY_AUTH,	"Authentication" },
+	{ DASD_FC_SECURITY_ENC_FCSP2,	DASD_FC_SECURITY_ENC_STR },
+	{ DASD_FC_SECURITY_ENC_ERAS,	DASD_FC_SECURITY_ENC_STR },
+};
+
+static inline char *dasd_path_get_fcs_str(int val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dasd_path_fcs_mnemonics); i++) {
+		if (dasd_path_fcs_mnemonics[i].value == val)
+			return dasd_path_fcs_mnemonics[i].name;
+	}
+
+	return dasd_path_fcs_mnemonics[0].name;
+}
+
 struct dasd_path {
 	unsigned long flags;
 	u8 cssid;
@@ -434,8 +463,18 @@  struct dasd_path {
 	struct dasd_conf_data *conf_data;
 	atomic_t error_count;
 	unsigned long errorclk;
+	u8 fc_security;
+	struct kobject kobj;
+	bool in_sysfs;
 };
 
+#define to_dasd_path(path) container_of(path, struct dasd_path, kobj)
+
+static inline void dasd_path_release(struct kobject *kobj)
+{
+/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
+}
+
 
 struct dasd_profile_info {
 	/* legacy part of profile data, as in dasd_profile_info_t */
@@ -547,6 +586,7 @@  struct dasd_device {
 	struct dentry *hosts_dentry;
 	struct dasd_profile profile;
 	struct dasd_format_entry format_entry;
+	struct kset *paths_info;
 };
 
 struct dasd_block {
@@ -824,6 +864,9 @@  int dasd_set_feature(struct ccw_device *, int, int);
 
 int dasd_add_sysfs_files(struct ccw_device *);
 void dasd_remove_sysfs_files(struct ccw_device *);
+void dasd_path_create_kobj(struct dasd_device *, int);
+void dasd_path_create_kobjects(struct dasd_device *);
+void dasd_path_remove_kobj(struct dasd_device *, int);
 
 struct dasd_device *dasd_device_from_cdev(struct ccw_device *);
 struct dasd_device *dasd_device_from_cdev_locked(struct ccw_device *);
@@ -1113,6 +1156,31 @@  static inline __u8 dasd_path_get_hpfpm(struct dasd_device *device)
 	return hpfpm;
 }
 
+static inline u8 dasd_path_get_fcs_path(struct dasd_device *device, int chp)
+{
+	return device->path[chp].fc_security;
+}
+
+static inline int dasd_path_get_fcs_device(struct dasd_device *device)
+{
+	u8 fc_sec = 0;
+	int chp;
+
+	for (chp = 0; chp < 8; chp++) {
+		if (device->opm & (0x80 >> chp)) {
+			fc_sec = device->path[chp].fc_security;
+			break;
+		}
+	}
+	for (; chp < 8; chp++) {
+		if (device->opm & (0x80 >> chp))
+			if (device->path[chp].fc_security != fc_sec)
+				return -EINVAL;
+	}
+
+	return fc_sec;
+}
+
 /*
  * add functions for path masks
  * the existing path mask will be extended by the given path mask