On Wed, Sep 16, 2020 at 05:34:25PM +0300, Leon Romanovsky wrote:
> On Wed, Sep 16, 2020 at 11:12:02AM -0300, Jason Gunthorpe wrote:
> > On Wed, Sep 16, 2020 at 03:44:29PM +0300, Leon Romanovsky wrote:
> > > On Wed, Sep 16, 2020 at 09:04:40AM -0300, Jason Gunthorpe wrote:
> > > > On Wed, Sep 16, 2020 at 01:37:10PM +0300, Leon Romanovsky wrote:
> > > > > It depends on how you want to treat errors from rdma_read_gid_attr_ndev_rcu().
> > > > > Current check allows us to ensure that any error returned by this call is
> > > > > handled.
> > > > >
> > > > > Otherwise we will find ourselves with something like this:
> > > > > ndev = rdma_read_gid_attr_ndev_rcu(gid_attr);
> > > > > if (IS_ERR(ndev)) {
> > > > > if (rdma_protocol_roce())
> > > > > goto error;
> > > > > if (ERR_PTR(ndev) != -ENODEV)
> > > > > goto error;
> > > > > }
> > > >
> > > > Isn't it just
> > > >
> > > > if (IS_ERR(ndev)) {
> > > > if (ERR_PTR(ndev) != -ENODEV)
> > > > goto error;
> > > > index = -1;
> > > > }
> > > >
> > > > Which seems fine and clear enough
> > >
> > > It is a problem if roce device returned -ENODEV.
> >
> > Can it happen? RCU I suppose, but I think this is an issue in
> > rdma_read_gid_attr_ndev_rcu() - it should not return ENODEV if the RCU
> > shows the gid_attr is being concurrently destroyed
>
> From RoCE point of view, it is a problem if device is destroyed or gid
> not valid, the different returned values won't change much. For the IB,
> we don't care.
I'm saying I prefer things layered properly. Having random
rdma_protocol's all over the place has proven to be hard to maintain
One inside rdma_read_gid_attr_ndev_rcu() to make a NULL netdev an
error return for roce is much more understandable
Jason
