[RFC Patch] virtio: export model and type in /sys

[RFC Patch] virtio: export model and type in /sys

[Amerigo Wang]

Our kdump script needs /sys/block/X/device/{vendor, model, type},
but virtio devices don't have {model, type}, this patch adds them.
Actually, I don't know how to fill the model field, other block devices
seem read it from SCSI. Any comments?

Signed-off-by: WANG Cong <amwang@redhat.com>

---
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index efb35aa..5ce70b7 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -17,6 +17,18 @@ static ssize_t vendor_show(struct device *_d,
 	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
 	return sprintf(buf, "0x%04x\n", dev->id.vendor);
 }
+static ssize_t model_show(struct device *_d,
+			   struct device_attribute *attr, char *buf)
+{
+	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
+	return sprintf(buf, "0x%04x\n", dev->id.model);
+}
+static ssize_t type_show(struct device *_d,
+			   struct device_attribute *attr, char *buf)
+{
+	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
+	return sprintf(buf, "0x%04x\n", dev->id.type);
+}
 static ssize_t status_show(struct device *_d,
 			   struct device_attribute *attr, char *buf)
 {
@@ -49,6 +61,8 @@ static ssize_t features_show(struct device *_d,
 static struct device_attribute virtio_dev_attrs[] = {
 	__ATTR_RO(device),
 	__ATTR_RO(vendor),
+	__ATTR_RO(model),
+	__ATTR_RO(type),
 	__ATTR_RO(status),
 	__ATTR_RO(modalias),
 	__ATTR_RO(features),
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4fb5b2b..3bde99d 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -656,6 +656,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	 * the subsystem ids */
 	vp_dev->vdev.id.vendor = pci_dev->subsystem_vendor;
 	vp_dev->vdev.id.device = pci_dev->subsystem_device;
+	vp_dev->vdev.id.type = pci_dev->pcie_type;
 
 	/* finally register the virtio device */
 	err = register_virtio_device(&vp_dev->vdev);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 48c007d..b72cd50 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -385,6 +385,8 @@ struct ssb_device_id {
 struct virtio_device_id {
 	__u32 device;
 	__u32 vendor;
+	__u32 model;
+	__u32 type;
 };
 #define VIRTIO_DEV_ANY_ID	0xffffffff
 

Re: [RFC Patch] virtio: export model and type in /sys

[Rusty Russell]

On Mon, 31 Jan 2011 01:23:00 pm Amerigo Wang wrote:
> Our kdump script needs /sys/block/X/device/{vendor, model, type},
> but virtio devices don't have {model, type}, this patch adds them.
> Actually, I don't know how to fill the model field, other block devices
> seem read it from SCSI. Any comments?

This seems deeply wrong.  Can't you fix your script?  

Model might sanely map to the feature bits, but making the type the same
as the PCI type is weird...

Confused,
Rusty.

Re: [RFC Patch] virtio: export model and type in /sys

[Cong Wang]

(Adding Neil into Cc.)

于 2011年01月31日 12:05, Rusty Russell 写道:
> On Mon, 31 Jan 2011 01:23:00 pm Amerigo Wang wrote:
>> Our kdump script needs /sys/block/X/device/{vendor, model, type},
>> but virtio devices don't have {model, type}, this patch adds them.
>> Actually, I don't know how to fill the model field, other block devices
>> seem read it from SCSI. Any comments?
>
> This seems deeply wrong.  Can't you fix your script?

Well, we use (vendor, model, type) to identify a disk, and so far we only
see virtio devices don't have this. So, we hope virtio at least provides
some dummy files for us.

>
> Model might sanely map to the feature bits, but making the type the same
> as the PCI type is weird...
>

Yeah, that is why I mark the patch as RFC, I don't know which is the correct way
to fill these two values. Providing some dummy values is also fine for us.

Thanks.

Re: [RFC Patch] virtio: export model and type in /sys

[Anthony Liguori]

On 01/30/2011 11:45 PM, Cong Wang wrote:
> (Adding Neil into Cc.)
>
> 于 2011年01月31日 12:05, Rusty Russell 写道:
>> On Mon, 31 Jan 2011 01:23:00 pm Amerigo Wang wrote:
>>> Our kdump script needs /sys/block/X/device/{vendor, model, type},
>>> but virtio devices don't have {model, type}, this patch adds them.
>>> Actually, I don't know how to fill the model field, other block devices
>>> seem read it from SCSI. Any comments?
>>
>> This seems deeply wrong.  Can't you fix your script?
>
> Well, we use (vendor, model, type) to identify a disk, and so far we only
> see virtio devices don't have this. So, we hope virtio at least provides
> some dummy files for us.

Uniquely identify the disk or identify the class of disk?

If it's the former, would serial be a better attribute to key off of?

Regards,

Anthony Liguori

>>
>> Model might sanely map to the feature bits, but making the type the same
>> as the PCI type is weird...
>>
>
> Yeah, that is why I mark the patch as RFC, I don't know which is the 
> correct way
> to fill these two values. Providing some dummy values is also fine for 
> us.
>
> Thanks.

Re: [RFC Patch] virtio: export model and type in /sys

[Christoph Hellwig]

On Mon, Jan 31, 2011 at 08:02:19AM -0600, Anthony Liguori wrote:
> Uniquely identify the disk or identify the class of disk?
> 
> If it's the former, would serial be a better attribute to key off of?

It wouldn't just be the better way but the only working way.  

>From one of my test systems:

$ for i in /sys/block/*/device; do echo "vendor = $(cat $i/vendor) model = $(cat $i/model) type = $(cat $i/type)"; done
vendor = ServeRA  model = RAID 1           type = 0
vendor = HP       model = MSA2012fc        type = 0
vendor = HP       model = MSA2012fc        type = 0
vendor = HP       model = MSA2012fc        type = 0
vendor = HP       model = MSA2012fc        type = 0
vendor = NETAPP   model = LUN              type = 0
vendor = NETAPP   model = LUN              type = 0
vendor = MATSHITA model = UJDA780 DVD/CDRW type = 5

Re: [RFC Patch] virtio: export model and type in /sys

[Michael S. Tsirkin]

On Mon, Jan 31, 2011 at 10:53:00AM +0800, Amerigo Wang wrote:
> Our kdump script needs /sys/block/X/device/{vendor, model, type},

BTW, it's sys/class/block now? One is new, one is legacy and
deprecated, I am not sure which is which ...

> but virtio devices don't have {model, type}, this patch adds them.
> Actually, I don't know how to fill the model field, other block devices
> seem read it from SCSI. Any comments?
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>

For model, I am guessing "virtio-blk" would be a good value.  But I
think this attribute is specific to block so should be added only for
virito-blk.
As far as I can tell, type is from scsi/scsi.h,
so we can just stick TYPE_DISK (0) there.

BTW, I note that other devices in drivers/block might not
have these either. Does your script work for xen-blkfront?
If we want this to work for all block devices maybe these
attributes should go into block core? model could
default to the driver name, type could default to 0...

> ---
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index efb35aa..5ce70b7 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -17,6 +17,18 @@ static ssize_t vendor_show(struct device *_d,
>  	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
>  	return sprintf(buf, "0x%04x\n", dev->id.vendor);
>  }
> +static ssize_t model_show(struct device *_d,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
> +	return sprintf(buf, "0x%04x\n", dev->id.model);
> +}
> +static ssize_t type_show(struct device *_d,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct virtio_device *dev = container_of(_d,struct virtio_device,dev);
> +	return sprintf(buf, "0x%04x\n", dev->id.type);
> +}
>  static ssize_t status_show(struct device *_d,
>  			   struct device_attribute *attr, char *buf)
>  {
> @@ -49,6 +61,8 @@ static ssize_t features_show(struct device *_d,
>  static struct device_attribute virtio_dev_attrs[] = {
>  	__ATTR_RO(device),
>  	__ATTR_RO(vendor),
> +	__ATTR_RO(model),
> +	__ATTR_RO(type),
>  	__ATTR_RO(status),
>  	__ATTR_RO(modalias),
>  	__ATTR_RO(features),
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 4fb5b2b..3bde99d 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -656,6 +656,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
>  	 * the subsystem ids */
>  	vp_dev->vdev.id.vendor = pci_dev->subsystem_vendor;
>  	vp_dev->vdev.id.device = pci_dev->subsystem_device;
> +	vp_dev->vdev.id.type = pci_dev->pcie_type;
>  
>  	/* finally register the virtio device */
>  	err = register_virtio_device(&vp_dev->vdev);
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 48c007d..b72cd50 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -385,6 +385,8 @@ struct ssb_device_id {
>  struct virtio_device_id {
>  	__u32 device;
>  	__u32 vendor;
> +	__u32 model;
> +	__u32 type;
>  };
>  #define VIRTIO_DEV_ANY_ID	0xffffffff
>  

Re: [RFC Patch] virtio: export model and type in /sys

[Christoph Hellwig]

On Mon, Jan 31, 2011 at 10:53:00AM +0800, Amerigo Wang wrote:
> Our kdump script needs /sys/block/X/device/{vendor, model, type},
> but virtio devices don't have {model, type}, this patch adds them.
> Actually, I don't know how to fill the model field, other block devices
> seem read it from SCSI. Any comments?

It does not make any sense to fill this at all in virtio.  The
attributes are simply exporting the SCSI INQUIRY data, and do not make
any sense at all outside a SCSI context.  In fact the type field even
contains numbers from the scsi protocol spec that just don't make sense.

Re: [RFC Patch] virtio: export model and type in /sys

[Michael S. Tsirkin]

On Tue, Feb 01, 2011 at 12:37:38PM -0500, Christoph Hellwig wrote:
> On Mon, Jan 31, 2011 at 10:53:00AM +0800, Amerigo Wang wrote:
> > Our kdump script needs /sys/block/X/device/{vendor, model, type},
> > but virtio devices don't have {model, type}, this patch adds them.
> > Actually, I don't know how to fill the model field, other block devices
> > seem read it from SCSI. Any comments?
> 
> It does not make any sense to fill this at all in virtio.  The
> attributes are simply exporting the SCSI INQUIRY data, and do not make
> any sense at all outside a SCSI context.
>  In fact the type field even
> contains numbers from the scsi protocol spec that just don't make sense.

ide seems to export these too.

$ cat /sys/class/block/sr0/device/model 
DVDRAM GSA-U20N 

and I am pretty sure my dvd isn't scsi ...

Re: [RFC Patch] virtio: export model and type in /sys

[Christoph Hellwig]

On Tue, Feb 01, 2011 at 07:46:07PM +0200, Michael S. Tsirkin wrote:
> ide seems to export these too.
> 
> $ cat /sys/class/block/sr0/device/model 
> DVDRAM GSA-U20N 
> 
> and I am pretty sure my dvd isn't scsi ...

And I'm pretty sure it is, not just in one but two different ways.
For one all ATA attached CDROMS are ATAPI, which just means a SCSI
command set (MMC in this case) tunneled over an ATA transport.  Second
libata always is a scsi low level driver, even when talking to ATA
disk, it just responds to the SCSI commands by translating them to
equivalent ATA commands or faking them up.

Re: [RFC Patch] virtio: export model and type in /sys

[Cong Wang]

于 2011年02月02日 01:37, Christoph Hellwig 写道:
> On Mon, Jan 31, 2011 at 10:53:00AM +0800, Amerigo Wang wrote:
>> Our kdump script needs /sys/block/X/device/{vendor, model, type},
>> but virtio devices don't have {model, type}, this patch adds them.
>> Actually, I don't know how to fill the model field, other block devices
>> seem read it from SCSI. Any comments?
>
> It does not make any sense to fill this at all in virtio.  The
> attributes are simply exporting the SCSI INQUIRY data, and do not make
> any sense at all outside a SCSI context.  In fact the type field even
> contains numbers from the scsi protocol spec that just don't make sense.
>

Hmm, so other non-SCSI devices don't have these neither?
Do you have any suggestions to find another way to identify a disk?

Thanks.

Re: [RFC Patch] virtio: export model and type in /sys

[Christoph Hellwig]

On Fri, Feb 04, 2011 at 02:29:55PM +0800, Cong Wang wrote:
> >It does not make any sense to fill this at all in virtio.  The
> >attributes are simply exporting the SCSI INQUIRY data, and do not make
> >any sense at all outside a SCSI context.  In fact the type field even
> >contains numbers from the scsi protocol spec that just don't make sense.
> >
> 
> Hmm, so other non-SCSI devices don't have these neither?
> Do you have any suggestions to find another way to identify a disk?

Many modern disks have uuid strings identifying them, e.g. virtio-blk
has a serial attribute for it.  For SCSI and ATA they are retreived
by issuing the commands to get them through the pass through ioctls
right now, but I'd love to see the serial sysfs attribute implemented
there as well.

Re: [RFC Patch] virtio: export model and type in /sys

[Cong Wang]

于 2011年02月12日 15:17, Christoph Hellwig 写道:
> On Fri, Feb 04, 2011 at 02:29:55PM +0800, Cong Wang wrote:
>>> It does not make any sense to fill this at all in virtio.  The
>>> attributes are simply exporting the SCSI INQUIRY data, and do not make
>>> any sense at all outside a SCSI context.  In fact the type field even
>>> contains numbers from the scsi protocol spec that just don't make sense.
>>>
>>
>> Hmm, so other non-SCSI devices don't have these neither?
>> Do you have any suggestions to find another way to identify a disk?
>
> Many modern disks have uuid strings identifying them, e.g. virtio-blk
> has a serial attribute for it.  For SCSI and ATA they are retreived
> by issuing the commands to get them through the pass through ioctls
> right now, but I'd love to see the serial sysfs attribute implemented
> there as well.

Hmm, looks promising but needs more efforts.

Thanks!