Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
On 2017/12/11 13:38, Al Viro wrote:
> On Mon, Dec 11, 2017 at 05:05:20AM +, Al Viro wrote:
>
>> What for? Sure, this variant will work, but why bother with
>> a = le32_to_cpu(b);
>> (cpu_to_le32(a) & ) |
>> and how is that better than
>> (b & ...) | ...
>>
>> IDGI... Mind you, I'm not sure if there is any point keeping _var in that
>> thing,
>> seeing that we use var only once - might be better off with
>> ((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) |\
>> cpu_to_le32(_val); \
> FWIW, seeing how many drivers end up open-coding that, I'm rather tempted to
> add to linux/bitops.h or linux/byteorder/generic.h the following:
>
> static inline __le16 le16_replace_bits(__le16 old, u16 v, int bit, int size)
> {
> __le16 mask = cpu_to_le16(GENMASK(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_le16(v << bit) & mask);
> }
>
> static inline __le32 le32_replace_bits(__le32 old, u32 v, int bit, int size)
> {
> __le32 mask = cpu_to_le32(GENMASK(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_le32(v << bit) & mask);
> }
>
> static inline __le64 le64_replace_bits(__le64 old, u64 v, int bit, int size)
> {
> __le64 mask = cpu_to_le64(GENMASK_ULL(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_le64(v << bit) & mask);
> }
>
> static inline __be16 be16_replace_bits(__be16 old, u16 v, int bit, int size)
> {
> __be16 mask = cpu_to_be16(GENMASK(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_be16(v << bit) & mask);
> }
>
> static inline __be32 be32_replace_bits(__be32 old, u32 v, int bit, int size)
> {
> __be32 mask = cpu_to_be32(GENMASK(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_be32(v << bit) & mask);
> }
>
> static inline __be64 be64_replace_bits(__be64 old, u64 v, int bit, int size)
> {
> __be64 mask = cpu_to_be64(GENMASK_ULL(bit + size - 1, bit));
> return (old & ~mask) | (cpu_to_be64(v << bit) & mask);
> }
>
> static inline u16 le16_get_bits(__le16 v, int bit, int size)
> {
> return (le16_to_cpu(v) >> bit) & (BIT(size) - 1);
> }
>
> static inline u32 le32_get_bits(__le32 v, int bit, int size)
> {
> return (le32_to_cpu(v) >> bit) & (BIT(size) - 1);
> }
>
> static inline u64 le64_get_bits(__le64 v, int bit, int size)
> {
> return (le64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
> }
>
> static inline u16 be16_get_bits(__be16 v, int bit, int size)
> {
> return (be16_to_cpu(v) >> bit) & (BIT(size) - 1);
> }
>
> static inline u32 be32_get_bits(__be32 v, int bit, int size)
> {
> return (be32_to_cpu(v) >> bit) & (BIT(size) - 1);
> }
>
> static inline u64 be64_get_bits(__be64 v, int bit, int size)
> {
> return (be64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
> }
>
> and let drivers use those...
Sounds good. As a driver developer, I'm happy to see this.
Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
On 2017/12/11 13:05, Al Viro wrote: > On Mon, Dec 11, 2017 at 12:33:42PM +0800, Jie Deng wrote: >> Hi AI Viro, >>> @@ -125,8 +125,8 @@ >>> typeof(len) _len = (len); \ >>> typeof(val) _val = (val); \ >>> _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \ >>> - _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \ >>> - cpu_to_le32(_var); \ >>> + (_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | \ >>> + cpu_to_le32(_val); \ >>> }) >>> >>> struct xlgmac_pdata; >> Make sense. But I think what you want is fix as follows. Right ? >> >> @@ -125,8 +125,8 @@ >> typeof(len) _len = (len); >> \ >> -typeof(val) _val = (val); >> \ >> + u32 _var = le32_to_cpu((var)); >>\ >> _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); >> \ >> -_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; >> \ >> -cpu_to_le32(_var); >> \ >> +(cpu_to_le32(_var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | >> \ >> +cpu_to_le32(_val); >> \ >> }) > What for? Sure, this variant will work, but why bother with > a = le32_to_cpu(b); > (cpu_to_le32(a) & ) | > and how is that better than > (b & ...) | ... > > IDGI... Mind you, I'm not sure if there is any point keeping _var in that > thing, > seeing that we use var only once - might be better off with > ((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) |\ > cpu_to_le32(_val); \ I agree. Could you please send the patch with this better fix ?
Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
On Mon, Dec 11, 2017 at 05:05:20AM +, Al Viro wrote:
> What for? Sure, this variant will work, but why bother with
> a = le32_to_cpu(b);
> (cpu_to_le32(a) & ) |
> and how is that better than
> (b & ...) | ...
>
> IDGI... Mind you, I'm not sure if there is any point keeping _var in that
> thing,
> seeing that we use var only once - might be better off with
> ((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) |\
> cpu_to_le32(_val); \
FWIW, seeing how many drivers end up open-coding that, I'm rather tempted to
add to linux/bitops.h or linux/byteorder/generic.h the following:
static inline __le16 le16_replace_bits(__le16 old, u16 v, int bit, int size)
{
__le16 mask = cpu_to_le16(GENMASK(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_le16(v << bit) & mask);
}
static inline __le32 le32_replace_bits(__le32 old, u32 v, int bit, int size)
{
__le32 mask = cpu_to_le32(GENMASK(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_le32(v << bit) & mask);
}
static inline __le64 le64_replace_bits(__le64 old, u64 v, int bit, int size)
{
__le64 mask = cpu_to_le64(GENMASK_ULL(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_le64(v << bit) & mask);
}
static inline __be16 be16_replace_bits(__be16 old, u16 v, int bit, int size)
{
__be16 mask = cpu_to_be16(GENMASK(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_be16(v << bit) & mask);
}
static inline __be32 be32_replace_bits(__be32 old, u32 v, int bit, int size)
{
__be32 mask = cpu_to_be32(GENMASK(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_be32(v << bit) & mask);
}
static inline __be64 be64_replace_bits(__be64 old, u64 v, int bit, int size)
{
__be64 mask = cpu_to_be64(GENMASK_ULL(bit + size - 1, bit));
return (old & ~mask) | (cpu_to_be64(v << bit) & mask);
}
static inline u16 le16_get_bits(__le16 v, int bit, int size)
{
return (le16_to_cpu(v) >> bit) & (BIT(size) - 1);
}
static inline u32 le32_get_bits(__le32 v, int bit, int size)
{
return (le32_to_cpu(v) >> bit) & (BIT(size) - 1);
}
static inline u64 le64_get_bits(__le64 v, int bit, int size)
{
return (le64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
}
static inline u16 be16_get_bits(__be16 v, int bit, int size)
{
return (be16_to_cpu(v) >> bit) & (BIT(size) - 1);
}
static inline u32 be32_get_bits(__be32 v, int bit, int size)
{
return (be32_to_cpu(v) >> bit) & (BIT(size) - 1);
}
static inline u64 be64_get_bits(__be64 v, int bit, int size)
{
return (be64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
}
and let drivers use those...
Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
On Mon, Dec 11, 2017 at 12:33:42PM +0800, Jie Deng wrote: > Hi AI Viro, > > @@ -125,8 +125,8 @@ > > typeof(len) _len = (len); \ > > typeof(val) _val = (val); \ > > _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \ > > - _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \ > > - cpu_to_le32(_var); \ > > + (_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | \ > > + cpu_to_le32(_val); \ > > }) > > > > struct xlgmac_pdata; > > Make sense. But I think what you want is fix as follows. Right ? > > @@ -125,8 +125,8 @@ > typeof(len) _len = (len); > \ > - typeof(val) _val = (val); > \ > + u32 _var = le32_to_cpu((var)); > \ > _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); > \ > - _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; > \ > - cpu_to_le32(_var); > \ > + (cpu_to_le32(_var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | > \ > + cpu_to_le32(_val); > \ > }) What for? Sure, this variant will work, but why bother with a = le32_to_cpu(b); (cpu_to_le32(a) & ) | and how is that better than (b & ...) | ... IDGI... Mind you, I'm not sure if there is any point keeping _var in that thing, seeing that we use var only once - might be better off with ((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) |\ cpu_to_le32(_val); \
Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
Hi AI Viro,
On 2017/12/10 12:53, Al Viro wrote:
> In xlgmac_dev_xmit():
>
> /* Mark it as a CONTEXT descriptor */
> dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
> dma_desc->desc3,
> TX_CONTEXT_DESC3_CTXT_POS,
> TX_CONTEXT_DESC3_CTXT_LEN,
> 1);
>
>
>
> Looking at XLGMAC_SET_REG_BITS_LE() we see this:
> #define XLGMAC_SET_REG_BITS_LE(var, pos, len, val) ({ \
> typeof(var) _var = (var); \
> typeof(pos) _pos = (pos); \
> typeof(len) _len = (len); \
> typeof(val) _val = (val); \
> _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \
> _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \
> cpu_to_le32(_var); \
> })
>
> That thing assumes var to be host-endian and has a little-endian result.
> Unfortunately, we feed it a little-endian and store the result back into
> the same place. That might work if the original values *was* host-endian
> and we wanted to end up with little-endian. However, that is immediately
> followed by
> /* Indicate this descriptor contains the MSS */
> dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
> dma_desc->desc3,
> TX_CONTEXT_DESC3_TCMSSV_POS,
> TX_CONTEXT_DESC3_TCMSSV_LEN,
> 1);
>
> where we operate on the now definitely little-endian value. That really
> can't be right. I don't have the hardware in question, so I can't test
> that, but it smells like this needs something like diff below, making
> XLGMAC_SET_REG_BITS_LE take le32 and return le32. GET side of things
> is le32 -> u32; definition looks correct, but slightly misannotated.
>
> Signed-off-by: Al Viro
> ---
> diff --git a/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> index cab3e40a86b9..e95c4c250e16 100644
> --- a/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> +++ b/drivers/net/ethernet/synopsys/dwc-xlgmac.h
> @@ -106,7 +106,7 @@
> #define XLGMAC_GET_REG_BITS_LE(var, pos, len) ({ \
> typeof(pos) _pos = (pos); \
> typeof(len) _len = (len); \
> - typeof(var) _var = le32_to_cpu((var)); \
> + u32 _var = le32_to_cpu((var)); \
> ((_var) & GENMASK(_pos + _len - 1, _pos)) >> (_pos);\
> })
>
> @@ -125,8 +125,8 @@
> typeof(len) _len = (len); \
> typeof(val) _val = (val); \
> _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos); \
> - _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val; \
> - cpu_to_le32(_var); \
> + (_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | \
> + cpu_to_le32(_val); \
> })
>
> struct xlgmac_pdata;
Make sense. But I think what you want is fix as follows. Right ?
@@ -125,8 +125,8 @@
typeof(len) _len = (len);
\
- typeof(val) _val = (val);
\
+ u32 _var = le32_to_cpu((var));
\
_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);
\
- _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;
\
- cpu_to_le32(_var);
\
+ (cpu_to_le32(_var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) |
\
+ cpu_to_le32(_val);
\
})
