On 6/17/24 22:04, Leon Romanovsky wrote: > [Some people who received this message don't often get email from leon@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Mon, Jun 17, 2024 at 05:43:49PM +0000, Omer Shpigelman wrote: >> On 6/13/24 22:18, Leon Romanovsky wrote: >>> [Some people who received this message don't often get email from leon@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >>> >>> On Thu, Jun 13, 2024 at 11:22:04AM +0300, Omer Shpigelman wrote: >>>> Add an RDMA driver of Gaudi ASICs family for AI scaling. >>>> The driver itself is agnostic to the ASIC in action, it operates according >>>> to the capabilities that were passed on device initialization. >>>> The device is initialized by the hbl_cn driver via auxiliary bus. >>>> The driver also supports QP resource tracking and port/device HW counters. >>>> >>>> Signed-off-by: Omer Shpigelman <oshpigelman@xxxxxxxxx> >>>> Co-developed-by: Abhilash K V <kvabhilash@xxxxxxxxx> >>>> Signed-off-by: Abhilash K V <kvabhilash@xxxxxxxxx> >>>> Co-developed-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >>>> Signed-off-by: Andrey Agranovich <aagranovich@xxxxxxxxx> >>>> Co-developed-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >>>> Signed-off-by: Bharat Jauhari <bjauhari@xxxxxxxxx> >>>> Co-developed-by: David Meriin <dmeriin@xxxxxxxxx> >>>> Signed-off-by: David Meriin <dmeriin@xxxxxxxxx> >>>> Co-developed-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >>>> Signed-off-by: Sagiv Ozeri <sozeri@xxxxxxxxx> >>>> Co-developed-by: Zvika Yehudai <zyehudai@xxxxxxxxx> >>>> Signed-off-by: Zvika Yehudai <zyehudai@xxxxxxxxx> >>> >>> I afraid that you misinterpreted the "Co-developed-by" tag. All these >>> people are probably touch the code and not actually sit together at >>> the same room and write the code together. So, please remove the >>> extensive "Co-developed-by" tags. >>> >>> It is not full review yet, but simple pass-by-comments. >>> >> >> Actually except of two, all of the mentioned persons sat in the same room >> and developed the code together. >> The remaining two are located on a different site (but also together). >> Isn't that what "Co-developed-by" tag for? >> I wanted to give them credit for writing the code but I can remove if it's >> not common. > > Signed-off-by will be enough to give them credit. > Ok, good enough. >> >>>> --- >>>> MAINTAINERS | 10 + >>>> drivers/infiniband/Kconfig | 1 + >>>> drivers/infiniband/hw/Makefile | 1 + >>>> drivers/infiniband/hw/hbl/Kconfig | 17 + >>>> drivers/infiniband/hw/hbl/Makefile | 8 + >>>> drivers/infiniband/hw/hbl/hbl.h | 326 +++ >>>> drivers/infiniband/hw/hbl/hbl_main.c | 478 ++++ >>>> drivers/infiniband/hw/hbl/hbl_verbs.c | 2686 ++++++++++++++++++++++ >>>> include/uapi/rdma/hbl-abi.h | 204 ++ >>>> include/uapi/rdma/hbl_user_ioctl_cmds.h | 66 + >>>> include/uapi/rdma/hbl_user_ioctl_verbs.h | 106 + >>>> include/uapi/rdma/ib_user_ioctl_verbs.h | 1 + >>>> 12 files changed, 3904 insertions(+) >>>> create mode 100644 drivers/infiniband/hw/hbl/Kconfig >>>> create mode 100644 drivers/infiniband/hw/hbl/Makefile >>>> create mode 100644 drivers/infiniband/hw/hbl/hbl.h >>>> create mode 100644 drivers/infiniband/hw/hbl/hbl_main.c >>>> create mode 100644 drivers/infiniband/hw/hbl/hbl_verbs.c >>>> create mode 100644 include/uapi/rdma/hbl-abi.h >>>> create mode 100644 include/uapi/rdma/hbl_user_ioctl_cmds.h >>>> create mode 100644 include/uapi/rdma/hbl_user_ioctl_verbs.h >>> >>> <...> >>> >>>> +#define hbl_ibdev_emerg(ibdev, format, ...) ibdev_emerg(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_alert(ibdev, format, ...) ibdev_alert(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_crit(ibdev, format, ...) ibdev_crit(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_err(ibdev, format, ...) ibdev_err(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_warn(ibdev, format, ...) ibdev_warn(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_notice(ibdev, format, ...) ibdev_notice(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_info(ibdev, format, ...) ibdev_info(ibdev, format, ##__VA_ARGS__) >>>> +#define hbl_ibdev_dbg(ibdev, format, ...) ibdev_dbg(ibdev, format, ##__VA_ARGS__) >>>> + >>>> +#define hbl_ibdev_emerg_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_emerg_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_alert_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_alert_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_crit_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_crit_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_err_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_err_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_warn_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_warn_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_notice_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_notice_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_info_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_info_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> +#define hbl_ibdev_dbg_ratelimited(ibdev, fmt, ...) \ >>>> + ibdev_dbg_ratelimited(ibdev, fmt, ##__VA_ARGS__) >>>> + >>> >>> Please don't redefine the existing macros. Just use the existing ones. >>> >>> >>> <...> >>> >> >> That's a leftover from some debug code. I'll remove. >> >>>> + if (hbl_ib_match_netdev(ibdev, netdev)) >>>> + ib_port = hbl_to_ib_port_num(hdev, netdev->dev_port); >>>> + else >>>> + return NOTIFY_DONE; >>> >>> It is not kernel coding style. Please write: >>> if (!hbl_ib_match_netdev(ibdev, netdev)) >>> return NOTIFY_DONE; >>> >>> ib_port = hbl_to_ib_port_num(hdev, netdev->dev_port); >>> >> >> I'll fix the code, thanks. >> >>>> + >>> >>> <...> >>> >>>> +static int hbl_ib_probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id) >>>> +{ >>>> + struct hbl_aux_dev *aux_dev = container_of(adev, struct hbl_aux_dev, adev); >>>> + struct hbl_ib_aux_ops *aux_ops = aux_dev->aux_ops; >>>> + struct hbl_ib_device *hdev; >>>> + ktime_t timeout; >>>> + int rc; >>>> + >>>> + rc = hdev_init(aux_dev); >>>> + if (rc) { >>>> + dev_err(&aux_dev->adev.dev, "Failed to init hdev\n"); >>>> + return -EIO; >>>> + } >>>> + >>>> + hdev = aux_dev->priv; >>>> + >>>> + /* don't allow module unloading while it is attached */ >>>> + if (!try_module_get(THIS_MODULE)) { >>> >>> This part makes wonder, what are you trying to do here? What doesn't work for you >>> in standard driver core and module load mechanism? >>> >> >> Before auxiliary bus was introduced, we used EXPORT_SYMBOLs for inter >> driver communication. That incremented the refcount of the used module so >> it couldn't be removed while it is in use. >> Auxiliary bus usage doesn't increment the used module refcount and hence >> the used module can be removed while it is in use and that's something >> we don't want to allow. >> We could solve it by some global locking or in_use atomic but the most >> simple and clean way is just to increment the used module refcount on >> auxiliary device probe and decrement it on auxiliary device removal. > > No, you was supposed to continue to use EXPORT_SYMBOLs and don't > invent auxiliary ops structure (this is why you lost module > reference counting). > Sorry, but according to the auxiliary bus doc, a domain-specific ops structure can be used. We followed the usage example described at drivers/base/auxiliary.c. What am I missing? Moreover, we'd like to support the mode where the IB or the ETH driver is not loaded at all. But this cannot be achieved if we use EXPORT_SYMBOLs exclusively for inter driver communication. >> >>>> + dev_err(hdev->dev, "Failed to increment %s module refcount\n", >>>> + module_name(THIS_MODULE)); >>>> + rc = -EIO; >>>> + goto module_get_err; >>>> + } >>>> + >>>> + timeout = ktime_add_ms(ktime_get(), hdev->pending_reset_long_timeout * MSEC_PER_SEC); >>>> + while (1) { >>>> + aux_ops->hw_access_lock(aux_dev); >>>> + >>>> + /* if the device is operational, proceed to actual init while holding the lock in >>>> + * order to prevent concurrent hard reset >>>> + */ >>>> + if (aux_ops->device_operational(aux_dev)) >>>> + break; >>>> + >>>> + aux_ops->hw_access_unlock(aux_dev); >>>> + >>>> + if (ktime_compare(ktime_get(), timeout) > 0) { >>>> + dev_err(hdev->dev, "Timeout while waiting for hard reset to finish\n"); >>>> + rc = -EBUSY; >>>> + goto timeout_err; >>>> + } >>>> + >>>> + dev_notice_once(hdev->dev, "Waiting for hard reset to finish before probing IB\n"); >>>> + >>>> + msleep_interruptible(MSEC_PER_SEC); >>>> + } >>> >>> The code above is unexpected. >>> >> >> We have no control on when the user insmod the IB driver. > > It is not true, this is controlled through module dependencies > mechanism. > Yeah, if we would use EXPORT_SYMBOLs for inter driver communication but we don't. >> As a result it is possible that the IB auxiliary device will be probed >> while the compute device is under reset (due to some HW error). > > No, it is not possible. If you structure your driver right. > Again, it is not possible if we would use EXPORT_SYMBOLs. Please let me know if we misunderstood something because AFAIU we followed the auxiliary bus doc usage example. > Thanks
