cvs commit: src/sys/dev/ata atapi-cd.c atapi-cd.h

[Scott Long]

Søren Schmidt wrote:
> Nate Lawson wrote:
>> Søren Schmidt wrote:
>>  
>>> Scott Long wrote:
>>>    
>>>> SXren Schmidt wrote:
>>>>      
>>>>> sos         2007-10-26 08:59:24 UTC
>>>>>
>>>>>   FreeBSD src repository
>>>>>
>>>>>   Modified files:
>>>>>     sys/dev/ata          atapi-cd.c atapi-cd.h   Log:
>>>>>   Update the way we get the mode pages on probe.
>>>>>     Revision  Changes    Path
>>>>>   1.194     +22 -25    src/sys/dev/ata/atapi-cd.c
>>>>>   1.47      +1 -0      src/sys/dev/ata/atapi-cd.h
>>>>>         
>>>> Just curious, what was the motivation for changing from doing a
>>>> TUR to doing a MODE_SENSE in g_access?  Your new code now relies
>>>> on what I'd consider is a fairly obscure feature of the SFF-8020i
>>>> spec, and one that contradicts the MMC and SPC specs that are now
>>>> used as the normative references for packet commands.  There's at
>>>> least once case, the virtual CDROM emulator in Parallels, that
>>>> appears not to support this feature, and I'd bet pretty heavily
>>>> that there are a number of real devices that won't support it
>>>> either.
>>>>
>>>> Without reverting to the old TUR code, an easy way forward is to
>>>> not fail the g_access command for the MST_FMT_NONE case.  This is
>>>> generally incorrect anyways as it just means that the device can't
>>>> specifically identify the media though it knows that the media is
>>>> inserted and valid.  Since this constant evaluates to 0x00, ignoring
>>>> it will also allow devices that don't support it to still work.  The
>>>> only side effect is that devices that don't support it will also not
>>>> be able to report MST_NO_DISC.  These devices will get handled later
>>>> as a side effect of reporting a bogus media size.
>>>>
>>>> Ultimately, I do think that the code needs to go back to using a TUR
>>>> and interpreting sense, asc, and ascq codes correctly.  The code prior
>>>> to 10/26 looks like it does this, so again I'm curious as to what ,
>>>> motivated the change.
>>>>       
>>> The code is only intended to work around the verbose error chatter from
>>> GEOM as it will read from a nonexisting media (if the drive is empty)
>>> during boot, and clutter the console needlessly. There is currently no
>>> way to prevent this, but I've talked to phk about it lately so this can
>>> be left out alltogether.
>>> Anyhow the only failure I've seen so far is Parallels when using a CD
>>> image as a virtual CDROM drive, it does a poor HW emu in that case if
>>> you ask me :)
>>>
>>> Anyhow you are free to do what you want with the code until I get a real
>>> fix via GEOM done, but mind you the old code will not work on SATA ATAPI
>>> drives so a simple rollback of the commit is not really a fix...
>>>     
>>
>> While there may be a way to solve this automatically for ATA, this is
>> also an issue for floppy drives.  GEOM tries to read the floppy on boot
>> to see if there's a label, even if no media is present.  I think GEOM
>> should have a way of asking drives if they can tell if they have media
>> present and not probe them if they can't.  A separate env flag could be
>> set to say "always probe removable media" if users want the previous
>> behavior.  For most of us, this just causes boot delay.
>>   
> What I've sortof talked phk into is that if I return the "right" error 
> code in the "start" function, it will back off and not retry nor chatter.
> We still need the "access" function to return OK if HW present or we 
> cannot send ioctl calls to an empty drive.
> 

IMHO, overloading g_start just adds more cruft to the problem.  I'm torn
between saying that g_access should be extended to give more than just a
simple yes/no error return, and saying that g_access is fundamentally
flawed and that this kind of info should be relayed via GEOM attributes.

As for rolling back the change or not, I don't see anywhere in the
SFF-8020i spec where it says that the CAP_PAGE command is guaranteed to
complete successfully and not return a CHECKCOND.  It seems perfectly
valid for it to fail if there is no media.  But even disregarding that,
the current code is still intended return a failure to g_access if
there's no media; doesn't that still preclude you from being able to
send ioctls to the device?

Scott