Date: Thu, 9 Dec 2010 19:58:56 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Cran <bruce@cran.org.uk> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, Andriy Gapon <avg@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, Bruce Cran <brucec@freebsd.org>, svn-src-head@freebsd.org, Matthew Jacob <mj@feral.com> Subject: Re: svn commit: r216269 - head/sys/geom/part Message-ID: <20101209191657.B1400@besplex.bde.org> In-Reply-To: <20101208225235.501ced0e@core.draftnet> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 8 Dec 2010, Bruce Cran wrote:
> On Wed, 8 Dec 2010 16:46:17 +1100 (EST)
> Bruce Evans <brde@optusnet.com.au> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101209191657.B1400>