Re: [PATCH net-next v1 3/8] pds_core: devlink health: use retained error fmsg API

On 10/10/23 18:53, Nelson, Shannon wrote:

On 10/10/2023 3:43 AM, Przemek Kitszel wrote:

quoted

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 <redacted>
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 | 27 ++++++---------------
  1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c 

b/drivers/net/ethernet/amd/pds_core/devlink.c
index d9607033bbf2..fcb407bdb25e 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);
         return devlink_fmsg_u32_pair_put(fmsg, "Recoveries",
                                          pdsc->fw_recoveries);
+

Please don't add an extra blank line here.

sure, thanks!

quoted

  }

Generally, I like this cleanup.  I did have a similar question about 
return values as Jiri's comment - how would this do with some code 
checkers that might complain about ignoring all those return values?

yeah, going full void makes things easier at the end

Going to full void functions would fix that.  You can add some temporary 
"scaffolding" code, an intermediate layer to help in the driver 
conversion, which would then get removed at the end once all the drivers 
are converted.

This do_something/check_error/do_something/check_err pattern happens in 
other APIs in the kernel - see the devlink_info_* APIs, for example: do 
you foresee changing other interfaces in a similar way?

in principle, it's a good idea; personally, I will be fixing those that
I somehow encounter (like here, I plan to add some devlink health report
for intel ice driver)

sln

quoted

-- 
2.40.1