Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Chuck Ebbert]

Jens Axboe wrote:


>> +	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
>>  }
>
> Seconded, it causes a lot more confusion than it does good.


  The return looks like a function call in that last line.

  That's one of the few things I find really annoying -- extra parens
are fine when they make code clearer, but not there.


-------
 Chuck [ C Style Police, badge #666 ]

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Richard B. Johnson]

On Thu, 24 Apr 2003, Chuck Ebbert wrote:

> Jens Axboe wrote:
>
>
> >> +	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
> >>  }
> >
> > Seconded, it causes a lot more confusion than it does good.
>
>   The return looks like a function call in that last line.
>
>   That's one of the few things I find really annoying -- extra parens
> are fine when they make code clearer, but not there.
>
>
> -------
>  Chuck [ C Style Police, badge #666 ]

return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
                                  ^^^^^^|__________ wtf?
These undefined numbers in the code are much more annoying to me...
but I don't have a C Style Police Badge.

Also, whatever that is, 0x400, I'll call it MASK, can have shorter
code like:

   return (drive->id->cfs_enable_1 && MASK); // TRUE/FALSE
... for pedantics...
   return (int) (drive->id->cfs_enable_1 && MASK);

Okay, gimmie a C Police Badge.......

Cheers,
Dick Johnson
Penguin : Linux version 2.4.20 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Timothy Miller]

Richard B. Johnson wrote:

>On Thu, 24 Apr 2003, Chuck Ebbert wrote:
>
>  
>
>>Jens Axboe wrote:
>>
>>
>>    
>>
>>>>+	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
>>>> }
>>>>        
>>>>
>>>Seconded, it causes a lot more confusion than it does good.
>>>      
>>>
>>  The return looks like a function call in that last line.
>>
>>  That's one of the few things I find really annoying -- extra parens
>>are fine when they make code clearer, but not there.
>>
>>
>>-------
>> Chuck [ C Style Police, badge #666 ]
>>    
>>
>
>return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
>                                  ^^^^^^|__________ wtf?
>These undefined numbers in the code are much more annoying to me...
>but I don't have a C Style Police Badge.
>
>Also, whatever that is, 0x400, I'll call it MASK, can have shorter
>code like:
>
>   return (drive->id->cfs_enable_1 && MASK); // TRUE/FALSE
>... for pedantics...
>   return (int) (drive->id->cfs_enable_1 && MASK);
>
>
>  
>

That wouldn't work, because && isn't a bitwise operator.  But I agree 
that the ( x ? 1 : 0 ) method may not be very efficient, because it may 
involve branches.

Two alternatives:

(a)     !!(x & 0x400)

(b)     (x & 0x400) >> 10

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Richard B. Johnson]

On Thu, 24 Apr 2003, Timothy Miller wrote:

>
>
> Richard B. Johnson wrote:
>
> >On Thu, 24 Apr 2003, Chuck Ebbert wrote:
> >
> >
> >
> >>Jens Axboe wrote:
> >>
> >>
> >>
> >>
> >>>>+	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
> >>>> }
> >>>>
> >>>>
> >>>Seconded, it causes a lot more confusion than it does good.
> >>>
> >>>
> >>  The return looks like a function call in that last line.
> >>
> >>  That's one of the few things I find really annoying -- extra parens
> >>are fine when they make code clearer, but not there.
> >>
> >>
> >>-------
> >> Chuck [ C Style Police, badge #666 ]
> >>
> >>
> >
> >return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
> >                                  ^^^^^^|__________ wtf?
> >These undefined numbers in the code are much more annoying to me...
> >but I don't have a C Style Police Badge.
> >
> >Also, whatever that is, 0x400, I'll call it MASK, can have shorter
> >code like:
> >
> >   return (drive->id->cfs_enable_1 && MASK); // TRUE/FALSE
> >... for pedantics...
> >   return (int) (drive->id->cfs_enable_1 && MASK);
> >
> >
> >
> >
>
> That wouldn't work, because && isn't a bitwise operator.  But I agree
> that the ( x ? 1 : 0 ) method may not be very efficient, because it may
> involve branches.
>
> Two alternatives:
>
> (a)     !!(x & 0x400)
>
> (b)     (x & 0x400) >> 10
>

I meant return ((foo & MASK) && 1);

Try it, you'll like it! No shifts, no jumps.

int main()
{
    int foo = 0x12340400;
    printf("%d\n", ((foo & 0x400) && 1));
    printf("%d\n", ((foo & 0x300) && 1));
    printf("%d\n", ((foo & 0x500) && 1));
}



Cheers,
Dick Johnson
Penguin : Linux version 2.4.20 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Timothy Miller]

>
>
>I meant return ((foo & MASK) && 1);
>
>Try it, you'll like it! No shifts, no jumps.
>
>  
>

Looks sweet!  If the compiler is smart, that is.  I'll add that to my 
repetoire.  I'll have to see what the asm output looks like.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Willy Tarreau]

Hi !

Well, although I usually don't like these endless coding-style threads, why
don't you simply use this common form ? :

    return !!(foo & MASK);

I found that the compilers like it much and easily emit conditionnal set
instructions. Eg, on x86, this should be something like :

   testl MASK, foo
   setnz retcode

Cheers,
Willy

On Thu, Apr 24, 2003 at 01:15:39PM -0400, Timothy Miller wrote:
> >
> >
> >I meant return ((foo & MASK) && 1);
> >
> >Try it, you'll like it! No shifts, no jumps.
> >
> > 
> >
> 
> Looks sweet!  If the compiler is smart, that is.  I'll add that to my 
> repetoire.  I'll have to see what the asm output looks like.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Timothy Miller]

Willy Tarreau wrote:

>Hi !
>
>Well, although I usually don't like these endless coding-style threads, why
>don't you simply use this common form ? :
>
>    return !!(foo & MASK);
>
>I found that the compilers like it much and easily emit conditionnal set
>instructions. Eg, on x86, this should be something like :
>
>   testl MASK, foo
>   setnz retcode
>
>  
>
>  
>

That was the first thing I'd suggested.

Let's canonize (in the "add to the canon" sense) this.  :)

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Bill Davidsen]

On Thu, 24 Apr 2003, Richard B. Johnson wrote:


> > Two alternatives:
> >
> > (a)     !!(x & 0x400)
> >
> > (b)     (x & 0x400) >> 10
> >
> 
> I meant return ((foo & MASK) && 1);
> 
> Try it, you'll like it! No shifts, no jumps.

Sorry, I still find !!(foo & MASK) easier to read, because !! is only used
to convert to boolean. Sort of a "boolean cast" in effect. It jumps out at
you what is intended.

Anyway, a matter of taste, both generate jumpless code.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Richard B. Johnson]

On Fri, 25 Apr 2003, Bill Davidsen wrote:

> On Thu, 24 Apr 2003, Richard B. Johnson wrote:
>
>
> > > Two alternatives:
> > >
> > > (a)     !!(x & 0x400)
> > >
> > > (b)     (x & 0x400) >> 10
> > >
> >
> > I meant return ((foo & MASK) && 1);
> >
> > Try it, you'll like it! No shifts, no jumps.
>
> Sorry, I still find !!(foo & MASK) easier to read, because !! is only used
> to convert to boolean. Sort of a "boolean cast" in effect. It jumps out at
> you what is intended.
>
> Anyway, a matter of taste, both generate jumpless code.
Yes, you win. This looks good, both as code and as op-codes on
Intel...
#include <stdio.h>
int main()
{
    int i;
    int mask = 0x40;
    for(i=0; i< 0x1000; i++)
        printf("%08x\n", !!(i & mask));
}

You need to actually change something, hense the for() loop. Otherwise
gcc just optimizes everything to a 1!

Cheers,
Dick Johnson
Penguin : Linux version 2.4.20 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Erik Andersen]

On Sun Apr 27, 2003 at 02:21:04PM +0200, Adrian Bunk wrote:
> Looking at the only user of this function it seems we can completely 
> remove it (patch below).
> 
> Alan:
> Is the patch below OK or are there any future plans for more uses of
> idedisk_supports_host_protected_area?
> 
> >  -Erik

Looks fine to me.  Even better,

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Adrian Bunk]

On Thu, Apr 24, 2003 at 03:34:43AM -0600, Erik Andersen wrote:
> The ide driver does not list whether drives support things like
> write cache, SMART, SECURITY ERASE UNIT.  But for some silly
> reason it tells us at boot whether each drive is capable of
> supporting the Host Protected Area feature set.  If people want
> to know the capabilites of their drive, they can run 'hdparm' 
> and find out.
> 
> This patch removes this pointless noise.  Please apply,
> 
> 
> --- linux/drivers/ide/ide-disk.c.orig	2003-04-24 03:23:53.000000000 -0600
> +++ linux/drivers/ide/ide-disk.c	2003-04-24 03:24:54.000000000 -0600
> @@ -1133,10 +1133,7 @@
>   */
>  static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
>  {
> -	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
> -	if (flag)
> -		printk("%s: host protected area => %d\n", drive->name, flag);
> -	return flag;
> +	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
>  }
>  
>  /*


Looking at the only user of this function it seems we can completely 
remove it (patch below).

Alan:
Is the patch below OK or are there any future plans for more uses of
idedisk_supports_host_protected_area?

>  -Erik

cu
Adrian


--- linux-2.4.21-rc1-full/drivers/ide/ide-disk.c.old	2003-04-27 13:26:17.000000000 +0200
+++ linux-2.4.21-rc1-full/drivers/ide/ide-disk.c	2003-04-27 13:30:48.000000000 +0200
@@ -1128,18 +1128,6 @@
 #endif /* CONFIG_IDEDISK_STROKE */
 
 /*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
-	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
-	if (flag)
-		printk("%s: host protected area => %d\n", drive->name, flag);
-	return flag;
-}
-
-/*
  * Compute drive->capacity, the full capacity of the drive
  * Called with drive->id != NULL.
  *
@@ -1165,8 +1153,6 @@
 	drive->capacity48 = 0;
 	drive->select.b.lba = 0;
 
-	(void) idedisk_supports_host_protected_area(drive);
-
 	if (id->cfs_enable_2 & 0x0400) {
 		capacity_2 = id->lba_capacity_2;
 		drive->head		= drive->bios_head = 255;

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Timothy Miller]

Chuck Ebbert wrote:

>  
>
>
>Those double exclamation points should be hidden in macros. :)
>
>  
>
Not any more.  They've been canonized!  :)

>  
>

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Chuck Ebbert]

Timothy Miller wrote:


> Two alternatives:
>
> (a)     !!(x & 0x400)
> 
> (b)     (x & 0x400) >> 10



I like either of these:

   #define FOO_BITS 0x400
   #define test_mask(t,m) ( !!((t) & (m)) )
   ...
   return test_mask(x, FOO_BITS);

or

   return (x & FOO_BITS) != 0;

Those double exclamation points should be hidden in macros. :)



------
 Chuck

Re: [PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Jens Axboe]

On Thu, Apr 24 2003, Erik Andersen wrote:
> The ide driver does not list whether drives support things like
> write cache, SMART, SECURITY ERASE UNIT.  But for some silly
> reason it tells us at boot whether each drive is capable of
> supporting the Host Protected Area feature set.  If people want
> to know the capabilites of their drive, they can run 'hdparm' 
> and find out.
> 
> This patch removes this pointless noise.  Please apply,
> 
> 
> --- linux/drivers/ide/ide-disk.c.orig	2003-04-24 03:23:53.000000000 -0600
> +++ linux/drivers/ide/ide-disk.c	2003-04-24 03:24:54.000000000 -0600
> @@ -1133,10 +1133,7 @@
>   */
>  static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
>  {
> -	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
> -	if (flag)
> -		printk("%s: host protected area => %d\n", drive->name, flag);
> -	return flag;
> +	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
>  }

Seconded, it causes a lot more confusion than it does good.

-- 
Jens Axboe

[PATCH] 2.4.21-rc1 pointless IDE noise reduction

[Erik Andersen]

The ide driver does not list whether drives support things like
write cache, SMART, SECURITY ERASE UNIT.  But for some silly
reason it tells us at boot whether each drive is capable of
supporting the Host Protected Area feature set.  If people want
to know the capabilites of their drive, they can run 'hdparm' 
and find out.

This patch removes this pointless noise.  Please apply,


--- linux/drivers/ide/ide-disk.c.orig	2003-04-24 03:23:53.000000000 -0600
+++ linux/drivers/ide/ide-disk.c	2003-04-24 03:24:54.000000000 -0600
@@ -1133,10 +1133,7 @@
  */
 static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
 {
-	int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
-	if (flag)
-		printk("%s: host protected area => %d\n", drive->name, flag);
-	return flag;
+	return((drive->id->cfs_enable_1 & 0x0400) ? 1 : 0);
 }
 
 /*

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--