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 <v...@zeniv.linux.org.uk>
> ---
> 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);
\
})
