diff mbox series

[net-next,v2,03/11] pds_core: devlink health: use retained error fmsg API

Message ID 20231017105341.415466-4-przemyslaw.kitszel@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series devlink: retain error in struct devlink_fmsg | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
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 No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Przemek Kitszel Oct. 17, 2023, 10:53 a.m. UTC 
Drop unneeded error checking.

devlink_fmsg_*() family of functions is now retaining errors,
so there is no need to check for them after each call.

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
---
 drivers/net/ethernet/amd/pds_core/devlink.c | 29 ++++++---------------
 1 file changed, 8 insertions(+), 21 deletions(-)

Comments

Jiri Pirko Oct. 17, 2023, 11:06 a.m. UTC | #1 
Tue, Oct 17, 2023 at 12:53:33PM CEST, przemyslaw.kitszel@intel.com wrote:
>Drop unneeded error checking.
>
>devlink_fmsg_*() family of functions is now retaining errors,
>so there is no need to check for them after each call.
>
>Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>---
>add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
>---
> drivers/net/ethernet/amd/pds_core/devlink.c | 29 ++++++---------------
> 1 file changed, 8 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
>index d9607033bbf2..8b2b9e0d59f3 100644
>--- a/drivers/net/ethernet/amd/pds_core/devlink.c
>+++ b/drivers/net/ethernet/amd/pds_core/devlink.c
>@@ -154,33 +154,20 @@ int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
> 			      struct netlink_ext_ack *extack)
> {
> 	struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
>-	int err;
> 
> 	mutex_lock(&pdsc->config_lock);
>-
> 	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>+		devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
> 	else if (!pdsc_is_fw_good(pdsc))
>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>+		devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
> 	else
>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>-
>+		devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
> 	mutex_unlock(&pdsc->config_lock);
> 
>-	if (err)
>-		return err;
>-
>-	err = devlink_fmsg_u32_pair_put(fmsg, "State",
>-					pdsc->fw_status &
>-						~PDS_CORE_FW_STS_F_GENERATION);
>-	if (err)
>-		return err;
>-
>-	err = devlink_fmsg_u32_pair_put(fmsg, "Generation",
>-					pdsc->fw_generation >> 4);
>-	if (err)
>-		return err;
>+	devlink_fmsg_u32_pair_put(fmsg, "State",
>+				  pdsc->fw_status & ~PDS_CORE_FW_STS_F_GENERATION);
>+	devlink_fmsg_u32_pair_put(fmsg, "Generation", pdsc->fw_generation >> 4);
>+	devlink_fmsg_u32_pair_put(fmsg, "Recoveries", pdsc->fw_recoveries);
> 
>-	return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
>-					 pdsc->fw_recoveries);
>+	return 0;

Could you please covert the function to return void? Please make sure
to do this in the rest of the patchset as well.

Thanks!

pw-bot: cr


> }
>-- 
>2.40.1
>
Jiri Pirko Oct. 17, 2023, 11:15 a.m. UTC | #2 
Tue, Oct 17, 2023 at 01:06:54PM CEST, jiri@resnulli.us wrote:
>Tue, Oct 17, 2023 at 12:53:33PM CEST, przemyslaw.kitszel@intel.com wrote:
>>Drop unneeded error checking.
>>
>>devlink_fmsg_*() family of functions is now retaining errors,
>>so there is no need to check for them after each call.
>>
>>Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>---
>>add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
>>---
>> drivers/net/ethernet/amd/pds_core/devlink.c | 29 ++++++---------------
>> 1 file changed, 8 insertions(+), 21 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
>>index d9607033bbf2..8b2b9e0d59f3 100644
>>--- a/drivers/net/ethernet/amd/pds_core/devlink.c
>>+++ b/drivers/net/ethernet/amd/pds_core/devlink.c
>>@@ -154,33 +154,20 @@ int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
>> 			      struct netlink_ext_ack *extack)
>> {
>> 	struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
>>-	int err;
>> 
>> 	mutex_lock(&pdsc->config_lock);
>>-
>> 	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
>>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>>+		devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>> 	else if (!pdsc_is_fw_good(pdsc))
>>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>>+		devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>> 	else
>>-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>>-
>>+		devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>> 	mutex_unlock(&pdsc->config_lock);
>> 
>>-	if (err)
>>-		return err;
>>-
>>-	err = devlink_fmsg_u32_pair_put(fmsg, "State",
>>-					pdsc->fw_status &
>>-						~PDS_CORE_FW_STS_F_GENERATION);
>>-	if (err)
>>-		return err;
>>-
>>-	err = devlink_fmsg_u32_pair_put(fmsg, "Generation",
>>-					pdsc->fw_generation >> 4);
>>-	if (err)
>>-		return err;
>>+	devlink_fmsg_u32_pair_put(fmsg, "State",
>>+				  pdsc->fw_status & ~PDS_CORE_FW_STS_F_GENERATION);
>>+	devlink_fmsg_u32_pair_put(fmsg, "Generation", pdsc->fw_generation >> 4);
>>+	devlink_fmsg_u32_pair_put(fmsg, "Recoveries", pdsc->fw_recoveries);
>> 
>>-	return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
>>-					 pdsc->fw_recoveries);
>>+	return 0;
>
>Could you please covert the function to return void? Please make sure
>to do this in the rest of the patchset as well.
>
>Thanks!

Sorry, I messed up, this is a cb. Looks fine.

pw-bot: under-review

>
>pw-bot: cr
>
>
>> }
>>-- 
>>2.40.1
>>
Przemek Kitszel Oct. 17, 2023, 11:35 a.m. UTC | #3 
On 10/17/23 13:15, Jiri Pirko wrote:
> Tue, Oct 17, 2023 at 01:06:54PM CEST, jiri@resnulli.us wrote:
>> Tue, Oct 17, 2023 at 12:53:33PM CEST, przemyslaw.kitszel@intel.com wrote:
>>> Drop unneeded error checking.
>>>
>>> devlink_fmsg_*() family of functions is now retaining errors,
>>> so there is no need to check for them after each call.
>>>
>>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> ---
>>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
>>> ---
>>> drivers/net/ethernet/amd/pds_core/devlink.c | 29 ++++++---------------
>>> 1 file changed, 8 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
>>> index d9607033bbf2..8b2b9e0d59f3 100644
>>> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
>>> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
>>> @@ -154,33 +154,20 @@ int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> 			      struct netlink_ext_ack *extack)
>>> {
>>> 	struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
>>> -	int err;
>>>
>>> 	mutex_lock(&pdsc->config_lock);
>>> -
>>> 	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
>>> -		err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>>> +		devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>>> 	else if (!pdsc_is_fw_good(pdsc))
>>> -		err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>>> +		devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>>> 	else
>>> -		err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>>> -
>>> +		devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>>> 	mutex_unlock(&pdsc->config_lock);
>>>
>>> -	if (err)
>>> -		return err;
>>> -
>>> -	err = devlink_fmsg_u32_pair_put(fmsg, "State",
>>> -					pdsc->fw_status &
>>> -						~PDS_CORE_FW_STS_F_GENERATION);
>>> -	if (err)
>>> -		return err;
>>> -
>>> -	err = devlink_fmsg_u32_pair_put(fmsg, "Generation",
>>> -					pdsc->fw_generation >> 4);
>>> -	if (err)
>>> -		return err;
>>> +	devlink_fmsg_u32_pair_put(fmsg, "State",
>>> +				  pdsc->fw_status & ~PDS_CORE_FW_STS_F_GENERATION);
>>> +	devlink_fmsg_u32_pair_put(fmsg, "Generation", pdsc->fw_generation >> 4);
>>> +	devlink_fmsg_u32_pair_put(fmsg, "Recoveries", pdsc->fw_recoveries);
>>>
>>> -	return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
>>> -					 pdsc->fw_recoveries);
>>> +	return 0;
>>
>> Could you please covert the function to return void? Please make sure
>> to do this in the rest of the patchset as well.
>>
>> Thanks!
> 
> Sorry, I messed up, this is a cb. Looks fine.

Thanks :)

I was also tempted to convert, but there are other possibilities of
error to report from callbacks :)
There are still some places in mlx5 that seems as possible candidates,
but this series is big enough to draw the line here.

> 
> pw-bot: under-review
> 
>>
>> pw-bot: cr
>>
>>
>>> }
>>> -- 
>>> 2.40.1
>>>
Nelson, Shannon Oct. 17, 2023, 4:52 p.m. UTC | #4 
On 10/17/2023 3:53 AM, Przemek Kitszel wrote:
> 
> Drop unneeded error checking.
> 
> devlink_fmsg_*() family of functions is now retaining errors,
> so there is no need to check for them after each call.
> 
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Thanks,

Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>

> ---
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-57 (-57)
> ---
>   drivers/net/ethernet/amd/pds_core/devlink.c | 29 ++++++---------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
> index d9607033bbf2..8b2b9e0d59f3 100644
> --- a/drivers/net/ethernet/amd/pds_core/devlink.c
> +++ b/drivers/net/ethernet/amd/pds_core/devlink.c
> @@ -154,33 +154,20 @@ int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
>                                struct netlink_ext_ack *extack)
>   {
>          struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
> -       int err;
> 
>          mutex_lock(&pdsc->config_lock);
> -
>          if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
> -               err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
> +               devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>          else if (!pdsc_is_fw_good(pdsc))
> -               err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
> +               devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>          else
> -               err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
> -
> +               devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>          mutex_unlock(&pdsc->config_lock);
> 
> -       if (err)
> -               return err;
> -
> -       err = devlink_fmsg_u32_pair_put(fmsg, "State",
> -                                       pdsc->fw_status &
> -                                               ~PDS_CORE_FW_STS_F_GENERATION);
> -       if (err)
> -               return err;
> -
> -       err = devlink_fmsg_u32_pair_put(fmsg, "Generation",
> -                                       pdsc->fw_generation >> 4);
> -       if (err)
> -               return err;
> +       devlink_fmsg_u32_pair_put(fmsg, "State",
> +                                 pdsc->fw_status & ~PDS_CORE_FW_STS_F_GENERATION);
> +       devlink_fmsg_u32_pair_put(fmsg, "Generation", pdsc->fw_generation >> 4);
> +       devlink_fmsg_u32_pair_put(fmsg, "Recoveries", pdsc->fw_recoveries);
> 
> -       return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
> -                                        pdsc->fw_recoveries);
> +       return 0;
>   }
> --
> 2.40.1
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
index d9607033bbf2..8b2b9e0d59f3 100644
--- a/drivers/net/ethernet/amd/pds_core/devlink.c
+++ b/drivers/net/ethernet/amd/pds_core/devlink.c
@@ -154,33 +154,20 @@  int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
 			      struct netlink_ext_ack *extack)
 {
 	struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
-	int err;
 
 	mutex_lock(&pdsc->config_lock);
-
 	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
+		devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
 	else if (!pdsc_is_fw_good(pdsc))
-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
+		devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
 	else
-		err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
-
+		devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
 	mutex_unlock(&pdsc->config_lock);
 
-	if (err)
-		return err;
-
-	err = devlink_fmsg_u32_pair_put(fmsg, "State",
-					pdsc->fw_status &
-						~PDS_CORE_FW_STS_F_GENERATION);
-	if (err)
-		return err;
-
-	err = devlink_fmsg_u32_pair_put(fmsg, "Generation",
-					pdsc->fw_generation >> 4);
-	if (err)
-		return err;
+	devlink_fmsg_u32_pair_put(fmsg, "State",
+				  pdsc->fw_status & ~PDS_CORE_FW_STS_F_GENERATION);
+	devlink_fmsg_u32_pair_put(fmsg, "Generation", pdsc->fw_generation >> 4);
+	devlink_fmsg_u32_pair_put(fmsg, "Recoveries", pdsc->fw_recoveries);
 
-	return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
-					 pdsc->fw_recoveries);
+	return 0;
 }