On Tue, Jan 15, 2019 at 01:16:00PM +0200, Gal Pressman wrote:
> On 14-Jan-19 22:25, Jason Gunthorpe wrote:
> > On Thu, Jan 10, 2019 at 04:23:35PM +0200, Gal Pressman wrote:
> >> Drivers that do not provide kernel verbs support should not be used by
> >> ib kernel clients and fail.
> >> In case a device does not implement all mandatory verbs for kverbs usage
> >> mark it as a non kverbs provider and prevent its usage for all clients
> >> except for uverbs.
> >>
> >> The device is marked as a non kverbs provider using the
> >> 'kverbs_provider' flag which should only be set by the core code.
> >> The clients can choose whether kverbs are requested for its usage using
> >> the 'no_kverbs_req' flag which is currently set for uverbs only.
> >>
> >> This patch allows drivers to remove mandatory verbs stubs and simply set
> >> the callback to NULL. The IB device will be registered as a non-kverbs
> >> provider.
> >>
> >> Signed-off-by: Gal Pressman <galpress@xxxxxxxxxx>
> >> drivers/infiniband/core/device.c | 35 ++++++++++++++++++++++-------------
> >> drivers/infiniband/core/uverbs_main.c | 1 +
> >> include/rdma/ib_verbs.h | 5 +++++
> >> 3 files changed, 28 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> >> index 8872453e26c0..3f32d05df55d 100644
> >> +++ b/drivers/infiniband/core/device.c
> >> @@ -121,13 +121,12 @@ static int ib_device_check_mandatory(struct ib_device *device)
> >> };
> >> int i;
> >>
> >> + device->kverbs_provider = true;
> >> for (i = 0; i < ARRAY_SIZE(mandatory_table); ++i) {
> >> if (!*(void **) ((void *) &device->ops +
> >> mandatory_table[i].offset)) {
> >> - dev_warn(&device->dev,
> >> - "Device is missing mandatory function %s\n",
> >> - mandatory_table[i].name);
> >> - return -EINVAL;
> >> + device->kverbs_provider = false;
> >> + break;
> >> }
> >> }
> >>
> >> @@ -330,6 +329,9 @@ static int add_client_context(struct ib_device *device, struct ib_client *client
> >> {
> >> struct ib_client_data *context;
> >>
> >> + if (!device->kverbs_provider && !client->no_kverbs_req)
> >> + return -EOPNOTSUPP;
> >
> > Return 0, it is not a failure, the client is just not supported on
> > this device. Not that it matters right now..
>
> Return 0 means that client->add() will be called, which is what we're trying to
> avoid.
Okay, I have a series that changes this..
> >
> >> @@ -374,10 +376,12 @@ static int read_port_immutable(struct ib_device *device)
> >> return -ENOMEM;
> >>
> >> for (port = start_port; port <= end_port; ++port) {
> >> - ret = device->ops.get_port_immutable(
> >> - device, port, &device->port_immutable[port]);
> >> - if (ret)
> >> - return ret;
> >> + if (device->ops.get_port_immutable) {
> >> + ret = device->ops.get_port_immutable(
> >> + device, port, &device->port_immutable[port]);
> >> + if (ret)
> >> + return ret;
> >> + }
> >>
> >> if (verify_immutable(device, port))
> >> return -EINVAL;
> >
> > Can we still call verify_immutable?
>
> static int verify_immutable(const struct ib_device *dev, u8 port)
> {
> return WARN_ON(!rdma_cap_ib_mad(dev, port) &&
> rdma_max_mad_size(dev, port) != 0);
> }
>
> Since port immutable struct is cleared, the check will pass.
> Do you prefer verify_immutable under the if statement?
Yes, no clarity in verifying unfilled data
> >> @@ -920,6 +926,9 @@ int ib_query_port(struct ib_device *device,
> >> union ib_gid gid;
> >> int err;
> >>
> >> + if (!device->ops.query_port)
> >> + return -EOPNOTSUPP;
> >
> > Again, if query_port is not supported then the related sysfs file
> > should not even be created.
>
> Needed for the cache setup as part of ib_register_device.
Hum, what does the gid cache even do for !kverbs? uverbs uses it I
suppose, but it won't work right at all if kverbs are not present..
That probably needs a similar fixing to sysfs, to safely disable the
functionality...
Jason
