diff mbox series

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

Message ID 20240306151240.1464884-1-jiri@resnulli.us (mailing list archive)
State Accepted
Commit b446631f355ece73b13c311dd712c47381a23172
Delegated to: Netdev Maintainers
Headers show
Series [net] dpll: fix dpll_xa_ref_*_del() for multiple registrations | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 958 this patch: 958
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: michal.michalik@intel.com; 1 maintainers not CCed: michal.michalik@intel.com
netdev/build_clang success Errors and warnings before: 973 this patch: 973
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 974 this patch: 974
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-03-06--21-00 (tests: 892)

Commit Message

Jiri Pirko March 6, 2024, 3:12 p.m. UTC 
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.

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(-)

Comments

Rahul Rameshbabu March 6, 2024, 3:18 p.m. UTC | #1 
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.

>
> 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);
Rahul Rameshbabu March 6, 2024, 3:38 p.m. UTC | #2 
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>
patchwork-bot+netdevbpf@kernel.org March 8, 2024, 11:20 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed,  6 Mar 2024 16:12:40 +0100 you 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
> 
> [...]

Here is the summary with links:
  - [net] dpll: fix dpll_xa_ref_*_del() for multiple registrations
    https://git.kernel.org/netdev/net/c/b446631f355e

You are awesome, thank you!
diff mbox series

Patch

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);