Hi,
On Sun, Jun 14, 2009 at 02:32:10PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 14 June 2009 12:06:06 Borislav Petkov wrote:
> > Hi,
> >
> > On Sat, Jun 13, 2009 at 06:59:53PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > --- Comment #20 from Borislav Petkov <bbpetkov@xxxxxxxx> 2009-06-13 16:29:05 ---
> > > > Hi Bart,
> > > >
> > > > thanks for analyzing this.
> > > >
> > > > I'm staring at the ATA_DRQ == 0 part in cdrom_newpc_intr:
> > > >
> > > > } else if (!blk_pc_request(rq)) {
> > > > ide_cd_request_sense_fixup(drive, cmd);
> > > > /* complain if we still have data left to transfer */
> > > > uptodate = cmd->nleft ? 0 : 1;
> > > > if (uptodate == 0)
> > > > rq->cmd_flags |= REQ_FAILED;
> > > > }
> > > > goto out_end;
> > > > }
> > > >
> > > > so, in our case ide_cd_error_cmd() kills the rq prematurely and that's
> > > > why ide_complete_rq() oopses later. And this is caused by uptodate ==
> > >
> > > Nope, it is block layer that kills it prematurely.
> >
> > ok, I still need to understand the whole code flow properly so please
> > bare with me.
> >
> > I got misled by the __blk_end_request() thing: am I right to assume that
> > you were using it to give a better example where it is more clear that
> > the block layer really kills rq->bio-less requests?
> >
> > Because we don't hit __blk_end_request from ide_cd_error_cmd() (or
> > ide_complete_rq() too, for that matter) - we do rather:
> >
> > ide_cd_error_cmd() does ide_complete_rq(), which ends up doing
> > blk_end_request(), then blk_end_io() and the rq->bio thing is checked
> > in end_that_request_data(). Which is actually the same thing but done
> > slightly differently.
>
> Yes, you're seeing block layer updates from 2 days ago.
Nope, I'm staring at .30-rc7 sources. I guess those updates are a bit
older.
> > > There are two issues here:
> > >
> > > * OOPS (*the*regression* which should be taken care of first (cause:
> > > unexpected interaction between ide/block)
> >
> > I'm thinking something like
> >
> > if (uptodate == 0 && !(OK_STAT(stat, 0, BAD_STAT)))
> > ide_cd_error_cmd(drive, cmd);
>
> Those are two separate issues, please stop mixing them.
>
> AFAICS ide-cd has been always failing !uptodate requests so the latter
> issue is nothing new. Which means that it is really not the right time
> to be scratching our heads about it while the former problem has been
> seriously affecting people for weeks now and also managed to slip into
> the final 2.6.30 release..
Ok, let's do that:
@Hans: can you apply the following ontop of the debugging patch and test and
send us the whole dmesg, thanks.
--
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 6f728da..219c303 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -764,7 +764,7 @@ out_end:
rq->errors = -EIO;
}
- if (uptodate == 0)
+ if (uptodate == 0 && rq->bio)
ide_cd_error_cmd(drive, cmd);
/* make sure it's fully ended */
--
> > > * handling of non-fully completed requests with "good" status (cause:
> > > stupid hardware)
> >
> > The non-intrusive solution, IMHO, would be to add another quirk flag for
> > such a device (SONY DVD-ROM DDU1615) and do not complete the rq in that
> > case, aka no partial completion for packet commands to that device. I'm
> > wondering what else is broken with it especially if you're requiring
> > bigger buffers like the capacities page.
> >
> > So, how about an ide_cd_complete_rq() helper which hides such special
> > cases and is called as an indirection at the end of the irq handler?
>
> I can't tell much without seeing the concept with more details.
>
> IOW please send patches, that is the best way to discuss things.
Ok.
--
Regards/Gruss,
Boris.
--
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
