Add serial number support for virtio_blk, V3
Subsystems (4)
PATCH 2/2 Add serial number support for virtio_blk, V3
Signature · 2 lines
-- [email protected]
drivers/block/virtio_blk.c | 32 +++++++++++++++++++++++++++++--- include/linux/virtio_blk.h | 6 ++++++ 2 files changed, 35 insertions(+), 3 deletions(-)
=================================================================
drivers/block/virtio_blk.c +29 -3
@@ -146,12 +146,37 @@ static void do_virtblk_request(struct re vblk->vq->vq_ops->kick (vblk->vq); } +/* return serial# in IDE identify data (all other fields are currently zero) +*/ +static int virtblk_identity (struct block_device *bdev,void *buf) +{ +struct virtio_blk *vblk = bdev->bd_disk->private_data; +u16 *id; +int rv; + +if (!(id =kzalloc (ATA_ID_WORDS ,GFP_KERNEL ))) + rv = -ENOMEM ; +else if ((rv =virtio_config_buf (vblk->vdev,VIRTIO_BLK_F_SN , + offsetof(struct virtio_blk_config , serial), &id[ATA_ID_SERNO ], +ATA_ID_SERNO_LEN ))) + ; +else if (copy_to_user (buf, id,ATA_ID_WORDS )) + rv = -EFAULT ; +else + rv =0 ; +if (id) +kfree (id); +return (rv); +} +static int virtblk_ioctl (struct block_device *bdev,fmode_t mode,unsigned cmd,unsigned long data) { -return scsi_cmd_ioctl (bdev->bd_disk->queue, - bdev->bd_disk, mode, cmd, - (void __user *)data); +if (cmd ==HDIO_GET_IDENTITY ) +return (virtblk_identity (bdev, (void __user *)data)); +else +return scsi_cmd_ioctl (bdev->bd_disk->queue, bdev->bd_disk, + mode, cmd, (void __user *)data); }/* We provide getgeo only to please some old bootloader/partitioning tools */
@@ -356,6 +381,7 @@ static struct virtio_device_id id_table[static unsigned int features[] = {VIRTIO_BLK_F_BARRIER ,VIRTIO_BLK_F_SEG_MAX ,VIRTIO_BLK_F_SIZE_MAX ,VIRTIO_BLK_F_GEOMETRY ,VIRTIO_BLK_F_RO ,VIRTIO_BLK_F_BLK_SIZE , +VIRTIO_BLK_F_SN };static struct virtio_driver virtio_blk = {
=================================================================
include/linux/virtio_blk.h +6 -0
@@ -15,7 +15,12 @@#define VIRTIO_BLK_F_GEOMETRY 4/* Legacy geometry available */ #define VIRTIO_BLK_F_RO 5/* Disk is read-only */ #define VIRTIO_BLK_F_BLK_SIZE 6/* Block size of disk is available*/ +#define VIRTIO_BLK_F_SN 8/* serial number supported */ +#define BLOCK_SERIAL_STRLEN 20 + +/* mapped into pci i/o region 0 +*/ struct virtio_blk_config {/* The capacity (in 512-byte sectors). */
@@ -32,6 +37,7 @@ struct virtio_blk_config } geometry;/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */ __u32 blk_size; +__u8 serial[BLOCK_SERIAL_STRLEN ]; } __attribute__((packed));/* These two define direction. */
You should probably include rusty as he's collecting the patches for the virtio guest drivers.
Also can you send the patch inline next time? That makes quoting it for review a lot easier.
drivers/block/virtio_blk.c | 32 +++++++++++++++++++++++++++++--- include/linux/virtio_blk.h | 6 ++++++ 2 files changed, 35 insertions(+), 3 deletions(-)
=================================================================
drivers/block/virtio_blk.c +28 -3
@@ -146,12 +146,37 @@ static void do_virtblk_request(struct re vblk->vq->vq_ops->kick (vblk->vq); } +/* return serial# in IDE identify data (all other fields are currently zero) +*/ +static int virtblk_identity (struct block_device *bdev,void *buf) +{ +struct virtio_blk *vblk = bdev->bd_disk->private_data; +u16 *id; +int rv; + +if (!(id =kzalloc (ATA_ID_WORDS ,GFP_KERNEL ))) + rv = -ENOMEM ; +else if ((rv =virtio_config_buf (vblk->vdev,VIRTIO_BLK_F_SN , + offsetof(struct virtio_blk_config , serial), &id[ATA_ID_SERNO ], +ATA_ID_SERNO_LEN ))) + ; +else if (copy_to_user (buf, id,ATA_ID_WORDS )) + rv = -EFAULT ; +else + rv =0 ; +if (id) +kfree (id); +return (rv); +} +static int virtblk_ioctl (struct block_device *bdev,fmode_t mode,unsigned cmd,unsigned long data) { -return scsi_cmd_ioctl (bdev->bd_disk->queue, - bdev->bd_disk, mode, cmd, - (void __user *)data); +if (cmd ==HDIO_GET_IDENTITY ) +return (virtblk_identity (bdev, (void __user *)data)); +else +return scsi_cmd_ioctl (bdev->bd_disk->queue, bdev->bd_disk, + mode, cmd, (void __user *)data); }
This looks functionally correct, but pretty far from normal kernel coding style. I'd envision something like this instead:
/*
* IDE-compatible identify ioctl.
*
* Currenlyt only returns the serial number and leaves all other fields
* zero.
*/
static int virtblk_identity(struct gendisk *disk, void *argp)
{
struct virtio_blk *vblk = disk->private_data;
u16 *id;
int error = -ENOMEM;
id = kzalloc(ATA_ID_WORDS, GFP_KERNEL)
if (!id)
goto out;
error = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_SN,
offsetof(struct virtio_blk_config, serial),
&id[ATA_ID_SERNO], ATA_ID_SERNO_LEN);
if (error)
goto out_kfree;
if (copy_to_user(argp, id, ATA_ID_WORDS))
error = -EFAULT;out_kfree:
kfree(id); out: return error; }
static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
unsigned cmd, unsigned long arg)
{
struct gendisk *disk = bdev->bd_disk;
void __user *argp = (void __user *arg);
switch (cmd) {
case HDIO_GET_IDENTITY:
return virtblk_identity(disk, argp);
default:
return scsi_cmd_ioctl(disk->queue, disk, mode, cmd, argp);
}
}Quoted 6 lines · Christoph Hellwig
/*
* IDE-compatible identify ioctl.
*
* Currenlyt only returns the serial number and leaves all other fields
* zero.
*/
Btw, thinking about it the rest of the information in the ioctl should probably be filled up with faked data, similar to how we do it for the ide emulation inside qemu.
Quoted 2 lines · Christoph Hellwig
This looks functionally correct, but pretty far from normal kernel coding style.
I tend to avoid 'goto's.
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines
/*
* IDE-compatible identify ioctl.
*
* Currenlyt only returns the serial number and leaves all other fields
* zero.
*/Btw, thinking about it the rest of the information in the ioctl should probably be filled up with faked data, similar to how we do it for the ide emulation inside qemu.
Doing so crossed my mind but thought it may be better to start here and provide data on an as-needed basis. But as you point out there is precedent (and likely reason) for hw/ide.c:ide_identify() doing as such. I don't have a strong bias either way. Comments from others?
-john
Signature · 1 line
[email protected]
Quoted 2 lines · Christoph Hellwig
You should probably include rusty as he's collecting the patches for the virtio guest drivers.
Yes. It *does* help to cc the maintainer if you want your patches applied :)
And I particularly love untested code like this!
drivers/block/virtio_blk.c static void do_virtblk_request(struct re · line 158
Quoted 2 lines
drivers/block/virtio_blk.c +2
-0+if (!(id =kzalloc (ATA_ID_WORDS ,GFP_KERNEL ))) + rv = -ENOMEM ;
Doesn't ATA_ID_WORDS seem like a strange name for a number of bytes?
What's this *for* BTW?
Rusty.
Quoted 11 lines · Christoph Hellwig
Quoted 6 lines · Christoph Hellwig
/*
* IDE-compatible identify ioctl.
*
* Currenlyt only returns the serial number and leaves all other fields
* zero.
*/Btw, thinking about it the rest of the information in the ioctl should probably be filled up with faked data, similar to how we do it for the ide emulation inside qemu.
Agreed. I've done so to the extent it makes sense in the case of the more generic fields. A fair amount of the identify information being generated by hw/ide.c appears obsoleted.
-john
Signature · 1 line
[email protected]
Quoted 5 lines
drivers/block/virtio_blk.c static void do_virtblk_request(struct re · line 158
Quoted 3 lines
drivers/block/virtio_blk.c +2
-0+if (!(id =kzalloc (ATA_ID_WORDS ,GFP_KERNEL ))) + rv = -ENOMEM ;Doesn't
ATA_ID_WORDSseem like a strange name for a number of bytes?
Yes I caught that bug in the rework as well.
Quoted 1 line
What's this *for* BTW?
Sorry -- I assumed you were on either list.
Please see patch to follow.
-john
Signature · 1 line
[email protected]