Re: [patch 2/3] libata: expose AN support to user space via sysfs

Re: [patch 2/3] libata: expose AN support to user space via sysfs

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Allow user space to determine if an ATAPI device supports
> async notification (AN) of media changes.  This is done by
> adding a new sysfs file "async_notification" to genhd.
> If the file reads 1, then the device supports async 
> notification.  If the file reads 0, it does not.  
> 
> A flag is set in the generic disk to indicate whether
> or not AN is supported.  This flag is set by the SCSI
> subsystem when it registers with add_disk.  The SCSI
> system gets information from libata on whether the
> device supports AN during dev_configure. 
> 
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Index: 2.6-mm/block/genhd.c
> ===================================================================
> --- 2.6-mm.orig/block/genhd.c
> +++ 2.6-mm/block/genhd.c
> @@ -372,6 +372,11 @@ static ssize_t disk_size_read(struct gen
>  {
>  	return sprintf(page, "%llu\n", (unsigned long long)get_capacity(disk));
>  }
> +static ssize_t disk_AN_read(struct gendisk * disk, char *page)
> +{
> +	return sprintf(page, "%d\n",
> +		(disk->flags & GENHD_FL_ASYNC_NOTIFICATION ? 1 : 0));
> +}
>  
>  static ssize_t disk_stats_read(struct gendisk * disk, char *page)
>  {
> @@ -419,6 +424,10 @@ static struct disk_attribute disk_attr_s
>  	.attr = {.name = "stat", .mode = S_IRUGO },
>  	.show	= disk_stats_read
>  };
> +static struct disk_attribute disk_attr_AN = {
> +	.attr = {.name = "media_change_events", .mode = S_IRUGO },
> +	.show	= disk_AN_read
> +};
>  
>  #ifdef CONFIG_FAIL_MAKE_REQUEST
>  
> @@ -455,6 +464,7 @@ static struct attribute * default_attrs[
>  	&disk_attr_removable.attr,
>  	&disk_attr_size.attr,
>  	&disk_attr_stat.attr,
> +	&disk_attr_AN.attr,
>  #ifdef CONFIG_FAIL_MAKE_REQUEST
>  	&disk_attr_fail.attr,
>  #endif
> Index: 2.6-mm/include/linux/genhd.h
> ===================================================================
> --- 2.6-mm.orig/include/linux/genhd.h
> +++ 2.6-mm/include/linux/genhd.h
> @@ -94,6 +94,7 @@ struct hd_struct {
>  
>  #define GENHD_FL_REMOVABLE			1
>  #define GENHD_FL_DRIVERFS			2
> +#define GENHD_FL_ASYNC_NOTIFICATION		4
>  #define GENHD_FL_CD				8
>  #define GENHD_FL_UP				16
>  #define GENHD_FL_SUPPRESS_PARTITION_INFO	32
> Index: 2.6-mm/include/scsi/scsi_device.h
> ===================================================================
> --- 2.6-mm.orig/include/scsi/scsi_device.h
> +++ 2.6-mm/include/scsi/scsi_device.h
> @@ -126,7 +126,7 @@ struct scsi_device {
>  	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */
>  	unsigned guess_capacity:1;	/* READ_CAPACITY might be too high by 1 */
>  	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
> -
> +	unsigned async_notification:1;	/* device supports async notification */
>  	unsigned int device_blocked;	/* Device returned QUEUE_FULL. */
>  
>  	unsigned int max_device_blocked; /* what device_blocked counts down from  */
> Index: 2.6-mm/drivers/ata/libata-scsi.c
> ===================================================================
> --- 2.6-mm.orig/drivers/ata/libata-scsi.c
> +++ 2.6-mm/drivers/ata/libata-scsi.c
> @@ -899,6 +899,9 @@ static void ata_scsi_dev_config(struct s
>  		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
>  	}
>  
> +	if (dev->flags & ATA_DFLAG_AN)
> +		sdev->async_notification = 1;
> +
>  	if (dev->flags & ATA_DFLAG_NCQ) {
>  		int depth;
>  
> Index: 2.6-mm/drivers/scsi/sr.c
> ===================================================================
> --- 2.6-mm.orig/drivers/scsi/sr.c
> +++ 2.6-mm/drivers/scsi/sr.c
> @@ -603,6 +603,8 @@ static int sr_probe(struct device *dev)
>  
>  	dev_set_drvdata(dev, cd);
>  	disk->flags |= GENHD_FL_REMOVABLE;
> +	if (sdev->async_notification)
> +		disk->flags |= GENHD_FL_ASYNC_NOTIFICATION;
>  	add_disk(disk);

(added linux-scsi to CC)

Comments:

1) From a procedural standpoint, you'll want to separate this patch into 
three patches:  generic block layer stuff, SCSI stuff, and libata stuff.

2) I don't claim to be a sysfs expert, but this seems like a reasonable 
approach for reporting async-notification capabilities

3) I would make the contents of 'media_change_events' be a list of 
flags, rather than a boolean.  Thus, when AN is present, 
media_change_events would return "AN\n".  It would return "\n" (no 
flags) when AN is absent.  This permits future expansion of this 
capabilities reporting variable.

4) Figure out some place to document 'media_change_events', in 
Documentation/*

5) I think the method of delivery probably needs discussing, and some 
work.  Presumably the normal hotplug paths should be traversed for this 
sort of thing.

Re: [patch 2/3] libata: expose AN support to user space via sysfs

[Tejun Heo]

Jeff Garzik wrote:
> Kristen Carlson Accardi wrote:
>> Allow user space to determine if an ATAPI device supports
>> async notification (AN) of media changes.  This is done by
>> adding a new sysfs file "async_notification" to genhd.
>> If the file reads 1, then the device supports async notification.  If 
>> the file reads 0, it does not. 
>> A flag is set in the generic disk to indicate whether
>> or not AN is supported.  This flag is set by the SCSI
>> subsystem when it registers with add_disk.  The SCSI
>> system gets information from libata on whether the
>> device supports AN during dev_configure.
>> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
>>
> 
> 3) I would make the contents of 'media_change_events' be a list of 
> flags, rather than a boolean.  Thus, when AN is present, 
> media_change_events would return "AN\n".  It would return "\n" (no 
> flags) when AN is absent.  This permits future expansion of this 
> capabilities reporting variable.

I'm not sure about this.  AN is kind of specific term for ATA while 
media change event is generic.  So, I think the original approach is 
okay.  No matter how the actual thing is implemented, it's the same 
media change event and as long as event delivery interface is the same, 
upper layer shouldn't care about how it's done.

Thanks.

-- 
tejun

Re: [patch 2/3] libata: expose AN support to user space via sysfs

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Kristen Carlson Accardi wrote:
>>> Allow user space to determine if an ATAPI device supports
>>> async notification (AN) of media changes.  This is done by
>>> adding a new sysfs file "async_notification" to genhd.
>>> If the file reads 1, then the device supports async notification.  If 
>>> the file reads 0, it does not. A flag is set in the generic disk to 
>>> indicate whether
>>> or not AN is supported.  This flag is set by the SCSI
>>> subsystem when it registers with add_disk.  The SCSI
>>> system gets information from libata on whether the
>>> device supports AN during dev_configure.
>>> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
>>>
>>
>> 3) I would make the contents of 'media_change_events' be a list of 
>> flags, rather than a boolean.  Thus, when AN is present, 
>> media_change_events would return "AN\n".  It would return "\n" (no 
>> flags) when AN is absent.  This permits future expansion of this 
>> capabilities reporting variable.
> 
> I'm not sure about this.  AN is kind of specific term for ATA while 
> media change event is generic.  So, I think the original approach is 
> okay.  No matter how the actual thing is implemented, it's the same 
> media change event and as long as event delivery interface is the same, 
> upper layer shouldn't care about how it's done.

AN is a generic concept that I feel will propagate elsewhere.

Though perhaps it should be in a 'capability_flags' file rather than a 
'media_change_event' file.

	Jeff

Re: [patch 2/3] libata: expose AN support to user space via sysfs

[Tejun Heo]

Jeff Garzik wrote:
> AN is a generic concept that I feel will propagate elsewhere.

I think SCSI already has it or am I imagining things again?  :-)

> Though perhaps it should be in a 'capability_flags' file rather than a 
> 'media_change_event' file.

IMHO, if it's genhd.capability_flags then the flag should be 
MEDIA_CHANGE_NOTIFY not ASYNC_NOTIFICATION because AN itself doesn't 
imply any specific event.  It's just a notification mechanism, for ATAPI 
devices, it means media change, for PMP it has a different meaning, so I 
think we need to export the processed meaning not the specific mechanism 
to userland.

Thanks.

-- 
tejun

Re: [patch 2/3] libata: expose AN support to user space via sysfs

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> AN is a generic concept that I feel will propagate elsewhere.
> 
> I think SCSI already has it or am I imagining things again?  :-)
> 
>> Though perhaps it should be in a 'capability_flags' file rather than a 
>> 'media_change_event' file.
> 
> IMHO, if it's genhd.capability_flags then the flag should be 
> MEDIA_CHANGE_NOTIFY not ASYNC_NOTIFICATION because AN itself doesn't 
> imply any specific event.  It's just a notification mechanism, for ATAPI 
> devices, it means media change, for PMP it has a different meaning, so I 
> think we need to export the processed meaning not the specific mechanism 
> to userland.

Agreed, sounds good.

	Jeff