Re: [PATCH 2/2] net/mlx5: fix probe return value polarity

[Shahaf Shuler <shahafs@mellanox.com>]

Wednesday, May 2, 2018 9:50 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH 2/2] net/mlx5: fix probe return value polarity
> 
> On Wed, May 02, 2018 at 01:54:37AM +0000, Yongseok Koh wrote:
> >
> > > On May 1, 2018, at 4:18 AM, Shahaf Shuler <shahafs@mellanox.com>
> wrote:
> > >
> > > mlx5 prefixed function returns a negative errno value.
> > > the error handler on mlx5_pci_probe is doing the same.
> > >
> > > Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno
> > > values")
> > > Cc: nelio.laranjeiro@6wind.com
> > >
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > ---
> > > drivers/net/mlx5/mlx5.c | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > > 46cb370a29..ab860b5985 100644
> > > --- a/drivers/net/mlx5/mlx5.c
> > > +++ b/drivers/net/mlx5/mlx5.c
> > > @@ -804,12 +804,16 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> > > 				goto error;
> >
> > Shouldn't you do the same for mlx5_uar_init_secondary()?
> > Looks like a few more. E.g. mlx5_args(), mlx5_get_mtu() and
> mlx5_uar_init_primary().

Yes thanks.

> > What about ibv_query_port() 

No need, according to man:

RETURN VALUE                                                                                                       
       ibv_query_port() returns 0 on success, or the value of errno on failure (which indicates  the  failure  rea-
       son).                                                                                                       

This is also the case for ibv_query_port_ex, I will align it on the next version. 

and mlx5_flow_create_drop_queue()?

Yes, needed. 

> >
> > Thanks
> >
> > > 			/* Receive command fd from primary process */
> > > 			err = mlx5_socket_connect(eth_dev);
> > > -			if (err < 0)
> > > +			if (err < 0) {
> > > +				err = -err;
> >
> > Instead of changing sign, how about 'err = rte_errno;' ?
> 
> +1

Ok.

> 
> > However, err looks redundant to me because mlx5_* funcs set rte_errno.
> 
> Not it is not, rte_errno is the strict equivalent of errno but instead of being
> global to the process, it is global per logical core, its cannot be determined
> nor modified at the beginning of the function thus the function must track
> any failure and report it accordingly.
> 
> > Here, err is used to set rte_errno at the end.
> 
> It is just a optimization to avoid a lot of rte_errno sets among the function, it
> must only be set if err != 0.

I think I will keep the local err for the meanwhile. 

> 
> > Thanks,
> > Yongseok
> >
> > > 				goto error;
> > > +			}
> > > 			/* Remap UAR for Tx queues. */
> > > 			err = mlx5_tx_uar_remap(eth_dev, err);
> > > -			if (err)
> > > +			if (err) {
> > > +				err = -err;
> > > 				goto error;
> > > +			}
> > > 			/*
> > > 			 * Ethdev pointer is still required as input since
> > > 			 * the primary device is not accessible from the
> > > --
> > > 2.12.0
> > >
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND