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 <ecas...@coraid.com>
>> ---
>> 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
ecas...@coraid.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
