Bug 12320

Created attachment 19515 [details]
Linux 2.6.28 .config

Created attachment 19516 [details]
full 2.6.28 dmesg

I tested so far the following versions:

2.6.28 bad
2.6.28-rc9 bad
2.6.28-rc3 bad
2.6.28-rc1 good

I started to git bisecting the problem but this might take a while.

Hi,

can you try the following patch on a failing kernel and send me the whole dmesg output?

Thanks.

Created attachment 19520 [details]
dump offending cmd

Created attachment 19528 [details]
git bisect log

(In reply to comment #3)
> I started to git bisecting the problem but this might take a while.

The result of git bisecting is:

9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0 is first bad commit
commit 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0
Author: Borislav Petkov <petkovbb@gmail.com>
Date:   Sun Nov 2 21:40:07 2008 +0100

    ide-cd: fix DMA alignment regression
    
    e5318b531b008c79d2a0c0df06a7b8628da38e2f ("ide: use the dma safe check for
    REQ_TYPE_ATA_PC") introduced a regression which caused some ATAPI drives to
    turn off DMA for REQ_TYPE_BLOCK_PC commands while burning and thus degrading
    performance and ultimately causing an excessive amount of underruns.
    
    The issue is documented also in:
    http://bugzilla.kernel.org/show_bug.cgi?id=11742.
    
    Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
    Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
    Tested-by: Valerio Passini <valerio.passini@unicam.it>
    [bart: fixup patch description per comments from Sergei Shtylyov]
    Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

:040000 040000 998c57acb8e63f2f1d64543f2c03ecfc05beb3a0 fef197de8e3c9db1308917eaeb1c10f196a8a8ab M      drivers





(In reply to comment #4)
> can you try the following patch on a failing kernel and send me the whole
> dmesg output?

I'll have a look.

Created attachment 19529 [details]
full dmesg of 2.6.28 + debug patch from comment #5

(In reply to comment #6)
> (In reply to comment #4)
> > can you try the following patch on a failing kernel and send me the whole
> dmesg output?
> 
> I'll have a look.

Here you are.

It is the same problem as:

http://bugzilla.kernel.org/show_bug.cgi?id=11732

[ also http://marc.info/?l=linux-ide&m=122865770510761&w=2 ]

Hmm... with additional debug data it all starts to make sense now! :)

Borislav, it seems that ide_map_sg() need to check for REQ_TYPE_ATA_PC now that DMA is allowed for such requests (so it can use sg_init_one() path for them instead of the usual blk_rq_map_sg() one)?

(In reply to comment #8)
> It is the same problem as:
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=11732
> 
> [ also http://marc.info/?l=linux-ide&m=122865770510761&w=2 ]
> 
> Hmm... with additional debug data it all starts to make sense now! :)
> 
> Borislav, it seems that ide_map_sg() need to check for REQ_TYPE_ATA_PC now
> that
> DMA is allowed for such requests (so it can use sg_init_one() path for them
> instead of the usual blk_rq_map_sg() one)?

Hmm,

shouldn't we do that for all rq's which aren't bio-backed? As a means to handle the more general case?

Created attachment 19538 [details]
revert 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0

(In reply to comment #6)
> Created an attachment (id=19528) [details]
> git bisect log
> 
> (In reply to comment #3)
> > I started to git bisecting the problem but this might take a while.
> 
> The result of git bisecting is:
> 
> 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0 is first bad commit
> commit 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0
> Author: Borislav Petkov <petkovbb@gmail.com>
> Date:   Sun Nov 2 21:40:07 2008 +0100
> 
>     ide-cd: fix DMA alignment regression
> 
>     e5318b531b008c79d2a0c0df06a7b8628da38e2f ("ide: use the dma safe check
>     for
>     REQ_TYPE_ATA_PC") introduced a regression which caused some ATAPI drives
>     to
>     turn off DMA for REQ_TYPE_BLOCK_PC commands while burning and thus
> degrading
>     performance and ultimately causing an excessive amount of underruns.
> 
>     The issue is documented also in:
>     http://bugzilla.kernel.org/show_bug.cgi?id=11742.
> 
>     Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>     Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>     Tested-by: Valerio Passini <valerio.passini@unicam.it>
>     [bart: fixup patch description per comments from Sergei Shtylyov]
>     Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> :040000 040000 998c57acb8e63f2f1d64543f2c03ecfc05beb3a0
> fef197de8e3c9db1308917eaeb1c10f196a8a8ab M      drivers

Reverting this patch on top of 2.6.28 removes the warn_on_slowpath messages from dmesg.

(In reply to comment #9)
> (In reply to comment #8)
> > It is the same problem as:
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=11732
> > 
> > [ also http://marc.info/?l=linux-ide&m=122865770510761&w=2 ]
> > 
> > Hmm... with additional debug data it all starts to make sense now! :)
> > 
> > Borislav, it seems that ide_map_sg() need to check for REQ_TYPE_ATA_PC now
> that
> > DMA is allowed for such requests (so it can use sg_init_one() path for them
> > instead of the usual blk_rq_map_sg() one)?
> 
> Hmm,
> 
> shouldn't we do that for all rq's which aren't bio-backed? As a means to
> handle
> the more general case?

It would be even better...

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)

[.. ]

> > > Borislav, it seems that ide_map_sg() need to check for REQ_TYPE_ATA_PC
> now that
> > > DMA is allowed for such requests (so it can use sg_init_one() path for
> them
> > > instead of the usual blk_rq_map_sg() one)?
> > 
> > Hmm,
> > 
> > shouldn't we do that for all rq's which aren't bio-backed? As a means to
> handle
> > the more general case?
> 
> It would be even better...

How about something similar to the following:

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index cc35d6d..4bee817 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -574,14 +574,15 @@ void ide_map_sg(ide_drive_t *drive, struct request *rq)
 	ide_hwif_t *hwif = drive->hwif;
 	struct scatterlist *sg = hwif->sg_table;
 
-	if (hwif->sg_mapped)	/* needed by ide-scsi */
-		return;
-
-	if (rq->cmd_type != REQ_TYPE_ATA_TASKFILE) {
-		hwif->sg_nents = blk_rq_map_sg(drive->queue, rq, sg);
-	} else {
+	if (rq->cmd_type == REQ_TYPE_ATA_TASKFILE) {
 		sg_init_one(sg, rq->buffer, rq->nr_sectors * SECTOR_SIZE);
 		hwif->sg_nents = 1;
+	} else if (!rq->bio) {
+		sg_init_one(sg, rq->data, rq->data_len);
+		hwif->sg_nents = 1;
+	}
+	else {
+		hwif->sg_nents = blk_rq_map_sg(drive->queue, rq, sg);
 	}
 }

?

Created attachment 19651 [details]
fix setting up dma for non bio-backed requests

Hi Márton,

can you please try the attached patch on top of the previous debugging one and send me the dmesg output, thanks.

Boris.

Created attachment 19666 [details]
dmesg 2.6.28 + patches from comment #5 & comment #13

I booted the system, inserted an audio CD. The kaffeine application started and tried to play the tracks but nothing was played, instead, after 3..5 seconds kaffeine jumped to the next track. Then I stopped kaffeine and removed the CD.

Hi,

thanks for testing. Hmm, I can reproduce it here with kaffeine. Can you retry playing the audio cd with kscd with "Use direct digital playback" enabled, under Settings? 

P.S. I'll be away until Sa. and will able to look into it more seriously then.

Thanks,
Boris.

(In reply to comment #16)
> thanks for testing. Hmm, I can reproduce it here with kaffeine. Can you retry
> playing the audio cd with kscd with "Use direct digital playback" enabled,
> under Settings? 

kscd is working with and without "Use direct digital playback" settings. There is no new message in dmesg while kscd is playing. kaffeine still does not work and debug messages are printed in dmesg. The version numbers I used: kaffeine 0.8.7, kscd 1.6.

kaffeine reports the following messages on stdout/stderr when trying to play audio CD:
$ kaffeine 
Checking /dev/hdc for cdrom...
        Testing /dev/hdc for SCSI/MMC interface
                SG_IO device: /dev/hdc

CDROM model sensed sensed: Optiarc DVD RW AD-7543A 1-00 

Checking for SCSI emulation...
        Drive is ATAPI (using SG_IO host adaptor emulation)

Checking for MMC style command set...
        Drive is MMC style
        DMA scatter/gather table entries: 1
        table entry size: 131072 bytes
        maximum theoretical transfer: 55 sectors
        Setting default read size to 27 sectors (63504 bytes).

Verifying CDDA command set...
        Expected command set reads OK.
CDROMREADRAW: Input/output error
CDROMREADRAW: Input/output error
CDROMREADRAW: Input/output error
CDROMREADRAW: Input/output error
CDROMREADRAW: Input/output error
CDROMREADRAW: Input/output error

Plain 2.6.27.10 gives the warn_on_slowpath backtrace (as documented for 2.6.28).

With patches from comment nos. 5 & 13:

ide_build_sglist: Uninitialized none, cmd: 0x28
none: dev hdd: type=1, flags=10484
  sector 64, nr/cnr 8/8
  bio ffff88002d797240, biotail ffff88002d797240, buffer ffff88002c179000, data 0000000000000000, len 4096
ide_build_sglist: Uninitialized none, cmd: 0xbe
none: dev hdd: type=b, flags=104c8
  sector 18446744073709551615, nr/cnr 0/0
  bio 0000000000000000, biotail 0000000000000000, buffer 0000000000000000, data ffff88002d62e000, len 2336

Hi Darren and Márton,

has there been a setup/kernel with which cd audio play has been functioning using kaffeine?

Thanks,
Boris.

(In reply to comment #19)
> has there been a setup/kernel with which cd audio play has been functioning
> using kaffeine?

I haven't played with different configs. What config option do you suggest to try to change?

Ok,

let me put in another way: does it work after you revert 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0?

(In reply to comment #21)
> let me put in another way: does it work after you revert
> 9bd27cba1aeacb6b12d05f4e5ed6361072f08fe0?

Yes, as I mentioned in comment #10, reverting the patch stopped sending messages to dmesg. I was able to play CDs with vanilla kernels, the only diffecence was the warning messages.

Hi,

here's a fix, finally. It is against 29-rc2 + current (24.01) pata-tree.

@Márton: can you please apply and test, thanks. The patch applies on plain 2.6.28 too, albeit with a little fuzz.

@Bart: We should probably backport that one to 2.6.28 after enough testing, right?

Thanks,
Boris.

Created attachment 19979 [details]
fix DMA for non bio-backed requests

On Saturday 24 January 2009, bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=12320
> 
> 
> bbpetkov@yahoo.de changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>          AssignedTo|io_ide@kernel-bugs.osdl.org |bbpetkov@yahoo.de
>              Status|NEW                         |ASSIGNED
> 
> 
> 
> 
> ------- Comment #23 from bbpetkov@yahoo.de  2009-01-24 06:19 -------
> Hi,
> 
> here's a fix, finally. It is against 29-rc2 + current (24.01) pata-tree.
> 
> @Márton: can you please apply and test, thanks. The patch applies on plain
> 2.6.28 too, albeit with a little fuzz.
> 
> @Bart: We should probably backport that one to 2.6.28 after enough testing,
> right?

That's correct -- it is a regression and fix is rather non-invasive.

Thanks,
Bart

(In reply to comment #23)
> here's a fix, finally. It is against 29-rc2 + current (24.01) pata-tree.
> 
> @Márton: can you please apply and test, thanks. The patch applies on plain
> 2.6.28 too, albeit with a little fuzz.

I applied the patch from commend #24 on top of 2.6.29-rc2. The audio CD playing seems to work correctly. I tested with Kaffeine 0.8.7 (KDE 3.5.9) and with KsCD 1.6 (KDE 3.5.9) with "Use direct digital playback" disabled and enabled also.

Thank you for your work!

(In reply to comment #26)
> (In reply to comment #23)
> > here's a fix, finally. It is against 29-rc2 + current (24.01) pata-tree.
> > 
> > @Márton: can you please apply and test, thanks. The patch applies on plain
> > 2.6.28 too, albeit with a little fuzz.
> 
> I applied the patch from commend #24 on top of 2.6.29-rc2. The audio CD
> playing
> seems to work correctly. I tested with Kaffeine 0.8.7 (KDE 3.5.9) and with
> KsCD
> 1.6 (KDE 3.5.9) with "Use direct digital playback" disabled and enabled also.
> 
> Thank you for your work!
> 

That's cool, thanks for testing.