On 09-Jan-19 01:39, Jason Gunthorpe wrote:
> On Tue, Jan 08, 2019 at 09:55:54PM +0000, Majd Dibbiny wrote:
>>> @@ -537,11 +538,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) {
>
>> Why do we need these kind of checks now? In case device doesn’t
>> implement all mandatory kverbs, then clients won’t add it.. uverbs
>> has uverbs_cmd_mask in write path, and checks function pointer in
>> the ioctl path.. Maybe I’m missing something..
>
> Yah, I agree, the checks should be in the uverbs layer, and presumably
> they are all there already, it just needs careful checking.
I've inspected each flow that comes down to using these callbacks, the checks
are only added for flows that are not already protected by NULL checks (by the
uverbs layer or in use by kverbs only which guarantees callback existence).
Specifically, the check Majd pointed out in setup_device is called as part of
ib_register_device which is not protected by the uverbs layer.
Other checks need careful inspection:
ib_query_pkey is called from sysfs flow for example, which assumes the callback
exists.
ib_query_port has many call sites in drivers which assume the callback exist.
Create CQ/QP flows are checked for create_qp/cq existence by the uverbs layer,
but in case of failure call destroy_qp/cq which are no longer guaranteed to exist.
And the list goes on..
Every additional if I added is there since I believe that it's not already
covered by the uverbs layer/kverbs usage only. If you find an unnecessary one
please let me know so I'll remove it.
