netdev - Re: [PATCH net 4/4] ice: Add correct PHY lane assignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <633062bb-5c70-409d-a55b-5785294d59da@intel.com>
Date: Thu, 19 Dec 2024 16:49:12 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Paolo Abeni <pabeni@...hat.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Jiri Pirko <jiri@...nulli.us>
CC: Karol Kolacinski <karol.kolacinski@...el.com>, <richardcochran@...il.com>,
	<horms@...nel.org>, Arkadiusz Kubalewski <Arkadiusz.kubalewski@...el.com>,
	Grzegorz Nitka <grzegorz.nitka@...el.com>, Pucha Himasekhar Reddy
	<himasekharx.reddy.pucha@...el.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <edumazet@...gle.com>, <andrew+netdev@...n.ch>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net 4/4] ice: Add correct PHY lane assignment

On 12/11/24 11:36, Przemek Kitszel wrote:
> On 12/10/24 15:54, Paolo Abeni wrote:
>> On 12/6/24 20:35, Tony Nguyen wrote:
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/ 
>>> net/ethernet/intel/ice/ice_common.c
>>> index 496d86cbd13f..ab25ccd7e8ec 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>>> @@ -4095,6 +4095,51 @@ ice_aq_set_port_option(struct ice_hw *hw, u8 
>>> lport, u8 lport_valid,
>>>       return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
>>>   }
>>> +/**
>>> + * ice_get_phy_lane_number - Get PHY lane number for current adapter
>>> + * @hw: pointer to the hw struct
>>> + *
>>> + * Return: PHY lane number on success, negative error code otherwise.
>>> + */
>>> +int ice_get_phy_lane_number(struct ice_hw *hw)
>>> +{
>>> +    struct ice_aqc_get_port_options_elem *options __free(kfree);
>>
>> Please avoid the __free() construct:
>>
>> https://elixir.bootlin.com/linux/v6.13-rc2/source/Documentation/ 
>> process/maintainer-netdev.rst#L393
> 
> My understanding was that conversions to __free() (w/o any other reason)
> are bad. But for new code, it's fine. I get the "discourage" part from
> the doc, as "don't use __free() by default, for all pointers, or all
> allocations, but apply your judgement and sanity to tell if that makes
> given function better".
> I still believe that this function is better with __free(). We develop
> (new code) with such assumptions for the better part of the year.
> 
> I think that static analysis tools/Reviewers already got used to that
> (after the first false-positive memleak reported). Developers (and
> Reviewers for sure) know that those pointers could not be left
> uninitialized at function return. The only concern that is unresolved
> for me yet, is: "there is a lot of characters to type", but that is also
> good in some way, as one needs bigger function to justify the added
> "complexity".
> 
>>
>> Thanks,
>>
>> Paolo
>>
> 

Paolo, could you please revisit this? In short I propose: let the 
developer and reviewers to decide if __free() makes given code
better, instead of an umbrella "just no" statement.

I believe that Jiri would be happy too, to back it up as another
HW Vendor.
https://lore.kernel.org/lkml/ZfW9lVhnClqr9Han@nanopsycho/T/