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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 10 Oct 2023 09:53:41 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Michael Chan <michael.chan@...adcom.com>,
 Edwin Peer <edwin.peer@...adcom.com>, Cai Huoqing <cai.huoqing@...ux.dev>,
 Luo bin <luobin9@...wei.com>, George Cherian <george.cherian@...vell.com>,
 Danielle Ratson <danieller@...dia.com>, Moshe Shemesh <moshe@...dia.com>,
 Saeed Mahameed <saeedm@...dia.com>
Cc: Brett Creeley <brett.creeley@....com>,
 Vasundhara Volam <vasundhara-v.volam@...adcom.com>,
 Sunil Goutham <sgoutham@...vell.com>, Linu Cherian <lcherian@...vell.com>,
 Geetha sowjanya <gakula@...vell.com>, Jerin Jacob <jerinj@...vell.com>,
 hariprasad <hkelam@...vell.com>, Subbaraya Sundeep <sbhatta@...vell.com>,
 Ido Schimmel <idosch@...dia.com>, Petr Machata <petrm@...dia.com>,
 Eran Ben Elisha <eranbe@...dia.com>, Aya Levin <ayal@...lanox.com>,
 Leon Romanovsky <leon@...nel.org>,
 Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH net-next v1 3/8] pds_core: devlink health: use retained
 error fmsg API

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@...el.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.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
>