[net-next,1/4] bnx2x: refactor nvram read procedure

introduce a parameter to allow nvram read to return
data in BE or cpu order.

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |   53 +++++++++-----------
 1 files changed, 24 insertions(+), 29 deletions(-)

Dmitry Kravkov <dmitry@broadcom.com> :
> introduce a parameter to allow nvram read to return
> data in BE or cpu order.
[...]
> @@ -1295,10 +1296,14 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
>  		if (val & MCPR_NVM_COMMAND_DONE) {
>  			val = REG_RD(bp, MCP_REG_MCPR_NVM_READ);
>  			/* we read nvram data in cpu order
> -			 * but ethtool sees it as an array of bytes
> +			 * but ethtool uses it as an array of bytes
>  			 * converting to big-endian will do the work
> +			 * if requested.

You memcpy a u32 to an array of bytes instead of copying it byte after
byte with proper shift operators and now you are paving the way for more
endianess headaches. I'd rather avoid the memcpy when readying data for
ethtool in the first place.

Nit: the true/false method parameter style in middle layers is mildly
readable when compared to usual _{be/le} kernel style (you should be
able to avoid both almost completely anyway :o) ).

> -----Original Message-----
> From: Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Saturday, April 13, 2013 2:09 PM
> To: Dmitry Kravkov
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Eilon Greenstein
> Subject: Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure
> 
> Dmitry Kravkov <dmitry@broadcom.com> :
> > introduce a parameter to allow nvram read to return
> > data in BE or cpu order.
> [...]
> > @@ -1295,10 +1296,14 @@ static int bnx2x_nvram_read_dword(struct bnx2x *bp, u32 offset, __be32 *ret_val,
> >  		if (val & MCPR_NVM_COMMAND_DONE) {
> >  			val = REG_RD(bp, MCP_REG_MCPR_NVM_READ);
> >  			/* we read nvram data in cpu order
> > -			 * but ethtool sees it as an array of bytes
> > +			 * but ethtool uses it as an array of bytes
> >  			 * converting to big-endian will do the work
> > +			 * if requested.
> 
> You memcpy a u32 to an array of bytes instead of copying it byte after
> byte with proper shift operators and now you are paving the way for more
> endianess headaches. I'd rather avoid the memcpy when readying data for
> ethtool in the first place.
In case of _be data I don't see any issue with copying _be32 it into byte array.
When internal driver function (not exposed to external usage) requests data in CPU order - it meant to bring u32 data ( or array of u32) and memcpy dword by dword also should be harmless.
 
> Nit: the true/false method parameter style in middle layers is mildly
> readable when compared to usual _{be/le} kernel style (you should be
> able to avoid both almost completely anyway :o) ).
Boolean parameter used to share pre- and post-configuring nvram for read operation.
_be/le is less suitable here, since the usage is BE or cpu order.
I was thinking about separating stream read and u32 read to separate callbacks, but this will add another level and this is less preferable than true/false param.
 
> --
> Ueimor


--
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

Dmitry Kravkov <dmitry@broadcom.com> :
[...]
> > You memcpy a u32 to an array of bytes instead of copying it byte after
> > byte with proper shift operators and now you are paving the way for more
> > endianess headaches. I'd rather avoid the memcpy when readying data for
> > ethtool in the first place.
> In case of _be data I don't see any issue with copying _be32 it into byte array.

The modified code (bnx2x_nvram_read) will not be copying __be32 but u32.
It issmuggling _be data behind neutral u32 and using casting when the
kernel has provided cpu_to_{le / be} helpers for years. Think of type
checking as kind of messed up as soon as u32 *ret_val contains a _le or
_be data depending on the value of bool to_be.

I don't want to worry about the endianness of a u32. If it's a u32 (u16,
s32, whatever), I only want to think of bytes in it through '>>' or '<<'
operators. No need to remember the semantic of the last bnx2x_nvram_read_dword
parameter, if it's internal, external, nor assume that the lower layers
enforce some ability to memcpy it blindly. So I'd just favor using cpu order
as soon and as much as possible.

Of course it's your driver, whence your maintenance choices.

> -----Original Message-----
> From: Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Tuesday, April 16, 2013 2:32 AM
> To: Dmitry Kravkov
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Eilon Greenstein
> Subject: Re: [PATCH net-next 1/4] bnx2x: refactor nvram read procedure
> 
> Dmitry Kravkov <dmitry@broadcom.com> :
> [...]
> > > You memcpy a u32 to an array of bytes instead of copying it byte after
> > > byte with proper shift operators and now you are paving the way for more
> > > endianess headaches. I'd rather avoid the memcpy when readying data for
> > > ethtool in the first place.
> > In case of _be data I don't see any issue with copying _be32 it into byte array.
> 
> The modified code (bnx2x_nvram_read) will not be copying __be32 but u32.
> It issmuggling _be data behind neutral u32 and using casting when the
> kernel has provided cpu_to_{le / be} helpers for years. Think of type
> checking as kind of messed up as soon as u32 *ret_val contains a _le or
> _be data depending on the value of bool to_be.
> 
> I don't want to worry about the endianness of a u32. If it's a u32 (u16,
> s32, whatever), I only want to think of bytes in it through '>>' or '<<'
> operators. No need to remember the semantic of the last bnx2x_nvram_read_dword
> parameter, if it's internal, external, nor assume that the lower layers
> enforce some ability to memcpy it blindly. So I'd just favor using cpu order
> as soon and as much as possible.
> 
> Of course it's your driver, whence your maintenance choices.

I will re-work it soon! Thanks

--
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