[PATCH] one more hdparm patch with command line parsing bug fix
Bernhard Fischer
rep.nop at aon.at
Sat May 13 16:43:59 UTC 2006
Tito,
On Sat, May 13, 2006 at 04:20:46PM +0200, Tito wrote:
>Hi,
>this is a new hdparm patch to fix one bug, reduce size and clean up the code.
>The main changes are:
>
>Fixed bug in command line arg parsing: flags for options without args are reset on next getopt_long iteration.
>Updated the timing algorithm in do_time() to that of hdparm 6.6 (gives better results on my drives and code is cleaner).
>Changed switch() to if() else if() else to reduce size.
>Renamed if_printf_on_off() to print_flag_on() and optimised for size.
>Removed if_printf() as it increases size and substituted with print_flag() where appropriate.
>Removed check_if_min_and_set_val() as it increases size.
>Removed check_if_maj_and_set_val() as it increases size.
>Removed if_else_printf() as it increases size.
>Removed sync_and_sleep() as now it was used only in one place.
>Moved code used 2 times in do_time() to print_timing().
>Optimised some text strings to reduce size.
>
>Commandline option parsing should now work in all cases.
>Size reduction for hdparm.o (with all options enabled) is:
> text data bss dec hex filename
> 23525 176 872 24573 5ffd hdparm.o.orig
> 22209 176 864 23249 5ad1hdparm.o
>
>Please apply before feature freeze.
allnoconfig, enable all hdparm stuff:
$ size miscutils/hdparm.o*
text data bss dec hex filename
31717 148 872 32737 7fe1 miscutils/hdparm.o.oorig
29870 148 864 30882 78a2 miscutils/hdparm.o.10
29743 148 864 30755 7823 miscutils/hdparm.o.11a
For my compiler it helps if you explicitely cache the array-derefs like
in identify() near /* reset result */, can you reproduce this with your
compiler?
Also, we should use "strng" more often. I renamed 's' to strng for better
grep'ability
The putchar and puts usage makes absolutely no difference for any
version of gcc i have here. Do you see any adverse/positive effect if you
change these?
>
>Thanks and Ciao,
>Tito
>
>PS.: Rob, I know the patch is rather big, but it does mostly trivial stuff, the only real changes
> are the bugfix in main and the timing algorithm update in do_time(). Sorry!
> If you really, really, really want this patch splitted in pieces....... i will do it.
>
>_______________________________________________
>busybox mailing list
>busybox at busybox.net
>http://busybox.net/cgi-bin/mailman/listinfo/busybox
-------------- next part --------------
--- miscutils/hdparm.c.10 2006-05-13 17:04:56.000000000 +0200
+++ miscutils/hdparm.c.11a 2006-05-13 18:25:24.000000000 +0200
@@ -560,7 +560,7 @@ static void print_ascii(uint16_t *p, uin
break;
if ((cl = (char) 0x00ff&(*p)) != ' ')
{
- if (cl != '\0') printf("%c",cl);
+ if (cl != '\0') putchar(cl);
p++;
ii++;
break;
@@ -575,7 +575,7 @@ static void print_ascii(uint16_t *p, uin
printf("%c%c",(char)0x00ff&((*p)>>8),(char)(*p)&0x00ff);
p++;
}
- printf("\n");
+ putchar('\n');
}
/* identify() is the only extern function used across two source files. The
@@ -592,7 +592,7 @@ static void identify(uint16_t *id_suppli
uint8_t chksum = 0;
uint32_t ll, mm, nn, oo;
uint64_t bbbig; /* (:) */
- const char *s;
+ const char *strng;
if (BB_BIG_ENDIAN) {
swab(id_supplied, buf, sizeof(buf));
@@ -602,7 +602,7 @@ static void identify(uint16_t *id_suppli
chksum &= 0xff;
/* check if we recognise the device type */
- printf("\n");
+ putchar('\n');
if(!(val[GEN_CONFIG] & NOT_ATA))
{
dev = ATA_DEV;
@@ -638,9 +638,9 @@ static void identify(uint16_t *id_suppli
{
like_std = 5;
if((val[CONFIG]==STBY_NID_VAL) || (val[CONFIG]==STBY_ID_VAL))
- printf("powers-up in standby; SET FEATURES subcmd spins-up.\n");
+ puts("powers-up in standby; SET FEATURES subcmd spins-up.");
if(((val[CONFIG]==STBY_NID_VAL) || (val[CONFIG]==PWRD_NID_VAL)) && (val[GEN_CONFIG] & INCOMPLETE))
- printf("\n\tWARNING: ID response incomplete.\n\tFollowing data may be incorrect.\n\n");
+ puts("\n\tWARNING: ID response incomplete.\n\tFollowing data may be incorrect.\n");
}
/* output the model and serial numbers and the fw revision */
@@ -734,7 +734,7 @@ static void identify(uint16_t *id_suppli
else if (like_std > std)
printf("& some of %u\n",like_std);
else
- printf("\n");
+ putchar('\n');
}
else
{
@@ -764,7 +764,7 @@ static void identify(uint16_t *id_suppli
if (min_std == 0xffff)
min_std = like_std > 4 ? like_std - 3 : 1;
- printf("Configuration:\n");
+ puts("Configuration:");
/* more info from the general configuration word */
if ((eqpt != CDROM) && (like_std == 1))
{
@@ -777,23 +777,24 @@ static void identify(uint16_t *id_suppli
}
if (dev == ATAPI_DEV)
{
+ printf("\tDRQ response: "); /* Data Request (DRQ) */
if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) == DRQ_3MS_VAL)
- s = "3ms";
+ strng = "3ms";
else if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) == DRQ_INTR_VAL)
- s = "<=10ms with INTRQ";
+ strng = "<=10ms with INTRQ";
else if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) == DRQ_50US_VAL)
- s ="50us";
- else
- s = "Unknown";
- printf("\tDRQ response: %s\n", s); /* Data Request (DRQ) */
+ strng = "50us";
+ else
+ strng = "Unknown";
+ printf("%s\tPacket size: ", strng);
if ((val[GEN_CONFIG] & PKT_SIZE_SUPPORTED) == PKT_SIZE_12_VAL)
- s = "12 bytes";
+ strng = "12 bytes";
else if ((val[GEN_CONFIG] & PKT_SIZE_SUPPORTED) == PKT_SIZE_16_VAL)
- s = "16 bytes";
+ strng = "16 bytes";
else
- s = "Unknown";
- printf("\tPacket size: %s\n", s);
+ strng = "Unknown";
+ puts(strng);
}
else
{
@@ -801,7 +802,7 @@ static void identify(uint16_t *id_suppli
ll = (uint32_t)val[LBA_SECTS_MSB] << 16 | val[LBA_SECTS_LSB];
mm = 0; bbbig = 0;
if ( (ll > 0x00FBFC10) && (!val[LCYLS]))
- printf("\tCHS addressing not supported\n");
+ puts("\tCHS addressing not supported");
else
{
jj = val[WHATS_VALID] & OK_W54_58;
@@ -845,8 +846,8 @@ static void identify(uint16_t *id_suppli
if (bbbig > 1000)
printf("(%llu GB)\n", bbbig/1000);
- else
- printf("\n");
+ else
+ putchar('\n');
}
/* hw support of commands (capabilities) */
@@ -867,10 +868,10 @@ static void identify(uint16_t *id_suppli
printf("(can");
else
printf("(cannot");
- printf(" be disabled)\n");
+ puts(" be disabled)");
}
else
- printf("no IORDY\n");
+ puts("no IORDY");
if ((like_std == 1) && val[BUF_TYPE])
{
@@ -882,25 +883,15 @@ static void identify(uint16_t *id_suppli
printf("dual port, multi-sector");
if (kk > 2) printf(" with read caching ability");
- printf("\n");
+ putchar('\n');
}
- jj = 0;
+
if ((min_std == 1) && (val[BUFFER__SIZE] && (val[BUFFER__SIZE] != NOVAL_1)))
- {
- printf("\tBuffer size: %.1fkB",(float)val[BUFFER__SIZE]/2);
- jj = 1;
- }
+ printf("\tBuffer size: %.1fkB\n",(float)val[BUFFER__SIZE]/2);
if ((min_std < 4) && (val[RW_LONG]))
- {
- printf("\tbytes avail on r/w long: %u",val[RW_LONG]);
- jj = 1;
- }
+ printf("\tbytes avail on r/w long: %u\n",val[RW_LONG]);
if ((eqpt != CDROM) && (like_std > 3))
- {
- printf("\tQueue depth: %u",(val[QUEUE_DEPTH] & DEPTH_BITS)+1);
- jj = 1;
- }
- if (jj) printf("\n");
+ printf("\tQueue depth: %u\n",(val[QUEUE_DEPTH] & DEPTH_BITS)+1);
if (dev == ATA_DEV)
{
@@ -923,7 +914,7 @@ static void identify(uint16_t *id_suppli
if (val[SECTOR_XFER_CUR] & MULTIPLE_SETTING_VALID)
printf("%u\n", val[SECTOR_XFER_CUR] & SECTOR_XFER);
else
- printf("?\n");
+ puts("?");
}
if ((like_std > 3) && (val[CMDS_SUPP_1] & 0x0008))
{
@@ -948,21 +939,21 @@ static void identify(uint16_t *id_suppli
{
/* ATAPI */
if (eqpt != CDROM && (val[CAPAB_0] & SWRST_REQ))
- printf("\tATA sw reset required\n");
+ puts("\tATA sw reset required");
if (val[PKT_REL] || val[SVC_NBSY])
{
printf("\tOverlap support:");
if (val[PKT_REL]) printf(" %uus to release bus.",val[PKT_REL]);
if (val[SVC_NBSY]) printf(" %uus to clear BSY after SERVICE cmd.",val[SVC_NBSY]);
- printf("\n");
+ putchar('\n');
}
}
/* DMA stuff. Check that only one DMA mode is selected. */
printf("\tDMA: ");
if (!(val[CAPAB_0] & DMA_SUP))
- printf("not supported\n");
+ puts("not supported");
else
{
if (val[DMA_MODE] && !val[SINGLE_DMA] && !val[MULTI_DMA])
@@ -1013,16 +1004,15 @@ static void identify(uint16_t *id_suppli
if (jj & 0x0001) printf("pio%d ",ii);
jj >>=1;
}
- printf("\n");
}
else if (((min_std < 5) || (eqpt == CDROM)) && (val[PIO_MODE] & MODE) )
{
for (ii = 0; ii <= val[PIO_MODE]>>8; ii++)
printf("pio%d ",ii);
- printf("\n");
}
else
- printf("unknown\n");
+ printf("unknown");
+ putchar('\n');
if (val[WHATS_VALID] & OK_W64_70)
{
@@ -1096,20 +1086,16 @@ static void identify(uint16_t *id_suppli
}
/* reset result */
- if ((val[HWRST_RSLT] & VALID) == VALID_VAL)
+ jj = val[HWRST_RSLT];
+ if ((jj & VALID) == VALID_VAL)
{
- printf("HW reset results:\n\tCBLID- %s Vih\n", (val[HWRST_RSLT] & CBLID) ? "above" : "below");
+ oo = jj & RST0;
+ printf("HW reset results:\n\tCBLID- %s Vih\n\tDevice num = %i",
+ (jj & CBLID) ? "above" : "below",
+ !oo);
- if (val[HWRST_RSLT] & RST0)
- {
- printf("\tDevice num = 0");
- jj = val[HWRST_RSLT];
- }
- else
- {
- printf("\tDevice num = 1");
- jj = val[HWRST_RSLT] >> 8;
- }
+ if (!oo)
+ jj >>= 8;
if ((jj & DEV_DET) == JUMPER_VAL)
printf(" determined by the jumper");
@@ -1121,12 +1107,13 @@ static void identify(uint16_t *id_suppli
/* more stuff from std 5 */
if ((like_std > 4) && (eqpt != CDROM))
{
- if (val[CFA_PWR_MODE] & VALID_W160)
+ jj = val[CFA_PWR_MODE];
+ if (jj & VALID_W160)
{
- printf("CFA power mode 1:\n\t%s%s\n", (val[CFA_PWR_MODE] & PWR_MODE_OFF) ? "disabled" : "enabled",
- (val[CFA_PWR_MODE] & PWR_MODE_REQ) ? " and required by some commands" : "");
+ printf("CFA power mode 1:\n\t%s%s\n", (jj & PWR_MODE_OFF) ? "disabled" : "enabled",
+ (jj & PWR_MODE_REQ) ? " and required by some commands" : "");
- if (val[CFA_PWR_MODE] & MAX_AMPS) printf("\tMaximum current = %uma\n",val[CFA_PWR_MODE] & MAX_AMPS);
+ if (jj & MAX_AMPS) printf("\tMaximum current = %uma\n", jj & MAX_AMPS);
}
if ((val[INTEGRITY] & SIG) == SIG_VAL)
{
More information about the busybox
mailing list