Date: Fri, 17 Oct 2008 08:55:49 +1100 From: Antony Mawer <fbsd-stable@mawer.org> To: Jeremy Chadwick <koitsu@FreeBSD.org> Cc: kib@freebsd.org, "Andrey V. Elsukov" <bu7cher@yandex.ru>, freebsd-stable@freebsd.org, sos@freebsd.org Subject: Re: Request for testing: ata(4) MFC Message-ID: <48F7B865.2040604@mawer.org> In-Reply-To: <20081016071700.GA2793@icarus.home.lan> References: <676151223134689@webmail38.yandex.ru> <20081005004808.GA70137@icarus.home.lan> <48E99C18.6070602@yandex.ru> <20081006051211.GA10542@icarus.home.lan> <20081010115855.GA31707@icarus.home.lan> <20081016071700.GA2793@icarus.home.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
Jeremy Chadwick wrote: > On Fri, Oct 10, 2008 at 04:58:55AM -0700, Jeremy Chadwick wrote: >> On Sun, Oct 05, 2008 at 10:12:11PM -0700, Jeremy Chadwick wrote: >>> On Mon, Oct 06, 2008 at 09:03:20AM +0400, Andrey V. Elsukov wrote: >>>> Jeremy Chadwick wrote: >>>>> Also, does your patch include any fixes (intentional or inadvertent) for >>>>> Intel MatrixRAID? This has been a sore spot for FreeBSD for quite >>>>> some time, and I'm curious to know if that has been fixed. >>>> There is only one fix for Intel Matrix RAID: >>>> http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/121899 >>> Ahh, yeah, I've seen that one as well. I'll apply the patch and let you >>> know if the behaviour documented in the PR happens. >> I'm sorry I haven't gotten around to testing this -- my day (night) job >> has kept me incredibly busy, and I've had hardly any time at home to >> work on personal projects. It sucks. >> >> I'll try to make time for testing either today or tomorrow. ... > Other items: > > - Could someone provide an explanation of the 48-bit LBA addressing > changes (see lines 988-1003 in the patch)? I'd like to know what they > do, and if further QA/testing is needed with this. This is probably more a question for Søren... I started looking at the moment, as the original 28->48bit crossover bug was in the same function not so long ago (ata-all.c rev1.280). The 48-bit LBA changes are introduced from ata-all.c rev1.282, which was the port multiplier changes. The logic just doesn't seem quite right to it to me, but I'm not an expert on the code or on ATA, so all of this could just be amateur mis-interpretation. From my reading of it, the code does this: /* * Check to see if we need to use a 48-bit command in place of the * standard 28-bit command, and if so, substitute as appropriate */ IF ((request_lba_addr + lba_count) >= max addressable by 28-bit LBA or lba_count > 256) and device supports 48-bit LBA THEN /* * The request either: * * - extends past the 28-bit boundary * - is for more than 256 sectors * * which necessitates the use of 48-bit LBA. Translate commands * into their 48-bit equivalents. */ ... ELSEIF the device supports 48-bit lba: /* * We prefer (need?) to use the 48-bit equivalents of these * commands regardless of what the LBA address of the reqest is */ ... END IF In rev 1.282, the ATA_READ_NATIVE_MAX_ADDRESS was moved down to the "ELSE" case of the IF statement. In otherwords, when the request is beyond the 28-bit boundary, OR it's > 256 sectors, ATA_READ_NATIVE_MAX_ADDRESS won't be translated... Søren, is this change intentional, or should it be also added to the switch statement in the top half of the IF block? The ATA code appears to be very lightly commented, which no doubt is something of a barrier to entry to those who are not familiar with issues such as the above... would any volunteers be helpful to help comment and/or document some of the code? We would likely need to confer with Søren and others to ensure that our interpretation was accurate, but it would certainly make tracking down issues easier for those unfamiliar with the code... --Antony
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48F7B865.2040604>