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

Hi,

On Monday 11 August 2008, Elias Oltmanns wrote:

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.

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?

Regards,

Elias

From: Elias Oltmanns <redacted>
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). :)

[...]

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

+#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;

[...]

What if 'cmd' == -1?

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

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