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

[Larysa Zaremba <larysa.zaremba@intel.com>]

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.
 
> > 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.

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`
> 
> >  
> >> 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