On Mon, Jun 10, 2019 at 02:47:42PM +0000, Jason Gunthorpe wrote:
> On Mon, Jun 10, 2019 at 05:23:25PM +0300, Leon Romanovsky wrote:
> > On Wed, Jun 05, 2019 at 03:32:51PM -0300, Jason Gunthorpe wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > >
> > > Allow userspace to issue a netlink query against the ib_device for
> > > something like "uverbs" and get back the char dev name, inode major/minor,
> > > and interface ABI information for "uverbs0".
> > >
> > > Since we are now in netlink this can also trigger a module autoload to
> > > make the uverbs device come into existence.
> > >
> > > Largely this will let us replace searching and reading inside sysfs to
> > > setup devices, and provides an alternative (using driver_id) to device
> > > name based provider binding for things like rxe.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > drivers/infiniband/core/core_priv.h | 9 +++
> > > drivers/infiniband/core/device.c | 88 ++++++++++++++++++++++++++++
> > > drivers/infiniband/core/nldev.c | 91 +++++++++++++++++++++++++++++
> > > include/rdma/ib_verbs.h | 4 ++
> > > include/rdma/rdma_netlink.h | 2 +
> > > include/uapi/rdma/rdma_netlink.h | 10 ++++
> > > 6 files changed, 204 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
> > > index ff40a450b5d28e..a953c2fa2e7811 100644
> > > +++ b/drivers/infiniband/core/core_priv.h
> > > @@ -88,6 +88,15 @@ typedef int (*nldev_callback)(struct ib_device *device,
> > > int ib_enum_all_devs(nldev_callback nldev_cb, struct sk_buff *skb,
> > > struct netlink_callback *cb);
> > >
> > > +struct ib_client_nl_info {
> > > + struct sk_buff *nl_msg;
> > > + struct device *cdev;
> > > + unsigned int port;
> > > + u64 abi;
> > > +};
> > > +int ib_get_client_nl_info(struct ib_device *ibdev, const char *client_name,
> > > + struct ib_client_nl_info *res);
> > > +
> > > enum ib_cache_gid_default_mode {
> > > IB_CACHE_GID_DEFAULT_MODE_SET,
> > > IB_CACHE_GID_DEFAULT_MODE_DELETE
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index 49e5ea3a530f53..80e7911951f6f6 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -1749,6 +1749,94 @@ void ib_unregister_client(struct ib_client *client)
> > > }
> > > EXPORT_SYMBOL(ib_unregister_client);
> > >
> > > +static int __ib_get_client_nl_info(struct ib_device *ibdev,
> > > + const char *client_name,
> > > + struct ib_client_nl_info *res)
> > > +{
> > > + unsigned long index;
> > > + void *client_data;
> > > + int ret = -ENOENT;
> > > +
> > > + if (!ibdev) {
> > > + struct ib_client *client;
> > > +
> > > + down_read(&clients_rwsem);
> > > + xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) {
> > > + if (strcmp(client->name, client_name) != 0)
> > > + continue;
> > > + if (!client->get_global_nl_info) {
> > > + ret = -EOPNOTSUPP;
> > > + break;
> > > + }
> > > + ret = client->get_global_nl_info(res);
> > > + if (WARN_ON(ret == -ENOENT))
> >
> > You are putting to much WARN_ON, sometimes printk can be enough.
>
> One should not use printk for a kernel bug. It just makes debugging
> harder. This is the appropriate pattern for 'things that cannot happen'
>
>
> > > + ret = -EINVAL;
> > > + if (!ret && res->cdev)
> > > + get_device(res->cdev);
> > > + break;
> > > + }
> > > + up_read(&clients_rwsem);
> > > + return ret;
> >
> > This flow is better to have in separate function, one function for
> > loaded client and another for no-loaded.
>
> Yah, maybe so
>
> > > diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h
> > > index f588e8551c6cea..15eb861d1324f4 100644
> > > +++ b/include/uapi/rdma/rdma_netlink.h
> > > @@ -279,6 +279,8 @@ enum rdma_nldev_command {
> > >
> > > RDMA_NLDEV_CMD_RES_PD_GET, /* can dump */
> > >
> > > + RDMA_NLDEV_CMD_GET_CHARDEV,
> > > +
> > > RDMA_NLDEV_NUM_OPS
> > > };
> > >
> > > @@ -491,6 +493,14 @@ enum rdma_nldev_attr {
> > > */
> > > RDMA_NLDEV_NET_NS_FD, /* u32 */
> > >
> > > + /*
> > > + * Information about a chardev
> > > + */
> > > + RDMA_NLDEV_ATTR_CHARDEV_TYPE, /* string */
> > > + RDMA_NLDEV_ATTR_CHARDEV_NAME, /* string */
> > > + RDMA_NLDEV_ATTR_CHARDEV_ABI, /* u64 */
> > > + RDMA_NLDEV_ATTR_CHARDEV, /* u64 */
> >
> > Please document them, especially RDMA_NLDEV_ATTR_CHARDEV and
> > RDMA_NLDEV_ATTR_CHARDEV_TYPE.
>
> Where do you want to put them? There is a distinct lack of
> documentation for the netlink attributes in this file. Every one I
> wanted to use I had to look up the implementation.
I believe that short description near their declaration in
rdna_netlink.h is fine enough.
Thanks
>
> Jason
