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 ?
