[PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Marcos Paulo de Souza]

WWID composed from VPD data from device, specifically page 0x83. So,
when a device does not have VPD support, for example USB storage devices
where VPD is specifically disabled, a read into <blk device>/device/wwid
file will always return ENXIO. To avoid this, change the
scsi_sdev_attr_is_visible function to hide wwid sysfs file when the
devices does not support VPD.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/scsi/scsi_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index dbb206c90ecf..bfd890fa0c69 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1159,6 +1159,9 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct scsi_device *sdev = to_scsi_device(dev);
 
+	/* do not present wwid if the device does not support VPD */
+	if (attr == &dev_attr_wwid.attr && sdev->skip_vpd_pages)
+		return 0;
 
 	if (attr == &dev_attr_queue_depth.attr &&
 	    !sdev->host->hostt->change_queue_depth)
-- 
2.21.0

Re: [PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Marcos Paulo de Souza]

ping? Can anybody take a look at this patch?

Thanks,
Marcos

On Tue, Jun 11, 2019 at 11:08:28PM -0300, Marcos Paulo de Souza wrote:
> WWID composed from VPD data from device, specifically page 0x83. So,
> when a device does not have VPD support, for example USB storage devices
> where VPD is specifically disabled, a read into <blk device>/device/wwid
> file will always return ENXIO. To avoid this, change the
> scsi_sdev_attr_is_visible function to hide wwid sysfs file when the
> devices does not support VPD.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
>  drivers/scsi/scsi_sysfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index dbb206c90ecf..bfd890fa0c69 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1159,6 +1159,9 @@ static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  
> +	/* do not present wwid if the device does not support VPD */
> +	if (attr == &dev_attr_wwid.attr && sdev->skip_vpd_pages)
> +		return 0;
>  
>  	if (attr == &dev_attr_queue_depth.attr &&
>  	    !sdev->host->hostt->change_queue_depth)
> -- 
> 2.21.0
> 

Re: [PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Martin K. Petersen]

Marcos,

> WWID composed from VPD data from device, specifically page 0x83. So,
> when a device does not have VPD support, for example USB storage
> devices where VPD is specifically disabled, a read into <blk
> device>/device/wwid file will always return ENXIO. To avoid this,
> change the scsi_sdev_attr_is_visible function to hide wwid sysfs file
> when the devices does not support VPD.

Not a big fan of attribute files that come and go.

Why not just return an empty string? Hannes?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Hannes Reinecke]

On 6/19/19 5:35 AM, Martin K. Petersen wrote:
> 
> Marcos,
> 
>> WWID composed from VPD data from device, specifically page 0x83. So,
>> when a device does not have VPD support, for example USB storage
>> devices where VPD is specifically disabled, a read into <blk
>> device>/device/wwid file will always return ENXIO. To avoid this,
>> change the scsi_sdev_attr_is_visible function to hide wwid sysfs file
>> when the devices does not support VPD.
> 
> Not a big fan of attribute files that come and go.
> 
> Why not just return an empty string? Hannes?
> 
Actually, the intention of the 'wwid' attribute was to have a common
place where one could look up the global id.
As such it actually serves a dual purpose, namely indicating that there
_is_ a global ID _and_ that this kernel (version) has support for 'wwid'
attribute. This is to resolve one big issue we have to udev nowadays,
which is figuring out if a specific sysfs attribute is actually
supported on this particular kernel.
Dynamic attributes are 'nicer' on a conceptual level, but make the above
test nearly impossible, as we now have _two_ possibilities why a
specific attribute is not present.
So making 'wwid' conditional would actually defeat its very purpose, and
we should leave it blank if not supported.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Marcos Paulo de Souza]

On Wed, Jun 19, 2019 at 08:34:56AM +0200, Hannes Reinecke wrote:
> On 6/19/19 5:35 AM, Martin K. Petersen wrote:
> > 
> > Marcos,
> > 
> >> WWID composed from VPD data from device, specifically page 0x83. So,
> >> when a device does not have VPD support, for example USB storage
> >> devices where VPD is specifically disabled, a read into <blk
> >> device>/device/wwid file will always return ENXIO. To avoid this,
> >> change the scsi_sdev_attr_is_visible function to hide wwid sysfs file
> >> when the devices does not support VPD.
> > 
> > Not a big fan of attribute files that come and go.
> > 
> > Why not just return an empty string? Hannes?
> > 
> Actually, the intention of the 'wwid' attribute was to have a common
> place where one could look up the global id.
> As such it actually serves a dual purpose, namely indicating that there
> _is_ a global ID _and_ that this kernel (version) has support for 'wwid'
> attribute. This is to resolve one big issue we have to udev nowadays,
> which is figuring out if a specific sysfs attribute is actually
> supported on this particular kernel.
> Dynamic attributes are 'nicer' on a conceptual level, but make the above
> test nearly impossible, as we now have _two_ possibilities why a
> specific attribute is not present.
> So making 'wwid' conditional would actually defeat its very purpose, and
> we should leave it blank if not supported.

My intention was to apply the same approach used for VPD pages, which currently
also hides the attributes if not supported by the device. So, if vpd pages are
hidden, there is no usage for wwid. But I also like the idea of the vpd pages
being blank if not supported by the device.

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		               zSeries & Storage
> hare@suse.com			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: scsi_sysfs.c: Hide wwid sdev attr if VPD is not supported

[Hannes Reinecke]

On 6/19/19 11:52 AM, Marcos Paulo de Souza wrote:
> On Wed, Jun 19, 2019 at 08:34:56AM +0200, Hannes Reinecke wrote:
>> On 6/19/19 5:35 AM, Martin K. Petersen wrote:
>>>
>>> Marcos,
>>>
>>>> WWID composed from VPD data from device, specifically page 0x83. So,
>>>> when a device does not have VPD support, for example USB storage
>>>> devices where VPD is specifically disabled, a read into <blk
>>>> device>/device/wwid file will always return ENXIO. To avoid this,
>>>> change the scsi_sdev_attr_is_visible function to hide wwid sysfs file
>>>> when the devices does not support VPD.
>>>
>>> Not a big fan of attribute files that come and go.
>>>
>>> Why not just return an empty string? Hannes?
>>>
>> Actually, the intention of the 'wwid' attribute was to have a common
>> place where one could look up the global id.
>> As such it actually serves a dual purpose, namely indicating that there
>> _is_ a global ID _and_ that this kernel (version) has support for 'wwid'
>> attribute. This is to resolve one big issue we have to udev nowadays,
>> which is figuring out if a specific sysfs attribute is actually
>> supported on this particular kernel.
>> Dynamic attributes are 'nicer' on a conceptual level, but make the above
>> test nearly impossible, as we now have _two_ possibilities why a
>> specific attribute is not present.
>> So making 'wwid' conditional would actually defeat its very purpose, and
>> we should leave it blank if not supported.
> 
> My intention was to apply the same approach used for VPD pages, which currently
> also hides the attributes if not supported by the device. So, if vpd pages are
> hidden, there is no usage for wwid. But I also like the idea of the vpd pages
> being blank if not supported by the device.
> 
Not quite.
As outlined above, the non-existence of the vpd sysfs attribute doesn't
automatically imply that the device doesn't support VPD pages; we might
as well running on older kernels simply not supporting VPD pages in sysfs.
The whole idea of the wwid is that the attribute is _always_ present, so
we don't have to out-guess the kernel here; if the kernel supports the
wwid attribute it will be present, full stop.

(Background: we do was to avoid doing I/O from uevents, as the events
are handled asynchronously, so by the time the event is handled the
device might not be accesible anymore, leading to a stuck udev process.)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)