| Message ID | 171dafa48f9bab494d1fe766ae662e264af222a2.1522341686.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 29, 2018 at 09:09:51AM -0700, Steve Wise wrote: > Add fill_dev_info and fill_port_info functions to rdma_restrack_root. > This allows providers to have provider-specific fill functions for device > and port restrack operations. > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > --- > drivers/infiniband/core/nldev.c | 35 +++++++++++++++++++++++++++++------ > include/rdma/restrack.h | 15 +++++++++++++++ > 2 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 99df8d4..902517f 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -115,6 +115,22 @@ static int provider_fill_res_entry(struct rdma_restrack_root *resroot, > resroot->fill_res_entry(msg, cb, res) : 0; > } > > +static int provider_fill_dev_info(struct sk_buff *msg, > + struct netlink_callback *cb, > + struct ib_device *device) > +{ > + return device->res.fill_dev_info ? > + device->res.fill_dev_info(msg, cb, device) : 0; > +} > + > +static int provider_fill_port_info(struct sk_buff *msg, > + struct netlink_callback *cb, > + struct ib_device *device, u32 port) > +{ > + return device->res.fill_port_info ? > + device->res.fill_port_info(msg, cb, device, port) : 0; > +} > + > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) > { > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device->index)) > @@ -125,7 +141,8 @@ static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) > return 0; > } > > -static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) > +static int fill_dev_info(struct sk_buff *msg, struct netlink_callback *cb, > + struct ib_device *device) > { > char fw[IB_FW_VERSION_NAME_MAX]; > > @@ -156,10 +173,14 @@ static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) > return -EMSGSIZE; > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_DEV_NODE_TYPE, device->node_type)) > return -EMSGSIZE; > + > + if (provider_fill_dev_info(msg, cb, device)) > + return -EMSGSIZE; > + > return 0; > } > > -static int fill_port_info(struct sk_buff *msg, > +static int fill_port_info(struct sk_buff *msg, struct netlink_callback *cb, > struct ib_device *device, u32 port) > { > struct ib_port_attr attr; > @@ -195,6 +216,8 @@ static int fill_port_info(struct sk_buff *msg, > return -EMSGSIZE; > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state)) > return -EMSGSIZE; > + if (provider_fill_port_info(msg, cb, device, port)) > + return -EMSGSIZE; > return 0; > } > > @@ -554,7 +577,7 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, > RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), > 0, 0); > > - err = fill_dev_info(msg, device); > + err = fill_dev_info(msg, NULL, device); I look on this line and think that providing "struct netlink_callback *cb" to drivers is too broad approach. The right thing will be to pass "u64 caps_falgs" variable which will have bits on/off per uapi/linux/capability.h. However, you don't use this functionality for now, so it will make sense do not implement it completely and simply leave comment in the code how it should be implemented in the future. Thanks
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Leon Romanovsky > Sent: Friday, March 30, 2018 5:36 AM > To: Steve Wise <swise@opengridcomputing.com> > Cc: jgg@mellanox.com; dledford@redhat.com; linux-rdma@vger.kernel.org > Subject: Re: [PATCH v2 rdma-next 3/5] RDMA/nldev: add provider-specific > device/port tracking > > On Thu, Mar 29, 2018 at 09:09:51AM -0700, Steve Wise wrote: > > Add fill_dev_info and fill_port_info functions to rdma_restrack_root. > > This allows providers to have provider-specific fill functions for device > > and port restrack operations. > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > --- > > drivers/infiniband/core/nldev.c | 35 +++++++++++++++++++++++++++++--- > --- > > include/rdma/restrack.h | 15 +++++++++++++++ > > 2 files changed, 44 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > index 99df8d4..902517f 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -115,6 +115,22 @@ static int provider_fill_res_entry(struct > rdma_restrack_root *resroot, > > resroot->fill_res_entry(msg, cb, res) : 0; > > } > > > > +static int provider_fill_dev_info(struct sk_buff *msg, > > + struct netlink_callback *cb, > > + struct ib_device *device) > > +{ > > + return device->res.fill_dev_info ? > > + device->res.fill_dev_info(msg, cb, device) : 0; > > +} > > + > > +static int provider_fill_port_info(struct sk_buff *msg, > > + struct netlink_callback *cb, > > + struct ib_device *device, u32 port) > > +{ > > + return device->res.fill_port_info ? > > + device->res.fill_port_info(msg, cb, device, port) : 0; > > +} > > + > > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) > > { > > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device- > >index)) > > @@ -125,7 +141,8 @@ static int fill_nldev_handle(struct sk_buff *msg, > struct ib_device *device) > > return 0; > > } > > > > -static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) > > +static int fill_dev_info(struct sk_buff *msg, struct netlink_callback *cb, > > + struct ib_device *device) > > { > > char fw[IB_FW_VERSION_NAME_MAX]; > > > > @@ -156,10 +173,14 @@ static int fill_dev_info(struct sk_buff *msg, > struct ib_device *device) > > return -EMSGSIZE; > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_DEV_NODE_TYPE, device- > >node_type)) > > return -EMSGSIZE; > > + > > + if (provider_fill_dev_info(msg, cb, device)) > > + return -EMSGSIZE; > > + > > return 0; > > } > > > > -static int fill_port_info(struct sk_buff *msg, > > +static int fill_port_info(struct sk_buff *msg, struct netlink_callback *cb, > > struct ib_device *device, u32 port) > > { > > struct ib_port_attr attr; > > @@ -195,6 +216,8 @@ static int fill_port_info(struct sk_buff *msg, > > return -EMSGSIZE; > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, > attr.phys_state)) > > return -EMSGSIZE; > > + if (provider_fill_port_info(msg, cb, device, port)) > > + return -EMSGSIZE; > > return 0; > > } > > > > @@ -554,7 +577,7 @@ static int nldev_get_doit(struct sk_buff *skb, > struct nlmsghdr *nlh, > > RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > RDMA_NLDEV_CMD_GET), > > 0, 0); > > > > - err = fill_dev_info(msg, device); > > + err = fill_dev_info(msg, NULL, device); > > I look on this line and think that providing "struct netlink_callback *cb" > to drivers is too broad approach. The right thing will be to pass > "u64 caps_falgs" variable which will have bits on/off per > uapi/linux/capability.h. > > However, you don't use this functionality for now, so it will make sense > do not implement it completely and simply leave comment in the code how > it should be implemented in the future. > > Thanks Do you think the fill_res_entry API should also have this change? Is there still time for me to respin this and make the merge window? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > -----Original Message----- > > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > > owner@vger.kernel.org> On Behalf Of Leon Romanovsky > > Sent: Friday, March 30, 2018 5:36 AM > > To: Steve Wise <swise@opengridcomputing.com> > > Cc: jgg@mellanox.com; dledford@redhat.com; linux- > rdma@vger.kernel.org > > Subject: Re: [PATCH v2 rdma-next 3/5] RDMA/nldev: add provider-specific > > device/port tracking > > > > On Thu, Mar 29, 2018 at 09:09:51AM -0700, Steve Wise wrote: > > > Add fill_dev_info and fill_port_info functions to rdma_restrack_root. > > > This allows providers to have provider-specific fill functions for device > > > and port restrack operations. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/nldev.c | 35 +++++++++++++++++++++++++++++- > -- > > --- > > > include/rdma/restrack.h | 15 +++++++++++++++ > > > 2 files changed, 44 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 99df8d4..902517f 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -115,6 +115,22 @@ static int provider_fill_res_entry(struct > > rdma_restrack_root *resroot, > > > resroot->fill_res_entry(msg, cb, res) : 0; > > > } > > > > > > +static int provider_fill_dev_info(struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct ib_device *device) > > > +{ > > > + return device->res.fill_dev_info ? > > > + device->res.fill_dev_info(msg, cb, device) : 0; > > > +} > > > + > > > +static int provider_fill_port_info(struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct ib_device *device, u32 port) > > > +{ > > > + return device->res.fill_port_info ? > > > + device->res.fill_port_info(msg, cb, device, port) : 0; > > > +} > > > + > > > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device > *device) > > > { > > > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device- > > >index)) > > > @@ -125,7 +141,8 @@ static int fill_nldev_handle(struct sk_buff *msg, > > struct ib_device *device) > > > return 0; > > > } > > > > > > -static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) > > > +static int fill_dev_info(struct sk_buff *msg, struct netlink_callback *cb, > > > + struct ib_device *device) > > > { > > > char fw[IB_FW_VERSION_NAME_MAX]; > > > > > > @@ -156,10 +173,14 @@ static int fill_dev_info(struct sk_buff *msg, > > struct ib_device *device) > > > return -EMSGSIZE; > > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_DEV_NODE_TYPE, device- > > >node_type)) > > > return -EMSGSIZE; > > > + > > > + if (provider_fill_dev_info(msg, cb, device)) > > > + return -EMSGSIZE; > > > + > > > return 0; > > > } > > > > > > -static int fill_port_info(struct sk_buff *msg, > > > +static int fill_port_info(struct sk_buff *msg, struct netlink_callback *cb, > > > struct ib_device *device, u32 port) > > > { > > > struct ib_port_attr attr; > > > @@ -195,6 +216,8 @@ static int fill_port_info(struct sk_buff *msg, > > > return -EMSGSIZE; > > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, > > attr.phys_state)) > > > return -EMSGSIZE; > > > + if (provider_fill_port_info(msg, cb, device, port)) > > > + return -EMSGSIZE; > > > return 0; > > > } > > > > > > @@ -554,7 +577,7 @@ static int nldev_get_doit(struct sk_buff *skb, > > struct nlmsghdr *nlh, > > > RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > > RDMA_NLDEV_CMD_GET), > > > 0, 0); > > > > > > - err = fill_dev_info(msg, device); > > > + err = fill_dev_info(msg, NULL, device); > > > > I look on this line and think that providing "struct netlink_callback *cb" > > to drivers is too broad approach. The right thing will be to pass > > "u64 caps_falgs" variable which will have bits on/off per > > uapi/linux/capability.h. > > > > However, you don't use this functionality for now, so it will make sense > > do not implement it completely and simply leave comment in the code > how > > it should be implemented in the future. > > > > Thanks > > Do you think the fill_res_entry API should also have this change? Is there > still time for me to respin this and make the merge window? > Hmm, Look at __netlink_ns_capable(). I'm not sure how to compose the correct cap_flags, and there is no existing API to return them. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 30, 2018 at 08:38:53AM -0500, Steve Wise wrote: > > > > -----Original Message----- > > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > > owner@vger.kernel.org> On Behalf Of Leon Romanovsky > > Sent: Friday, March 30, 2018 5:36 AM > > To: Steve Wise <swise@opengridcomputing.com> > > Cc: jgg@mellanox.com; dledford@redhat.com; linux-rdma@vger.kernel.org > > Subject: Re: [PATCH v2 rdma-next 3/5] RDMA/nldev: add provider-specific > > device/port tracking > > > > On Thu, Mar 29, 2018 at 09:09:51AM -0700, Steve Wise wrote: > > > Add fill_dev_info and fill_port_info functions to rdma_restrack_root. > > > This allows providers to have provider-specific fill functions for > device > > > and port restrack operations. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/nldev.c | 35 +++++++++++++++++++++++++++++--- > > --- > > > include/rdma/restrack.h | 15 +++++++++++++++ > > > 2 files changed, 44 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 99df8d4..902517f 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -115,6 +115,22 @@ static int provider_fill_res_entry(struct > > rdma_restrack_root *resroot, > > > resroot->fill_res_entry(msg, cb, res) : 0; > > > } > > > > > > +static int provider_fill_dev_info(struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct ib_device *device) > > > +{ > > > + return device->res.fill_dev_info ? > > > + device->res.fill_dev_info(msg, cb, device) : 0; > > > +} > > > + > > > +static int provider_fill_port_info(struct sk_buff *msg, > > > + struct netlink_callback *cb, > > > + struct ib_device *device, u32 port) > > > +{ > > > + return device->res.fill_port_info ? > > > + device->res.fill_port_info(msg, cb, device, port) : 0; > > > +} > > > + > > > static int fill_nldev_handle(struct sk_buff *msg, struct ib_device > *device) > > > { > > > if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device- > > >index)) > > > @@ -125,7 +141,8 @@ static int fill_nldev_handle(struct sk_buff *msg, > > struct ib_device *device) > > > return 0; > > > } > > > > > > -static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) > > > +static int fill_dev_info(struct sk_buff *msg, struct netlink_callback > *cb, > > > + struct ib_device *device) > > > { > > > char fw[IB_FW_VERSION_NAME_MAX]; > > > > > > @@ -156,10 +173,14 @@ static int fill_dev_info(struct sk_buff *msg, > > struct ib_device *device) > > > return -EMSGSIZE; > > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_DEV_NODE_TYPE, device- > > >node_type)) > > > return -EMSGSIZE; > > > + > > > + if (provider_fill_dev_info(msg, cb, device)) > > > + return -EMSGSIZE; > > > + > > > return 0; > > > } > > > > > > -static int fill_port_info(struct sk_buff *msg, > > > +static int fill_port_info(struct sk_buff *msg, struct netlink_callback > *cb, > > > struct ib_device *device, u32 port) > > > { > > > struct ib_port_attr attr; > > > @@ -195,6 +216,8 @@ static int fill_port_info(struct sk_buff *msg, > > > return -EMSGSIZE; > > > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, > > attr.phys_state)) > > > return -EMSGSIZE; > > > + if (provider_fill_port_info(msg, cb, device, port)) > > > + return -EMSGSIZE; > > > return 0; > > > } > > > > > > @@ -554,7 +577,7 @@ static int nldev_get_doit(struct sk_buff *skb, > > struct nlmsghdr *nlh, > > > RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > > RDMA_NLDEV_CMD_GET), > > > 0, 0); > > > > > > - err = fill_dev_info(msg, device); > > > + err = fill_dev_info(msg, NULL, device); > > > > I look on this line and think that providing "struct netlink_callback *cb" > > to drivers is too broad approach. The right thing will be to pass > > "u64 caps_falgs" variable which will have bits on/off per > > uapi/linux/capability.h. > > > > However, you don't use this functionality for now, so it will make sense > > do not implement it completely and simply leave comment in the code how > > it should be implemented in the future. > > > > Thanks > > Do you think the fill_res_entry API should also have this change? Is there > still time for me to respin this and make the merge window? I think so, drop that "struct netlink_callback *cb" parameter and leave this headache to the developers who actually will need it and they will implement it. Thanks > >
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 99df8d4..902517f 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -115,6 +115,22 @@ static int provider_fill_res_entry(struct rdma_restrack_root *resroot, resroot->fill_res_entry(msg, cb, res) : 0; } +static int provider_fill_dev_info(struct sk_buff *msg, + struct netlink_callback *cb, + struct ib_device *device) +{ + return device->res.fill_dev_info ? + device->res.fill_dev_info(msg, cb, device) : 0; +} + +static int provider_fill_port_info(struct sk_buff *msg, + struct netlink_callback *cb, + struct ib_device *device, u32 port) +{ + return device->res.fill_port_info ? + device->res.fill_port_info(msg, cb, device, port) : 0; +} + static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) { if (nla_put_u32(msg, RDMA_NLDEV_ATTR_DEV_INDEX, device->index)) @@ -125,7 +141,8 @@ static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device) return 0; } -static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) +static int fill_dev_info(struct sk_buff *msg, struct netlink_callback *cb, + struct ib_device *device) { char fw[IB_FW_VERSION_NAME_MAX]; @@ -156,10 +173,14 @@ static int fill_dev_info(struct sk_buff *msg, struct ib_device *device) return -EMSGSIZE; if (nla_put_u8(msg, RDMA_NLDEV_ATTR_DEV_NODE_TYPE, device->node_type)) return -EMSGSIZE; + + if (provider_fill_dev_info(msg, cb, device)) + return -EMSGSIZE; + return 0; } -static int fill_port_info(struct sk_buff *msg, +static int fill_port_info(struct sk_buff *msg, struct netlink_callback *cb, struct ib_device *device, u32 port) { struct ib_port_attr attr; @@ -195,6 +216,8 @@ static int fill_port_info(struct sk_buff *msg, return -EMSGSIZE; if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state)) return -EMSGSIZE; + if (provider_fill_port_info(msg, cb, device, port)) + return -EMSGSIZE; return 0; } @@ -554,7 +577,7 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), 0, 0); - err = fill_dev_info(msg, device); + err = fill_dev_info(msg, NULL, device); if (err) goto err_free; @@ -585,7 +608,7 @@ static int _nldev_get_dumpit(struct ib_device *device, RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), 0, NLM_F_MULTI); - if (fill_dev_info(skb, device)) { + if (fill_dev_info(skb, cb, device)) { nlmsg_cancel(skb, nlh); goto out; } @@ -645,7 +668,7 @@ static int nldev_port_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET), 0, 0); - err = fill_port_info(msg, device, port); + err = fill_port_info(msg, NULL, device, port); if (err) goto err_free; @@ -705,7 +728,7 @@ static int nldev_port_get_dumpit(struct sk_buff *skb, RDMA_NLDEV_CMD_PORT_GET), 0, NLM_F_MULTI); - if (fill_port_info(skb, device, p)) { + if (fill_port_info(skb, cb, device, p)) { nlmsg_cancel(skb, nlh); goto out; } diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h index bd3cd9a..936b56f 100644 --- a/include/rdma/restrack.h +++ b/include/rdma/restrack.h @@ -45,6 +45,7 @@ enum rdma_restrack_type { #define RDMA_RESTRACK_HASH_BITS 8 struct rdma_restrack_entry; +struct ib_device; /** * struct rdma_restrack_root - main resource tracking management @@ -67,6 +68,20 @@ struct rdma_restrack_root { int (*fill_res_entry)(struct sk_buff *msg, struct netlink_callback *cb, struct rdma_restrack_entry *entry); + /** + * @fill_dev_info: provider-specific fill function + * + * Allows rdma providers to add their own device attributes. + */ + int (*fill_dev_info)(struct sk_buff *msg, struct netlink_callback *cb, + struct ib_device *device); + /** + * @fill_port_info: provider-specific fill function + * + * Allows rdma providers to add their own port attributes. + */ + int (*fill_port_info)(struct sk_buff *msg, struct netlink_callback *cb, + struct ib_device *device, u32 port); }; /**
Add fill_dev_info and fill_port_info functions to rdma_restrack_root. This allows providers to have provider-specific fill functions for device and port restrack operations. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/core/nldev.c | 35 +++++++++++++++++++++++++++++------ include/rdma/restrack.h | 15 +++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-)