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.
>
>> @@ -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?
>
>> @@ -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.
ACK.
>
>> @@ -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.
>
>> 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?
Right.
