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

On Fri, 7 Aug 2009, Juergen Lock wrote:
> On Fri, Aug 07, 2009 at 10:19:26AM +0300, 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.
>
> Ok checking for XPORT_(S)ATA works for me as well:
>
> Index: interface/scsi_interface.c
> _at__at_ -1480,10 +1480,16 _at__at_
> 	/*
> 	 * if the bus device name is `ata', we're (obviously)
> 	 * running ATAPICAM.
> +	 * same for the new ahci(4) and siis(4) drivers and future others
> +	 * which use SATA transport too...
> 	 */
>
> -	if (strncmp(d->ccb->cpi.dev_name, "ata", 3) == 0) {
> -		cdmessage(d, "\tDrive is ATAPI (using ATAPICAM)\n");
> +	if (strncmp(d->ccb->cpi.dev_name, "ata", 3) == 0 ||
> +#if __FreeBSD_version >= 800102
> +	    d->ccb->cpi.transport == XPORT_SATA ||
> +#endif
> +	    d->ccb->cpi.transport == XPORT_ATA) {
> +		cdmessage(d, "\tDrive is ATAPI (using ATAPICAM or direct CAM (S)ATA transport)\n");
> 		d->is_atapi = 1;
> 	} else {
> 		cdmessage(d, "\tDrive is SCSI\n");
>
> One question remains tho:  What if someone connects a sata drive
> to a sas controller, will that still be XPORT_SAS then?  In that case
> I'd say we'd need to check for that as well, or maybe just assume there
> are no `real' sas optical drives and treat everything sas as sata here...

It's very possible to connect a SATA ATAPI drive to a SAS controller, 
though I'm not sure it's very common at the moment as it likely requires 
special support by the SAS controller and firmware.

Scott