Message-ID: <8583ff17-a8fe-c5c3-7618-3ef82364c97e@intel.com>
Date:   Fri, 24 Feb 2023 17:11:11 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH net-next v3 4/5] ip_tunnel: constify input argument of
 ip_tunnel_info_opts( )
Content-Language: en-US
To:     Gavin Li <gavinl@nvidia.com>,
        Simon Horman <simon.horman@corigine.com>
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <roopa@nvidia.com>,
        <eng.alaamohamedsoliman.am@gmail.com>, <bigeasy@linutronix.de>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <gavi@nvidia.com>, <roid@nvidia.com>, <maord@nvidia.com>,
        <saeedm@nvidia.com>
References: <20230217033925.160195-1-gavinl@nvidia.com>
 <20230217033925.160195-5-gavinl@nvidia.com> <Y/KGJvFutRN0YjFr@corigine.com>
 <Y/KKiNHnJ5vHqWrf@corigine.com>
 <021dd47a-03e6-6d27-af3d-c7ff179fe0bd@nvidia.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <021dd47a-03e6-6d27-af3d-c7ff179fe0bd@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RXRpZ2NvYkw2dFBuSWUxYUxsUEEyU2lGTjZPMmd6YjVvS2Z6S0RJRUxHbXIw?=
 =?utf-8?B?NndDSkN4YWxpMkRMa0xSSHZTazVkVG8rc3ROM0hNWGZvdzRKaktHTjllL1cw?=
 =?utf-8?B?R2ozVVZiMXhkKzRCNWorT0dKVy9KN2lNeG1tTnRjbXNaVkYwUm9ENU1MT1NO?=
 =?utf-8?B?UDVNSmVUbkJQRTk0djlCbFMxZHliNVJXbGRJVnhEcytJd3E5eFpHUWliaS9K?=
 =?utf-8?B?ZitCYmFYai9ic2taU0lCK0swQkxQTFNUL1RFTXM4S2tGS0RXZDUrRFVpNEhS?=
 =?utf-8?B?WnhNb3QvK1dORThkK0pITFVMRXg4MFhrcW83bzB5Z2hJclhLd0J0Z0dSWUEv?=
 =?utf-8?B?eWxZa2Y5MzF0SzZVQ2JldUJpUVZzeHB6bGY1MStvUzByeHhoNVNZSjd5VG52?=
 =?utf-8?B?eURuMkJQZUhyckNwVUhXbDZjVUxHVmRaVFJoaWFOam4xTFI0ZmtqTDRVZWFX?=
 =?utf-8?B?NU1lQWdYQVBod1NaeVkwUHFUNjJqaHo4eXdacjZzTHh1anBrL2Q3Y0FTYWlH?=
 =?utf-8?B?VmEycXFkMHArb1g2S212NjhBNHFHTUdOM2NoZENLM2JwUlJkZ1FTY2ZVeU5v?=
 =?utf-8?B?VDdnQktDY01mb3J1WlE5VmtIWXBsZXpsc2RmNGNVbVZjMDI5aFpHblpQT3ZC?=
 =?utf-8?B?Q08xT01jak5qM3E0aVVnMDd1Mi9nNERkc1hraVNETHczekdLekhPamtMcTZE?=
 =?utf-8?B?UmVwTS9KTWRwVldBbjZwbmZtZU82OWhnUU5Ma2hJVXNXa3cyUU9UTFhkcEJF?=
 =?utf-8?B?YlpvbzZsOWJGajJNM2RwYWxFclZ4YU1aM2R6UVFKdDZXeGM3eGsyVEc4cnlK?=
 =?utf-8?B?NEhzVndRancrVG95dzFaUCt1UlpaRVNLUCszclU3ZjBqUlk3MWE0blBvRitL?=
 =?utf-8?B?TXNEempVMVNUakkyaVpvVmZjUjBaVEhDMnZjeXNxbjd5cUpXdElYVDhBcUlo?=
 =?utf-8?B?K201ZzdjaUdnWUFpQ1dCZWx6THIxcVlMNjFjanZXZjMzLzQxZUFSSnBBRXdu?=
 =?utf-8?B?ZXQ0R01NTWUvWUF2SWpKM1hPZkJMZ2dkNTh0RUs0UUloL3ZEQ1FMckZwYUpG?=
 =?utf-8?B?bDJBbVZYZnd4RDdVL0h0aGFzL3RXWG4yRzFZMmxYSTJlUG5WNXliaEhkVGFL?=
 =?utf-8?B?RkJHbDZHUzJWaVA0M3hlbkd5Mk9RZGNvTjRicDJRYy9OaUZYL2FFOGo0U1Jw?=
 =?utf-8?B?bDR2cjRpK0ZhNkY3dnhYS1NKZERrMm4yeUE3YUV4S29XU1gwZnNab2lCRVRS?=
 =?utf-8?B?bTZOU2NpUSs0UE1wQkZzTUgzbXNRaDJjaGt4U2ZvT1dmWEFyNVVLbWo3UmZi?=
 =?utf-8?B?SjkvUytxOWhXNFM4UXRZakdBTlBKSlBMbTBVRHZiem5rQk5kN3lJZnkyd2N1?=
 =?utf-8?B?SEpnY0tBSlQxS29sNWlZVTZVb3RmcUp5OEd3WG9Oc1orRTlremtacjZ3VVRE?=
 =?utf-8?B?OFJXaWErSmdsalkvYWhkSDNqa1lid1RIdGNjQThndHZsNHpZRHNxQ1VZZVhR?=
 =?utf-8?B?NlMrMGgxUWlnRURoYjF5bE1jdVNYK0t1NGt1dXJqSnRwMk1YK1FTY1JjRk5j?=
 =?utf-8?B?QkNKU2pJd2Rub29rU1RydGJNUDZkNXpEaWt1U0NGcFp2ZThqOEM2UE9id1cr?=
 =?utf-8?B?UUdHRzZzTnJZL0cxMHkzRk04Mmt5cTFHZXNTb2psSWZDaFJ3dkQwRzJjaTNI?=
 =?utf-8?B?a1M0TEVBSEVQdzhxL1pZMzBPS2VxOE9oc081UUxmT0ZPZTZZOTJtVHNFbXR1?=
 =?utf-8?B?cm9KRktyano3WnVVVnQ2My9zbm1weWV3YkJoWnl0d05CUitRK3p2UHFDVFFH?=
 =?utf-8?B?S3V2OFNlSUpoRlZFVVAzSFc3Ym1wcnhrRUhKYkxwcjZBTU1GbUVhOWZiVUxm?=
 =?utf-8?B?bW12bjgyalVZUCsrcHd2Nkx2L2dUSFpBVS9ORkdsdFFaeDhOV2RnNFY4TGk3?=
 =?utf-8?B?VW1oWHVSc3lsbWtZQmN5d1JzbFNHT2FxU3lGRzlPTGc4SWVsODV6Mlc0SnpY?=
 =?utf-8?B?SmZzVjNUVzVqZlhrd3hQQ1I5R0ZoNDhtMVlYNitZbjBmR3YxNFo5WVlhc1pu?=
 =?utf-8?B?a3RhdXhNZkFReTBHNnJpbEx0TUxlTWl4Z1p5c1d1RVZ0NzVnd21pRERNcm1y?=
 =?utf-8?B?d1FqNWN0eGJDWDJlUFpqS3kwdC9ZRVVSNDcraWNLSlZueEt5Q1Ixd0pkQVR6?=
 =?utf-8?Q?GdRk9ww2+j+la+2LbrrWQbM=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5bcfa73a-b840-430c-94f4-08db1681e33d
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Feb 2023 16:12:12.4925
 (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: 7r/N8p95JaIM/wZtV3yA0Yi4cBseiURPYTj8kElCiPeCzUWsFlWJwLhqoG8t2SUGvHPKcp5Q7wquYtm7mS2mfTky8J7YgUMnSgtEwT21RAA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7962
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Gavin Li <gavinl@nvidia.com>
Date: Mon, 20 Feb 2023 18:42:14 +0800

> 
> On 2/20/2023 4:46 AM, Simon Horman wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each
>>>> time
>>>> in each driver.
>>>>
>>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>>> ---
>>>>   include/net/ip_tunnels.h | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>>> index fca357679816..32c77f149c6e 100644
>>>> --- a/include/net/ip_tunnels.h
>>>> +++ b/include/net/ip_tunnels.h
>>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct
>>>> net_device *dev, int pkt_len)
>>>>      }
>>>>   }
>>>>
>>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info
>>>> *info)
>>>>   {
>>>> -   return info + 1;
>>>> +   return (void *)(info + 1);
>>> I'm unclear on what problem this is trying to solve,
>>> but info being const, and then returning (info +1)
>>> as non-const feels like it is masking rather than fixing a problem.
>> I now see that an example of the problem is added by path 5/5.
>>
>> ...
>>    CC [M]  drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function
>> 'mlx5e_gen_ip_tunnel_header_vxlan':
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43:
>> error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const'
>> qualifier from pointer target type [-Werror=discarded-qualifiers]
>>    103 |                 md = ip_tunnel_info_opts(e->tun_info);
>>        |                                          ~^~~~~~~~~~
>> In file included from
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
>> ./include/net/ip_tunnels.h:488:64: note: expected 'struct
>> ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
>>    488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info
>> *info)
>>        |                                        
>> ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> ...
>>
>> But I really do wonder if this patch masks rather than fixes the problem.
> Hi Olek, any comment?

Hi,

Sorry for the late reply, was busy at work ._.

I initially proposed a solution via _Generic or __builtin_choose_expr to
return const or non-const opts basing on the input pointer type. I don't
like simple cast-aways.

See container_of_const() how it dynamically chooses whether to add
`const` or not when returning the result.

>>
>>>>   }
>>>>
>>>>   static inline void ip_tunnel_info_opts_get(void *to,
>>>> -- 
>>>> 2.31.1
>>>>

Thanks,
Olek