From owner-svn-src-all@FreeBSD.ORG Thu Dec 9 08:59:17 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 37AA9106564A; Thu, 9 Dec 2010 08:59:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail03.syd.optusnet.com.au (mail03.syd.optusnet.com.au [211.29.132.184]) by mx1.freebsd.org (Postfix) with ESMTP id 8B50B8FC0A; Thu, 9 Dec 2010 08:59:16 +0000 (UTC) Received: from c122-106-172-0.carlnfd1.nsw.optusnet.com.au (c122-106-172-0.carlnfd1.nsw.optusnet.com.au [122.106.172.0]) by mail03.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id oB98wusg012169 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 9 Dec 2010 19:58:58 +1100 Date: Thu, 9 Dec 2010 19:58:56 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Cran In-Reply-To: <20101208225235.501ced0e@core.draftnet> Message-ID: <20101209191657.B1400@besplex.bde.org> References: <201012072046.oB7KkB4L079555@svn.freebsd.org> <4CFEAD09.30904@freebsd.org> <4CFEAFA6.4020103@feral.com> <4CFEB1AD.70906@freebsd.org> <20101208153857.H1428@besplex.bde.org> <20101208225235.501ced0e@core.draftnet> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, Andriy Gapon , Bruce Evans , Bruce Cran , svn-src-head@freebsd.org, Matthew Jacob Subject: Re: svn commit: r216269 - head/sys/geom/part X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Dec 2010 08:59:17 -0000 On Wed, 8 Dec 2010, Bruce Cran wrote: > On Wed, 8 Dec 2010 16:46:17 +1100 (EST) > Bruce Evans wrote: > >> - for ATA drives, it uses the "firmware" geometry supplied by the >> drive firmware. The ATA standard specifies this to be H=16/S=63 for >> all drives larger than a certain size (IIRC, about 8GB, which is the >> size at which the better default fake of H=255/S=63 generated by >> cam_calc_geometry() for drives larger than 1GB, stops working for >> systems that can't handle more than 1024 fake cylinders. The magic >> 1GB is the size at which the even better default fake of H=64/S=32 >> stops working with more than 1024 fake cylinders. >> cam_calc_geometry() has no comment about this either). Since all >> modern ATA drives are larger than 8GB, H=16/S=63 is almost hard-coded >> for ATA. > > I've put together a patch which will hopefully improve the reporting of > geometry, though as you say nothing actually uses it nowadays - we > don't even appear to support CHS addressing in the disk drivers. It still does for the non-cam case. From ata_lowlevel.c: % #ifndef ATA_CAM % if (atadev->flags & ATA_D_USE_CHS) { New drives might not support CHS. Then we can't get here. Old drives don't support LBA, so we must get here. Not so old drives support both, but we never use CHS if LBA is available. So we only get here for old drivers. % int heads, sectors; % % if (atadev->param.atavalid & ATA_FLAG_54_58) { % heads = atadev->param.current_heads; % sectors = atadev->param.current_sectors; I had understood the ATA_FLAG_54_58 backwards. It tells us if the drive is not so old that it doesn't support IDENTIFY records for words 54-58. I think we rarely get here. Drives old enough to use CHS may be so old that they don't support words 54-58. Only drives manufactured during a few years or months in the 1990's will support words 54-58 but not LBA. Maybe I'm misremembering the length of this time. When the drive supports these words and we use CHS, we use these words. % } % else { % heads = atadev->param.heads; % sectors = atadev->param.sectors; We get here for very old drives (ones that don't support LBA and don't support changing their CHS from their default CHS to a current CHS, or is just ones that don't support words 54-58 to report such changes?). % } % // rest of CHS accces method % } % else { % #endif % // LBA accces method % #ifndef ATA_CAM % } % #endif % } % } It's strange that ata uses the ATA_FLAG_54_58 feature for CHS. 6 years ago, long after you couldn't buy a drive for which this code is executed, sys/ata.h declared words 54-58 as obsolete54[5] instead of as current_heads, etc., used in the above, and of course it didn't use them. The corresponding code was quite different. It was essentially the same for CHS and LBA. The CHS case was faked in ad_start() to look like the LBA case here. The faking used only the non-ATA_FLAG_54_58 parameters, so it corresponded to not having the clause for that here. This depended on the current parameters being the same as the default parameters. Ata ignored all the early 1990's features for changing the parameters to more suitable ones, and used a reset to force the current ones to defaults, then just used the defaults. Why does it now do more? > ata_da.c now calls cam_calc_geometry() which is updated to clip the > number of cylinders to 65535. I'm not familiar with ata_da.c. Presumably it is only used in the ATA_CAM case, when CHS addressing is not even an option (it is killed by the above ifdef). > This introduces a problem because the > calculated geometry will no longer match the disklabel: however, since > CHS isn't used I don't know if we need to warn if the geometries don't > match? Since it appears superfluous I've removed the warning in the > patch. I think it is too late to change this. It should have been changed before any metadata using old values was created. This may be impossible, since the metadata may have been created with another driver (ad say) that reports different geoemtry. > atacontrol and camcontrol are updated to only display CHS > parameters from the drive if the disk conforms to ATA-5 or before. I don't like this. If the drive supports CHS, then its geometries for this should be reported, if CHS reporting is implemented at all. There are many to choose from :-), but only 1 or 2 (if any) to report -- the default one and the current one. Capabilities reporting should make it clear if the default one can be changed. I consider CHS features to be unimportant, so they should be reported if the capabilites reporting tries to be complete; otherwise they are optional. hdparm for Linux always seemed to report more features than atacontrol. % Index: sbin/atacontrol/atacontrol.c % =================================================================== % --- sbin/atacontrol/atacontrol.c (revision 216266) % +++ sbin/atacontrol/atacontrol.c (working copy) % @@ -180,14 +183,16 @@ % printf("Unknown SATA revision\n"); % } % else % - printf("ATA/ATAPI revision %d\n", version(parm->version_major)); % + printf("ATA/ATAPI revision %d\n", ataver); % printf("device model %.40s\n", parm->model); % printf("serial number %.20s\n", parm->serial); % printf("firmware revision %.8s\n", parm->revision); % % - printf("cylinders %d\n", parm->cylinders); % - printf("heads %d\n", parm->heads); % - printf("sectors/track %d\n", parm->sectors); % + if (ataver <= 5) { % + printf("cylinders %d\n", parm->cylinders); % + printf("heads %d\n", parm->heads); % + printf("sectors/track %d\n", parm->sectors); % + } I don't like removing them depending on the version. atacontrol is about the only place that you can see CHS without it possibly having gone through several layers of fakery. Now I see more clearly that these are only the "firmware" = default parameters. There are also the current parameters in words 54-58. These are not reported here. One use for reporting them here is to make it easy to debug whether the kernel code for using them is ever reached :-). I don't know if a full unfakedparameter struct is available here for easy printing. If it is, then the capabilities flag for words 54-58 is in it, and should be used instead of atarev for deciding whether to print these words (though setting of capability bits for old drives is as unreliable as setting the capability words. The driver seems to trust it). I don't know of any capability bits for the default CHS. The words for this are so old that all drives support it if they support CHS. ATA_USE_CHS is a non-firmware-feature flag that is set if the version or lbasize is 0. It is the driver's use flag, not a capability flag. % Index: sys/cam/ata/ata_da.c % =================================================================== % --- sys/cam/ata/ata_da.c (revision 216266) % +++ sys/cam/ata/ata_da.c (working copy) % @@ -1129,27 +1129,14 @@ % struct disk_params *dp = &softc->params; % u_int64_t lbasize48; % u_int32_t lbasize; % + struct ccb_calc_geometry cg; Insertion sort error. % % dp->secsize = ata_logical_sector_size(&cgd->ident_data); % - if ((cgd->ident_data.atavalid & ATA_FLAG_54_58) && % - cgd->ident_data.current_heads && cgd->ident_data.current_sectors) { % - dp->heads = cgd->ident_data.current_heads; % - dp->secs_per_track = cgd->ident_data.current_sectors; % - dp->cylinders = cgd->ident_data.cylinders; % - dp->sectors = (u_int32_t)cgd->ident_data.current_size_1 | % - ((u_int32_t)cgd->ident_data.current_size_2 << 16); % - } else { % - dp->heads = cgd->ident_data.heads; % - dp->secs_per_track = cgd->ident_data.sectors; % - dp->cylinders = cgd->ident_data.cylinders; % - dp->sectors = cgd->ident_data.cylinders * dp->heads * dp->secs_per_track; % - } Hmm, it has the complications for words 54-58 too % + % lbasize = (u_int32_t)cgd->ident_data.lba_size_1 | % ((u_int32_t)cgd->ident_data.lba_size_2 << 16); % % - /* use the 28bit LBA size if valid or bigger than the CHS mapping */ % - if (cgd->ident_data.cylinders == 16383 || dp->sectors < lbasize) % - dp->sectors = lbasize; % + dp->sectors = lbasize; The old code allows for CHS to give more sectors than LBA. I don't know if that is possible. It is more common for CHS to give less sectors. CHS certainly gives more sectors if LBA is not supported, but I think CAM ad doesn't support that case elsewhere. Strange that it supports it here. % % /* use the 48bit LBA size if valid */ % lbasize48 = ((u_int64_t)cgd->ident_data.lba_size48_1) | % @@ -1159,6 +1146,14 @@ % if ((cgd->ident_data.support.command2 & ATA_SUPPORT_ADDRESS48) && % lbasize48 > ATA_MAX_28BIT_LBA) % dp->sectors = lbasize48; % + % + cg.block_size = dp->secsize; % + cg.volume_size = dp->sectors * dp->secsize; % + cam_calc_geometry(&cg, 1); % + % + dp->heads = cg.heads; % + dp->secs_per_track = cg.secs_per_track; % + dp->cylinders = cg.cylinders; Risky to change this so late. Actually, I have too much experience with the system generating wrong geometries, and it goes the other way. I normally run older FreeBSDs which generate the BIOS geometry of H=255/C=63 which matches all metadata. When I boot -current it generates H=16/C=63. I get annoyed by disk^Wbsdlabel complaining about this and more, and can't risk using it to change labels. So I'd be happy if you changed this, so everyone sees any problems in this area :-). Its main inconsistency seems to be that only CAM ad does it. % } % % static void % Index: sys/geom/part/g_part_bsd.c % =================================================================== % --- sys/geom/part/g_part_bsd.c (revision 216266) % +++ sys/geom/part/g_part_bsd.c (working copy) % @@ -387,10 +387,6 @@ % goto invalid_label; % if (heads != basetable->gpt_heads && !basetable->gpt_fixgeom) % basetable->gpt_heads = heads; % - if (sectors != basetable->gpt_sectors || heads != basetable->gpt_heads) % - printf("GEOM: %s: geometry does not match label" % - " (%uh,%us != %uh,%us).\n", pp->name, heads, sectors, % - basetable->gpt_heads, basetable->gpt_sectors); I would just put it under bootverbose. Labels really should have the same geometry as the drive, whatever that is, in case someone reads them and is confused or something uses them. But the geometry may be O/S or even driver-dependent. Bruce