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

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

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