Received-SPF: pass (google.com: domain of linux-kernel+bounces-127884-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <912d1f9f-a88d-4751-8d91-b82a75f82a32@intel.com>
Date: Tue, 2 Apr 2024 13:53:54 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow
 warnings throughout the file
To: Jakub Kicinski <kuba@kernel.org>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Dmitry Safonov
	<0x7f454c46@gmail.com>, Heiner Kallweit <hkallweit1@gmail.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240329170000.3241460-1-aleksander.lobakin@intel.com>
 <20240329170000.3241460-3-aleksander.lobakin@intel.com>
 <20240329131857.730c6528@kernel.org> <20240329135344.1a310f31@kernel.org>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <20240329135344.1a310f31@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MUlzVmpkNTc1aWRlaTFldU5JV3hPZXFEbmcwZjEzRDQvdGo4a1J0WEw4ZHFQ?=
 =?utf-8?B?cGhtVlozcGhzNndGdjVUczRUOWhlRlMvOXVNN3dTZ3pDWDFhMzdNeWtHOXA2?=
 =?utf-8?B?V0xWNEltK2hFK29mNW54dXVWajgxbmxYTEtSSnJvSDhSblN4Z0pMNGk5S3Za?=
 =?utf-8?B?djNxK1YrSjc5WUFia3BYaENMNDZhYkJEWmhCeVhxM3Y4RERzQ3FQUGZ6d05t?=
 =?utf-8?B?eVhwZW9xOW9iZTAzeWo5Q25Jams1T3FrVU1xN3c0YWZzWjdwSTM2YzNwRVVx?=
 =?utf-8?B?VHVZalIyQ0Nwdi9COTVnbW5ham9mQjdWNGNrZ0FhYnBmcDdFQ3MwOTVIbXRY?=
 =?utf-8?B?bmd0TThvTStZQVBobGkwaFpOQU5hajQzNU9JYlRzWmVrWkdkZytSVEpxQUpG?=
 =?utf-8?B?NDNQSmFEMEV2Tk11dlE1Q3BCR1Mxbnk4MHM5TWhjNU5nU0tRV0pmVTdIc09S?=
 =?utf-8?B?OEtOUktKajdMdm0zZ1djOUFuNFFKRDQ4Ym5xMUFrMVdMcDdMRkxIeUdDR0lX?=
 =?utf-8?B?Q3RSVUlzV0FMQnZzTisyaS9ZY2NXV2dpa1IwNG5qcW1sVXYxNHRrMElPZ2NW?=
 =?utf-8?B?MjRkbVhPNCtoQzBINGFWbFpGcmNNZkNNcE9qdkt0RHQ4RDYzZUdPVDFLR3B5?=
 =?utf-8?B?QWFJUnBhaEkxMzFkY1d6Z2ZNY2UyNFlJR0NWZUxwTVhRY1hIcUNydmRSV0Mv?=
 =?utf-8?B?UEtqVHUzTDFkTVEyUjgvRlEyTzVwQ1Z6L1lzNHdoWVVhVmdGalNEYUZrUW5D?=
 =?utf-8?B?eDZIMTRWNVpNL1hKeXRaWjFhWTMyek52cWpIdkJsZnVicWpHMThDajA2NkYv?=
 =?utf-8?B?enBIaVdZQVRKeE8zQlBIT0t2WjZNNURkbERGS1ZHVXFpbzJyajY4VTl3YWNt?=
 =?utf-8?B?SWdEK25CRjg0K2svUUpHSUhDc00zYTNROXhNbU5NYlNmcXJ3S2hhWGdNRHNk?=
 =?utf-8?B?ME1GWGcxMUJFVFR5RFZUckl5elJab1J6U2RKUnRaZXFLeHl4a3JxR1htRTJR?=
 =?utf-8?B?V0dhS0JtRXExUCtIMy9qOWZtZGtPdk5GeWt0Z2lCZW9VRzJhSGo5L1hScEZ6?=
 =?utf-8?B?RCs1TGZrc1BhV0I3cGtJZkhiYmhWZUdlbkZNcEZ5SFhuMWJ4ZlZ4Z1VCYjdn?=
 =?utf-8?B?SU1vM2l6ekVoV0VpYVRxYXNwdFdkR3lmMGhoalIxM25ZZXRZQjNTbno5UXMx?=
 =?utf-8?B?Y2NqTkEyZ2h1V3AzMjZTUU93cmdydFAzWGpvb0F5TjFRY3RzdkZiSFhiVmdD?=
 =?utf-8?B?SWFFMHRVYXpIUDdqdW1CRElJeklQOEFSc3dSNXdtdXFac3N2RDlOQVg0Mkw3?=
 =?utf-8?B?ajJwNU9RSUR6L1ZyNURYWkY2NjJtYWFnNDA2ZVNQUXphajFaaDNWeEI0dkZj?=
 =?utf-8?B?dDQrWnRkS24xaHBaOURuWU85VVllM0hSem81TTFxeEpZaXljby9oVlBUbFNk?=
 =?utf-8?B?UDRFVythVEtHMnYwUXZkZGVRVnZrYlk1dmVFL1U4eUE1WXhnRTRoRUdHT2Ew?=
 =?utf-8?B?ZGo1ZjJLckFUZ3dENHVNeWdkM2VQVjdUZ2ZFdlBNNlFJcGtxdlprRjlQRng4?=
 =?utf-8?B?UWpsbVZmSjNjZTQvNFlPS3hUbjlUeFpkVXlJckx0TjJkQUtpNlB4RVRkUE9Z?=
 =?utf-8?B?ZU9SNktGL0NSM1ZRQk9ja3p5cXFjSWY1a2gzb3l5NlRHcnRoczc0UFQ0VUZ0?=
 =?utf-8?B?dFM5czh0QUZmeENacXJHTUMxSkcyUzdBcFVzTEdYMlFUZ0x0TnJGTGFKV0NG?=
 =?utf-8?B?elQ1Uit5NjdyMHI4cFNCTTljMkNTQU9Yb1AvZmgxQ09NTWh1cGpaWk40dmZm?=
 =?utf-8?B?RFI0aGRKRjlIbHcvdGRXcHhvZ2FjT2NCZGhqVUt6U3ZCUTc2aHE0U3JncEFE?=
 =?utf-8?B?YXpjRk9RYmVhYlVYQWxmVEhrQ0svUk4zTEY4YU9sc2RlL2l6M0h6ZEwzVWM4?=
 =?utf-8?B?OTQwc2tRVzB3ekZKbUc1QTUvVW80S1hucm85M0krRjRPcXBHVk5CM1NyOTBC?=
 =?utf-8?B?YzBEaUxvZmN0THEzQjVnN29ML3hLTStmZXdWV2U5L296T2NJOFB3RWZTQWVZ?=
 =?utf-8?B?ZUJDV2xGY1hqQVhid3FQckJidGU1L0Z2a2pwVGtCWm4rcFhMUkZiSFlqcTJW?=
 =?utf-8?B?Sy9yOWlMb3FjdXExUDhMemc2S1RKVmNrRmVMdy9qZXBSVGtYZ0oyQXkrNEFQ?=
 =?utf-8?B?d1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c89d9562-1040-4f86-62f1-08dc530bb80d
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 11:54:57.9792
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UdEoA/06d8Gy1brZg/ILuwbrnhBzXy++Xb4g1gHukJqKt9M1TBNBRWcPwyuK1TbkLBluiLZiatZ4Hu9e6fcGCkNmWAR5lrEu0rthIeuL+Lw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7738
X-OriginatorOrg: intel.com

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 29 Mar 2024 13:53:44 -0700

> On Fri, 29 Mar 2024 13:18:57 -0700 Jakub Kicinski wrote:
>>> Sparse:
>>>
>>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: warning: symbol '_res' shadows an earlier one
>>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: originally declared here  
>>
>> I don't see these building with LLVM=1 W=12 C=1
>> and I really don't like complicating the code because the compiler
>> is stupid. Can't you solve this with some renames? Add another

It's not the compiler, its warnings are valid actually. Shadowing makes
it very easy to confuse variables and make bugs...

>> underscore or something?
> 
> I'm stupid I tried on the test branch which already had your fix..

:D Sometimes it happens.

> 
> This is enough:
> 
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 1ec408585373..2270fbb99cf7 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -89,7 +89,7 @@ struct netdev_stat_ops {
>  
>  #define netif_txq_try_stop(txq, get_desc, start_thrs)			\
>  	({								\
> -		int _res;						\
> +		int __res;						\
>  									\
>  		netif_tx_stop_queue(txq);				\
>  		/* Producer index and stop bit must be visible		\
> @@ -101,12 +101,12 @@ struct netdev_stat_ops {
>  		/* We need to check again in a case another		\
>  		 * CPU has just made room available.			\
>  		 */							\
> -		_res = 0;						\
> +		__res = 0;						\
>  		if (unlikely(get_desc >= start_thrs)) {			\
>  			netif_tx_start_queue(txq);			\
> -			_res = -1;					\
> +			__res = -1;					\
>  		}							\
> -		_res;							\
> +		__res;							\
>  	})								\
>  
>  /**

But what if there's a function which calls one of these functions and
already has _res or __res or something? I know renaming is enough for
the warnings I mentioned, but without __UNIQUE_ID() anything can happen
anytime, so I wanted to fix that once and for all :z

I already saw some macros which have a layer of indirection for
__UNIQUE_ID(), but previously they didn't and then there were fixes
which added underscores, renamed variables etc etc...

Thanks,
Olek