Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

["Anwar, Md Danish" <a0501179@ti.com>]

Hi,

On 12/19/2024 10:29 PM, Larysa Zaremba wrote:
> On Thu, Dec 19, 2024 at 10:36:57AM +0530, MD Danish Anwar wrote:
>>
>>
>> On 18/12/24 10:29 pm, Larysa Zaremba wrote:
>>> On Mon, Dec 16, 2024 at 03:30:41PM +0530, MD Danish Anwar wrote:
>>>> HSR offloading is supported by ICSSG driver. Select the symbol HSR for
>>>> TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
>>>> remove recursive dependency.
>>>>
>>>
>>> 2 things:
>>> 1) The explanation from the cover should have been included in the commit 
>>>    message.
>>
>> I wanted to keep the commit message brief so I provided the actual
>> errors in cover letter. I will add the logs here as well.
>>
> 
> Commit message has to be as verbose as needed to provide enough context for 
> whoever needs to explore the code history later.
>  

Sure I will update the commit message.

>>> 2) Why not `depends on HSR`?
>>
>> Adding `depends on HSR` in `config TI_ICSSG_PRUETH` is not setting HSR.
>> I have tried below scenarios and only one of them work.
>>
>> 1) depends on NET_SWITCHDEV
>>    depends on HSR
>>
>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>> disabled.
> 
> I do not understand your problem with this option, CONFIG_HSR is a visible 
> option that you can enable manually only then you will be able to successfully 
> set CONFIG_TI_ICSSG_PRUETH to m/y, this is how the relation with NET_SWITCHDEV 
> currently works.
> 

The only problem with this option is that when I do `make defconfig`, it
will unset CONFIG_TI_ICSSG_PRUETH.

I will have to manually change the arch/arm64/configs/defconfig to set
HSR=m and then only `make defconfig` will enable CONFIG_TI_ICSSG_PRUETH

Currently HSR is not enabled in defconfig. I will have to send out a
patch to set HSR=m in defconfig

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c62831e61586..ff3e5d960e2a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -129,6 +129,7 @@ CONFIG_MEMORY_FAILURE=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_NET=y
 CONFIG_PACKET=y
+CONFIG_HSR=m
 CONFIG_UNIX=y
 CONFIG_INET=y
 CONFIG_IP_MULTICAST=y


Since I am the only one needing HSR, I thought it would be better if I
select HSR and it only gets built if CONFIG_TI_ICSSG_PRUETH is enabled
instead of always getting built.

For this reason I thought selecting HSR would be good choice, since just
selecting HSR wasn't enough and resulted in recursive dependency,  I had
to change NET_SWITCHDEV also to select.

BTW `NET_DSA` selects `NET_SWITCHDEV` (net/dsa/Kconfig:9)

> Just 'depends on' is still a preferred way for me, as there is not a single 
> driver that does 'select NET_SWITCHDEV'
> 
>>
>> 2) select NET_SWITCHDEV
>>    depends on HSR
>>
>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>> disabled.
>>
>> 3) depends on NET_SWITCHDEV
>>    select HSR
>> 	
>> 	Results in recursive dependency
>>
>> error: recursive dependency detected!
>> 	symbol NET_DSA depends on HSR
>> 	symbol HSR is selected by TI_ICSSG_PRUETH
>> 	symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
>> 	symbol NET_SWITCHDEV is selected by NET_DSA
>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>> subsection "Kconfig recursive dependency limitations"
>>
>> make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
>> make[1]: *** [/home/danish/workspace/net-next/Makefile:733: defconfig]
>> Error 2
>> make: *** [Makefile:251: __sub-make] Error 2
>>
>> 4) select NET_SWITCHDEV
>>    select HSR
>>
>> 	HSR is set as `m` along with `CONFIG_TI_ICSSG_PRUETH`
>>
>> CONFIG_HSR=m
>> CONFIG_NET_SWITCHDEV=y
>> CONFIG_TI_ICSSG_PRUETH=m
>>
>> #4 is the only secnario where HSR gets built. That's why I sent the
>> patch with `select NET_SWITCHDEV` and `select HSR`
>>

I still think 4 is the best option. Only difference here is that we have
to `select NET_SWITCHDEV` as well.

>>>  
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/Kconfig | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>>>> index 0d5a862cd78a..ad366abfa746 100644
>>>> --- a/drivers/net/ethernet/ti/Kconfig
>>>> +++ b/drivers/net/ethernet/ti/Kconfig
>>>> @@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
>>>>  	select PHYLIB
>>>>  	select TI_ICSS_IEP
>>>>  	select TI_K3_CPPI_DESC_POOL
>>>> +	select NET_SWITCHDEV
>>>> +	select HSR
>>>>  	depends on PRU_REMOTEPROC
>>>> -	depends on NET_SWITCHDEV
>>>>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
>>>>  	depends on PTP_1588_CLOCK_OPTIONAL
>>>>  	help
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>
>> -- 
>> Thanks and Regards,
>> Danish

-- 
Thanks and Regards,
Md Danish Anwar