Re: [PATCH 11/15] RDMA/hbl: add habanalabs RDMA driver

[Omer Shpigelman <oshpigelman@habana.ai>]

On 6/17/24 22:04, Leon Romanovsky wrote:
> [Some people who received this message don't often get email from leon@kernel.org. 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@kernel.org. 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@habana.ai>
>>>> Co-developed-by: Abhilash K V <kvabhilash@habana.ai>
>>>> Signed-off-by: Abhilash K V <kvabhilash@habana.ai>
>>>> Co-developed-by: Andrey Agranovich <aagranovich@habana.ai>
>>>> Signed-off-by: Andrey Agranovich <aagranovich@habana.ai>
>>>> Co-developed-by: Bharat Jauhari <bjauhari@habana.ai>
>>>> Signed-off-by: Bharat Jauhari <bjauhari@habana.ai>
>>>> Co-developed-by: David Meriin <dmeriin@habana.ai>
>>>> Signed-off-by: David Meriin <dmeriin@habana.ai>
>>>> Co-developed-by: Sagiv Ozeri <sozeri@habana.ai>
>>>> Signed-off-by: Sagiv Ozeri <sozeri@habana.ai>
>>>> Co-developed-by: Zvika Yehudai <zyehudai@habana.ai>
>>>> Signed-off-by: Zvika Yehudai <zyehudai@habana.ai>
>>>
>>> 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