problems with AHCI on FreeBSD 8.2

[Victor Balada Diaz]

On Tue, Feb 14, 2012 at 04:10:20PM -0800, Jeremy Chadwick wrote:
> I'm too tired to quite understand (in full) what's wrong with my patch,
> but I think you're referring to situations where someone would have
> kern.cam.ada.X.quirks set in loader.conf?
> 
> If so, I believe that same situation would happen presently if someone
> set kern.cam.ada.X.quirks in their loader.conf to a value that did not
> contain bit #0 set to 1, and used one of the 4K sector disks listed in
> ada_quirk_table -- what's in loader.conf looks like it would overwrite
> whatever the kernel code bits chose automatically:
> 
>  910         match = cam_quirkmatch((caddr_t)&cgd->ident_data,
>  911                                (caddr_t)ada_quirk_table,
>  912                                sizeof(ada_quirk_table)/sizeof(*ada_quirk_table),
>  913                                sizeof(*ada_quirk_table), ata_identify_match);
>  914         if (match != NULL)
>  915                 softc->quirks = ((struct ada_quirk_entry *)match)->quirks;
>  916         else
>  917                 softc->quirks = ADA_Q_NONE;
>  ...
>  931         snprintf(announce_buf, sizeof(announce_buf),
>  932             "kern.cam.ada.%d.quirks", periph->unit_number);
>  933         quirks = softc->quirks;
>  934         TUNABLE_INT_FETCH(announce_buf, &quirks);
>  935         softc->quirks = quirks;
> 
> I read this to mean:
> 
> Lines 910-917 -- if there's a device ID string match in ada_quirk_table,
> set softc->quirks to the content of that struct entry.  So, for example,
> 4K sector disks would set softc->quirks to 0x01.
> 
> Lines 931-933 -- assign the "quirks" variable to what softc->quirks
> currently contains.  Thus, for 4K sector disks, quirks = 0x01.
> 
> Line 934 -- load into "quirks" variable the contents of loader.conf
> entries pertaining to kern.cam.ada.N.quirks, if set.  If someone had an
> entry in loader.conf saying kern.cam.ada.N.quirks=0 then yes, this would
> overwrite what the kernel "auto-chose".
> 
> Line 935 -- assign softc->quirks = quirks, thus softc->quirks = 0x00,
> assuming someone set it to such in loader.conf.

That's exactly what i meant. 

> > See my attached patch. I can confirm it works for me.
> 
> I thought of taking that approach, but for me it's "dirty".  Here's what
> I mean by that:
> 
> ADA_FLAG_CAN_NCQ gets set in softc->flags around line 892, but then
> roughly a hundred lines later, you clear the exact same flag you just
> set (based on quirk conditionals).
> 
> I dunno how people feel about that, but my impression is that it's
> dirty/confusing.  My opinion is to only set the bit once and not mess
> about with repeated if() statements that set it, then clear it, etc...

Indeed you're right. It's a hack. Would be better to move quirk evaluation before checking
capabilities.

-- 
La prueba más fehaciente de que existe vida inteligente en otros
planetas, es que no han intentado contactar con nosotros.