Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

Borislav Petkov
2009-03-05T13:53:41Z

Hi,

On Thu, Mar 5, 2009 at 1:12 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>
> Hi,
>
> On Wednesday 04 March 2009, Borislav Petkov wrote:
>> ... since some do not transfer any data from the drive and
>> rq->buffer is unset leading to an OOPS when checking VM translations
>> (CONFIG_DEBUG_VIRTUAL).
>>
>> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>
> Thanks for fixing it but I worry that the patch is not entirely correct
> (why o why did you have to throw in unrelated changes...?)

What unrelated changes? This is the ide-cd fix for when mapping a NULL
rq->buffer pointer to an sg. If you mean the [2/3] patch, I had to
remove the partial completions for ide-floppy because it was OOPSing on NULL
rq-pointer in the interrupt handler. I sent you a pretty detailed OOPS along
with backtracking to the offending code line, remember? Also, for reasons of
bisectability.

> also ideally there should have been two patches:
> - minimal bugfix for the buggy patch
> - unification/cleanup of PIO sg setup
>
>> ---
>>  drivers/ide/ide-atapi.c  |    9 ++++++++-
>>  drivers/ide/ide-cd.c     |    4 ----
>>  drivers/ide/ide-floppy.c |    4 ----
>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>

..

>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
>> index 091d414..106cacb 100644
>> --- a/drivers/ide/ide-cd.c
>> +++ b/drivers/ide/ide-cd.c
>> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>>
>>       cmd.rq = rq;
>>
>> -     ide_init_sg_cmd(&cmd,
>> -             blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
>> -     ide_map_sg(drive, &cmd);
>> -
>>       return ide_issue_pc(drive, &cmd);
>>  out_end:
>>       nsectors = rq->hard_nr_sectors;
>> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
>> index 2f8f453..b91deef 100644
>> --- a/drivers/ide/ide-floppy.c
>> +++ b/drivers/ide/ide-floppy.c
>> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>>               cmd.tf_flags |= IDE_TFLAG_WRITE;
>>
>>       cmd.rq = rq;
>> -
>> -     ide_init_sg_cmd(&cmd, pc->req_xfer);
>
> There was a reason for using ->req_xfer.

Yeah, this is also one of those painful places where I cringe everytime I have
to look at. We finally have to sit down and decide which is which; sometimes
pc->req_xfer _is_ rq->data_len (idefloppy_blockpc_cmd()) and sometimes obviously
not. I'll take a look at that whole mess next and try to come up with something
more clean.

> I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
> (ide_queue_pc_tail() and friends)?

me neither :(.

>> -     ide_map_sg(drive, &cmd);
>> -
>>       pc->rq = rq;
>>
>>       return ide_floppy_issue_pc(drive, &cmd, pc);

Man, this sucks!

--
Regards/Gruss,
Boris