Re: cdparanoia patch for ahci(4)/siis(4)

Juergen Lock wrote:
> On Thu, Aug 06, 2009 at 10:07:15PM +0200, Juergen Lock wrote:
>> On Thu, Aug 06, 2009 at 10:46:48PM +0300, Alexander Motin wrote:
>>> Juergen Lock wrote:
> 
>>>> 2. cdda/dae seems to be broken entirely with ahci(4) as well as
>>>> siis(4) (I remember a report about it being broken for usb optical
>>>> drives too so maybe this is related?) - I tested with the
>>>> audio/cdparanoia port as well as with
>>>> 	mplayer -cdrom-device /dev/cd{0,1} cdda://...
>>>> (mplayer needs to be built with the libparanoia knob on for this) - this
>>>> does work with atapicam(4) without ahci/siis so it can't be cd(4)'s
>>>> fault alone.  On siis(4) it seems to just fail while on ahci(4) (I still
>>>> have another optical drive on there, it's on the board's amd sb700)
>>>> it causes the sata channel to be reset endlessly until I ^C mplayer:
>>>>
>>>> ahcich1: AHCI reset...
>>>> ahcich1: hardware reset ...
>>>> ahcich1: SATA connect time=0ms status=00000113
>>>> ahcich1: ready wait time=144ms
>>>> ahcich1: AHCI reset done: devices=00000001
>>>> ahcich1: AHCI reset...
>>>> ahcich1: hardware reset ...
>>>> ahcich1: SATA connect time=0ms status=00000113
>>>> ahcich1: ready wait time=144ms
>>>> ahcich1: AHCI reset done: devices=00000001
>>>>
>>>> (Remeber if you want to reproduce this libparanoia needs permissions
>>>> on the optical drive's pass(4) device node and possibly /dev/xpt0 too.
>>>> And of course you need an audio cd. :)
>>>>
>>>>  Soo, anyone have ideas/patches/things they want me to check for this?
>>> But this appeared to to be really trivial. cdparanoia uses extremely 
>>> simple method for detecting ATAPI devices - it checks that SIM is named 
>>> "ata". Trivial single line hack made it successfully play some old 
>>> AudioCD in SATA drive on SiI3132 controller for me, while I am typing 
>>> this. Probably we should invent better way to do this.
>> Oooh! :)  I need to test this...
> 
> Yup, works here too on siis and ahci with the following patch:
> (maintainer Cc'd)
> 
> Index: interface/scsi_interface.c
> _at__at_ -1480,9 +1480,12 _at__at_
>  	/*
>  	 * if the bus device name is `ata', we're (obviously)
>  	 * running ATAPICAM.
> +	 * XXX same for the new ahci(4) and siis(4) drivers...
>  	 */
>  
> -	if (strncmp(d->ccb->cpi.dev_name, "ata", 3) == 0) {
> +	if (strncmp(d->ccb->cpi.dev_name, "ata", 3) == 0 ||
> +	    strncmp(d->ccb->cpi.dev_name, "ahcich", 6) == 0 ||
> +	    strncmp(d->ccb->cpi.dev_name, "siisch", 6) == 0) {
>  		cdmessage(d, "\tDrive is ATAPI (using ATAPICAM)\n");
>  		d->is_atapi = 1;
>  	} else {
> 
>  Thanx, :)
> 	Juergen

This is fine for the moment, but unmaintainable in the long run as more 
and more drives are written.  cdparanoia needs to look at protocol and
transport attributes, not device names.

Scott