Message-ID: <a096fa59-4a32-237a-e06f-a86dfc74a310@vivo.com>
Date:   Wed, 8 Dec 2021 15:09:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH] net: gro: use IS_ERR before PTR_ERR
Content-Language: en-US
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel <kernel@vivo.com>
References: <20211207073116.3856-1-guozhengkui@vivo.com>
 <AG2AqAARE0*4ic4l2539l4rv.9.1638888118554.Hmail.guozhengkui@vivo.com.@PDIwMjExMjA3MTQ0MTM3LjIyNDU0LTEtYWxleGFuZHIubG9iYWtpbkBpbnRlbC5jb20+>
From:   Guo Zhengkui <guozhengkui@vivo.com>
In-Reply-To: <AG2AqAARE0*4ic4l2539l4rv.9.1638888118554.Hmail.guozhengkui@vivo.com.@PDIwMjExMjA3MTQ0MTM3LjIyNDU0LTEtYWxleGFuZHIubG9iYWtpbkBpbnRlbC5jb20+>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UCswR1RDdk5ILy9OeXJ0UFZQOTFPeHBvY3Q4bEJLRDVaMVp0QUdHV0FOa25V?=
 =?utf-8?B?eFM3Ums4bDVjT25VZWMreG9GL0l4SUN6NFpDZGtqRGtJa09rakRuUXNuMWFn?=
 =?utf-8?B?Y1FvSzJJOG4xWHpNcWFGN3d2ZGFVZmFXcE1iL2ZWc1lsRzN1NVQxdWRLeXVH?=
 =?utf-8?B?bW5zL1U3TklTeWN3dG1xWDVnS1B4K0Uxdkd4S0Nsc3ppTTFDN3pxdEZzc1dm?=
 =?utf-8?B?Mk44MmhYa1hIcXlmbEhDd3h4cW16V0pURllZSzBtRSs2U0RKNXdTSUtPSGVr?=
 =?utf-8?B?UkR3czRxenBISjV6dlNqckNhMS9TaFNDODMyNGI3MFlDMkM3R0tOT1RrSDVn?=
 =?utf-8?B?SXZyLzZjWW5ndFF5c1k5cHhqSnVTSUlURGxwS1hwZWlzSFQvRUpoYi93Rm9r?=
 =?utf-8?B?VXB6dmwrTnRSb1luR2pNTVV2dFRsVlJlMGluanhYY1VjT3pXRXpueW1EcjFQ?=
 =?utf-8?B?NFR0ZlVSTWFpMHhidnk5a0hMVzY1WmhPSEdlaHFIL3pQYnF0cmFUTEZISHVn?=
 =?utf-8?B?TlJONGVFVU0xVnp6TE94OFNROTBTdDBrekhBTWl0aXlTaFhXVFYwZEtDT0pw?=
 =?utf-8?B?SDE4ZUhnOGhUcG9jR2Ntd1JoVG56eHZlNzAyUEtnSHlGMy9uSExpNnRGdFpN?=
 =?utf-8?B?ME90aStaUEZHZW1HM24vUFpCUTJUMHloUG9MeEpEU042RklmWGhrWUJHb3k5?=
 =?utf-8?B?MGxuVW0wRFVYbzlZZUZMZFZSQnpaQTVRYkZhMVMzaCtBak9rYXJtYUd6OGRG?=
 =?utf-8?B?UnVsU3lLT0VjSzVIZ2Y3RkZUVlJXR2pzQldTTElyT1hHcy9LcW1taDRSaVNu?=
 =?utf-8?B?aTg2cVFEVHdySmF2djZmR09SdTFndnVBb3FXV0x2V1FQd3poeDdJOUxFSTRB?=
 =?utf-8?B?SGgzZzZzdFFtby9vOTlBdnFsWTdvdGZpZE1zS2VXbW1IL1p0Rklsb0xjcVdk?=
 =?utf-8?B?UitjOFRTK2FBUnl0K2h0Qlp1RW5QMmJpNVVHMXRRM0hjL04vZXZuMmMxOTZE?=
 =?utf-8?B?UmdWeC9YSk5oOXZJMVZRcllhUUEvTmgwbEJpQzJIclRxVGlOSWRpTUZ6bHN2?=
 =?utf-8?B?bDhQbWNjVVkvYmlTS3lHbHRBZlVwTk5xeFVza1RGYlQ2WkZ1eGlESFQ1R0Vx?=
 =?utf-8?B?R3VkUi9CVnVmTzNYc2JWN1ROV3R0OXJ4MisyN2FrZkJOM0N6R1VJcmhnS0hO?=
 =?utf-8?B?akVLQ0Q3SGdQYkl4Z3VKWFFZUWpPeUVUbkRtY2dMMlQ0VTFWYlRRQ2JLV2ps?=
 =?utf-8?B?cWFXN05FYUhkQUc3MVZoeFRoQWx1bFNJc0c0MmdMNndEZU9oZTNzanJJMGVl?=
 =?utf-8?B?UG5UOU5qd0x2d29QUlVKWUtpb0lyYjVPdkpWMWM3YllMQTBCWDlzNGpFVjJq?=
 =?utf-8?B?d3hleWRpemtBcGZITmNmZk9jcHpBcStVZ09oaG1vM0pKWGRsSkNmNExvTE1M?=
 =?utf-8?B?ZWQvUnVmN2EzZXBadjhINHgxR1FaczVLa2FCUS9BZUJTcFMxQ01LN0IxVGty?=
 =?utf-8?B?V1V6dnFseG8ydXpZNnJKQmdVYllWV1FYUGU4Q2ZjSzVGN3p3SnVhTzNaQVZi?=
 =?utf-8?B?RU1WdGhKdUwxVHpTcTlna091R0huTnBscE9SbENNNzhUMTFvcE5iUTFFUndK?=
 =?utf-8?B?RmhSUjdHczNjcnFJV0gvVjZlbVlzZUVEKzhIUFlGV2w2TXdkclh3YWFpVUox?=
 =?utf-8?B?SDZ4WDFKYVFzcVhIV00ybEhRMVEzZXdxTmtweVZxeTZnT05mNjNVWEJOS1BB?=
 =?utf-8?B?NmI2S2NTM3d3d2htODdBa3laOXR2Zm01dDFWZW0wTUdpamtkUmRtVGQ5aGl1?=
 =?utf-8?B?NEl3c3ZDaFBIQ1NjYlcyZVJMbzFGSU10QXgwbVcwd2hWRStJQjMrK2wveFRK?=
 =?utf-8?B?bWRjak5INkt1WkJkWnJPekpDVkM1TzFhUis1eG5mZFh0WFg2TWFHV284Skds?=
 =?utf-8?B?UTJOaW5CcEhwbTFGRjQ4cS9IUlVEbnc2dUl1ZEJDYzdvYWVzYjhtZnNRYkJ4?=
 =?utf-8?B?amltd0g3OGR2bUlmM29GcFVhakFWbklDVXVKeTdYbEVleVZkTDMrWkQ5ejZ4?=
 =?utf-8?B?N3JVd05iNko2eGV4RmdOU1RjZHlDSGN2ZFpyeG5Dd2RtS2pMSWs4ZkU5NVFy?=
 =?utf-8?B?VUt0b0NUYzBwSElRUWNVMUM2QmJQWXYyajFpM1hJT1lOUGtQdXp4YmZ1TlRZ?=
 =?utf-8?Q?XX3eQX3xc3aMHzwbM7Vkm/M=3D?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d91e801-1422-4814-1df9-08d9ba19b6ea
X-MS-Exchange-CrossTenant-AuthSource: HK2PR06MB3492.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2021 07:09:45.8037
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dJ6It0hwmnZD/N8JvO2J/NZrEZGt6/xWMe9372Y9Kviswj2/w0PgkjBPZRB3IgcT2s3cKrbbfGskp2R9iy7VWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HK0PR06MB2882
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2021/12/7 22:41, Alexander Lobakin wrote:
> From: Guo Zhengkui <guozhengkui@vivo.com>
> Date: Tue,  7 Dec 2021 15:31:09 +0800
> 
> Hi, thanks for your patch.
> 
>> fix following cocci warning:
>> ./net/core/gro.c:493:5-12: ERROR: PTR_ERR applied after initialization to constant on line 441
>>
>> Signed-off-by: Guo Zhengkui <guozhengkui@vivo.com>
>> ---
>>   net/core/gro.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/core/gro.c b/net/core/gro.c
>> index 8ec8b44596da..ee08f7b23793 100644
>> --- a/net/core/gro.c
>> +++ b/net/core/gro.c
>> @@ -490,9 +490,11 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
>>   	if (&ptype->list == head)
>>   		goto normal;
>>   
>> -	if (PTR_ERR(pp) == -EINPROGRESS) {
>> -		ret = GRO_CONSUMED;
>> -		goto ok;
>> +	if (IS_ERR(pp)) {
>> +		if (PTR_ERR(pp) == -EINPROGRESS) {
>> +			ret = GRO_CONSUMED;
>> +			goto ok;
>> +		}
>>   	}
> 
> `if (PTR_ERR(ptr) == -ERRNO)` itself is correct without a check for
> IS_ERR(). The former basically is a more precise test comparing to
> the latter.

Yes, even without `IS_ERR`, it runs well.

At least, `IS_ERR` before `PTR_ERR` is a good habit. :)

Zhengkui

> Not sure if compilers can get it well, but in ideal case the first
> will be omitted from the object code at all, and so do we.
> 
> In case I'm wrong and this is a correct fix, it at least shouldn't
> increase the indentation by one, these two conditions can be placed
> into one `if` statement.
> 
> NAK.
> 
>>   
>>   	same_flow = NAPI_GRO_CB(skb)->same_flow;
>> -- 
>> 2.20.1
> 
> Al
>