[PATCH 5/11v2] ata: replace macro with static inline in libata.h

[PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Harvey Harrison]

Move to using a static inline which will force the same typechecking
that min_t/max_t do (in this case, short).  As a bonus, avoid a ton
of sparse warnings like:

drivers/ata/pata_ali.c:176:14: warning: symbol '__x' shadows an earlier one
drivers/ata/pata_ali.c:176:14: originally declared here

Due to nesting min_t macro inside max_t macro which both use a __x
identifier internally.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/libata.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..b5590fb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -764,7 +764,14 @@ struct ata_timing {
 	unsigned short udma;		/* t2CYCTYP/2 */
 };
 
-#define FIT(v, vmin, vmax)	max_t(short, min_t(short, v, vmax), vmin)
+static inline short FIT(short v, short vmin, short vmax)
+{
+	if (v >= vmax)
+		return vmax;
+	if (v <= vmin)
+		return vmin;
+	return v;
+}
 
 extern const unsigned long sata_deb_timing_normal[];
 extern const unsigned long sata_deb_timing_hotplug[];
-- 
1.5.4.1.1278.gc75be

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Alan Cox]

On Fri, 15 Feb 2008 14:06:55 -0800
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Move to using a static inline which will force the same typechecking
> that min_t/max_t do (in this case, short).  As a bonus, avoid a ton
> of sparse warnings like:
> 
> drivers/ata/pata_ali.c:176:14: warning: symbol '__x' shadows an earlier one
> drivers/ata/pata_ali.c:176:14: originally declared here
> 
> Due to nesting min_t macro inside max_t macro which both use a __x
> identifier internally.

NAK. This is a sparse bug, fix sparse.

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Harvey Harrison]

On Fri, 2008-02-15 at 22:30 +0000, Alan Cox wrote:
> On Fri, 15 Feb 2008 14:06:55 -0800
> Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > Move to using a static inline which will force the same typechecking
> > that min_t/max_t do (in this case, short).  As a bonus, avoid a ton
> > of sparse warnings like:
> > 
> > drivers/ata/pata_ali.c:176:14: warning: symbol '__x' shadows an earlier one
> > drivers/ata/pata_ali.c:176:14: originally declared here
> > 
> > Due to nesting min_t macro inside max_t macro which both use a __x
> > identifier internally.
> 
> NAK. This is a sparse bug, fix sparse.

Yes, fair enough, but that's not all the patch is about.

1) it's using a max_t and min_t to force the comparisons as shorts, why
not just make it a static inline?

2) the static inline is a little clearer about the intent here.

3) the sparse warnings are entirely secondary (and technically correct
when the macros expand, __x is shadowed)

4) I may be mistaken, but I thought then when something can be written
as a static inline instead of a macro it was preferred. At least I've
seen akpm say so, but I'll let him speak for himself (added to CC:)

Cheers,

Harvey

From: Harvey Harrison <harvey.harrison@gmail.com>
Subject: [PATCH] ata: replace macro with static inline in libata.h

Avoid a metric ton of sparse warnings like:
drivers/ata/pata_ali.c:176:14: warning: symbol '__x' shadows an earlier one
drivers/ata/pata_ali.c:176:14: originally declared here

Due to nesting min_t macro inside max_t macro which both use a __x
identifier internally.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/libata.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..b5590fb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -764,7 +764,14 @@ struct ata_timing {
 	unsigned short udma;		/* t2CYCTYP/2 */
 };
 
-#define FIT(v, vmin, vmax)	max_t(short, min_t(short, v, vmax), vmin)
+static inline short FIT(short v, short vmin, short vmax)
+{
+	if (v >= vmax)
+		return vmax;
+	if (v <= vmin)
+		return vmin;
+	return v;
+}
 
 extern const unsigned long sata_deb_timing_normal[];
 extern const unsigned long sata_deb_timing_hotplug[];
-- 
1.5.4.1.1278.gc75be

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Alan Cox]

> > NAK. This is a sparse bug, fix sparse.
> 
> Yes, fair enough, but that's not all the patch is about.
> 
> 1) it's using a max_t and min_t to force the comparisons as shorts, why
> not just make it a static inline?

Because max_t and min_t also force the comparsion types

> 2) the static inline is a little clearer about the intent here.

Why ?

> 3) the sparse warnings are entirely secondary (and technically correct
> when the macros expand, __x is shadowed)

In a controlled manner. I guess you could make min and max use __x and __y

> 4) I may be mistaken, but I thought then when something can be written
> as a static inline instead of a macro it was preferred. At least I've
> seen akpm say so, but I'll let him speak for himself (added to CC:)

gcc still sometimes seems to optimise macros better than inlines.

Alan

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Harvey Harrison]

On Fri, 2008-02-15 at 22:53 +0000, Alan Cox wrote:
> > > NAK. This is a sparse bug, fix sparse.
> > 
> > Yes, fair enough, but that's not all the patch is about.
> > 
> > 1) it's using a max_t and min_t to force the comparisons as shorts, why
> > not just make it a static inline?
> 
> Because max_t and min_t also force the comparsion types

Umm, maybe I'm missing something then, but how does the static inline
not do this?

> 
> > 2) the static inline is a little clearer about the intent here.
> 
> Why ?

OK, maybe not much clearer.  But isn't the inline easier to see at
a glance that it is returning a value constrained to be

vmin <= v <= vmax

I suppose the variable names make it clear, but the macro construction
is (slightly) less obvious.

> 
> > 3) the sparse warnings are entirely secondary (and technically correct
> > when the macros expand, __x is shadowed)
> 
> In a controlled manner. I guess you could make min and max use __x and __y
> 

__mint __maxt...but I'm not proposing that.

> > 4) I may be mistaken, but I thought then when something can be written
> > as a static inline instead of a macro it was preferred. At least I've
> > seen akpm say so, but I'll let him speak for himself (added to CC:)
> 
> gcc still sometimes seems to optimise macros better than inlines.

OK, I didn't realize that, any pointers?

Harvey

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Alan Cox]

On Fri, 15 Feb 2008 15:08:50 -0800
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> On Fri, 2008-02-15 at 22:53 +0000, Alan Cox wrote:
> > > > NAK. This is a sparse bug, fix sparse.
> > > 
> > > Yes, fair enough, but that's not all the patch is about.
> > > 
> > > 1) it's using a max_t and min_t to force the comparisons as shorts, why
> > > not just make it a static inline?
> > 
> > Because max_t and min_t also force the comparsion types
> 
> Umm, maybe I'm missing something then, but how does the static inline
> not do this?

You claimed it was an advantage of the static inline earlier but both do
anyway

> OK, maybe not much clearer.  But isn't the inline easier to see at
> a glance that it is returning a value constrained to be
> 
> vmin <= v <= vmax
> 
> I suppose the variable names make it clear, but the macro construction
> is (slightly) less obvious.

Perhaps then clamp_t()

> __mint __maxt...but I'm not proposing that.

I am - as I bet there are other examples of that construct in the tree.

> > gcc still sometimes seems to optimise macros better than inlines.
> 
> OK, I didn't realize that, any pointers?

Not offhand, there is discussion in the archives but it may be somewhat
out of date for the latest gcc.

I'm not arguing your change is -wrong- I just think the original is
tidier and clearer. Its up to Jeff anyway

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Harvey Harrison]

On Sat, 2008-02-16 at 00:05 +0000, Alan Cox wrote:
> On Fri, 15 Feb 2008 15:08:50 -0800
> Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > On Fri, 2008-02-15 at 22:53 +0000, Alan Cox wrote:
> > > > > NAK. This is a sparse bug, fix sparse.
> > > > 
> > > > Yes, fair enough, but that's not all the patch is about.
> > > > 
> > > > 1) it's using a max_t and min_t to force the comparisons as shorts, why
> > > > not just make it a static inline?
> > > 
> > > Because max_t and min_t also force the comparsion types
> > 
> > Umm, maybe I'm missing something then, but how does the static inline
> > not do this?
> 
> You claimed it was an advantage of the static inline earlier but both do
> anyway

Oh, I thought I said it accomplished the _same_ typechecking, my bad.

> 
> > OK, maybe not much clearer.  But isn't the inline easier to see at
> > a glance that it is returning a value constrained to be
> > 
> > vmin <= v <= vmax
> > 
> > I suppose the variable names make it clear, but the macro construction
> > is (slightly) less obvious.
> 
> Perhaps then clamp_t()
> 
> > __mint __maxt...but I'm not proposing that.
> 
> I am - as I bet there are other examples of that construct in the tree.

Lots, but form what I've seen, most could use a helper inline that is a
lot cleaner than what is currently there.  This case is about the same
either way.

> 
> > > gcc still sometimes seems to optimise macros better than inlines.
> > 
> > OK, I didn't realize that, any pointers?
> 
> Not offhand, there is discussion in the archives but it may be somewhat
> out of date for the latest gcc.
> 
> I'm not arguing your change is -wrong- I just think the original is
> tidier and clearer. Its up to Jeff anyway

Fair enough.  This change would make sparse a whole lot more useful
for libata, as it's down to 6 warnings with this and my 3/3 patch
removing another min/max nesting.  Going forward this would probably
make it easier on the maintainer to script up some automated checking.

Cheers,

Harvey

Re: [PATCH 5/11v2] ata: replace macro with static inline in libata.h

[Harvey Harrison]

From: Harvey Harrison <harvey.harrison@gmail.com>
Subject: [PATCH 3/3] ata: simplify clock divisor logic in pata_amd.c

Current code is essentially choosing between dividing by 1 or
dividing by two, make the conditions a little more obvious.

As a bonus, removes a sparse error:
drivers/ata/pata_amd.c:59:11: warning: symbol '__x' shadows an earlier one
drivers/ata/pata_amd.c:59:11: originally declared here

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Thought I'd follow up with just what the 3/3 patch I referenced was.

 drivers/ata/pata_amd.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index 4b8d9b5..7ef4847 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -56,7 +56,9 @@ static void timing_setup(struct ata_port *ap, struct ata_device *adev, int offse
 	u8 t;
 
 	T = 1000000000 / amd_clock;
-	UT = T / min_t(int, max_t(int, clock, 1), 2);
+	UT = T;
+	if (clock >= 2)
+		UT = T / 2;
 
 	if (ata_timing_compute(adev, speed, &at, T, UT) < 0) {
 		dev_printk(KERN_ERR, &pdev->dev, "unknown mode %d.\n", speed);
-- 
1.5.4.1.1278.gc75be