Date: Wed, 15 Feb 2012 09:29:05 +0100 From: Victor Balada Diaz <victor@bsdes.net> To: Jeremy Chadwick <freebsd@jdc.parodius.com> Cc: Harald Schmalzbauer <h.schmalzbauer@omnilan.de>, Alexander Motin <mav@freebsd.org>, freebsd-stable@freebsd.org, Claudius Herder <claudius@ambtec.de> Subject: Re: problems with AHCI on FreeBSD 8.2 Message-ID: <20120215082905.GV2010@equilibrium.bsdes.net> In-Reply-To: <20120215001020.GA9386@icarus.home.lan> References: <20120214100513.GA94501@icarus.home.lan> <20120214135435.GQ2010@equilibrium.bsdes.net> <20120214141601.GA98986@icarus.home.lan> <4F3A83DE.3000200@ambtec.de> <20120214165029.GA1852@icarus.home.lan> <4F3A971F.9040407@omnilan.de> <20120214221527.GT2010@equilibrium.bsdes.net> <20120214230958.GA8434@icarus.home.lan> <20120214233420.GU2010@equilibrium.bsdes.net> <20120215001020.GA9386@icarus.home.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120215082905.GV2010>