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

[MD Danish Anwar <danishanwar@ti.com>]

Hi,

On 20/12/24 11:01 am, Anwar, Md Danish wrote:
> 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.
> 

I see there is some issue seen
https://lore.kernel.org/all/202412210336.BmgcX3Td-lkp@intel.com/ with
this patch.

For now I'll drop this patch and send out a separate patch to defconfig
to set HSR=m.

Once the defconfig patch gets merged, I will add `depends on HSR` in
Kconfig for TI_ICSSG_PRUETH (the method suggested by you)

Thanks for the review.

>>>>  
>>>>> 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,
Danish