On Thu, Jan 17, 2019 at 12:41:12PM -0800, Dennis Dalessandro wrote:
> From: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
>
> Commit 896de0090a85 ("RDMA/core: Use dev_name instead of
> ibdev->name"), changes the API for setting the IB device name.
>
> HFI, QIB, and RDMAVT make assumptions with regards to this API (i.e.
> as to when the device name string is available). This assumption is
> no longer valid.
>
> Update the drivers to reflect the commit by storing the specific
> device name in device local information. Create a calldown for the
> RVT driver to access the drivers name information.
>
> Rework the logging macros to use the best device name.
>
> Fixes: 11f0e89710eb ("IB/{hfi1, qib}: Fix a concurrency issue with device name in logging")
> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> ---
> drivers/infiniband/hw/hfi1/affinity.c | 6 +++--
> drivers/infiniband/hw/hfi1/driver.c | 8 +++++++
> drivers/infiniband/hw/hfi1/hfi.h | 22 ++++++++-----------
> drivers/infiniband/hw/hfi1/init.c | 2 +-
> drivers/infiniband/hw/hfi1/verbs.c | 1 +
> drivers/infiniband/hw/qib/qib.h | 12 +++++-----
> drivers/infiniband/hw/qib/qib_driver.c | 8 +++++++
> drivers/infiniband/hw/qib/qib_init.c | 10 ++++++--
> drivers/infiniband/hw/qib/qib_verbs.c | 1 +
> drivers/infiniband/sw/rdmavt/trace.h | 6 +++--
> drivers/infiniband/sw/rdmavt/vt.c | 2 +-
> drivers/infiniband/sw/rdmavt/vt.h | 10 ++++++--
> include/rdma/rdma_vt.h | 38 ++++++--------------------------
> 13 files changed, 64 insertions(+), 62 deletions(-)
This is too ugly. Drivers should not be defining their own printing
macros, or dealing with this name stuff. They shouldn't be randomly
choosing to print to the PCI struct device vs the ib struct device and
all sorts of other inconsistent things we have.
If drivers need to print prior to registration, then we need to copy
what netdev has been doing and provide our own printing functions
accepting a struct ib_device* that go through various gyrations to
compute a suitable identification string across the entire life cycle
- ie netdev will use a PCI BDF and other options if the ethX name is
not yet assigned.
I stopped short of doing this when I injected dev_name, because it
didn't seem necessary, but if you say hfi has a problem, then this is
how to fix it.
> --- a/drivers/infiniband/hw/hfi1/init.c
> +++ b/drivers/infiniband/hw/hfi1/init.c
> @@ -1313,7 +1313,7 @@ void hfi1_free_devdata(struct hfi1_devdata *dd)
> "Could not allocate unit ID: error %d\n", -ret);
> goto bail;
> }
> - rvt_set_ibdev_name(&dd->verbs_dev.rdi, "%s_%d", class_name(), dd->unit);
> + kobject_set_name(&dd->kobj, "%s_%d", class_name(), dd->unit);
Woah, this driver has a kobj and a struct ib_device in the same
structure? Betcha the refcounting is done wrong :(
This kobject is completely redundant with the kobject that is already
in the struct ib_device, you should delete it, not use it for more
stuff.
> /*
> * Initialize all locks for the device. This needs to be as early as
> diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
> index ec582d8..6546cba 100644
> --- a/drivers/infiniband/hw/hfi1/verbs.c
> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> @@ -1680,6 +1680,7 @@ int hfi1_register_ib_device(struct hfi1_devdata *dd)
> * Fill in rvt info object.
> */
> dd->verbs_dev.rdi.driver_f.port_callback = hfi1_create_port_files;
> + dd->verbs_dev.rdi.driver_f.get_device_name = hfi1_get_device_name;
> dd->verbs_dev.rdi.driver_f.get_pci_dev = get_pci_dev;
> dd->verbs_dev.rdi.driver_f.check_ah = hfi1_check_ah;
> dd->verbs_dev.rdi.driver_f.notify_new_ah = hfi1_notify_new_ah;
> diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
> index 83d2349..3681701 100644
> --- a/drivers/infiniband/hw/qib/qib.h
> +++ b/drivers/infiniband/hw/qib/qib.h
> @@ -1075,6 +1075,8 @@ struct qib_devdata {
> struct tasklet_struct error_tasklet;
>
> int assigned_node_id; /* NUMA node closest to HCA */
> + /* device name */
> + char *name;
> };
Don't store your own names :(
> -/**
> - * rvt_set_ibdev_name - Craft an IB device name from client info
> - * @rdi: pointer to the client rvt_dev_info structure
> - * @name: client specific name
> - * @unit: client specific unit number.
> - */
> -static inline void rvt_set_ibdev_name(struct rvt_dev_info *rdi,
> - const char *fmt, const char *name,
> - const int unit)
> -{
> - /*
> - * FIXME: rvt and its users want to touch the ibdev before
> - * registration and have things like the name work. We don't have the
> - * infrastructure in the core to support this directly today, hack it
> - * to work by setting the name manually here.
> - */
> - dev_set_name(&rdi->ibdev.dev, fmt, name, unit);
> - strlcpy(rdi->ibdev.name, dev_name(&rdi->ibdev.dev), IB_DEVICE_NAME_MAX);
So why wasn't this hack good enough? What is the issue?
Jason
