From mboxrd@z Thu Jan  1 00:00:00 1970
From: Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
Date: Mon, 11 Dec 2017 05:05:20 +0000
Message-ID: <20171211050520.GV21978@ZenIV.linux.org.uk>
References: <20171210045326.GO21978@ZenIV.linux.org.uk>
 <420a198d-61f8-81cf-646d-10446cb41def@synopsys.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Cc: netdev@vger.kernel.org
To: Jie Deng <Jie.Deng1@synopsys.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:59472 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750707AbdLKFFW (ORCPT
        <rfc822;netdev@vger.kernel.org>); Mon, 11 Dec 2017 00:05:22 -0500
Content-Disposition: inline
In-Reply-To: <420a198d-61f8-81cf-646d-10446cb41def@synopsys.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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);					\