From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jie Deng <Jie.Deng1@synopsys.com>
Subject: Re: [RFC][PATCH] apparent big-endian bugs in dwc-xlgmac
Date: Mon, 11 Dec 2017 14:18:08 +0800
Message-ID: <51551924-28b6-2c1c-2b3d-1d307e1d70ab@synopsys.com>
References: <20171210045326.GO21978@ZenIV.linux.org.uk>
 <420a198d-61f8-81cf-646d-10446cb41def@synopsys.com>
 <20171211050520.GV21978@ZenIV.linux.org.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Cc: <netdev@vger.kernel.org>
To: Al Viro <viro@ZenIV.linux.org.uk>,
        Jie Deng <Jie.Deng1@synopsys.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from smtprelay2.synopsys.com ([198.182.60.111]:39302 "EHLO
        smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750836AbdLKGTq (ORCPT
        <rfc822;netdev@vger.kernel.org>); Mon, 11 Dec 2017 01:19:46 -0500
Received: from mailhost.synopsys.com (mailhost2.synopsys.com [10.13.184.66])
        by smtprelay.synopsys.com (Postfix) with ESMTP id 1768910C1376
        for <netdev@vger.kernel.org>; Sun, 10 Dec 2017 22:19:46 -0800 (PST)
Received: from mailhost.synopsys.com (localhost [127.0.0.1])
        by mailhost.synopsys.com (Postfix) with ESMTP id 0764AE9A
        for <netdev@vger.kernel.org>; Sun, 10 Dec 2017 22:19:46 -0800 (PST)
Received: from us01wehtc1.internal.synopsys.com (us01wehtc1.internal.synopsys.com [10.12.239.235])
        by mailhost.synopsys.com (Postfix) with ESMTP id ED3E6E97
        for <netdev@vger.kernel.org>; Sun, 10 Dec 2017 22:19:45 -0800 (PST)
In-Reply-To: <20171211050520.GV21978@ZenIV.linux.org.uk>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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 ?