[2/3] bnx2x: Casting page alignment

Subject: [PATCH 2/3] bnx2x: Casting page alignment

Adding a proper cast to the argument of PAGE_ALIGN macro so that the output
won't depend on its original type. Without this cast aligned value will be
truncated to the size of the argument type.

Reported-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Signed-off-by: Vladislav Zolotarov <vladz@broadcom.com>
Tested-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/bnx2x.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--
1.5.6.3




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Monday 09 March 2009 4:52:17 am Eilon Greenstein wrote:
> Subject: [PATCH 2/3] bnx2x: Casting page alignment
>
> Adding a proper cast to the argument of PAGE_ALIGN macro so that the output
> won't depend on its original type. Without this cast aligned value will be
> truncated to the size of the argument type.

I tested these patches (1 & 2) in the SLES11 RC5 kernel and verified
that they fix https://bugzilla.novell.com/show_bug.cgi?id=481074

However, I'm not 100% comfortable with this patch:

> diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> index 15a5cf0..3cf2b92 100644
> --- a/drivers/net/bnx2x.h
> +++ b/drivers/net/bnx2x.h
> @@ -152,7 +152,7 @@ struct sw_rx_page {
>  #define PAGES_PER_SGE			(1 << PAGES_PER_SGE_SHIFT)
>  #define SGE_PAGE_SIZE			PAGE_SIZE
>  #define SGE_PAGE_SHIFT			PAGE_SHIFT
> -#define SGE_PAGE_ALIGN(addr)		PAGE_ALIGN(addr)
> +#define SGE_PAGE_ALIGN(addr)		PAGE_ALIGN((typeof(PAGE_SIZE))addr)

For one thing, we started with SGE_PAGE_SIZE,SHIFT,ALIGN being
exact duplicates of PAGE_SIZE,SHIFT,ALIGN, which makes me wonder
why we need the SGE_ variety.

But more importantly, the patch adds a cast in SGE_PAGE_ALIGN to
ensure that its argument doesn't get truncated in the process of
being rounded up.  This could happen anywhere, not just in bnx2x,
so my question is whether the cast should be done in PAGE_ALIGN
itself so we don't trip over this again somewhere else.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

See below. 

> > diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> > index 15a5cf0..3cf2b92 100644
> > --- a/drivers/net/bnx2x.h
> > +++ b/drivers/net/bnx2x.h
> > @@ -152,7 +152,7 @@ struct sw_rx_page {
> >  #define PAGES_PER_SGE			(1 << 
PAGES_PER_SGE_SHIFT)
> >  #define SGE_PAGE_SIZE			PAGE_SIZE
> >  #define SGE_PAGE_SHIFT			PAGE_SHIFT
> > -#define SGE_PAGE_ALIGN(addr)		PAGE_ALIGN(addr)
> > +#define SGE_PAGE_ALIGN(addr)		PAGE_ALIGN((typeof(PAGE_SIZE))addr)
> 
> For one thing, we started with SGE_PAGE_SIZE,SHIFT,ALIGN being
> exact duplicates of PAGE_SIZE,SHIFT,ALIGN, which makes me wonder
> why we need the SGE_ variety.

SGE_X macros represent the interface to our microcode, which currently
works with native page sizes, however this may change in the future (and
it was different in the past). So, as long as it doesn't harm the performance,
adds readability and is a better programming style (as long as it
implements specific functionality) I'd prefer to leave it the way it is. 

But thanks for reviewing, Bjorn.

> 
> But more importantly, the patch adds a cast in SGE_PAGE_ALIGN to
> ensure that its argument doesn't get truncated in the process of
> being rounded up.  This could happen anywhere, not just in bnx2x,
> so my question is whether the cast should be done in PAGE_ALIGN
> itself so we don't trip over this again somewhere else.
> 

Frankly speaking I thought of it too, when I found the "bug" and I was quite pissed off
with the change in PAGE_ALIGN macro introduced in 2.6.27 (http://lkml.org/lkml/2008/6/15/85).
It's always pissing off when u have to change your code... ;)
But then I gave it another look and agreed that it's only the mater of semantics.

To recall, the change in general was to perform the alignment inside the range of the type
of the argument, which means, for example, when u have something like:

/* e.g. ppc64 with 64K page size */
#define PAGE_SIZE 65536  /* 64KB */

u16 k = 10;
u32 n = PAGE_ALIGN(k); /* This will give u 0 and not 64K as u might expect */

/* End of example */

The semantics had been changed to implement the following sentence:
"PAGE_ALIGN(k) is a value that k would have if aligned to the PAGE_SIZE"
rather than "PAGE_ALIGN(k) is the smallest multiple of PAGE_SIZE larger than k".
There were reasons for that - see the comments for the Andrea's patch.

So, as I said, when I was going to submit a patch fixing PAGE_ALIGN
macro:
 - PAGE_SIZE should be of the integer type having the same size as void*.
 - PAGE_ALIGN(x) should be defined as (((x)+PAGE_SIZE-1) & PAGE_MASK).

I understood that there is no actually important reason not to use ALIGN(x,a) macro for
PAGE_ALIGN and redefine it.

Right, there is this "feature" that I've described above but it's quite fair: aligned value stays inside
the boundaries of the value u r going to align, and if u expect it to the larger - do the proper cast.
This is exactly what my patch is doing.

Once again, thanks for reviewing and sorry if I talked to much... :)

Regards,
vlad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html