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

[Omer Shpigelman <oshpigelman@habana.ai>]

On 6/30/24 16:29, Leon Romanovsky wrote:
> On Fri, Jun 28, 2024 at 10:24:32AM +0000, Omer Shpigelman wrote:
>> On 6/19/24 13:52, Leon Romanovsky wrote:
>>> On Wed, Jun 19, 2024 at 09:27:54AM +0000, Omer Shpigelman wrote:
>>>> On 6/18/24 15:58, Leon Romanovsky wrote:
>>>>> On Tue, Jun 18, 2024 at 11:08:34AM +0000, Omer Shpigelman wrote:
>>>>>> 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>
>>>>>>>>>
>>
>> <...>
>>
>>>> mlx5 IB driver doesn't export any symbol that is used by the core driver,
>>>> that's why the core driver can be loaded without the IB driver (althought
>>>> you'll get circular dependency if you would export).
>>>
>>> Yes, IB and ETH drivers are "users" of core driver. As RDMA maintainer,
>>> I'm reluctant to accept code that exports symbols from IB drivers to
>>> other subsystems. We have drivers/infiniband/core/ for that.
>>>
>>
>> We need the core driver to access the IB driver (and to the ETH driver as
>> well). As you wrote, we can't use exported symbols from our IB driver nor
>> rely on function pointers, but what about providing the core driver an ops
>> structure? meaning exporting a register function from the core driver that
>> should be called by the IB driver during auxiliary device probe.
>> Something like:
>>
>> int hbl_cn_register_ib_aux_dev(struct auxiliary_device *adev,
>> 			       struct hbl_ib_ops *ops)
>> {
>> ...
>> }
>> EXPORT_SYMBOL(hbl_cn_register_ib_aux_dev);
>>
>> That's how only the parent driver exports symbols to the son driver so the
>> IB driver is a "user" of the core driver and so we count on the internal
>> module reference counter. But we also get the ability to access the IB
>> driver from the core driver (to report a HW error for example).
> 
> Before you are talking about solutions, please explain in technical
> terms why you absolutely need to access IB from core driver and any
> other possible way is not possible.
> 
> Thanks

First of all, as a general assumption, everything we do today can also be
done with unidirectional drivers communication only. If the parent driver
cannot access the son driver directly, then we can have a blocking command
queue on the parent side that the parent driver will push to it and the
son driver will fetch from it, execute the command and unblock the parent.
That will work but it adds complexity which I'm not sure that is needed.
The second point is not necessarily about the direction of the
communication but more about generally using function pointers rather than
exported symbols - we have 2 flavors of functions for inter driver
communications: common functions and ASIC specific functions. The ASIC
specific functions are exposed and initialized per ASIC. If we convert
them to EXPORT_SYMBOLs then we expose ASIC specific functions regardless
of the ASIC in action.
Again, that will work but seems unnecessary. We can check the ASIC type
that was passed in each exported function and fail if a wrong ASIC type
was used, but it seems to me like an incorrect approach to use exported
symbols for ASIC specific communication. EXPORT_SYMBOLs were meant to be
used for driver level communication, not for utilizing device specific
capabilities. For that, an ops struct seems more appropriate.
That's why I'm suggesting to combine both exported symbols and function
pointers.