| Message ID | 20231010104318.3571791-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 |
| 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: 1386 this patch: 1386 |
| 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: 1386 this patch: 1386 |
| 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 |
On 10/10/2023 3:43 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> > --- > 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. > } 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? 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? sln > -- > 2.40.1 >
On 10/10/23 18:53, Nelson, Shannon wrote: > On 10/10/2023 3:43 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> >> --- >> 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! > >> } > > 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 > >> -- >> 2.40.1 >> >
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); + }