[IDE] meaningless #ifndef?

[IDE] meaningless #ifndef?

[Pozsar Balazs]

from drivers/ide/ide-features.c:

/*
 *  All hosts that use the 80c ribbon mus use!
 */
byte eighty_ninty_three (ide_drive_t *drive)
{
        return ((byte) ((HWIF(drive)->udma_four) &&
#ifndef CONFIG_IDEDMA_IVB
                        (drive->id->hw_config & 0x4000) &&
#endif /* CONFIG_IDEDMA_IVB */
                        (drive->id->hw_config & 0x6000)) ? 1 : 0);
}

If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
defined or not.
What's the clue?

-- 
pozsy.

Re: [IDE] meaningless #ifndef?

[Andre Hedrick]

On Mon, 19 Feb 2001, Pozsar Balazs wrote:

> from drivers/ide/ide-features.c:
> 
> /*
>  *  All hosts that use the 80c ribbon mus use!
>  */
> byte eighty_ninty_three (ide_drive_t *drive)
> {
>         return ((byte) ((HWIF(drive)->udma_four) &&
> #ifndef CONFIG_IDEDMA_IVB
>                         (drive->id->hw_config & 0x4000) &&
> #endif /* CONFIG_IDEDMA_IVB */
>                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> }
> 
> If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
> defined or not.
> What's the clue?

(drive->id->hw_config & 0x4000)
	mask off 0x4000 for presence.
(drive->id->hw_config & 0x6000) 
	mask off 0x2000 ot 0x4000 for presence.

The first is true only if bit 14 is set.
The second is true if either bit 13 or 14 is set.

Togather they test for two bits.
The first test is exclusive to bit 14
The second test is inclusive of bits 13 and 14.

Because some older drives set only bit 13 for the device side cable-detect,
then newer drives than these did a supported mode bit 14 and no bits 13,
then others do both.

So in order to have a test that supports ATA-4/5/6 you have to allow
users the option to disable the newer sense code that is only valid for
ATA-5/6.  It will get messier still.

Andre Hedrick
Linux ATA Development
ASL Kernel Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

Re: [IDE] meaningless #ifndef?

[Bill Wendling]

Also sprach Andre Hedrick:
} On Mon, 19 Feb 2001, Pozsar Balazs wrote:
} 
} > from drivers/ide/ide-features.c:
} > 
} > /*
} >  *  All hosts that use the 80c ribbon mus use!
} >  */
} > byte eighty_ninty_three (ide_drive_t *drive)
} > {
} >         return ((byte) ((HWIF(drive)->udma_four) &&
} > #ifndef CONFIG_IDEDMA_IVB
} >                         (drive->id->hw_config & 0x4000) &&
} > #endif /* CONFIG_IDEDMA_IVB */
} >                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
} > }
} > 
} > If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
} > defined or not.
} > What's the clue?
} 
[snip...]

The use of the ternary operator is superfluous, though...and makes the
code look ugly IMNSHO :).

-- 
|| Bill Wendling			wendling@ganymede.isdn.uiuc.edu

Re: [IDE] meaningless #ifndef?

[Andre Hedrick]

On Mon, 19 Feb 2001, Bill Wendling wrote:

> The use of the ternary operator is superfluous, though...and makes the
> code look ugly IMNSHO :).

What is ugly is that the commitee can not decide if there is going to be
host-side only, device-side only or both-side.

Cheers,

Andre Hedrick
Linux ATA Development
ASL Kernel Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

Re: [IDE] meaningless #ifndef?

[Tom Leete]

Bill Wendling wrote:
> 
> Also sprach Andre Hedrick:
> } On Mon, 19 Feb 2001, Pozsar Balazs wrote:
> }
> } > from drivers/ide/ide-features.c:
> } >
> } > /*
> } >  *  All hosts that use the 80c ribbon mus use!
> } >  */
> } > byte eighty_ninty_three (ide_drive_t *drive)
> } > {
> } >         return ((byte) ((HWIF(drive)->udma_four) &&
> } > #ifndef CONFIG_IDEDMA_IVB
> } >                         (drive->id->hw_config & 0x4000) &&
> } > #endif /* CONFIG_IDEDMA_IVB */
> } >                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> } > }
> } >
> } > If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
> } > defined or not.
> } > What's the clue?
> }
> [snip...]
> 
> The use of the ternary operator is superfluous, though...and makes the
> code look ugly IMNSHO :).
> 

Check return type. 0 == ((byte) 0x6000).

Tom

Re: [IDE] meaningless #ifndef?

[Tom Leete]

Bill Wendling wrote:
> 
> The use of the ternary operator is superfluous, though...and makes the
> code look ugly IMNSHO :).
> 

You are correct. Please ignore my thinko.

Tom

-- 
The Daemons lurk and are dumb. -- Emerson

Re: [IDE] meaningless #ifndef?

[Andrzej Krzysztofowicz]

"Pozsar Balazs wrote:"
> from drivers/ide/ide-features.c:
> 
> /*
>  *  All hosts that use the 80c ribbon mus use!
>  */
> byte eighty_ninty_three (ide_drive_t *drive)
> {
>         return ((byte) ((HWIF(drive)->udma_four) &&
> #ifndef CONFIG_IDEDMA_IVB
>                         (drive->id->hw_config & 0x4000) &&
> #endif /* CONFIG_IDEDMA_IVB */
>                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> }
> 
> If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
> defined or not.

No, it is not the same. But it duplicates the test when CONFIG_IDEDMA_IVB
is not set. If both tests are done and the first is true, the second also
must be true. If the first is false, the second is meaningless.
Maybe the above code should be:

        return ((byte) ((HWIF(drive)->udma_four) &&
#ifndef CONFIG_IDEDMA_IVB
                        (drive->id->hw_config & 0x4000)
#else
                        (drive->id->hw_config & 0x6000)
#endif /* CONFIG_IDEDMA_IVB */
			) ? 1 : 0);

Just my 0.03 PLN... 
--
=======================================================================
  Andrzej M. Krzysztofowicz               ankry@mif.pg.gda.pl
  phone (48)(58) 347 14 61
Faculty of Applied Phys. & Math.,   Technical University of Gdansk

Re: [IDE] meaningless #ifndef?

[Hugh Dickins]

On Mon, 19 Feb 2001, Andre Hedrick wrote:
> On Mon, 19 Feb 2001, Pozsar Balazs wrote:
> 
> > from drivers/ide/ide-features.c:
> > 
> > /*
> >  *  All hosts that use the 80c ribbon mus use!
> >  */
> > byte eighty_ninty_three (ide_drive_t *drive)
> > {
> >         return ((byte) ((HWIF(drive)->udma_four) &&
> > #ifndef CONFIG_IDEDMA_IVB
> >                         (drive->id->hw_config & 0x4000) &&
> > #endif /* CONFIG_IDEDMA_IVB */
> >                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> > }
> > 
> > If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
> > defined or not.
> > What's the clue?
> 
> (drive->id->hw_config & 0x4000)
> 	mask off 0x4000 for presence.
> (drive->id->hw_config & 0x6000) 
> 	mask off 0x2000 ot 0x4000 for presence.
> 
> The first is true only if bit 14 is set.
> The second is true if either bit 13 or 14 is set.
> 
> Togather they test for two bits.
> The first test is exclusive to bit 14
> The second test is inclusive of bits 13 and 14.
> 
> Because some older drives set only bit 13 for the device side cable-detect,
> then newer drives than these did a supported mode bit 14 and no bits 13,
> then others do both.
> 
> So in order to have a test that supports ATA-4/5/6 you have to allow
> users the option to disable the newer sense code that is only valid for
> ATA-5/6.  It will get messier still.

Andre, please read through that code again, and through your reply.
It seems to me that Poszar is absolutely right, and your reply is
(once again) just saying "there's lots of confusion out there, so
my code has to be confused too".  Or do your &s and &&s behave
differently from ours?

Hugh

Re: [IDE] meaningless #ifndef?

[Vojtech Pavlik]

On Tue, Feb 20, 2001 at 05:45:52PM +0000, Hugh Dickins wrote:
> > 
> > > from drivers/ide/ide-features.c:
> > > 
> > > /*
> > >  *  All hosts that use the 80c ribbon mus use!
> > >  */
> > > byte eighty_ninty_three (ide_drive_t *drive)
> > > {
> > >         return ((byte) ((HWIF(drive)->udma_four) &&
> > > #ifndef CONFIG_IDEDMA_IVB
> > >                         (drive->id->hw_config & 0x4000) &&
> > > #endif /* CONFIG_IDEDMA_IVB */
> > >                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> > > }
> > > 
> > > If i see well, then this is always same whether CONFIG_IDEDMA_IVB is
> > > defined or not.
> > > What's the clue?

> > The first is true only if bit 14 is set.
> > The second is true if either bit 13 or 14 is set.
> > 
> > Togather they test for two bits.
> > The first test is exclusive to bit 14
> > The second test is inclusive of bits 13 and 14.

> Andre, please read through that code again, and through your reply.
> It seems to me that Poszar is absolutely right, and your reply is
> (once again) just saying "there's lots of confusion out there, so
> my code has to be confused too".  Or do your &s and &&s behave
> differently from ours?

Well, the code looks weird. However, it doesn't behave the same when
CONFIG_IDEDMA_IVB is enabled or not. If it is not, normal case, it's
just:

(drive->id->hw_config & 0x6000)

If CONFIG_IDEDMA_IVB is enabled, it boils down to:

(drive->id->hw_config & 0x4000)

because the second bit test includes the earlier test already only
loosening it. Because of that, it's superfluous. And the code relies on
the compiler to optimize it out.

If written like:

#ifndef CONFIG_IDEDMA_IVB
#define IDE_UDMA_MASK 0x4000
#else
#define IDE_UDMA_MASK 0x6000
#endif /* CONFIG_IDEDMA_IVB */

byte eighty_ninty_three (ide_drive_t *drive)
{       
        return HWIF(drive)->udma_four &&
		(drive->id->hw_config & IDE_UDMA_MASK);
}

it'd be probably somewhat clearer.

-- 
Vojtech Pavlik
SuSE Labs

Re: [IDE] meaningless #ifndef?

[Hugh Dickins]

On Tue, 20 Feb 2001, Vojtech Pavlik wrote:
> On Tue, Feb 20, 2001 at 05:45:52PM +0000, Hugh Dickins wrote:
> > > > byte eighty_ninty_three (ide_drive_t *drive)
> > > > {
> > > >         return ((byte) ((HWIF(drive)->udma_four) &&
> > > > #ifndef CONFIG_IDEDMA_IVB
> > > >                         (drive->id->hw_config & 0x4000) &&
> > > > #endif /* CONFIG_IDEDMA_IVB */
> > > >                         (drive->id->hw_config & 0x6000)) ? 1 : 0);
> > > > }
> 
> Well, the code looks weird. However, it doesn't behave the same when
> CONFIG_IDEDMA_IVB is enabled or not. If it is not, normal case, it's
> just:
> 
> (drive->id->hw_config & 0x6000)
> 
> If CONFIG_IDEDMA_IVB is enabled, it boils down to:
> 
> (drive->id->hw_config & 0x4000)
> 
> because the second bit test includes the earlier test already only
> loosening it. Because of that, it's superfluous. And the code relies on
> the compiler to optimize it out.

Thank you for re-explaining this to me.
Yes, you are right, I was wrong, I now
admit my &s and &&s behave like yours,
and I apologize to Andre!

> If written like:
> 
> #ifndef CONFIG_IDEDMA_IVB
> #define IDE_UDMA_MASK 0x4000
> #else
> #define IDE_UDMA_MASK 0x6000
> #endif /* CONFIG_IDEDMA_IVB */
> 
> byte eighty_ninty_three (ide_drive_t *drive)
> {       
>         return HWIF(drive)->udma_four &&
> 		(drive->id->hw_config & IDE_UDMA_MASK);
> }
> 
> it'd be probably somewhat clearer.

That would certainly have helped us!

Hugh