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.. Jason
