LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC Patch] virtio: export model and type in /sys
@ 2011-01-31  2:53 Amerigo Wang
  2011-01-31  4:05 ` Rusty Russell
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Amerigo Wang @ 2011-01-31  2:53 UTC (permalink / raw)
  To: linux-kernel
  Cc: WANG Cong, Stephen Hemminger, Rusty Russell, Michael S. Tsirkin,
	Anthony Liguori, Jamie Lokier, Thomas Weber, Ben Hutchings,
	David Woodhouse, Andy Fleming, David S. Miller

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
 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31  2:53 [RFC Patch] virtio: export model and type in /sys Amerigo Wang
@ 2011-01-31  4:05 ` Rusty Russell
  2011-01-31  5:45   ` Cong Wang
  2011-01-31  9:52 ` Michael S. Tsirkin
  2011-02-01 17:37 ` Christoph Hellwig
  2 siblings, 1 reply; 12+ messages in thread
From: Rusty Russell @ 2011-01-31  4:05 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, Stephen Hemminger, Michael S. Tsirkin,
	Anthony Liguori, Jamie Lokier, Thomas Weber, Ben Hutchings,
	David Woodhouse, Andy Fleming, David S. Miller

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31  4:05 ` Rusty Russell
@ 2011-01-31  5:45   ` Cong Wang
  2011-01-31 14:02     ` Anthony Liguori
  0 siblings, 1 reply; 12+ messages in thread
From: Cong Wang @ 2011-01-31  5:45 UTC (permalink / raw)
  To: Rusty Russell
  Cc: linux-kernel, Stephen Hemminger, Michael S. Tsirkin,
	Anthony Liguori, Jamie Lokier, Thomas Weber, Ben Hutchings,
	David Woodhouse, Andy Fleming, David S. Miller, Neil Horman

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31  2:53 [RFC Patch] virtio: export model and type in /sys Amerigo Wang
  2011-01-31  4:05 ` Rusty Russell
@ 2011-01-31  9:52 ` Michael S. Tsirkin
  2011-02-01 17:37 ` Christoph Hellwig
  2 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2011-01-31  9:52 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, Stephen Hemminger, Rusty Russell, Anthony Liguori,
	Jamie Lokier, Thomas Weber, Ben Hutchings, David Woodhouse,
	Andy Fleming, David S. Miller, axboe, hch

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
>  

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31  5:45   ` Cong Wang
@ 2011-01-31 14:02     ` Anthony Liguori
  2011-02-01 17:41       ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Anthony Liguori @ 2011-01-31 14:02 UTC (permalink / raw)
  To: Cong Wang
  Cc: Rusty Russell, linux-kernel, Stephen Hemminger,
	Michael S. Tsirkin, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller,
	Neil Horman

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31  2:53 [RFC Patch] virtio: export model and type in /sys Amerigo Wang
  2011-01-31  4:05 ` Rusty Russell
  2011-01-31  9:52 ` Michael S. Tsirkin
@ 2011-02-01 17:37 ` Christoph Hellwig
  2011-02-01 17:46   ` Michael S. Tsirkin
  2011-02-04  6:29   ` Cong Wang
  2 siblings, 2 replies; 12+ messages in thread
From: Christoph Hellwig @ 2011-02-01 17:37 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, Stephen Hemminger, Rusty Russell,
	Michael S. Tsirkin, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-01-31 14:02     ` Anthony Liguori
@ 2011-02-01 17:41       ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2011-02-01 17:41 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: Cong Wang, Rusty Russell, linux-kernel, Stephen Hemminger,
	Michael S. Tsirkin, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller,
	Neil Horman

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-02-01 17:37 ` Christoph Hellwig
@ 2011-02-01 17:46   ` Michael S. Tsirkin
  2011-02-01 17:56     ` Christoph Hellwig
  2011-02-04  6:29   ` Cong Wang
  1 sibling, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2011-02-01 17:46 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Amerigo Wang, linux-kernel, Stephen Hemminger, Rusty Russell,
	Anthony Liguori, Jamie Lokier, Thomas Weber, Ben Hutchings,
	David Woodhouse, Andy Fleming, David S. Miller

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-02-01 17:46   ` Michael S. Tsirkin
@ 2011-02-01 17:56     ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2011-02-01 17:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Christoph Hellwig, Amerigo Wang, linux-kernel, Stephen Hemminger,
	Rusty Russell, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-02-01 17:37 ` Christoph Hellwig
  2011-02-01 17:46   ` Michael S. Tsirkin
@ 2011-02-04  6:29   ` Cong Wang
  2011-02-12  7:17     ` Christoph Hellwig
  1 sibling, 1 reply; 12+ messages in thread
From: Cong Wang @ 2011-02-04  6:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, Stephen Hemminger, Rusty Russell,
	Michael S. Tsirkin, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-02-04  6:29   ` Cong Wang
@ 2011-02-12  7:17     ` Christoph Hellwig
  2011-02-12  9:56       ` Cong Wang
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2011-02-12  7:17 UTC (permalink / raw)
  To: Cong Wang
  Cc: Christoph Hellwig, linux-kernel, Stephen Hemminger,
	Rusty Russell, Michael S. Tsirkin, Anthony Liguori, Jamie Lokier,
	Thomas Weber, Ben Hutchings, David Woodhouse, Andy Fleming,
	David S. Miller

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC Patch] virtio: export model and type in /sys
  2011-02-12  7:17     ` Christoph Hellwig
@ 2011-02-12  9:56       ` Cong Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Cong Wang @ 2011-02-12  9:56 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, Stephen Hemminger, Rusty Russell,
	Michael S. Tsirkin, Anthony Liguori, Jamie Lokier, Thomas Weber,
	Ben Hutchings, David Woodhouse, Andy Fleming, David S. Miller

于 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!

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-02-12  9:57 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-31  2:53 [RFC Patch] virtio: export model and type in /sys Amerigo Wang
2011-01-31  4:05 ` Rusty Russell
2011-01-31  5:45   ` Cong Wang
2011-01-31 14:02     ` Anthony Liguori
2011-02-01 17:41       ` Christoph Hellwig
2011-01-31  9:52 ` Michael S. Tsirkin
2011-02-01 17:37 ` Christoph Hellwig
2011-02-01 17:46   ` Michael S. Tsirkin
2011-02-01 17:56     ` Christoph Hellwig
2011-02-04  6:29   ` Cong Wang
2011-02-12  7:17     ` Christoph Hellwig
2011-02-12  9:56       ` Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).