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..
> @@ -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?
> @@ -537,11 +541,13 @@ static int setup_device(struct ib_device *device)
> }
>
> memset(&device->attrs, 0, sizeof(device->attrs));
> - ret = device->ops.query_device(device, &device->attrs, &uhw);
> - if (ret) {
> - dev_warn(&device->dev,
> - "Couldn't query the device attributes\n");
> - goto port_cleanup;
> + if (device->ops.query_device) {
> + ret = device->ops.query_device(device, &device->attrs, &uhw);
> + if (ret) {
> + dev_warn(&device->dev,
> + "Couldn't query the device attributes\n");
> + goto port_cleanup;
> + }
Even uverbs_cmd.c needs attrs to be valid, drop this hunk. If a driver
providers a NULL query_device then it will rightly crash on
registration.
> @@ -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.
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index c073a4720d28..fb6074d2e394 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -2566,6 +2566,8 @@ struct ib_device {
> __be64 node_guid;
> u32 local_dma_lkey;
> u16 is_switch:1;
> + /* Indicates kernel verbs support, should not be used in drivers */
> + u8 kverbs_provider:1;
u16 surely?
Jason
