Re: ide: Remove ide_spin_wait_hwgroup() and use special requests instead

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Wednesday 13 August 2008, Elias Oltmanns wrote:
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > Hi,
> >
> > On Monday 11 August 2008, Elias Oltmanns wrote:
> [...]
> >> 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.
> 
> [...]
> >> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> >> index ec6664b..5451e50 100644
> >> --- a/drivers/ide/ide-io.c
> >> +++ b/drivers/ide/ide-io.c
> >> @@ -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.
> 
> As far as typedefs are concerned, fair enough. With regard to the
> struct, see below.
> 
> >
> >> +#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))
> >> +		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
> >
> >> +	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;
> 
>   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;

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

> Thanks for reviewing. Find the revised patch below (applies to
> next-20080813).
> 
> Elias
> 
> 
> From: Elias Oltmanns <eo@nebensachen.de>
> 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 <eo@nebensachen.de>

Thanks, applied.


diff -u b/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- 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