by Bartlomiej Zolnierkiewicz

[permalink] [raw]

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

On Thursday 12 March 2009, Borislav Petkov wrote:
> On Wed, Mar 11, 2009 at 05:34:28PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 10 March 2009, Borislav Petkov wrote:
> > > Hi,
> > >
> > > > If the mainline is broken sg fix can wait but to be honest I don't see much
> > > > point in delaying it (it is an independent problem and the bugfix should be
> > > > a completely safe one-liner).
> > >
> > > --
> > > From: Borislav Petkov <[email protected]>
> > > Date: Tue, 10 Mar 2009 07:04:52 +0100
> > > Subject: [PATCH] ide-floppy: do not map dataless cmds to an sg
> > >
> > > since it fails the virt_to_page() translation check with DEBUG_VIRTUAL
> > > enabled.
> > >
> > > Signed-off-by: Borislav Petkov <[email protected]>
> >
> > I applied it with some changes:
> >
> > > ---
> > > drivers/ide/ide-atapi.c | 12 ++++++++++++
> > > drivers/ide/ide-floppy.c | 6 ++++--
> > > 2 files changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> > > index a5596a6..11a680c 100644
> > > --- a/drivers/ide/ide-atapi.c
> > > +++ b/drivers/ide/ide-atapi.c
> > > @@ -90,6 +90,12 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
> > > rq->cmd_flags |= REQ_PREEMPT;
> > > rq->buffer = (char *)pc;
> > > rq->rq_disk = disk;
> > > +
> > > + if (pc->req_xfer) {
> > > + rq->data = pc->buf;
> > > + rq->data_len = pc->req_xfer;
> > > + }
> > > +
> > > memcpy(rq->cmd, pc->c, 12);
> > > if (drive->media == ide_tape)
> > > rq->cmd[13] = REQ_IDETAPE_PC1;
> > > @@ -112,6 +118,12 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
> > > rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
> > > rq->cmd_type = REQ_TYPE_SPECIAL;
> > > rq->buffer = (char *)pc;
> > > +
> > > + if (pc->req_xfer) {
> > > + rq->data = pc->buf;
> > > + rq->data_len = pc->req_xfer;
> > > + }
> > > +
> > > memcpy(rq->cmd, pc->c, 12);
> > > if (drive->media == ide_tape)
> > > rq->cmd[13] = REQ_IDETAPE_PC1;
> >
> > ide-atapi.c part doesn't seem to be needed for fixing the issue
> > so I removed it (IMO it would fit much better with your sg setup
> > cleanup patch than this one)
>
> No, you need that part. And especially the rq->data assignment.
> Take a look at ide_floppy_get_capacity() - it calls into
> ide_queue_pc_tail() with pc->req_xfer == 255 resulting from the
> ide_floppy_create_read_capacity_cmd(). However, the rq->data is still
> NULL if you'd remove the chunk I added and you get

[...]

Ah I see it now but this is a separate issue from the original one
(OOPS on DEBUG_VIRTUAL=y)! I somehow missed it (probably becasue
patch description fails to mention this important information)
and thus tried to be smart while merging the patch...

Anyway, I replaced the patch in pata tree by your original one.

Thanks,
Bart

2009-03-12 17:10:23

by Borislav Petkov

[permalink] [raw]

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

Hi,

> Ah I see it now but this is a separate issue from the original one
> (OOPS on DEBUG_VIRTUAL=y)!

Hmm, no, this oopses on the same DEBUG_VIRTUAL check due to rq->data being NULL,
it is the same issue happening when entered from a different path:

> I somehow missed it (probably becasue
> patch description fails to mention this important information)
> and thus tried to be smart while merging the patch...

... and yes, we could add something along the lines of "Make sure ->data pointer
in sg mapping is set from all command issuing paths" to the commit message. My
bad, I'm being too economical wrt commit messages, as always :).

--
Regards/Gruss,
Boris