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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Thursday 05 March 2009, Borislav Petkov wrote:
> 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

Unrelated to the original issue that the patch tries to fix
(sg mapping no data commands which OOPS-es w/ DEBUG_VIRTUAL=y):
- moving ide_init_sg_cmd()+ide_map_sg() to ide_issue_pc()
- converting the code to use blk_rq_bytes()

[ Moreover if you actually cared to write down the proper changelog
  I'm pretty sure that you would ask yourself whether these changes
  really belong to a bugfix patch... ]

Why not simply do what originally has been suggested:

--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -916,9 +916,11 @@ static ide_startstop_t ide_cd_do_request
 
 	cmd.rq = rq;
 
-	ide_init_sg_cmd(&cmd,
-		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
-	ide_map_sg(drive, &cmd);
+	if (blk_fs_request(rq) || rq->data_len) {
+		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:

?

The bonus is that it can be either queued after the problematic patch
or just merged with it later (even if it needs to be applied by-hand
the change is so trivial that it is very unlikely to cause problems).

Doing cleanup is fine but not at the same time because we don't want to
make things more complex (they are pretty complex already) and thus risk
introducing new bugs.  Yes, it is tempting to do few things at once and
save some work, and it is also perfectly fine to do it sometimes but we
have to draw a line somewhere...

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

I meant this patch, 2/3 is fine and was applied
(thanks for fixing it again).

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

This is exactly my point -- these things are tricky so it is very risky to do
drive-by cleanups together with fixes.

> not. I'll take a look at that whole mess next and try to come up with something
> more clean.

Ok.

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

:)