| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45ed487e-7489-4e4f-9e41-ac741ca7882d@ti.com>
Date: Mon, 23 Dec 2024 12:00:58 +0530
From: MD Danish Anwar <danishanwar@...com>
To: "Anwar, Md Danish" <a0501179@...com>,
Larysa Zaremba
<larysa.zaremba@...el.com>
CC: <aleksander.lobakin@...el.com>, <lukma@...x.de>, <m-malladi@...com>,
<diogo.ivo@...mens.com>, <rdunlap@...radead.org>,
<schnelle@...ux.ibm.com>, <vladimir.oltean@....com>,
<horms@...nel.org>, <rogerq@...nel.org>, <pabeni@...hat.com>,
<kuba@...nel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
<andrew+netdev@...n.ch>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, <srk@...com>,
Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG
Driver
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@...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
Powered by blists - more mailing lists