On 21-Jan-19 04:15, Jason Gunthorpe wrote: > On Sun, Jan 20, 2019 at 11:05:12AM +0200, Gal Pressman wrote: >> On 18-Jan-19 23:22, Jason Gunthorpe wrote: >>>>>> @@ -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... >> >> Why is the GID cache table dependent on kverbs? EFA for example uses it when >> querying the GID sysfs. > > It used to be mostly used by kverbs only, but I guess we now have some > other stuff with the various new lifetime things and what not. > >> Sure, it can bypass the cache and query the driver, but it's >> probably better to use the same flow for both kverbs and non-kverbs. > > The gid cache code is some of the more terrifying code in the > tree.. Are you going to review all patches for it to make sure it > doesn't break your device? > > IMHO, just return the only GID in query device and be done with it - > forget about sysfs. > >> I can return 0 instead of -EOPNOTSUPP in ib_query_port in order to pass the >> device registration successfully. >> Alternatively, one could argue that if phys_port_cnt > 0 query_port should be >> implemented as well (and remove the if statement entirely). > > Maybe some of this stuff should be split from the 'gid cache', the > pkey cache and subnet prefix cache are really unrelated and maybe only > used by kverbs.. The query_port callback is used from netlink (link query) as well. I think that consolidating the checks to ib_query_port is the most reasonable thing to do in order to protect all different flows. Both returning 0 and -EOPNOTSUPP are fine with me. We can also split the mandatory funcs to two: mandatory and mandatory for kverbs. This way if a mandatory function (one that is needed for device registration, such as query_device, query_port, port_immutable) is missing we will fail the registration. If a kverb function is missing we'll mark the device as non-kverbs provider. This way we can remove all the additional checks added in this patch.
