Re: [patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Wed, 06 Mar, 2024 07:18:35 -0800 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote:
> On Wed, 06 Mar, 2024 16:12:40 +0100 Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Currently, if there are multiple registrations of the same pin on the
>> same dpll device, following warnings are observed:
>> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:143 dpll_xa_ref_pin_del.isra.0+0x21e/0x230
>> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:223 __dpll_pin_unregister+0x2b3/0x2c0
>>
>> The problem is, that in both dpll_xa_ref_dpll_del() and
>> dpll_xa_ref_pin_del() registration is only removed from list in case the
>> reference count drops to zero. That is wrong, the registration has to
>> be removed always.
>
> What about the case where you have two functions/netdevs that refer to
> the same DPLL device/pin and you only remove a single function? You have
> another function/netdev left that now refers to the unregistered DPLL
> device/pin.
>
Actually, I see that being registered or not does not impact the use of
existing DPLL device/pin references in other functions. I agree with
this change.
>>
>> To fix this, remove the registration from the list and free
>> it unconditionally, instead of doing it only when the ref reference
>> counter reaches zero.
>>
>> Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  drivers/dpll/dpll_core.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>> index 7f686d179fc9..c751a87c7a8e 100644
>> --- a/drivers/dpll/dpll_core.c
>> +++ b/drivers/dpll/dpll_core.c
>> @@ -129,9 +129,9 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
>>  		reg = dpll_pin_registration_find(ref, ops, priv);
>>  		if (WARN_ON(!reg))
>>  			return -EINVAL;
>> +		list_del(&reg->list);
>> +		kfree(reg);
>>  		if (refcount_dec_and_test(&ref->refcount)) {
>> -			list_del(&reg->list);
>> -			kfree(reg);
>>  			xa_erase(xa_pins, i);
>>  			WARN_ON(!list_empty(&ref->registration_list));
>>  			kfree(ref);
>> @@ -209,9 +209,9 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
>>  		reg = dpll_pin_registration_find(ref, ops, priv);
>>  		if (WARN_ON(!reg))
>>  			return;
>> +		list_del(&reg->list);
>> +		kfree(reg);
>>  		if (refcount_dec_and_test(&ref->refcount)) {
>> -			list_del(&reg->list);
>> -			kfree(reg);
>>  			xa_erase(xa_dplls, i);
>>  			WARN_ON(!list_empty(&ref->registration_list));
>>  			kfree(ref);

Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>