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 <eo@xxxxxxxxxxxxxx>
> 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). :)
[...]
> 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.
> +#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;
[...]
> @@ -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. ]
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
