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 <shah...@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.laranje...@6wind.com
> > >
> > > Signed-off-by: Shahaf Shuler <shah...@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
