2015-07-19 06:51:22

by Yuval Mintz

[permalink] [raw]
Subject: RE: [PATCH 1/2] bnx2x:Fix error handling in the function bnxc2x_dbcx_set_params

> This fixes the error handling in the function bnxc2x_dbcx_params for both calls
> to bnx2x_dcbnl_update_applist by checking if they failed by returning a error
> code and if so return immediately to this function's caller due to this
> nonrecoverable internal failure.

Hi Nicholas,

Even assuming this is the reasonable thing to do [which I'm not fully convinced],
I don't think this solution is complete - you'd also need to break the iteration in
bnx2x_dcbnl_update_applist() [at the moment said function always returns 'rc'
from the last dcb_app weve tried setting].

Also, why do you send multiple patches for fixing the same issue in different
functions in the same file?

Thanks,
Yuval

>
> Signed-off-by: Nicholas Krause <[email protected]>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> index 6e4294e..e3cd0c6 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
> @@ -724,7 +724,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
> state)
> * Delete app tlvs from dcbnl before reading new
> * negotiation results
> */
> - bnx2x_dcbnl_update_applist(bp, true);
> + if (bnx2x_dcbnl_update_applist(bp, true))
> + return;
>
> /* Read remote mib if dcbx is in the FW */
> if (bnx2x_dcbx_read_shmem_remote_mib(bp))
> @@ -748,7 +749,8 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32
> state)
> /*
> * Add new app tlvs to dcbnl
> */
> - bnx2x_dcbnl_update_applist(bp, false);
> + if (bnx2x_dcbnl_update_applist(bp, false))
> + return;
> #endif
> /*
> * reconfigure the netdevice with the results of the new
> --
> 2.1.4