On Sun, May 19, 2019 at 01:07:34PM +0300, Kamal Heib wrote:
> On Fri, 2019-05-17 at 15:01 -0700, Ira Weiny wrote:
> > > >
> > > > This patch is doing that for all roce drivers that implement
> > > > modify
> > > > port, unless you mean none-roce drivers?
> > >
> > > I mean just delete it without any change to the core code.. Here we
> > > are now changing some roce drivers to have a working modify_port
> > >
> > > It is confusing what the intention is
> >
> > I see what Jason is saying here. If ib_modify_port() is meaningless
> > then lets
> > remove the call and let it return -EOPNOTSUPP.
> >
>
> Please see below the original implementation of ib_modify_port(), if
> modify_port() callback is implemented then it will be called, otherwise
> for RoCE driver the return value will be "ok" and for none RoCE driver
> it will be "-ENOSYS".
>
> So, what I tried to do in this patch is to return "ok" (like how it is
> done for hns, mlx4, mlx5, ocrdma, and qedr) in the ib_core instead in
> the drivers, and changed rxe and vmw_pvrdma to be aligned with this
> change.
I see that.
I don't really have a dog in this fight so I'm not really that concerned. But
from time to time these little changes do affect things like the
infiniband-diags and/or the perftests which I do occasionally use. So there
are 3 questions.
1) Do RoCE ports really require an "ib_modify_port()" function.
I think "Not"
2) If not, then what is going to happen if someone calls ib_modify_port()?
I think it should be an error not "ok" but this is debatable and again
since I don't really have stake one should probably not listen to me...
;-)
3) Again if not, then where best to return the error.
I think Jason and I are agreed that the core should just handle this.
Why duplicate the functionality in all the drivers? Unless you think
the answer to number #1 above is "maybe depending on the driver"...
Is that what you are suggesting?
Ira
>
> int ib_modify_port(struct ib_device *device,
> u8 port_num, int port_modify_mask,
> struct ib_port_modify *port_modify)
> {
> int rc;
>
> if (!rdma_is_port_valid(device, port_num))
> return -EINVAL;
>
> if (device->ops.modify_port)
> rc = device->ops.modify_port(device, port_num,
> port_modify_mask,
> port_modify);
> else
> rc = rdma_protocol_roce(device, port_num) ? 0 :
> -ENOSYS;
> return rc;
> }
>
> > Returning "ok" implies that something worked. I guess that is what
> > happens
> > now...
> >
> > Also FWIW you are changing the return from ENOSYS to EOPNOTSUPP. Did
> > you mean
> > to do that?
> >
>
> Yes, I changed the return value to "-EOPNOTSUPP" which is the proper
> return value for an unsupported option.
>
> > Ira
> >
> > > Jason
>
