Re: 2.5.69-70 ide-cd to guarantee fault-free CD/DVD burning experience?

Jens Axboe
2003-05-28T16:50:33Z

On Wed, May 28 2003, Andy Polyakov wrote:
> > > As for scsi_ioctl.c in more general sense. It apparently doesn't comply
> > > with SG HOWTO, in particular it mis-interprets time-out values.
> > > Background information and patch is available at
> > > http://fy.chalmers.se/~appro/linux/DVD+RW/scsi_ioctl-2.5.69.patch.
> > > There're couple of other issues, usage of 'bytes' variable in access_ok
>
> bytes = (hdr.dxfer_len + 511) & ~511;
> ... access_ok(VERIFY_READ, uaddr, bytes)...
> ^^^^^ Shouldn't this be
> hdr.dxfer_len? At least that's what memcpy-ed to kalloc-ated buffer.

Yes!

> > > and DMA being off when bio_map_user fails,
>
> While tracing problems down I've commented out call to bio_map_user. But
> of course it could have failed for more legitimate reasons, couldn't it?
> Reasons such as user buffer residing in non DMA-capable region(?) or
> being misaligned. But in either case I noticed that DMA is never engaged

Correct.

> on that buffer allocated with kmalloc. The question is if it's
> intentional? If answer is yes, then the case is dismissed. If not, then
> it should be looked into. Now I don't know if it's apporpriate to

Depends on the lower level driver, for ide-cd yes kmalloc'ed data will
not be dma'ed to. We require a valid bio setup for that, usually the bio
mapping will fail exactly because the length/alignment isn't correct for
ide-cd.

sr will dma to the kmalloced buffer just fine.

> complement GPF_USER with GFP_DMA, but it might be appropriate to retry
> bio_map_user on buffer. I'm actually stepping out of my competence
> domains here...

It's usually not worth it. If the buffer is < 4 bytes, we don't dma. Big
deal. It's the programs responsibility to make sure that data + length
is appropriately aligned for dma operations akin to O_DIRECT for
instance. And they already do that, so...

> > > @@ -1471,8 +1472,13 @@
> > > /* Keep count of how much data we've moved. */
> > > rq->data += thislen;
> > > rq->data_len -= thislen;
> > > +#if 0
> > > if (rq->cmd[0] == GPCMD_REQUEST_SENSE)
> > > rq->sense_len++;
> > > +#else
> > > + if (rq->flags & REQ_SENSE)
> > > + rq->sense_len+=thislen;
> > > +#endif
> > > } else {
> > > confused:
> > > printk ("%s: cdrom_pc_intr: The drive "
> >
> > Hmm confused, care to expand?
>
> rq->sense_len++ is obviously bogus as user-land will only get the first
> byte of the sense data [so that you can tell apart deferred and
> immediate errors, but you can't tell what was actually wrong]. As for
> "if (rq->cmd[0] == GPCMD_REQUEST_SENSE)" vs. "if (rq->flags &
> REQ_SENSE)." User-land is permitted to issue REQUEST SENSE on it's own
> behalf, isn't it? With "rq->cmd[0] == GPCMD_REQUEST_SENSE" kernel will
> provide user-land with sense buffer with bogus data (even if it's
> zeros:-). "rq->flags & REQ_SENSE" implies "rq->cmd[0] ==
> GPCMD_REQUEST_SENSE" as it happens only when kernel itself pulls the
> sense data on behalf of failed command and that's exactly what should be
> returned to user. Or is it #if 0/#else/#endif which is confusing? Well,
> we don't have to keep that, it's just left-overs from my working copy...

Ah good point on the REQ_SENSE bit, completely agree. The if 0 thing
cannot go in obviously, I'll kill that along the way.

> > Sorry I misread that, ->data is the one we want. I'm wondering how this
> > got mixed up... So to clarify:
> >
> > char *ibuf = req->data;
> >
> > if (!blk_pc_request(req))
> > return;
> > if (!ibuf)
> > return;
>
> But req->data is assigned NULL every time bio_map_user succeeds! Just
> follow it in sg_io():
>
> buffer=NULL; ...
> bio=bio_map_user(...);
> if (!bio) buffer=kmalloc(...);
> rq->data=buffer;

Hmm it looks pretty bogus actually, in most cases we have already
removed ->bio at this point.

> So that if(!req->data) is true most of the time [as bio_map_user
> succeeds most of the time]... As for req->buffer. Given that only first
> 4 bytes/32 bits are manipulated it's actually safe to dereference it
> directly, isn't it? A.

->buffer is not to be used in this context, so forget that. It's a relic
from when the pre-transform allocated extra data and we copied back and
freed in post-transform. That was killed, and I'm really wondering
whether we shouldn't just kill the post-transform completely too. For
reference, this is what is should look like...

===== drivers/ide/ide-cd.c 1.46 vs edited =====
--- 1.46/drivers/ide/ide-cd.c Thu May 8 10:39:34 2003
+++ edited/drivers/ide/ide-cd.c Wed May 28 19:02:10 2003
@@ -1609,12 +1609,19 @@

static void post_transform_command(struct request *req)
{
- char *ibuf = req->buffer;
u8 *c = req->cmd;
+ char *ibuf;

if (!blk_pc_request(req))
return;

+ if (rq->data)
+ ibuf = rq->data;
+ else if (rq->bio)
+ ibuf = bio_data(rq->bio);
+ else
+ return;
+
/*
* set ansi-revision and response data as atapi
*/
@@ -1664,8 +1671,8 @@
if (dma_error)
return DRIVER(drive)->error(drive, "dma error", stat);

+ post_transform_command(rq);
end_that_request_chunk(rq, 1, rq->data_len);
- rq->data_len = 0;
goto end_request;
}

@@ -1687,6 +1694,7 @@
if ((stat & DRQ_STAT) == 0) {
if (rq->data_len)
printk("%s: %u residual after xfer\n", __FUNCTION__, rq->data_len);
+ post_transform_command(rq);
goto end_request;
}

@@ -1765,9 +1773,6 @@
return ide_started;

end_request:
- if (!rq->data_len)
- post_transform_command(rq);
-
spin_lock_irqsave(&ide_lock, flags);
blkdev_dequeue_request(rq);
end_that_request_last(rq);

--
Jens Axboe