ide: Remove ide_spin_wait_hwgroup() and use special requests instead

PATCH Applied Elias Oltmanns 2 participants 5 messages Aug 11, 2008 – Aug 13, 2008

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Elias Oltmanns

author

2008-08-11 17:39 UTC #0 lore raw

Hi bart,

as you suggested, I've prepared a patch to get rid of ide_spin_wait_hwgroup(). Unfortunately, it turned out to be more intrusive and bigger than I'd hoped it would, so if you have any suggestions how to do this more elegantly, please let me know.

On a different matter, I've encountered several places where requests are being allocated with __GFP_WAIT for no obvious reason. Wouldn't it be more suitable to allocate them with GFP_KERNEL and fail with -ENOMEM in case of an error? Or is there some policy I'm not aware of?

Regards,

Elias

From: Elias Oltmanns <[email protected]>
Subject: ide: Remove ide_spin_wait_hwgroup() and use special requests instead

Use a special request for serialisation purposes and get rid of the awkward ide_spin_wait_hwgroup().

Signed-off-by Elias Oltmanns <[email protected]>

---

 drivers/ide/ide-cd.c     |    8 +--
 drivers/ide/ide-disk.c   |   66 +++++++++++-----------
 drivers/ide/ide-floppy.c |   20 +++----
 drivers/ide/ide-io.c     |   54 ++++++++++++++++++
 drivers/ide/ide-ioctls.c |   26 ++++-----
 drivers/ide/ide-proc.c   |  105 ++++++++++++++++-------------------
 drivers/ide/ide-tape.c   |   48 ++++++++--------
 drivers/ide/ide.c        |   81 +++++----------------------
 drivers/scsi/ide-scsi.c  |   34 ++++++-----
 include/linux/ide.h      |  138 ++++++++++++++++++++++++----------------------
 10 files changed, 288 insertions(+), 292 deletions(-)

drivers/ide/ide-cd.c +4 -4

index f675cee..6ebef54 100644
@@ -1807,11 +1807,11 @@ static ide_proc_entry_t idecd_proc[] = {
 	{ NULL, 0, NULL, NULL }
 };
 
-ide_devset_rw(dsc_overlap, 0, 1, dsc_overlap);
+ide_devset_rw_field(dsc_overlap, dsc_overlap);
 
-static const struct ide_devset *idecd_settings[] = {
-	&ide_devset_dsc_overlap,
-	NULL
+static const struct ide_proc_devset idecd_settings[] = {
+	IDE_PROC_DEVSET(dsc_overlap, 0, 1),
+	{ 0 },
 };
 #endif

drivers/ide/ide-disk.c +32 ~~-34~~

87 lines not on this page · raw message

drivers/ide/ide-floppy.c +10 ~~-10~~

index 8df1f9a..298008d 100644
@@ -947,9 +947,9 @@ static sector_t idefloppy_capacity(ide_drive_t *drive)
 }
 
 #ifdef CONFIG_IDE_PROC_FS
-ide_devset_rw(bios_cyl,  0, 1023, bios_cyl);
-ide_devset_rw(bios_head, 0,  255, bios_head);
-ide_devset_rw(bios_sect, 0,   63, bios_sect);
+ide_devset_rw_field(bios_cyl, bios_cyl);
+ide_devset_rw_field(bios_head, bios_head);
+ide_devset_rw_field(bios_sect, bios_sect);
 
 static int get_ticks(ide_drive_t *drive)
 {

@@ -964,14 +964,14 @@ static int set_ticks(ide_drive_t *drive, int arg)
 	return 0;
 }
 
-IDE_DEVSET(ticks, S_RW, 0, 255, get_ticks, set_ticks);
+IDE_DEVSET(ticks, DS_SYNC, get_ticks, set_ticks);
 
-static const struct ide_devset *idefloppy_settings[] = {
-	&ide_devset_bios_cyl,
-	&ide_devset_bios_head,
-	&ide_devset_bios_sect,
-	&ide_devset_ticks,
-	NULL
+static const struct ide_proc_devset idefloppy_settings[] = {
+	IDE_PROC_DEVSET(bios_cyl,  0, 1023),
+	IDE_PROC_DEVSET(bios_head, 0,  255),
+	IDE_PROC_DEVSET(bios_sect, 0,   63),
+	IDE_PROC_DEVSET(ticks,	   0,  255),
+	{ 0 },
 };
 #endif

drivers/ide/ide-io.c +54 -0

63 lines not on this page · raw message

drivers/ide/ide-ioctls.c +11 ~~-15~~

index 7a0d62e..421a68a 100644
@@ -6,11 +6,11 @@
 #include <linux/ide.h>
 
 static const struct ide_ioctl_devset ide_ioctl_settings[] = {
-{ HDIO_GET_32BIT,	 HDIO_SET_32BIT,	get_io_32bit,  set_io_32bit  },
-{ HDIO_GET_KEEPSETTINGS, HDIO_SET_KEEPSETTINGS,	get_ksettings, set_ksettings },
-{ HDIO_GET_UNMASKINTR,	 HDIO_SET_UNMASKINTR,	get_unmaskirq, set_unmaskirq },
-{ HDIO_GET_DMA,		 HDIO_SET_DMA,		get_using_dma, set_using_dma },
-{ -1,			 HDIO_SET_PIO_MODE,	NULL,	       set_pio_mode  },
+{ HDIO_GET_32BIT,	 HDIO_SET_32BIT,	&ide_devset_io_32bit  },
+{ HDIO_GET_KEEPSETTINGS, HDIO_SET_KEEPSETTINGS,	&ide_devset_keepsettings },
+{ HDIO_GET_UNMASKINTR,	 HDIO_SET_UNMASKINTR,	&ide_devset_unmaskirq },
+{ HDIO_GET_DMA,		 HDIO_SET_DMA,		&ide_devset_using_dma },
+{ -1,			 HDIO_SET_PIO_MODE,	&ide_devset_pio_mode  },
 { 0 }
 };

@@ -22,9 +22,9 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 	int err = -EOPNOTSUPP;
 
 	for (; s->get_ioctl; s++) {
-		if (s->get && s->get_ioctl == cmd)
+		if (s->get_ioctl == cmd)
 			goto read_val;
-		else if (s->set && s->set_ioctl == cmd)
+		else if (s->set_ioctl == cmd)
 			goto set_val;
 	}

@@ -33,7 +33,7 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 read_val:
 	mutex_lock(&ide_setting_mtx);
 	spin_lock_irqsave(&ide_lock, flags);
-	err = s->get(drive);
+	err = s->setting->get(drive);
 	spin_unlock_irqrestore(&ide_lock, flags);
 	mutex_unlock(&ide_setting_mtx);
 	return err >= 0 ? put_user(err, (long __user *)arg) : err;

@@ -42,13 +42,9 @@ set_val:
 	if (bdev != bdev->bd_contains)
 		err = -EINVAL;
 	else {
-		if (!capable(CAP_SYS_ADMIN))
-			err = -EACCES;
-		else {
-			mutex_lock(&ide_setting_mtx);
-			err = s->set(drive, arg);
-			mutex_unlock(&ide_setting_mtx);
-		}
+		mutex_lock(&ide_setting_mtx);
+		err = ide_devset_execute(drive, s->setting, arg);
+		mutex_unlock(&ide_setting_mtx);
 	}
 	return err;
 }

drivers/ide/ide-proc.c +48 ~~-57~~

171 lines not on this page · raw message

drivers/ide/ide-tape.c +24 ~~-24~~

64 lines not on this page · raw message

drivers/ide/ide.c +15 ~~-66~~

152 lines not on this page · raw message

drivers/scsi/ide-scsi.c +17 ~~-17~~

index 08d2bb4..fc77aba 100644
@@ -444,25 +444,25 @@ static int set_##name(ide_drive_t *drive, int arg) \
 	return 0; \
 }
 
-#define ide_scsi_devset_rw(_name, _min, _max, _field) \
+#define ide_scsi_devset_rw_field(_name, _field) \
 ide_scsi_devset_get(_name, _field); \
 ide_scsi_devset_set(_name, _field); \
-IDE_DEVSET(_name, S_RW, _min, _max, get_##_name, set_##_name)
-
-ide_devset_rw(bios_cyl,		0, 1023, bios_cyl);
-ide_devset_rw(bios_head,	0,  255, bios_head);
-ide_devset_rw(bios_sect,	0,   63, bios_sect);
-
-ide_scsi_devset_rw(transform,	0,    3, transform);
-ide_scsi_devset_rw(log,		0,    1, log);
-
-static const struct ide_devset *idescsi_settings[] = {
-	&ide_devset_bios_cyl,
-	&ide_devset_bios_head,
-	&ide_devset_bios_sect,
-	&ide_devset_log,
-	&ide_devset_transform,
-	NULL
+IDE_DEVSET(_name, DS_SYNC, get_##_name, set_##_name)
+
+ide_devset_rw_field(bios_cyl, bios_cyl);
+ide_devset_rw_field(bios_head, bios_head);
+ide_devset_rw_field(bios_sect, bios_sect);
+
+ide_scsi_devset_rw_field(transform, transform);
+ide_scsi_devset_rw_field(log, log);
+
+static const struct ide_proc_devset idescsi_settings[] = {
+	IDE_PROC_DEVSET(bios_cyl,  0, 1023),
+	IDE_PROC_DEVSET(bios_head, 0,  255),
+	IDE_PROC_DEVSET(bios_sect, 0,	63),
+	IDE_PROC_DEVSET(log,	   0,	 1),
+	IDE_PROC_DEVSET(transform, 0,	 3),
+	{ 0 },
 };
 #endif

include/linux/ide.h +73 ~~-65~~

205 lines not on this page · raw message

Bartlomiej Zolnierkiewicz

2008-08-12 22:43 UTC #1 lore raw

Hi,

Quoted 6 lines · Elias Oltmanns

Hi bart,
as you suggested, I've prepared a patch to get rid of ide_spin_wait_hwgroup(). Unfortunately, it turned out to be more intrusive and bigger than I'd hoped it would, so if you have any suggestions how to do this more elegantly, please let me know.

Thank you for working on this.

Patch looks good to me (there is a couple of minor things to address but nothing serious). I also really like how it adds struct ide_devset which is shared by struct ide_{ioctl,proc}_devset.

Quoted 4 lines

On a different matter, I've encountered several places where requests are being allocated with __GFP_WAIT for no obvious reason. Wouldn't it be more suitable to allocate them with GFP_KERNEL and fail with -ENOMEM in case of an error? Or is there some policy I'm not aware of?

Without more details it is hard to tell, maybe it has something to do with GFP_KERNEL also using __GFP_IO?

Quoted 9 lines

Regards,
Elias
From: Elias Oltmanns <[email protected]>
Subject: ide: Remove ide_spin_wait_hwgroup() and use special requests instead
Use a special request for serialisation purposes and get rid of the awkward ide_spin_wait_hwgroup().

This is really a 'compact' patch description for a rather large patch.

Especially given that there are some by-the-way improvements worth bragging about (i.e. 'shared' struct ide_devset). :)

[...]

drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 722

Quoted 12 lines

drivers/ide/ide-io.c +4 -0

index ec6664b..5451e50 100644
@@ -716,9 +716,63 @@ static ide_startstop_t execute_drive_cmd (ide_drive_t *drive,
  	return ide_stopped;
 }
 
+typedef struct {
+	u8 opcode;	/* always == REQ_DEVSET_EXEC */
+	int arg;
+} ide_devset_cdb_t;

Generally we don't want new typedefs in kernel
and here we shouldn't even need new struct.

Quoted 11 lines

drivers/ide/ide-io.c +11 -0

+#define DEVSET_CDB_LEN (sizeof(ide_devset_cdb_t))
+
+int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting,
+		       int arg)
+{
+	struct request_queue *q = drive->queue;
+	struct request *rq;
+	ide_devset_cdb_t *ds;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))

drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 744

Quoted 10 lines

drivers/ide/ide-io.c +10 -0

+		return -EACCES;
+	if (!(setting->flags & DS_SYNC))
+		return setting->set(drive, arg);
+
+	rq = blk_get_request(q, READ, GFP_KERNEL);
+	if (!rq)
+		return -ENOMEM;
+
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	BUILD_BUG_ON(DEVSET_CDB_LEN > BLK_MAX_CDB);

BLK_MAX_CDB would never be < 16 so this seems overcautious

drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 748

Quoted 4 lines

drivers/ide/ide-io.c +4 -0

+	rq->cmd_len = DEVSET_CDB_LEN;
+	ds = (ide_devset_cdb_t *)rq->cmd;
+	ds->opcode = REQ_DEVSET_EXEC;
+	ds->arg = arg;

How's about just doing:

	rq->cmd[0] = REQ_DEVSET_EXEC;
	(int *)rq->cmd[1] = arg;

[...]

drivers/ide/ide-io.c int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev, · line 26

Quoted 7 lines

drivers/ide/ide-io.c +1 -1

@@ -22,9 +22,9 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 	int err = -EOPNOTSUPP;
 
 	for (; s->get_ioctl; s++) {
-		if (s->get && s->get_ioctl == cmd)
+		if (s->get_ioctl == cmd)
 			goto read_val;

What if 'cmd' == -1?

[ That was the reason for s->get / s->set checks. ]

drivers/ide/ide-io.c set_val: · line 46

Quoted 6 lines

drivers/ide/ide-io.c +0 -2

@@ -42,13 +42,9 @@ set_val:
 	if (bdev != bdev->bd_contains)
 		err = -EINVAL;
 	else {
-		if (!capable(CAP_SYS_ADMIN))
-			err = -EACCES;

I would prefer that CAP_SYS_ADMIN check here and in ide_write_setting() to stay in their places and be done before checking other things (so error codes returned to user-space remain unchanged).

There is also a few CodingStyle errors/warnings catched by checkpatch.pl (some look bogus but others seem legitimate).

Otherwise it looks all good.

Thanks,
Bart

Elias Oltmanns

author

2008-08-13 15:26 UTC #2 lore raw

Quoted 3 lines · Bartlomiej Zolnierkiewicz

Hi,
On Monday 11 August 2008, Elias Oltmanns wrote:

[...]

Quoted 7 lines

Quoted 4 lines
On a different matter, I've encountered several places where requests are being allocated with __GFP_WAIT for no obvious reason. Wouldn't it be more suitable to allocate them with GFP_KERNEL and fail with -ENOMEM in case of an error? Or is there some policy I'm not aware of?
Without more details it is hard to tell, maybe it has something to do with GFP_KERNEL also using __GFP_IO?

I'll follow up with a patch to discuss the matter further.

[...]

Quoted 8 lines

drivers/ide/ide-io.c +0 -0

index ec6664b..5451e50 100644
@@ -716,9 +716,63 @@ static ide_startstop_t execute_drive_cmd (ide_drive_t *drive,
  	return ide_stopped;
 }

Quoted 7 lines

drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 722
Quoted 4 lines
drivers/ide/ide-io.c +4 -0
+typedef struct {
+	u8 opcode;	/* always == REQ_DEVSET_EXEC */
+	int arg;
+} ide_devset_cdb_t;
Generally we don't want new typedefs in kernel
and here we shouldn't even need new struct.

As far as typedefs are concerned, fair enough. With regard to the struct, see below.

Quoted 32 lines

Quoted 12 lines
drivers/ide/ide-io.c +11 -0
+#define DEVSET_CDB_LEN (sizeof(ide_devset_cdb_t))
+
+int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting,
+		       int arg)
+{
+	struct request_queue *q = drive->queue;
+	struct request *rq;
+	ide_devset_cdb_t *ds;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))
drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 743
Quoted 10 lines
drivers/ide/ide-io.c +10 -0
+		return -EACCES;
+	if (!(setting->flags & DS_SYNC))
+		return setting->set(drive, arg);
+
+	rq = blk_get_request(q, READ, GFP_KERNEL);
+	if (!rq)
+		return -ENOMEM;
+
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	BUILD_BUG_ON(DEVSET_CDB_LEN > BLK_MAX_CDB);
BLK_MAX_CDB would never be < 16 so this seems overcautious
drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 747
Quoted 4 lines
drivers/ide/ide-io.c +4 -0
+	rq->cmd_len = DEVSET_CDB_LEN;
+	ds = (ide_devset_cdb_t *)rq->cmd;
+	ds->opcode = REQ_DEVSET_EXEC;
+	ds->arg = arg;
How's about just doing:

Quoted 2 lines

	rq->cmd[0] = REQ_DEVSET_EXEC;
	(int *)rq->cmd[1] = arg;

CC [M] drivers/ide/ide-io.o
drivers/ide/ide-io.c: In function 'ide_devset_execute':
drivers/ide/ide-io.c:748: warning: cast to pointer from integer of different size drivers/ide/ide-io.c:748: error: lvalue required as left operand of assignment

Personally, I'd feel more comfortable with casting rq->cmd to a dedicated struct, especially since I'm not exactly a wizard when it comes to casting. Naturally, if you prefer something else (and I get it to work), I'll happily accept that too.

Quoted 6 lines

[...]

Quoted 3 lines

drivers/ide/ide-io.c +0 -0

@@ -22,9 +22,9 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 	int err = -EOPNOTSUPP;

Quoted 8 lines

drivers/ide/ide-io.c int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev, · line 26
Quoted 4 lines
drivers/ide/ide-io.c +1 -1
 	for (; s->get_ioctl; s++) {
-		if (s->get && s->get_ioctl == cmd)
+		if (s->get_ioctl == cmd)
 			goto read_val;
What if 'cmd' == -1?
[ That was the reason for s->get / s->set checks. ]

The obvious question, don't know why I didn't realise.

Quoted 11 lines

drivers/ide/ide-io.c set_val: · line 46
Quoted 7 lines
drivers/ide/ide-io.c +0 -2
@@ -42,13 +42,9 @@ set_val:
 	if (bdev != bdev->bd_contains)
 		err = -EINVAL;
 	else {
-		if (!capable(CAP_SYS_ADMIN))
-			err = -EACCES;
I would prefer that CAP_SYS_ADMIN check here and in ide_write_setting() to stay in their places and be done before checking other things (so error codes returned to user-space remain unchanged).

Right you are.

Quoted 4 lines

There is also a few CodingStyle errors/warnings catched by checkpatch.pl
(some look bogus but others seem legitimate).

Now this really is rather embarrassing. Apparently, I still have to get used to run checkpatch.pl regularly.

Thanks for reviewing. Find the revised patch below (applies to next-20080813).

Elias

From: Elias Oltmanns <[email protected]>
Subject: ide: Remove ide_spin_wait_hwgroup() and use special requests instead

Use a special request for serialisation purposes and get rid of the awkward ide_spin_wait_hwgroup(). This also involves converting the ide_devset structure so it can be shared by the /proc and the ioctl code.

Signed-off-by Elias Oltmanns <[email protected]>

---

 drivers/ide/ide-cd.c     |    8 +--
 drivers/ide/ide-disk.c   |   66 +++++++++++-----------
 drivers/ide/ide-floppy.c |   20 +++----
 drivers/ide/ide-io.c     |   50 +++++++++++++++++
 drivers/ide/ide-ioctls.c |   21 ++++---
 drivers/ide/ide-proc.c   |  102 ++++++++++++++++------------------
 drivers/ide/ide-tape.c   |   48 ++++++++--------
 drivers/ide/ide.c        |   81 +++++----------------------
 drivers/scsi/ide-scsi.c  |   34 ++++++-----
 include/linux/ide.h      |  138 ++++++++++++++++++++++++----------------------
 10 files changed, 284 insertions(+), 284 deletions(-)

drivers/ide/ide-cd.c +4 -4

index b0e248e..5c23ec9 100644
@@ -1807,11 +1807,11 @@ static ide_proc_entry_t idecd_proc[] = {
 	{ NULL, 0, NULL, NULL }
 };
 
-ide_devset_rw(dsc_overlap, 0, 1, dsc_overlap);
+ide_devset_rw_field(dsc_overlap, dsc_overlap);
 
-static const struct ide_devset *idecd_settings[] = {
-	&ide_devset_dsc_overlap,
-	NULL
+static const struct ide_proc_devset idecd_settings[] = {
+	IDE_PROC_DEVSET(dsc_overlap, 0, 1),
+	{ 0 },
 };
 #endif

drivers/ide/ide-disk.c +32 ~~-34~~

87 lines not on this page · raw message

drivers/ide/ide-floppy.c +10 ~~-10~~

index a63aba2..d36f155 100644
@@ -629,9 +629,9 @@ static sector_t idefloppy_capacity(ide_drive_t *drive)
 }
 
 #ifdef CONFIG_IDE_PROC_FS
-ide_devset_rw(bios_cyl,  0, 1023, bios_cyl);
-ide_devset_rw(bios_head, 0,  255, bios_head);
-ide_devset_rw(bios_sect, 0,   63, bios_sect);
+ide_devset_rw_field(bios_cyl, bios_cyl);
+ide_devset_rw_field(bios_head, bios_head);
+ide_devset_rw_field(bios_sect, bios_sect);
 
 static int get_ticks(ide_drive_t *drive)
 {

@@ -646,14 +646,14 @@ static int set_ticks(ide_drive_t *drive, int arg)
 	return 0;
 }
 
-IDE_DEVSET(ticks, S_RW, 0, 255, get_ticks, set_ticks);
+IDE_DEVSET(ticks, DS_SYNC, get_ticks, set_ticks);
 
-static const struct ide_devset *idefloppy_settings[] = {
-	&ide_devset_bios_cyl,
-	&ide_devset_bios_head,
-	&ide_devset_bios_sect,
-	&ide_devset_ticks,
-	NULL
+static const struct ide_proc_devset idefloppy_settings[] = {
+	IDE_PROC_DEVSET(bios_cyl,  0, 1023),
+	IDE_PROC_DEVSET(bios_head, 0,  255),
+	IDE_PROC_DEVSET(bios_sect, 0,   63),
+	IDE_PROC_DEVSET(ticks,	   0,  255),
+	{ 0 },
 };
 #endif

drivers/ide/ide-io.c +50 -0

59 lines not on this page · raw message

drivers/ide/ide-ioctls.c +11 ~~-10~~

index 7a0d62e..cf01564 100644
@@ -6,11 +6,11 @@
 #include <linux/ide.h>
 
 static const struct ide_ioctl_devset ide_ioctl_settings[] = {
-{ HDIO_GET_32BIT,	 HDIO_SET_32BIT,	get_io_32bit,  set_io_32bit  },
-{ HDIO_GET_KEEPSETTINGS, HDIO_SET_KEEPSETTINGS,	get_ksettings, set_ksettings },
-{ HDIO_GET_UNMASKINTR,	 HDIO_SET_UNMASKINTR,	get_unmaskirq, set_unmaskirq },
-{ HDIO_GET_DMA,		 HDIO_SET_DMA,		get_using_dma, set_using_dma },
-{ -1,			 HDIO_SET_PIO_MODE,	NULL,	       set_pio_mode  },
+{ HDIO_GET_32BIT,	 HDIO_SET_32BIT,	&ide_devset_io_32bit  },
+{ HDIO_GET_KEEPSETTINGS, HDIO_SET_KEEPSETTINGS,	&ide_devset_keepsettings },
+{ HDIO_GET_UNMASKINTR,	 HDIO_SET_UNMASKINTR,	&ide_devset_unmaskirq },
+{ HDIO_GET_DMA,		 HDIO_SET_DMA,		&ide_devset_using_dma },
+{ -1,			 HDIO_SET_PIO_MODE,	&ide_devset_pio_mode  },
 { 0 }
 };

@@ -18,13 +18,14 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 		      unsigned int cmd, unsigned long arg,
 		      const struct ide_ioctl_devset *s)
 {
+	const struct ide_devset *ds;
 	unsigned long flags;
 	int err = -EOPNOTSUPP;
 
-	for (; s->get_ioctl; s++) {
-		if (s->get && s->get_ioctl == cmd)
+	for (; (ds = s->setting); s++) {
+		if (ds->get && s->get_ioctl == cmd)
 			goto read_val;
-		else if (s->set && s->set_ioctl == cmd)
+		else if (ds->set && s->set_ioctl == cmd)
 			goto set_val;
 	}

@@ -33,7 +34,7 @@ int ide_setting_ioctl(ide_drive_t *drive, struct block_device *bdev,
 read_val:
 	mutex_lock(&ide_setting_mtx);
 	spin_lock_irqsave(&ide_lock, flags);
-	err = s->get(drive);
+	err = ds->get(drive);
 	spin_unlock_irqrestore(&ide_lock, flags);
 	mutex_unlock(&ide_setting_mtx);
 	return err >= 0 ? put_user(err, (long __user *)arg) : err;

@@ -46,7 +47,7 @@ set_val:
 			err = -EACCES;
 		else {
 			mutex_lock(&ide_setting_mtx);
-			err = s->set(drive, arg);
+			err = ide_devset_execute(drive, ds, arg);
 			mutex_unlock(&ide_setting_mtx);
 		}
 	}

drivers/ide/ide-proc.c +48 ~~-54~~

171 lines not on this page · raw message

drivers/ide/ide-tape.c +24 ~~-24~~

64 lines not on this page · raw message

drivers/ide/ide.c +15 ~~-66~~

152 lines not on this page · raw message

drivers/scsi/ide-scsi.c +17 ~~-17~~

index 3cd3d2a..e9256d2 100644
@@ -400,25 +400,25 @@ static int set_##name(ide_drive_t *drive, int arg) \
 	return 0; \
 }
 
-#define ide_scsi_devset_rw(_name, _min, _max, _field) \
+#define ide_scsi_devset_rw_field(_name, _field) \
 ide_scsi_devset_get(_name, _field); \
 ide_scsi_devset_set(_name, _field); \
-IDE_DEVSET(_name, S_RW, _min, _max, get_##_name, set_##_name)
-
-ide_devset_rw(bios_cyl,		0, 1023, bios_cyl);
-ide_devset_rw(bios_head,	0,  255, bios_head);
-ide_devset_rw(bios_sect,	0,   63, bios_sect);
-
-ide_scsi_devset_rw(transform,	0,    3, transform);
-ide_scsi_devset_rw(log,		0,    1, log);
-
-static const struct ide_devset *idescsi_settings[] = {
-	&ide_devset_bios_cyl,
-	&ide_devset_bios_head,
-	&ide_devset_bios_sect,
-	&ide_devset_log,
-	&ide_devset_transform,
-	NULL
+IDE_DEVSET(_name, DS_SYNC, get_##_name, set_##_name);
+
+ide_devset_rw_field(bios_cyl, bios_cyl);
+ide_devset_rw_field(bios_head, bios_head);
+ide_devset_rw_field(bios_sect, bios_sect);
+
+ide_scsi_devset_rw_field(transform, transform);
+ide_scsi_devset_rw_field(log, log);
+
+static const struct ide_proc_devset idescsi_settings[] = {
+	IDE_PROC_DEVSET(bios_cyl,  0, 1023),
+	IDE_PROC_DEVSET(bios_head, 0,  255),
+	IDE_PROC_DEVSET(bios_sect, 0,	63),
+	IDE_PROC_DEVSET(log,	   0,	 1),
+	IDE_PROC_DEVSET(transform, 0,	 3),
+	{ 0 },
 };
 #endif

include/linux/ide.h +73 ~~-65~~

205 lines not on this page · raw message

Bartlomiej Zolnierkiewicz

Applied

2008-08-13 21:06 UTC #3 lore raw

Quoted 67 lines · Elias Oltmanns

Quoted 3 lines · Bartlomiej Zolnierkiewicz
Hi,
On Monday 11 August 2008, Elias Oltmanns wrote:
[...]
Quoted 7 lines
Quoted 4 lines
On a different matter, I've encountered several places where requests are being allocated with __GFP_WAIT for no obvious reason. Wouldn't it be more suitable to allocate them with GFP_KERNEL and fail with -ENOMEM in case of an error? Or is there some policy I'm not aware of?
Without more details it is hard to tell, maybe it has something to do with GFP_KERNEL also using __GFP_IO?
I'll follow up with a patch to discuss the matter further.
[...]
Quoted 8 lines
Quoted 8 lines
drivers/ide/ide-io.c +0 -0
index ec6664b..5451e50 100644
@@ -716,9 +716,63 @@ static ide_startstop_t execute_drive_cmd (ide_drive_t *drive,
  	return ide_stopped;
 }
 
Quoted 7 lines
drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 722
Quoted 4 lines
drivers/ide/ide-io.c +4 -0
+typedef struct {
+	u8 opcode;	/* always == REQ_DEVSET_EXEC */
+	int arg;
+} ide_devset_cdb_t;
Generally we don't want new typedefs in kernel
and here we shouldn't even need new struct.
As far as typedefs are concerned, fair enough. With regard to the struct, see below.
Quoted 32 lines
Quoted 12 lines
drivers/ide/ide-io.c +11 -0
+#define DEVSET_CDB_LEN (sizeof(ide_devset_cdb_t))
+
+int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting,
+		       int arg)
+{
+	struct request_queue *q = drive->queue;
+	struct request *rq;
+	ide_devset_cdb_t *ds;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))
drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 743
Quoted 10 lines
drivers/ide/ide-io.c +10 -0
+		return -EACCES;
+	if (!(setting->flags & DS_SYNC))
+		return setting->set(drive, arg);
+
+	rq = blk_get_request(q, READ, GFP_KERNEL);
+	if (!rq)
+		return -ENOMEM;
+
+	rq->cmd_type = REQ_TYPE_SPECIAL;
+	BUILD_BUG_ON(DEVSET_CDB_LEN > BLK_MAX_CDB);
BLK_MAX_CDB would never be < 16 so this seems overcautious
drivers/ide/ide-io.c static ide_startstop_t execute_drive_cmd (ide_drive_t *drive, · line 747
Quoted 4 lines
drivers/ide/ide-io.c +4 -0
+	rq->cmd_len = DEVSET_CDB_LEN;
+	ds = (ide_devset_cdb_t *)rq->cmd;
+	ds->opcode = REQ_DEVSET_EXEC;
+	ds->arg = arg;
How's about just doing:

Quoted 7 lines

Quoted 2 lines
	rq->cmd[0] = REQ_DEVSET_EXEC;
	(int *)rq->cmd[1] = arg;
CC [M] drivers/ide/ide-io.o
drivers/ide/ide-io.c: In function 'ide_devset_execute':
drivers/ide/ide-io.c:748: warning: cast to pointer from integer of different size drivers/ide/ide-io.c:748: error: lvalue required as left operand of assignment

Heh, I must have been falling asleep already when writing this, it should have been:

	*(int *)&rq->cmd[1] = arg;

Quoted 4 lines

Personally, I'd feel more comfortable with casting rq->cmd to a dedicated struct, especially since I'm not exactly a wizard when it comes to casting. Naturally, if you prefer something else (and I get it to work), I'll happily accept that too.

What I find ugly about this dedicated struct is that it overlaps with rq->cmd[0] and in all other places we just use rq->cmd[0] directly (+ it is very easy for people unfamiliar with the code to overlook it).

Then if 'opcode' gets removed all that is left is 'arg' so the struct no loger makes much sense.

I fixed it locally (interdiff attached), I hope you're fine with it.

Quoted 6 lines

Thanks for reviewing. Find the revised patch below (applies to next-20080813).
Elias

Quoted 8 lines

From: Elias Oltmanns <[email protected]>
Subject: ide: Remove ide_spin_wait_hwgroup() and use special requests instead
Use a special request for serialisation purposes and get rid of the awkward ide_spin_wait_hwgroup(). This also involves converting the ide_devset structure so it can be shared by the /proc and the ioctl code.
Signed-off-by Elias Oltmanns <[email protected]>

Thanks, applied.

drivers/ide/ide-io.c +4 ~~-14~~

diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
@@ -716,19 +716,11 @@
  	return ide_stopped;
 }
 
-struct ide_devset_cdb {
-	u8 opcode;	/* always == REQ_DEVSET_EXEC */
-	int arg;
-};
-
-#define DEVSET_CDB_LEN sizeof(struct ide_devset_cdb)
-
 int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting,
 		       int arg)
 {
 	struct request_queue *q = drive->queue;
 	struct request *rq;
-	struct ide_devset_cdb *ds;
 	int ret = 0;
 
 	if (!(setting->flags & DS_SYNC))

@@ -739,10 +731,9 @@
 		return -ENOMEM;
 
 	rq->cmd_type = REQ_TYPE_SPECIAL;
-	rq->cmd_len = DEVSET_CDB_LEN;
-	ds = (struct ide_devset_cdb *)rq->cmd;
-	ds->opcode = REQ_DEVSET_EXEC;
-	ds->arg = arg;
+	rq->cmd_len = 5;
+	rq->cmd[0] = REQ_DEVSET_EXEC;
+	*(int *)&rq->cmd[1] = arg;
 	rq->special = setting->set;
 
 	if (blk_execute_rq(q, NULL, rq, 0))

@@ -758,10 +749,9 @@
 	switch (rq->cmd[0]) {
 	case REQ_DEVSET_EXEC:
 	{
-		struct ide_devset_cdb *ds = (struct ide_devset_cdb *)rq->cmd;
 		int err, (*setfunc)(ide_drive_t *, int) = rq->special;
 
-		err = setfunc(drive, ds->arg);
+		err = setfunc(drive, *(int *)&rq->cmd[1]);
 		if (err)
 			rq->errors = err;
 		else

Elias Oltmanns

author

2008-08-13 21:17 UTC #4 lore raw

Quoted 6 lines · Bartlomiej Zolnierkiewicz

Quoted 5 lines · Elias Oltmanns
Quoted 3 lines · Bartlomiej Zolnierkiewicz
Hi,
On Monday 11 August 2008, Elias Oltmanns wrote:

[...]

Quoted 15 lines

Quoted 7 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+	rq->cmd_len = DEVSET_CDB_LEN;
+	ds = (ide_devset_cdb_t *)rq->cmd;
+	ds->opcode = REQ_DEVSET_EXEC;
+	ds->arg = arg;
How's about just doing:
Quoted 7 lines
Quoted 2 lines
	rq->cmd[0] = REQ_DEVSET_EXEC;
	(int *)rq->cmd[1] = arg;
CC [M] drivers/ide/ide-io.o
drivers/ide/ide-io.c: In function 'ide_devset_execute':
drivers/ide/ide-io.c:748: warning: cast to pointer from integer of different size drivers/ide/ide-io.c:748: error: lvalue required as left operand of assignment

Quoted 4 lines

Heh, I must have been falling asleep already when writing this, it should have been:
	*(int *)&rq->cmd[1] = arg;

Well, this I can make sense of.

Quoted 9 lines

Quoted 5 lines
Personally, I'd feel more comfortable with casting rq->cmd to a dedicated struct, especially since I'm not exactly a wizard when it comes to casting. Naturally, if you prefer something else (and I get it to work), I'll happily accept that too.
What I find ugly about this dedicated struct is that it overlaps with rq->cmd[0] and in all other places we just use rq->cmd[0] directly (+ it is very easy for people unfamiliar with the code to overlook it).

Yes, I see.

Quoted 5 lines

Then if 'opcode' gets removed all that is left is 'arg' so the struct no loger makes much sense.
I fixed it locally (interdiff attached), I hope you're fine with it.

Certainly.

Regards,

Elias