| Message ID | 20241216100044.577489-2-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Multicast Filtering support for VLAN interface | expand |
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. > 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. 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 >> >>
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
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
Hi MD,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 92c932b9946c1e082406aa0515916adb3e662e24]
url: https://github.com/intel-lab-lkp/linux/commits/MD-Danish-Anwar/net-ti-Kconfig-Select-HSR-for-ICSSG-Driver/20241216-180412
base: 92c932b9946c1e082406aa0515916adb3e662e24
patch link: https://lore.kernel.org/r/20241216100044.577489-2-danishanwar%40ti.com
patch subject: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver
config: arm64-kismet-CONFIG_NET_SWITCHDEV-CONFIG_TI_ICSSG_PRUETH-0-0 (https://download.01.org/0day-ci/archive/20241221/202412210336.BmgcX3Td-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20241221/202412210336.BmgcX3Td-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412210336.BmgcX3Td-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for NET_SWITCHDEV when selected by TI_ICSSG_PRUETH
WARNING: unmet direct dependencies detected for NET_SWITCHDEV
Depends on [n]: NET [=y] && INET [=n]
Selected by [y]:
- TI_ICSSG_PRUETH [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_TI [=y] && PRU_REMOTEPROC [=y] && ARCH_K3 [=y] && OF [=y] && TI_K3_UDMA_GLUE_LAYER [=y] && PTP_1588_CLOCK_OPTIONAL [=y]
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 >
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
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. Signed-off-by: MD Danish Anwar <danishanwar@ti.com> --- drivers/net/ethernet/ti/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)