> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Thursday, November 22, 2018 3:04 PM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Daniel Jurgens <danielj@xxxxxxxxxxxx>; Parav Pandit
> <parav@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next 2/9] RDMA: Introduce and use
> rdma_device_to_ibdev()
>
> On Wed, Nov 21, 2018 at 10:44:54AM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > Introduce and use rdma_device_to_ibdev() API for those drivers which
> > are registering one sysfs group.
> > In subsequent patch, device->provider_ibdev one-to-one mapping is no
> > longer holds true during accessing sysfs entries.
> > Therefore, introduce an API rdma_device_to_ibdev() that provides such
> > information.
> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/hw/bnxt_re/main.c | 6 ++--
> > drivers/infiniband/hw/cxgb3/iwch_provider.c | 14 +++++----
> > drivers/infiniband/hw/cxgb4/provider.c | 14 +++++----
> > drivers/infiniband/hw/hfi1/sysfs.c | 24 ++++++++++------
> > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 5 ++--
> > drivers/infiniband/hw/mlx4/main.c | 10 +++++--
> > drivers/infiniband/hw/mlx5/main.c | 18 ++++++++----
> > drivers/infiniband/hw/mthca/mthca_provider.c | 12 ++++++--
> > drivers/infiniband/hw/nes/nes_verbs.c | 3 +-
> > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 6 ++--
> > drivers/infiniband/hw/qedr/main.c | 3 +-
> > drivers/infiniband/hw/qib/qib_sysfs.c | 27 ++++++++++++------
> > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c | 30 +++++++++++---------
> > drivers/infiniband/sw/rxe/rxe_verbs.c | 4 +--
> > include/rdma/ib_verbs.h | 16 +++++++++++
> > 15 files changed, 129 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/bnxt_re/main.c
> > b/drivers/infiniband/hw/bnxt_re/main.c
> > index cf2282654210..e5d110619aff 100644
> > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > @@ -538,7 +538,8 @@ static struct bnxt_en_dev
> > *bnxt_re_dev_probe(struct net_device *netdev) static ssize_t
> hw_rev_show(struct device *device, struct device_attribute *attr,
> > char *buf)
> > {
> > - struct bnxt_re_dev *rdev = to_bnxt_re_dev(device, ibdev.dev);
> > + struct ib_device *ibdev = rdma_device_to_ibdev(device);
> > + struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev);
> >
> > return scnprintf(buf, PAGE_SIZE, "0x%x\n",
> > rdev->en_dev->pdev->vendor); } @@ -547,7 +548,8 @@ static
> > DEVICE_ATTR_RO(hw_rev); static ssize_t hca_type_show(struct device
> > *device,
> > struct device_attribute *attr, char *buf) {
> > - struct bnxt_re_dev *rdev = to_bnxt_re_dev(device, ibdev.dev);
> > + struct ib_device *ibdev = rdma_device_to_ibdev(device);
> > + struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev);
> >
> > return scnprintf(buf, PAGE_SIZE, "%s\n", rdev->ibdev.node_desc); }
> > diff --git a/drivers/infiniband/hw/cxgb3/iwch_provider.c
> > b/drivers/infiniband/hw/cxgb3/iwch_provider.c
> > index ebbec02cebe0..df22f0634f59 100644
> > +++ b/drivers/infiniband/hw/cxgb3/iwch_provider.c
> > @@ -1130,8 +1130,9 @@ static int iwch_query_port(struct ib_device
> > *ibdev, static ssize_t hw_rev_show(struct device *dev,
> > struct device_attribute *attr, char *buf) {
> > - struct iwch_dev *iwch_dev = container_of(dev, struct iwch_dev,
> > - ibdev.dev);
> > + struct ib_device *ibdev = rdma_device_to_ibdev(dev);
> > + struct iwch_dev *iwch_dev = container_of(ibdev, struct iwch_dev,
> > +ibdev);
> > +
> > pr_debug("%s dev 0x%p\n", __func__, dev);
> > return sprintf(buf, "%d\n", iwch_dev->rdev.t3cdev_p->type); } @@
> > -1140,8 +1141,8 @@ static DEVICE_ATTR_RO(hw_rev); static ssize_t
> > hca_type_show(struct device *dev,
> > struct device_attribute *attr, char *buf) {
> > - struct iwch_dev *iwch_dev = container_of(dev, struct iwch_dev,
> > - ibdev.dev);
> > + struct ib_device *ibdev = rdma_device_to_ibdev(dev);
> > + struct iwch_dev *iwch_dev = container_of(ibdev, struct iwch_dev,
> > +ibdev);
>
> I think if we are are adding new functions we should also add one that does
> what all the drivers actually need:
>
> /* dev is a struct device *, return the driver device structure */ #define
> rdma_to_drv_device(dev, drv_struct, ib_dev_member) \
> container_of(rdma_device_to_ibdev(dev), drv_struct, member)
>
> And just replace the container_of's and various other versions with the
> above.
>
Yes, that is useful macro.
I enhanced it.
> > +/**
> > + * rdma_device_to_ibdev - Get ib_device pointer from device pointer
> > + *
> > + * @device: device pointer for which ib_device pointer to retrieve
> > + *
> > + * rdma_device_to_ibdev() retrieves ib_device pointer from device.
> > + *
> > + * NOTE: New drivers should not make use of this API; This API is
> > +only for
> > + * existing drivers who have exposed sysfs entries using
> > + * rdma_set_device_sysfs_group().
> > + */
> > +static inline struct ib_device *rdma_device_to_ibdev(struct device
> > +*device) {
> > + return dev_get_drvdata(device);
> > +}
>
> No, in the linux device model the drvdata belongs to the final driver. Core
> code should not co-opt it.
>
I relied on your commit [1] where ib_core sets the driver data and not final driver.
Though commit [1] doesn't seems to make use of drvdata using core defined API dev_get_drvdata() which I think you intent to use in release function but container_of was still right in release function. Not sure.
> This needs to use a container_of expression to get the ib_device pointer.
>
Yes, that is possible by storing additional pointer in ib_core_device so that original device and fake ib_core_devices can reach back to ib_device in consistent way.
This patch will just have additional changes of rdma_to_drv_device() what you described,
And,
In patch with subject [2] will have below additional changes and will update Documentation accordingly.
Does that look fine?
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index d8385d0..a4acbae 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -285,7 +285,7 @@ static void rdma_init_coredev(struct ib_core_device *coredev,
coredev->dev.class = &ib_class;
coredev->dev.groups = dev->groups;
device_initialize(&coredev->dev);
- dev_set_drvdata(&coredev->dev, dev);
+ coredev->owner = dev;
INIT_LIST_HEAD(&coredev->port_list);
}
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index c0b75f7..26bde00 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2250,6 +2250,7 @@ struct ib_core_device {
struct device dev;
struct kobject *ports_kobj;
struct list_head port_list;
+ struct ib_device *owner; /* reach back to owner ib_device */
};
struct ib_device {
@@ -4243,7 +4244,10 @@ int uverbs_destroy_def_handler(struct ib_uverbs_file *file,
*/
static inline struct ib_device *rdma_device_to_ibdev(struct device *device)
{
- return dev_get_drvdata(device);
+ struct ib_core_device *coredev =
+ container_of(device, struct ib_core_device, dev);
+
+ return coredev->owner;
}
[1] 55aeed06544f ("IB/core: Make ib_alloc_device init the kobject")
[2] Subject: RDMA/core: Introduce ib_core_device to hold device
