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

Alexander Motin wrote:
> Scott Long wrote:
>> 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:
>>>>>>
>>>>>>  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.
> 
> CAM reports SCSI protocol for ATAPI devices at this moment. It is not
> good probably. but changing it now may be painful. Checks like
> 	d->ccb->cpi.transport == XPORT_ATA ||
> 	d->ccb->cpi.transport == XPORT_SATA
> should be for now. "ata" hack should also stay there for now, as
> ATAPICAM emulates SCSI transport now, but not a new ATA one.
> 

What protocol should CAM be reporting for ATAPI devices?  It is SCSI.  I
don't understand why we have to keep on diverging from the goal of
having a unified and consistent interface here.

As for ATAPICAM, that hopefully will go away some day, as it's really
only a hack.

Scott