Re: [PATCH 2/8] aoe: provide ATA identify device content to user onrequest

[Ed Cashin]

On Nov 8, 2012, at 7:40 PM, Jeff Garzik wrote:

> On 11/08/2012 11:32 AM, Ed Cashin wrote:
>> This patch makes the aoe driver follow expected behavior when
>> the user uses ioctl to get the ATA device identify information.
>> 
>> Signed-off-by: Ed Cashin <ecashin@xxxxxxxxxx>
>> ---
>>  drivers/block/aoe/aoe.h    |    1 +
>>  drivers/block/aoe/aoeblk.c |   30 ++++++++++++++++++++++++++++++
>>  drivers/block/aoe/aoecmd.c |   16 ++++++++++++++++
>>  3 files changed, 47 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
>> index 536942b..f6e0c03 100644
>> --- a/drivers/block/aoe/aoe.h
>> +++ b/drivers/block/aoe/aoe.h
>> @@ -169,6 +169,7 @@ struct aoedev {
>>  	struct aoetgt *htgt;	/* target needing rexmit assistance */
>>  	ulong ntargets;
>>  	ulong kicked;
>> +	char ident[512];
>>  };
>> 
>>  /* kthread tracking */
>> diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
>> index 56736cd..7ba0fcf 100644
>> --- a/drivers/block/aoe/aoeblk.c
>> +++ b/drivers/block/aoe/aoeblk.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/export.h>
>>  #include <linux/moduleparam.h>
>> +#include <scsi/sg.h>
>>  #include "aoe.h"
>> 
>>  static DEFINE_MUTEX(aoeblk_mutex);
>> @@ -212,9 +213,38 @@ aoeblk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>  	return 0;
>>  }
>> 
>> +static int
>> +aoeblk_ioctl(struct block_device *bdev, fmode_t mode, uint cmd, ulong arg)
>> +{
>> +	struct aoedev *d;
>> +
>> +	if (!arg)
>> +		return -EINVAL;
>> +
>> +	d = bdev->bd_disk->private_data;
>> +	if ((d->flags & DEVFL_UP) == 0) {
>> +		pr_err("aoe: disk not up\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (cmd == HDIO_GET_IDENTITY) {
>> +		if (!copy_to_user((void __user *) arg, &d->ident,
>> +			sizeof(d->ident)))
>> +			return 0;
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* udev calls scsi_id, which uses SG_IO, resulting in noise */
>> +	if (cmd != SG_IO)
>> +		pr_info("aoe: unknown ioctl 0x%x\n", cmd);
>> +
>> +	return -ENOTTY;
>> +}
>> +
>>  static const struct block_device_operations aoe_bdops = {
>>  	.open = aoeblk_open,
>>  	.release = aoeblk_release,
>> +	.ioctl = aoeblk_ioctl,
>>  	.getgeo = aoeblk_getgeo,
>>  	.owner = THIS_MODULE,
>>  };
>> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
>> index 3ce01f6..c4ff70b 100644
>> --- a/drivers/block/aoe/aoecmd.c
>> +++ b/drivers/block/aoe/aoecmd.c
>> @@ -799,6 +799,17 @@ aoecmd_sleepwork(struct work_struct *work)
>>  }
>> 
>>  static void
>> +ata_ident_fixstring(u16 *id, int ns)
>> +{
>> +	u16 s;
>> +
>> +	while (ns-- > 0) {
>> +		s = *id;
>> +		*id++ = s >> 8 | s << 8;
>> +	}
>> +}
>> +
>> +static void
>>  ataid_complete(struct aoedev *d, struct aoetgt *t, unsigned char *id)
>>  {
>>  	u64 ssize;
>> @@ -833,6 +844,11 @@ ataid_complete(struct aoedev *d, struct aoetgt *t, unsigned char *id)
>>  		d->geo.sectors = get_unaligned_le16(&id[56 << 1]);
>>  	}
>> 
>> +	ata_ident_fixstring((u16 *) &id[10<<1], 10);	/* serial */
>> +	ata_ident_fixstring((u16 *) &id[23<<1], 4);	/* firmware */
>> +	ata_ident_fixstring((u16 *) &id[27<<1], 20);	/* model */
> 
> This duplicates ata_id_string() and/or ata_id_c_string(), does it not?


They're similar, yes, but aoecmd.c:ata_ident_fixstring performs the byte swapping 
in place, avoiding the need for any on-stack or other memory allocation.  The code
is pretty readable now as a simple in-place byte swap, and I'm concerned that the
extra mechanics of buffer allocation, any allocation failure handling, and memmov-ing 
the results back into the id array wouldn't simplify the code if aoecmd.c tried to use 
ata_id_string as it is.

-- 
  Ed Cashin
  ecashin@xxxxxxxxxx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/