Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Krzysztof Kozlowski]

On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> From: Boon Khai Ng <boon.khai.ng@intel.com>
> 
> This patch is to add the dts setting for the MAC controller on
> synopsys 10G Ethernet MAC which allow the 10G MAC to turn on
> hardware accelerated VLAN stripping. Once the hardware accelerated
> VLAN stripping is turn on, the VLAN tag will be stripped by the

Subject prefix is totally bogus.


> 10G Ethernet MAC.
> 
> Signed-off-by: Boon Khai Ng <boon.khai.ng@intel.com>
> Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by
automated tooling. Performing review on untested code might be a waste
of time, thus I will skip this patch entirely till you follow the
process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Krzysztof Kozlowski]

On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> From: Boon Khai Ng <boon.khai.ng@intel.com>
> 
> Currently, VLAN tag stripping is done by software driver in
> stmmac_rx_vlan(). This patch is to Add support for VLAN tag
> stripping by the MAC hardware and MAC drivers to support it.
> This is done by adding rx_hw_vlan() and set_hw_vlan_mode()
> callbacks at stmmac_ops struct which are called from upper
> software layer.
...

>  	if (priv->dma_cap.vlhash) {
>  		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>  		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 23d53ea04b24..bd7f3326a44c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  			plat->flags |= STMMAC_FLAG_TSO_EN;
>  	}
>  
> +	/* Rx VLAN HW Stripping */
> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");

Why? Drop.



Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Shevchenko Andriy]

On Fri, Jul 21, 2023 at 02:26:16PM +0800, Boon@ecsmtp.png.intel.com wrote:
> From: Boon Khai Ng <boon.khai.ng@intel.com>
> 
> This patch is to add the dts setting for the MAC controller on
> synopsys 10G Ethernet MAC which allow the 10G MAC to turn on
> hardware accelerated VLAN stripping. Once the hardware accelerated
> VLAN stripping is turn on, the VLAN tag will be stripped by the
> 10G Ethernet MAC.

...

> Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>

This is wrong:
- I never reviewed DT bindings in all your series.
- My name for the patches is also wrong.

P.S. What I mentioned in the internal mail is that you can add my tag to
    the code, and not to the DT. Sorry, I probably hadn't been clear.

-- 
With Best Regards,
Andy Shevchenko

RE: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Ng, Boon Khai]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, July 21, 2023 6:11 PM
> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
> <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; Tham, Mun Yew <mun.yew.tham@intel.com>;
> Swee, Leong Ching <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings:
> net: snps,dwmac: Add description for rx-vlan-offload
> 
> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> > From: Boon Khai Ng <boon.khai.ng@intel.com>
> >
> > This patch is to add the dts setting for the MAC controller on
> > synopsys 10G Ethernet MAC which allow the 10G MAC to turn on hardware
> > accelerated VLAN stripping. Once the hardware accelerated VLAN
> > stripping is turn on, the VLAN tag will be stripped by the
> 
> Subject prefix is totally bogus.
> 

Which part? It's a 10G Ethernet IP from Sysnopsys, in Roman character it is X (mean 10), so XGMAC.
Even the driver file I'm editing it is dw"xgmac".

> 
> > 10G Ethernet MAC.
> >
> > Signed-off-by: Boon Khai Ng <boon.khai.ng@intel.com>
> > Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
> to CC. It might happen, that command when run on an older kernel, gives you
> outdated entries. Therefore please be sure you base your patches on recent
> Linux kernel.
> 

This is based on net-next repository suggested by the get maintainer script.

I got the latest net-next just now at the Commit-id b44693495af8 
which just committed yesterday.

$ ./scripts/get_maintainer.pl  --scm  -f drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
Giuseppe Cavallaro <peppe.cavallaro@st.com> (supporter:STMMAC ETHERNET DRIVER)
Alexandre Torgue <alexandre.torgue@foss.st.com> (supporter:STMMAC ETHERNET DRIVER)
Jose Abreu <joabreu@synopsys.com> (supporter:STMMAC ETHERNET DRIVER)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
Maxime Coquelin <mcoquelin.stm32@gmail.com> (maintainer:ARM/STM32 ARCHITECTURE)
Richard Cochran <richardcochran@gmail.com> (maintainer:PTP HARDWARE CLOCK SUPPORT)
netdev@vger.kernel.org (open list:STMMAC ETHERNET DRIVER)
linux-stm32@st-md-mailman.stormreply.com (moderated list:ARM/STM32 ARCHITECTURE)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/STM32 ARCHITECTURE)
linux-kernel@vger.kernel.org (open list)
git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
git git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

> You missed at least DT list (maybe more), so this won't be tested by automated
> tooling. Performing review on untested code might be a waste of time, thus I
> will skip this patch entirely till you follow the process allowing the patch to be
> tested.
> 

This is a new device bringup, thus the DT is not available yet. The DTS will be upstreamed
by my another colleague, unless, if I can upstream only my part on the setting? 

> Please kindly resend and include all necessary To/Cc entries.
> 
> Best regards,
> Krzysztof

RE: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, July 21, 2023 6:11 PM
> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
> <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> 
> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> > From: Boon Khai Ng <boon.khai.ng@intel.com>
> >
> > Currently, VLAN tag stripping is done by software driver in
> > stmmac_rx_vlan(). This patch is to Add support for VLAN tag stripping
> > by the MAC hardware and MAC drivers to support it.
> > This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks
> > at stmmac_ops struct which are called from upper software layer.
> ...
> 
> >  	if (priv->dma_cap.vlhash) {
> >  		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> >  		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff --
> git
> > a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 23d53ea04b24..bd7f3326a44c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device
> *pdev, u8 *mac)
> >  			plat->flags |= STMMAC_FLAG_TSO_EN;
> >  	}
> >
> > +	/* Rx VLAN HW Stripping */
> > +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
> > +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
> 
> Why? Drop.
> 

This is an dts option export to dts for user to choose whether or not they 
Want a Hardware stripping or a software stripping. 

May I know what is the reason to drop this?

> 
> 
> Best regards,
> Krzysztof

RE: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Ng, Boon Khai]

> -----Original Message-----
> From: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> Sent: Friday, July 21, 2023 6:18 PM
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Ng,
> Boon Khai <boon.khai.ng@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-
> bindings: net: snps,dwmac: Add description for rx-vlan-offload
> 
> On Fri, Jul 21, 2023 at 02:26:16PM +0800, Boon@ecsmtp.png.intel.com wrote:
> > From: Boon Khai Ng <boon.khai.ng@intel.com>
> >
> > This patch is to add the dts setting for the MAC controller on
> > synopsys 10G Ethernet MAC which allow the 10G MAC to turn on hardware
> > accelerated VLAN stripping. Once the hardware accelerated VLAN
> > stripping is turn on, the VLAN tag will be stripped by the 10G
> > Ethernet MAC.
> 
> ...
> 
> > Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
> 
> This is wrong:
> - I never reviewed DT bindings in all your series.
> - My name for the patches is also wrong.
> 
> P.S. What I mentioned in the internal mail is that you can add my tag to
>     the code, and not to the DT. Sorry, I probably hadn't been clear.
> 

My bad, sorry for interpreting the meaning wrongly, I will remove all the 
"Reviewed-by" stamp from all the DT patches on the next update.

However I copied the Reviewed-by: from the previous email, your name 
Shouldn't be wrong. 

> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Shevchenko, Andriy]

On Fri, Jul 21, 2023 at 06:35:44PM +0300, Ng, Boon Khai wrote:
> > From: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> > Sent: Friday, July 21, 2023 6:18 PM
> > On Fri, Jul 21, 2023 at 02:26:16PM +0800, Boon@ecsmtp.png.intel.com wrote:
> > > From: Boon Khai Ng <boon.khai.ng@intel.com>

...

> > > Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
> > 
> > This is wrong:
> > - I never reviewed DT bindings in all your series.
> > - My name for the patches is also wrong.
> > 
> > P.S. What I mentioned in the internal mail is that you can add my tag to
> >     the code, and not to the DT. Sorry, I probably hadn't been clear.
> 
> My bad, sorry for interpreting the meaning wrongly, I will remove all the 
> "Reviewed-by" stamp from all the DT patches on the next update.
> 
> However I copied the Reviewed-by: from the previous email, your name
> Shouldn't be wrong.

Oh, this is a bit messy. The address for the kernel work should be
Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

RE: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Ng, Boon Khai]

> -----Original Message-----
> From: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> Sent: Friday, July 21, 2023 11:49 PM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> David S . Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Tham,
> Mun Yew <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-
> bindings: net: snps,dwmac: Add description for rx-vlan-offload
> 
> On Fri, Jul 21, 2023 at 06:35:44PM +0300, Ng, Boon Khai wrote:
> > > From: Shevchenko, Andriy <andriy.shevchenko@intel.com>
> > > Sent: Friday, July 21, 2023 6:18 PM
> > > On Fri, Jul 21, 2023 at 02:26:16PM +0800, Boon@ecsmtp.png.intel.com
> wrote:
> > > > From: Boon Khai Ng <boon.khai.ng@intel.com>
> 
> ...
> 
> > > > Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
> > >
> > > This is wrong:
> > > - I never reviewed DT bindings in all your series.
> > > - My name for the patches is also wrong.
> > >
> > > P.S. What I mentioned in the internal mail is that you can add my tag to
> > >     the code, and not to the DT. Sorry, I probably hadn't been clear.
> >
> > My bad, sorry for interpreting the meaning wrongly, I will remove all
> > the "Reviewed-by" stamp from all the DT patches on the next update.
> >
> > However I copied the Reviewed-by: from the previous email, your name
> > Shouldn't be wrong.
> 
> Oh, this is a bit messy. The address for the kernel work should be Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>
> 

Ah okay, got it. Will update that in the next patch.

> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Florian Fainelli]

On 7/21/2023 8:30 AM, Ng, Boon Khai wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Friday, July 21, 2023 6:11 PM
>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
>> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
>> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
>> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org
>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
>> <andriy.shevchenko@intel.com>; Tham, Mun Yew
>> <mun.yew.tham@intel.com>; Swee, Leong Ching
>> <leong.ching.swee@intel.com>; G Thomas, Rohan
>> <rohan.g.thomas@intel.com>; Shevchenko Andriy
>> <andriy.shevchenko@linux.intel.com>
>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
>>
>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
>>> From: Boon Khai Ng <boon.khai.ng@intel.com>
>>>
>>> Currently, VLAN tag stripping is done by software driver in
>>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag stripping
>>> by the MAC hardware and MAC drivers to support it.
>>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks
>>> at stmmac_ops struct which are called from upper software layer.
>> ...
>>
>>>   	if (priv->dma_cap.vlhash) {
>>>   		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>>>   		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff --
>> git
>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> index 23d53ea04b24..bd7f3326a44c 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device
>> *pdev, u8 *mac)
>>>   			plat->flags |= STMMAC_FLAG_TSO_EN;
>>>   	}
>>>
>>> +	/* Rx VLAN HW Stripping */
>>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
>>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
>>
>> Why? Drop.
>>
> 
> This is an dts option export to dts for user to choose whether or not they
> Want a Hardware stripping or a software stripping.
> 
> May I know what is the reason to drop this?

Because the networking stack already exposes knobs for drivers to 
advertise and control VLAN stripping/insertion on RX/TX using ethtool 
and feature bits (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX).

What you are doing here is encode a policy as a Device Tree property 
rather than describe whether the hardware supports a given feature and 
this is frowned upon.
-- 
Florian

RE: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Friday, July 21, 2023 11:59 PM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski
> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com;
> Ng, Boon Khai <boon.khai.ng@intel.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
> Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> 
> 
> 
> On 7/21/2023 8:30 AM, Ng, Boon Khai wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Friday, July 21, 2023 6:11 PM
> >> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
> >> <boon.khai.ng"@intel.com; Giuseppe Cavallaro
> >> <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> David S . Miller <davem@davemloft.net>; Eric Dumazet
> >> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> >> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> >> netdev@vger.kernel.org; linux-stm32@st- md-mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org
> >> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
> >> <andriy.shevchenko@intel.com>; Tham, Mun Yew
> >> <mun.yew.tham@intel.com>; Swee, Leong Ching
> >> <leong.ching.swee@intel.com>; G Thomas, Rohan
> >> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> >> <andriy.shevchenko@linux.intel.com>
> >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
> >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> >>
> >> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> >>> From: Boon Khai Ng <boon.khai.ng@intel.com>
> >>>
> >>> Currently, VLAN tag stripping is done by software driver in
> >>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag
> >>> stripping by the MAC hardware and MAC drivers to support it.
> >>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks
> >>> at stmmac_ops struct which are called from upper software layer.
> >> ...
> >>
> >>>   	if (priv->dma_cap.vlhash) {
> >>>   		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> >>>   		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff --
> >> git
> >>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> index 23d53ea04b24..bd7f3326a44c 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device
> >> *pdev, u8 *mac)
> >>>   			plat->flags |= STMMAC_FLAG_TSO_EN;
> >>>   	}
> >>>
> >>> +	/* Rx VLAN HW Stripping */
> >>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
> >>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
> >>
> >> Why? Drop.
> >>
> >
> > This is an dts option export to dts for user to choose whether or not
> > they Want a Hardware stripping or a software stripping.
> >
> > May I know what is the reason to drop this?
> 
> Because the networking stack already exposes knobs for drivers to advertise and
> control VLAN stripping/insertion on RX/TX using ethtool and feature bits
> (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX).
> 

Hi Florian,

Understood, but how does user choose to have the default option
either hardware strip or software strip, when the device just boot up?

I don’t think ethool can "remember" the setting once the device get rebooted?
Any other suggestion of doing it other than using the dts method?

> What you are doing here is encode a policy as a Device Tree property rather
> than describe whether the hardware supports a given feature and this is frowned
> upon.
> --
> Florian

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Krzysztof Kozlowski]

On 21/07/2023 17:28, Ng, Boon Khai wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Friday, July 21, 2023 6:11 PM
>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
>> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
>> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
>> Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
>> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org
>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
>> <andriy.shevchenko@intel.com>; Tham, Mun Yew <mun.yew.tham@intel.com>;
>> Swee, Leong Ching <leong.ching.swee@intel.com>; G Thomas, Rohan
>> <rohan.g.thomas@intel.com>; Shevchenko Andriy
>> <andriy.shevchenko@linux.intel.com>
>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings:
>> net: snps,dwmac: Add description for rx-vlan-offload
>>
>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
>>> From: Boon Khai Ng <boon.khai.ng@intel.com>
>>>
>>> This patch is to add the dts setting for the MAC controller on
>>> synopsys 10G Ethernet MAC which allow the 10G MAC to turn on hardware
>>> accelerated VLAN stripping. Once the hardware accelerated VLAN
>>> stripping is turn on, the VLAN tag will be stripped by the
>>
>> Subject prefix is totally bogus.
>>
> 
> Which part? It's a 10G Ethernet IP from Sysnopsys, in Roman character it is X (mean 10), so XGMAC.
> Even the driver file I'm editing it is dw"xgmac".

Everything in [].

> 
>>
>>> 10G Ethernet MAC.
>>>
>>> Signed-off-by: Boon Khai Ng <boon.khai.ng@intel.com>
>>> Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
>> to CC. It might happen, that command when run on an older kernel, gives you
>> outdated entries. Therefore please be sure you base your patches on recent
>> Linux kernel.
>>
> 
> This is based on net-next repository suggested by the get maintainer script.
> 
> I got the latest net-next just now at the Commit-id b44693495af8 
> which just committed yesterday.
> 
> $ ./scripts/get_maintainer.pl  --scm  -f drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c

That's not how you run it. get_maintainers.pl should be run on patches
or on all files, not just some selection.

> Giuseppe Cavallaro <peppe.cavallaro@st.com> (supporter:STMMAC ETHERNET DRIVER)
> Alexandre Torgue <alexandre.torgue@foss.st.com> (supporter:STMMAC ETHERNET DRIVER)
> Jose Abreu <joabreu@synopsys.com> (supporter:STMMAC ETHERNET DRIVER)
> "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
> Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
> Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
> Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
> Maxime Coquelin <mcoquelin.stm32@gmail.com> (maintainer:ARM/STM32 ARCHITECTURE)
> Richard Cochran <richardcochran@gmail.com> (maintainer:PTP HARDWARE CLOCK SUPPORT)
> netdev@vger.kernel.org (open list:STMMAC ETHERNET DRIVER)
> linux-stm32@st-md-mailman.stormreply.com (moderated list:ARM/STM32 ARCHITECTURE)
> linux-arm-kernel@lists.infradead.org (moderated list:ARM/STM32 ARCHITECTURE)
> linux-kernel@vger.kernel.org (open list)
> git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
> git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
> git git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 
>> You missed at least DT list (maybe more), so this won't be tested by automated
>> tooling. Performing review on untested code might be a waste of time, thus I
>> will skip this patch entirely till you follow the process allowing the patch to be
>> tested.
>>
> 
> This is a new device bringup, thus the DT is not available yet. The DTS will be upstreamed
> by my another colleague, unless, if I can upstream only my part on the setting? 

You are mixing now DTS and DT bindings. Sorry, we do not talk about DTS.

Follow our process of submitting patches. For sure there are folks in
Intel which can explain it to you.


Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Krzysztof Kozlowski]

On 21/07/2023 17:30, Ng, Boon Khai wrote:
>> git
>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> index 23d53ea04b24..bd7f3326a44c 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device
>> *pdev, u8 *mac)
>>>  			plat->flags |= STMMAC_FLAG_TSO_EN;
>>>  	}
>>>
>>> +	/* Rx VLAN HW Stripping */
>>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
>>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
>>
>> Why? Drop.
>>
> 
> This is an dts option export to dts for user to choose whether or not they 
> Want a Hardware stripping or a software stripping. 
> 
> May I know what is the reason to drop this?

Because we do not print simple confirmation of DT properties parsing.
It's usually useless and obvious from DT.

To be clear - we talk about dev_info.

Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Krzysztof Kozlowski]

On 21/07/2023 17:59, Florian Fainelli wrote:
>>>> +	/* Rx VLAN HW Stripping */
>>>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
>>>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
>>>
>>> Why? Drop.
>>>
>>
>> This is an dts option export to dts for user to choose whether or not they
>> Want a Hardware stripping or a software stripping.
>>
>> May I know what is the reason to drop this?
> 
> Because the networking stack already exposes knobs for drivers to 
> advertise and control VLAN stripping/insertion on RX/TX using ethtool 
> and feature bits (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX).
> 
> What you are doing here is encode a policy as a Device Tree property 
> rather than describe whether the hardware supports a given feature and 
> this is frowned upon.

That's even better reason...

Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Krzysztof Kozlowski]

On 21/07/2023 17:28, Ng, Boon Khai wrote:
> This is a new device bringup, thus the DT is not available yet. The DTS will be upstreamed
> by my another colleague, unless, if I can upstream only my part on the setting? 
> 
>> Please kindly resend and include all necessary To/Cc entries.

To be clear, since you do not agree with my comment you skipped vital
lists, this was not tested by automation so it is NAK from me.

Sorry.

Best regards,
Krzysztof

Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Florian Fainelli]

On 7/21/2023 9:12 AM, Ng, Boon Khai wrote:
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: Friday, July 21, 2023 11:59 PM
>> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski
>> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com;
>> Ng, Boon Khai <boon.khai.ng@intel.com>; Giuseppe Cavallaro
>> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
>> Jose Abreu <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>;
>> Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
>> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org
>> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew
>> <mun.yew.tham@intel.com>; Swee, Leong Ching
>> <leong.ching.swee@intel.com>; G Thomas, Rohan
>> <rohan.g.thomas@intel.com>; Shevchenko Andriy
>> <andriy.shevchenko@linux.intel.com>
>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
>>
>>
>>
>> On 7/21/2023 8:30 AM, Ng, Boon Khai wrote:
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzk@kernel.org>
>>>> Sent: Friday, July 21, 2023 6:11 PM
>>>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
>>>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro
>>>> <peppe.cavallaro@st.com>; Alexandre Torgue
>>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>>>> David S . Miller <davem@davemloft.net>; Eric Dumazet
>>>> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
>>>> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
>>>> netdev@vger.kernel.org; linux-stm32@st- md-mailman.stormreply.com;
>>>> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org
>>>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
>>>> <andriy.shevchenko@intel.com>; Tham, Mun Yew
>>>> <mun.yew.tham@intel.com>; Swee, Leong Ching
>>>> <leong.ching.swee@intel.com>; G Thomas, Rohan
>>>> <rohan.g.thomas@intel.com>; Shevchenko Andriy
>>>> <andriy.shevchenko@linux.intel.com>
>>>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
>>>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
>>>>
>>>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
>>>>> From: Boon Khai Ng <boon.khai.ng@intel.com>
>>>>>
>>>>> Currently, VLAN tag stripping is done by software driver in
>>>>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag
>>>>> stripping by the MAC hardware and MAC drivers to support it.
>>>>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode() callbacks
>>>>> at stmmac_ops struct which are called from upper software layer.
>>>> ...
>>>>
>>>>>    	if (priv->dma_cap.vlhash) {
>>>>>    		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>>>>>    		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER; diff --
>>>> git
>>>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>>> index 23d53ea04b24..bd7f3326a44c 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct platform_device
>>>> *pdev, u8 *mac)
>>>>>    			plat->flags |= STMMAC_FLAG_TSO_EN;
>>>>>    	}
>>>>>
>>>>> +	/* Rx VLAN HW Stripping */
>>>>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
>>>>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
>>>>
>>>> Why? Drop.
>>>>
>>>
>>> This is an dts option export to dts for user to choose whether or not
>>> they Want a Hardware stripping or a software stripping.
>>>
>>> May I know what is the reason to drop this?
>>
>> Because the networking stack already exposes knobs for drivers to advertise and
>> control VLAN stripping/insertion on RX/TX using ethtool and feature bits
>> (NETIF_F_HW_VLAN_CTAG_RX, NETIF_F_HW_VLAN_CTAG_TX).
>>
> 
> Hi Florian,
> 
> Understood, but how does user choose to have the default option
> either hardware strip or software strip, when the device just boot up?

You need the hardware to advertise it and decide as a maintainer of that 
driver whether it makes sense to have one or the other behavior by default.

> 
> I don’t think ethool can "remember" the setting once the device get rebooted?

If by "device" you mean a system that incorporates a XGMAC core, then I 
suppose that is true, though you could have some user-space logic that 
does remember the various ethtool options and re-applies them as soon as 
the device is made available to user-space, this would not be too far 
fetched.

> Any other suggestion of doing it other than using the dts method?

Let me ask you this question: what are you trying to solve by making 
this configurable? HW stripping should always be more efficient, should 
not it, if so, what would be the reasons for not enabling that by 
default? If not, then leave it off and let users enable it if they feel 
like they want it.
-- 
Florian

RE: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Ng, Boon Khai]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Saturday, July 22, 2023 12:22 AM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Boon@ecsmtp.png.intel.com;
> Khai@ecsmtp.png.intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-
> bindings: net: snps,dwmac: Add description for rx-vlan-offload
> 
> On 21/07/2023 17:28, Ng, Boon Khai wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Friday, July 21, 2023 6:11 PM
> >> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
> >> <boon.khai.ng"@intel.com; Giuseppe Cavallaro
> >> <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> David S . Miller <davem@davemloft.net>; Eric Dumazet
> >> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni
> >> <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>;
> >> netdev@vger.kernel.org; linux-stm32@st-md- mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org
> >> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
> >> <andriy.shevchenko@intel.com>; Tham, Mun Yew
> >> <mun.yew.tham@intel.com>; Swee, Leong Ching
> >> <leong.ching.swee@intel.com>; G Thomas, Rohan
> >> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> >> <andriy.shevchenko@linux.intel.com>
> >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-
> bindings:
> >> net: snps,dwmac: Add description for rx-vlan-offload
> >>
> >> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> >>> From: Boon Khai Ng <boon.khai.ng@intel.com>
> >>>
> >>> This patch is to add the dts setting for the MAC controller on
> >>> synopsys 10G Ethernet MAC which allow the 10G MAC to turn on
> >>> hardware accelerated VLAN stripping. Once the hardware accelerated
> >>> VLAN stripping is turn on, the VLAN tag will be stripped by the
> >>
> >> Subject prefix is totally bogus.
> >>
> >
> > Which part? It's a 10G Ethernet IP from Sysnopsys, in Roman character it is
> X (mean 10), so XGMAC.
> > Even the driver file I'm editing it is dw"xgmac".
> 
> Everything in [].
> 
> >
> >>
> >>> 10G Ethernet MAC.
> >>>
> >>> Signed-off-by: Boon Khai Ng <boon.khai.ng@intel.com>
> >>> Reviewed-by: Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
> >>
> >> Please use scripts/get_maintainers.pl to get a list of necessary
> >> people and lists to CC. It might happen, that command when run on an
> >> older kernel, gives you outdated entries. Therefore please be sure
> >> you base your patches on recent Linux kernel.
> >>
> >
> > This is based on net-next repository suggested by the get maintainer script.
> >
> > I got the latest net-next just now at the Commit-id b44693495af8 which
> > just committed yesterday.
> >
> > $ ./scripts/get_maintainer.pl  --scm  -f
> > drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> 
> That's not how you run it. get_maintainers.pl should be run on patches or on
> all files, not just some selection.
> 
> > Giuseppe Cavallaro <peppe.cavallaro@st.com> (supporter:STMMAC
> ETHERNET
> > DRIVER) Alexandre Torgue <alexandre.torgue@foss.st.com>
> > (supporter:STMMAC ETHERNET DRIVER) Jose Abreu
> <joabreu@synopsys.com>
> > (supporter:STMMAC ETHERNET DRIVER) "David S. Miller"
> > <davem@davemloft.net> (maintainer:NETWORKING DRIVERS) Eric
> Dumazet
> > <edumazet@google.com> (maintainer:NETWORKING DRIVERS) Jakub
> Kicinski
> > <kuba@kernel.org> (maintainer:NETWORKING DRIVERS) Paolo Abeni
> > <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS) Maxime
> Coquelin
> > <mcoquelin.stm32@gmail.com> (maintainer:ARM/STM32 ARCHITECTURE)
> > Richard Cochran <richardcochran@gmail.com> (maintainer:PTP HARDWARE
> > CLOCK SUPPORT) netdev@vger.kernel.org (open list:STMMAC ETHERNET
> > DRIVER) linux-stm32@st-md-mailman.stormreply.com (moderated
> > list:ARM/STM32 ARCHITECTURE) linux-arm-kernel@lists.infradead.org
> > (moderated list:ARM/STM32 ARCHITECTURE) linux-kernel@vger.kernel.org
> > (open list) git
> > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
> > git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> > git git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git
> > stm32-next git
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >
> >> You missed at least DT list (maybe more), so this won't be tested by
> >> automated tooling. Performing review on untested code might be a
> >> waste of time, thus I will skip this patch entirely till you follow
> >> the process allowing the patch to be tested.
> >>
> >
> > This is a new device bringup, thus the DT is not available yet. The
> > DTS will be upstreamed by my another colleague, unless, if I can upstream
> only my part on the setting?
> 
> You are mixing now DTS and DT bindings. Sorry, we do not talk about DTS.
> 
> Follow our process of submitting patches. For sure there are folks in Intel
> which can explain it to you.
> 

Ah see, so you were saying I'm missing the cc list of the maintainer related to the
DT binding. 

> 
> Best regards,
> Krzysztof

RE: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Ng, Boon Khai]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Saturday, July 22, 2023 12:26 AM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Boon@ecsmtp.png.intel.com;
> Khai@ecsmtp.png.intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-
> bindings: net: snps,dwmac: Add description for rx-vlan-offload
> 
> On 21/07/2023 17:28, Ng, Boon Khai wrote:
> > This is a new device bringup, thus the DT is not available yet. The
> > DTS will be upstreamed by my another colleague, unless, if I can upstream
> only my part on the setting?
> >
> >> Please kindly resend and include all necessary To/Cc entries.
> 
> To be clear, since you do not agree with my comment you skipped vital lists,
> this was not tested by automation so it is NAK from me.
> 
> Sorry.
> 

I understand that I already get a NAK at the beginning. But I don’t understand why,
Please don’t get me wrong, I'm not disagreeing your comments, was trying to understand
the reason behind and also which are the step that I made a mistake(s) on,  this is to help
me to learn at the same time to smoothen the upstreaming process.

> Best regards,
> Krzysztof

RE: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

[Ng, Boon Khai]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Saturday, July 22, 2023 12:30 AM
> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski
> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com;
> Khai@ecsmtp.png.intel.com; Giuseppe Cavallaro <peppe.cavallaro@st.com>;
> Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun Yew
> <mun.yew.tham@intel.com>; Swee, Leong Ching
> <leong.ching.swee@intel.com>; G Thomas, Rohan
> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> <andriy.shevchenko@linux.intel.com>
> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> 
> 
> 
> On 7/21/2023 9:12 AM, Ng, Boon Khai wrote:
> >> -----Original Message-----
> >> From: Florian Fainelli <f.fainelli@gmail.com>
> >> Sent: Friday, July 21, 2023 11:59 PM
> >> To: Ng, Boon Khai <boon.khai.ng@intel.com>; Krzysztof Kozlowski
> >> <krzk@kernel.org>; Boon@ecsmtp.png.intel.com;
> >> Khai@ecsmtp.png.intel.com; Ng, Boon Khai <boon.khai.ng@intel.com>;
> >> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> David S . Miller <davem@davemloft.net>; Eric Dumazet
> >> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni
> >> <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>;
> >> netdev@vger.kernel.org; linux-stm32@st-md- mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org
> >> Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Tham, Mun
> Yew
> >> <mun.yew.tham@intel.com>; Swee, Leong Ching
> >> <leong.ching.swee@intel.com>; G Thomas, Rohan
> >> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> >> <andriy.shevchenko@linux.intel.com>
> >> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net:
> >> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> >>
> >>
> >>
> >> On 7/21/2023 8:30 AM, Ng, Boon Khai wrote:
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzk@kernel.org>
> >>>> Sent: Friday, July 21, 2023 6:11 PM
> >>>> To: Boon@ecsmtp.png.intel.com; Khai@ecsmtp.png.intel.com; "Ng
> >>>> <boon.khai.ng"@intel.com; Giuseppe Cavallaro
> >>>> <peppe.cavallaro@st.com>; Alexandre Torgue
> >>>> <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>;
> >>>> David S . Miller <davem@davemloft.net>; Eric Dumazet
> >>>> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> >>>> Abeni <pabeni@redhat.com>; Maxime Coquelin
> >>>> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org;
> >>>> linux-stm32@st- md-mailman.stormreply.com;
> >>>> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org
> >>>> Cc: Ng, Boon Khai <boon.khai.ng@intel.com>; Shevchenko, Andriy
> >>>> <andriy.shevchenko@intel.com>; Tham, Mun Yew
> >>>> <mun.yew.tham@intel.com>; Swee, Leong Ching
> >>>> <leong.ching.swee@intel.com>; G Thomas, Rohan
> >>>> <rohan.g.thomas@intel.com>; Shevchenko Andriy
> >>>> <andriy.shevchenko@linux.intel.com>
> >>>> Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature 2/2]
> net:
> >>>> stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> >>>>
> >>>> On 21/07/2023 08:26, Boon@ecsmtp.png.intel.com wrote:
> >>>>> From: Boon Khai Ng <boon.khai.ng@intel.com>
> >>>>>
> >>>>> Currently, VLAN tag stripping is done by software driver in
> >>>>> stmmac_rx_vlan(). This patch is to Add support for VLAN tag
> >>>>> stripping by the MAC hardware and MAC drivers to support it.
> >>>>> This is done by adding rx_hw_vlan() and set_hw_vlan_mode()
> >>>>> callbacks at stmmac_ops struct which are called from upper software
> layer.
> >>>> ...
> >>>>
> >>>>>    	if (priv->dma_cap.vlhash) {
> >>>>>    		ndev->features |=
> NETIF_F_HW_VLAN_CTAG_FILTER;
> >>>>>    		ndev->features |=
> NETIF_F_HW_VLAN_STAG_FILTER; diff --
> >>>> git
> >>>>> a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>>>> index 23d53ea04b24..bd7f3326a44c 100644
> >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>>>> @@ -543,6 +543,12 @@ stmmac_probe_config_dt(struct
> platform_device
> >>>> *pdev, u8 *mac)
> >>>>>    			plat->flags |= STMMAC_FLAG_TSO_EN;
> >>>>>    	}
> >>>>>
> >>>>> +	/* Rx VLAN HW Stripping */
> >>>>> +	if (of_property_read_bool(np, "snps,rx-vlan-offload")) {
> >>>>> +		dev_info(&pdev->dev, "RX VLAN HW Stripping\n");
> >>>>
> >>>> Why? Drop.
> >>>>
> >>>
> >>> This is an dts option export to dts for user to choose whether or
> >>> not they Want a Hardware stripping or a software stripping.
> >>>
> >>> May I know what is the reason to drop this?
> >>
> >> Because the networking stack already exposes knobs for drivers to
> >> advertise and control VLAN stripping/insertion on RX/TX using ethtool
> >> and feature bits (NETIF_F_HW_VLAN_CTAG_RX,
> NETIF_F_HW_VLAN_CTAG_TX).
> >>
> >
> > Hi Florian,
> >
> > Understood, but how does user choose to have the default option either
> > hardware strip or software strip, when the device just boot up?
> 
> You need the hardware to advertise it and decide as a maintainer of that
> driver whether it makes sense to have one or the other behavior by default.
> 

Okay got it.

> >
> > I don’t think ethool can "remember" the setting once the device get
> rebooted?
> 
> If by "device" you mean a system that incorporates a XGMAC core, then I
> suppose that is true, though you could have some user-space logic that does
> remember the various ethtool options and re-applies them as soon as the
> device is made available to user-space, this would not be too far fetched.
> 

Okay, will try to search that, is adding the ethool command turning the
Hw vlan striping at the startup script consider one way of doing it?

> > Any other suggestion of doing it other than using the dts method?
> 
> Let me ask you this question: what are you trying to solve by making this
> configurable? HW stripping should always be more efficient, should not it, if
> so, what would be the reasons for not enabling that by default? If not, then
> leave it off and let users enable it if they feel like they want it.

Okay, so seem like it is solely depends on my side whether or not to turn it on by default,
Either way, if it go against the user will to have it on/off by default, they will need to write 
A startup script to turn the ethool on/off?

> --
> Florian

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Jakub Kicinski]

On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote:
> > $ ./scripts/get_maintainer.pl  --scm  -f drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c  
> 
> That's not how you run it. get_maintainers.pl should be run on patches
> or on all files, not just some selection.

Adding Joe for visibility (I proposed to print a warning when people 
do this and IIRC he wasn't on board).

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Joe Perches]

On Fri, 2023-07-21 at 18:55 -0700, Jakub Kicinski wrote:
> On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote:
> > > $ ./scripts/get_maintainer.pl  --scm  -f drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c  
> > 
> > That's not how you run it. get_maintainers.pl should be run on patches
> > or on all files, not just some selection.
> 
> Adding Joe for visibility (I proposed to print a warning when people 
> do this and IIRC he wasn't on board).

What's the issue here?  Other than _how_ the script was used,
I don't see an actual problem with the script itself.

https://lore.kernel.org/lkml/20230721062617.9810-1-boon.khai.ng@intel.com/

As far as I can tell, the patch series address list was identical
for the 0/2, 1/2, and 2/2 submissions:

--------------------------------

0/2:

   From: Boon@ecsmtp.png.intel.com, Khai@ecsmtp.png.intel.com, "Ng <boon.khai.ng"@intel.com
   To: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
   	Alexandre Torgue <alexandre.torgue@foss.st.com>,
   	Jose Abreu <joabreu@synopsys.com>,
   	"David S . Miller" <davem@davemloft.net>,
   	Eric Dumazet <edumazet@google.com>,
   	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
   	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
   	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
   	linux-arm-kernel@lists.infradead.org,
   	linux-kernel@vger.kernel.org
   Cc: Boon Khai Ng <boon.khai.ng@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@intel.com>,
   	Mun Yew Tham <mun.yew.tham@intel.com>,
   	Leong Ching Swee <leong.ching.swee@intel.com>,
   	G Thomas Rohan <rohan.g.thomas@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
   Subject: [Enable Designware XGMAC VLAN Stripping Feature 0/2]
   
1/2:

   From: Boon@ecsmtp.png.intel.com, Khai@ecsmtp.png.intel.com, "Ng <boon.khai.ng"@intel.com
   To: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
   	Alexandre Torgue <alexandre.torgue@foss.st.com>,
   	Jose Abreu <joabreu@synopsys.com>,
   	"David S . Miller" <davem@davemloft.net>,
   	Eric Dumazet <edumazet@google.com>,
   	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
   	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
   	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
   	linux-arm-kernel@lists.infradead.org,
   	linux-kernel@vger.kernel.org
   Cc: Boon Khai Ng <boon.khai.ng@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@intel.com>,
   	Mun Yew Tham <mun.yew.tham@intel.com>,
   	Leong Ching Swee <leong.ching.swee@intel.com>,
   	G Thomas Rohan <rohan.g.thomas@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
   Subject: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload
   
2/2:

   From: Boon@ecsmtp.png.intel.com, Khai@ecsmtp.png.intel.com, "Ng <boon.khai.ng"@intel.com
   To: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
   	Alexandre Torgue <alexandre.torgue@foss.st.com>,
   	Jose Abreu <joabreu@synopsys.com>,
   	"David S . Miller" <davem@davemloft.net>,
   	Eric Dumazet <edumazet@google.com>,
   	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
   	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
   	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
   	linux-arm-kernel@lists.infradead.org,
   	linux-kernel@vger.kernel.org
   Cc: Boon Khai Ng <boon.khai.ng@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@intel.com>,
   	Mun Yew Tham <mun.yew.tham@intel.com>,
   	Leong Ching Swee <leong.ching.swee@intel.com>,
   	G Thomas Rohan <rohan.g.thomas@intel.com>,
   	Shevchenko Andriy <andriy.shevchenko@linux.intel.com>
   Subject: [Enable Designware XGMAC VLAN Stripping Feature 2/2] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
   
--------------------------------

vger has a limit on the number of recipients for a single email
so patch series that cc all possible recipients can be bounced
and not forwarded by vger.

So I think when submitting a patch series, it's necessary to send
just the cover letter to all mailing lists for all files/paths
modified by any file in the patch series and specific patches
are sent to maintainers, reviewers and the specific mailing lists
modified by the specific patch.

I use the scripts below to send patch series where a patch series
are the only files in individual directories.

(Well I used to use, I'm not actively reading or creating kernel patches right now)

$ cat ~/bin/to.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats --pattern-depth=1"

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl --nom $opts  $(dirname $1)/*
else
    maint=$(./scripts/get_maintainer.pl --nol $opts $1)
    if [ "$maint" == "" ] ; then
	echo "linux-kernel@vger.kernel.org"
    else
	echo "$maint"
    fi
fi


$ cat ~/bin/cc.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats"
maint_file=$(mktemp -t XXXXXXXX.cc)

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl $opts $(dirname $1)/* |  \
	~/bin/remove_undesirable_emails.sh > $maint_file
    count=$(wc -c $maint_file | cut -f1 -d" ")
    if [[ $count -lt 512 ]] ; then
	cat $maint_file
    else
	./scripts/get_maintainer.pl -nom -nor $opts $(dirname $1)/* | \
	    ~/bin/remove_undesirable_emails.sh
    fi
else
    ./scripts/get_maintainer.pl $opts $1 | \
	~/bin/remove_undesirable_emails.sh > $maint_file
    count=$(wc -l $maint_file | cut -f1 -d" ")
    if [[ $count -gt 0 ]] ; then
	cat $maint_file
    else
	./scripts/get_maintainer.pl --git --git-fallback --norolestats $1 | \
	    ~/bin/remove_undesirable_emails.sh
    fi
fi
rm -f $maint_file


$ cat ~/bin/remove_undesirable_emails.sh
grep -vPi "(?:\bIngo\s+Molnar\b)"

$

(nb: Ingo asked not to receive any emails from me)

And these scripts are used with git send-email with a .gitconfig block

[sendemail]
	chainreplyto = false
	thread = false
	suppresscc = self
	cccmd = ~/bin/cc.sh
	tocmd = ~/bin/to.sh

These scripts would have added 2 mailing lists to patch 0/n:

devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-renesas-soc@vger.kernel.org (open list:ARM/RISC-V/RENESAS ARCHITECTURE)

and it would also have had a different recipient list for 1/2 as well

$ ./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/net/snps,dwmac.yaml
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Richard Cochran <richardcochran@gmail.com> (maintainer:PTP HARDWARE CLOCK SUPPORT)
Geert Uytterhoeven <geert+renesas@glider.be> (supporter:ARM/RISC-V/RENESAS ARCHITECTURE)
Magnus Damm <magnus.damm@gmail.com> (supporter:ARM/RISC-V/RENESAS ARCHITECTURE)
Alexandre Torgue <alexandre.torgue@foss.st.com> (in file)
Giuseppe Cavallaro <peppe.cavallaro@st.com> (in file)
Jose Abreu <joabreu@synopsys.com> (in file)
netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-kernel@vger.kernel.org (open list)
linux-renesas-soc@vger.kernel.org (open list:ARM/RISC-V/RENESAS ARCHITECTURE)

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Jakub Kicinski]

[clean up the CC list]

On Fri, 21 Jul 2023 20:32:44 -0700 Joe Perches wrote:
> On Fri, 2023-07-21 at 18:55 -0700, Jakub Kicinski wrote:
> > On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote:  
> > > That's not how you run it. get_maintainers.pl should be run on patches
> > > or on all files, not just some selection.  
> > 
> > Adding Joe for visibility (I proposed to print a warning when people 
> > do this and IIRC he wasn't on board).  
> 
> What's the issue here?  Other than _how_ the script was used,
> I don't see an actual problem with the script itself.

I just CCed you on another case. If people keep using get_maintainers
wrong, it *is* an issue with the script.

> I use the scripts below to send patch series where a patch series
> are the only files in individual directories.

The fact that most people end up wrapping get_maintainers in another
script is also a pretty strong indication that the user experience is
inadequate.

To be clear - I'm happy to put in the work to make the changes.
It's just that from past experience you seem to have strong opinions
which run counter to maintainers' needs, and I don't really enjoy
writing Perl for the sake of it.

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Joe Perches]

On Mon, 2023-07-24 at 18:04 -0700, Jakub Kicinski wrote:
> [clean up the CC list]
> 
> On Fri, 21 Jul 2023 20:32:44 -0700 Joe Perches wrote:
> > On Fri, 2023-07-21 at 18:55 -0700, Jakub Kicinski wrote:
> > > On Fri, 21 Jul 2023 18:21:32 +0200 Krzysztof Kozlowski wrote:  
> > > > That's not how you run it. get_maintainers.pl should be run on patches
> > > > or on all files, not just some selection.  
> > > 
> > > Adding Joe for visibility (I proposed to print a warning when people 
> > > do this and IIRC he wasn't on board).  
> > 
> > What's the issue here?  Other than _how_ the script was used,
> > I don't see an actual problem with the script itself.
> 
> I just CCed you on another case. If people keep using get_maintainers
> wrong, it *is* an issue with the script.

Silly.  No it's not.

> > I use the scripts below to send patch series where a patch series
> > are the only files in individual directories.
> 
> The fact that most people end up wrapping get_maintainers in another
> script is also a pretty strong indication that the user experience is
> inadequate.

Not a useful argument.  Process and documentation are required.

> To be clear - I'm happy to put in the work to make the changes.
> It's just that from past experience you seem to have strong opinions
> which run counter to maintainers' needs, and I don't really enjoy
> writing Perl for the sake of it.

Does anyone?  Knock yourself out doing whatever you like.

I do suggest you instead write wrapper scripts to get
the output you want rather than updating the defaults
for the script and update the process documentation
to let other people know what do to as well.

Something akin to Mario Limonciello's suggestion back in 2022:

https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Geert Uytterhoeven]

Hi Joe,

On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote:
> I do suggest you instead write wrapper scripts to get
> the output you want rather than updating the defaults
> for the script and update the process documentation
> to let other people know what do to as well.
>
> Something akin to Mario Limonciello's suggestion back in 2022:
>
> https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/

FTR, this is more or less what I am using to generate a script
to send out patches:

    OUT=...
    echo git send-email \\ > $OUT
    # Add -cc
    # Wrap comment inside $(: ...)
    # Replace (...) in comment by [...]
    # Replace ] at EOL by ) again
    # Add continuation to EOL
    scripts/get_maintainer.pl $* | \
    tr -d \" | \
    sed -e 's/^/--cc "/' \
        -e 's/ (/" $(: /' \
        -e 's/ (/ [/' -e 's/)/]/' \
        -e 's/]$/)/' \
        -e 's/$/ \\/' | \
    tee -a $OUT
    echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT

After generation, I edit the script to
  - Replace some --cc by --to,
  - Add/remove some people,
and run "source $OUT" to send the patches...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Mario Limonciello]

On 7/25/23 02:33, Geert Uytterhoeven wrote:
> Hi Joe,
> 
> On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote:
>> I do suggest you instead write wrapper scripts to get
>> the output you want rather than updating the defaults
>> for the script and update the process documentation
>> to let other people know what do to as well.
>>
>> Something akin to Mario Limonciello's suggestion back in 2022:
>>
>> https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/
> 
> FTR, this is more or less what I am using to generate a script
> to send out patches:
> 
>      OUT=...
>      echo git send-email \\ > $OUT
>      # Add -cc
>      # Wrap comment inside $(: ...)
>      # Replace (...) in comment by [...]
>      # Replace ] at EOL by ) again
>      # Add continuation to EOL
>      scripts/get_maintainer.pl $* | \
>      tr -d \" | \
>      sed -e 's/^/--cc "/' \
>          -e 's/ (/" $(: /' \
>          -e 's/ (/ [/' -e 's/)/]/' \
>          -e 's/]$/)/' \
>          -e 's/$/ \\/' | \
>      tee -a $OUT
>      echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT
> 
> After generation, I edit the script to
>    - Replace some --cc by --to,
>    - Add/remove some people,
> and run "source $OUT" to send the patches...
> 
> Gr{oetje,eeting}s,

My script is great for single subsystem patches as it gets all the right 
people but I've found problems whenever it crosses multiple subsystems.

Many subsystem owners want to see the whole series of patches to 
understand how they interact.  So the group of patches needs to be 
treated together which would need the wrapper to look at all patches 
instead.

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Joe Perches]

On Tue, 2023-07-25 at 08:19 -0500, Mario Limonciello wrote:
> On 7/25/23 02:33, Geert Uytterhoeven wrote:
> > Hi Joe,
> > 
> > On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote:
> > > I do suggest you instead write wrapper scripts to get
> > > the output you want rather than updating the defaults
> > > for the script and update the process documentation
> > > to let other people know what do to as well.
> > > 
> > > Something akin to Mario Limonciello's suggestion back in 2022:
> > > 
> > > https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/
> > 
> > FTR, this is more or less what I am using to generate a script
> > to send out patches:
> > 
> >      OUT=...
> >      echo git send-email \\ > $OUT
> >      # Add -cc
> >      # Wrap comment inside $(: ...)
> >      # Replace (...) in comment by [...]
> >      # Replace ] at EOL by ) again
> >      # Add continuation to EOL
> >      scripts/get_maintainer.pl $* | \
> >      tr -d \" | \
> >      sed -e 's/^/--cc "/' \
> >          -e 's/ (/" $(: /' \
> >          -e 's/ (/ [/' -e 's/)/]/' \
> >          -e 's/]$/)/' \
> >          -e 's/$/ \\/' | \
> >      tee -a $OUT
> >      echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT
> > 
> > After generation, I edit the script to
> >    - Replace some --cc by --to,
> >    - Add/remove some people,
> > and run "source $OUT" to send the patches...
> > 
> > Gr{oetje,eeting}s,
> 
> My script is great for single subsystem patches as it gets all the right 
> people but I've found problems whenever it crosses multiple subsystems.
> 
> Many subsystem owners want to see the whole series of patches to 
> understand how they interact.  So the group of patches needs to be 
> treated together which would need the wrapper to look at all patches 
> instead.

Which can't really work all the time as vger has a recipient limit
and subsystem spanning patches frequently exceed that limit.

bcc's don't work well either as the reply-to chain is broken.

No great solution to that.

Re: [Enable Designware XGMAC VLAN Stripping Feature 1/2] dt-bindings: net: snps,dwmac: Add description for rx-vlan-offload

[Krzysztof Kozlowski]

On 25/07/2023 15:43, Joe Perches wrote:
> On Tue, 2023-07-25 at 08:19 -0500, Mario Limonciello wrote:
>> On 7/25/23 02:33, Geert Uytterhoeven wrote:
>>> Hi Joe,
>>>
>>> On Tue, Jul 25, 2023 at 6:22 AM Joe Perches <joe@perches.com> wrote:
>>>> I do suggest you instead write wrapper scripts to get
>>>> the output you want rather than updating the defaults
>>>> for the script and update the process documentation
>>>> to let other people know what do to as well.
>>>>
>>>> Something akin to Mario Limonciello's suggestion back in 2022:
>>>>
>>>> https://lore.kernel.org/lkml/20220617183215.25917-1-mario.limonciello@amd.com/
>>>
>>> FTR, this is more or less what I am using to generate a script
>>> to send out patches:
>>>
>>>      OUT=...
>>>      echo git send-email \\ > $OUT
>>>      # Add -cc
>>>      # Wrap comment inside $(: ...)
>>>      # Replace (...) in comment by [...]
>>>      # Replace ] at EOL by ) again
>>>      # Add continuation to EOL
>>>      scripts/get_maintainer.pl $* | \
>>>      tr -d \" | \
>>>      sed -e 's/^/--cc "/' \
>>>          -e 's/ (/" $(: /' \
>>>          -e 's/ (/ [/' -e 's/)/]/' \
>>>          -e 's/]$/)/' \
>>>          -e 's/$/ \\/' | \
>>>      tee -a $OUT
>>>      echo "*[0-9][0-9][0-9][0-9]-*.*" >> $OUT
>>>
>>> After generation, I edit the script to
>>>    - Replace some --cc by --to,
>>>    - Add/remove some people,
>>> and run "source $OUT" to send the patches...
>>>
>>> Gr{oetje,eeting}s,
>>
>> My script is great for single subsystem patches as it gets all the right 
>> people but I've found problems whenever it crosses multiple subsystems.
>>
>> Many subsystem owners want to see the whole series of patches to 
>> understand how they interact.  So the group of patches needs to be 
>> treated together which would need the wrapper to look at all patches 
>> instead.
> 
> Which can't really work all the time as vger has a recipient limit
> and subsystem spanning patches frequently exceed that limit.
> 
> bcc's don't work well either as the reply-to chain is broken.
> 
> No great solution to that.
> 

For small patchsets (and recipients list) I recommend:
https://github.com/krzk/tools/blob/master/linux/.bash_aliases_linux#L91

For bigger patchsets - Rob's sendemail identity could work:
https://lore.kernel.org/all/CAL_JsqLubWBr2W3xZPsuPLOGav7CFgBdH=aCfT22F_m0_cx3cQ@mail.gmail.com/
but cover letter has to be treated separately.

Anyway, it is not the case here. This is small patchset and the
submitter should run get_maintainers.pl on *the patchset*, not on one
chosen file. Running it one one file, ignoring maintainers of all other
patches, does not make sense. There is nothing to fix in
get_maintainers.pl. I believe our docs are also correct here.

Best regards,
Krzysztof

[PATCH] scripts: checkpatch: steer people away from using file paths

[Jakub Kicinski]

We repeatedly see noobs misuse get_maintainer by running it on
the file paths rather than the patchfile. This leads to authors
of changes (quoted commits and commits under Fixes) not getting
CCed. These are usually the best reviewers!

The file option should really not be used by noobs, unless
they are just trying to find a maintainer to manually contact.

Print a warning when someone tries to use -f and remove
the "auto-guessing" of file paths.

This script may break people's "scripts on top of get_maintainer"
if they are using -f... but that's the point.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
This is what I had in mind.

CC: joe@perches.com
Cc: geert@linux-m68k.org
Cc: krzk@kernel.org
Cc: netdev@vger.kernel.org
Cc: workflows@vger.kernel.org
Cc: mario.limonciello@amd.com
---
 scripts/get_maintainer.pl | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..3635b3d40ebe 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -51,6 +51,7 @@ my $output_roles = 0;
 my $output_rolestats = 1;
 my $output_section_maxlen = 50;
 my $scm = 0;
+my $silence_file_warning = 0;
 my $tree = 1;
 my $web = 0;
 my $subsystem = 0;
@@ -267,6 +268,7 @@ if (!GetOptions(
 		'subsystem!' => \$subsystem,
 		'status!' => \$status,
 		'scm!' => \$scm,
+		'silence-file-warning!' => \$silence_file_warning,
 		'tree!' => \$tree,
 		'web!' => \$web,
 		'letters=s' => \$letters,
@@ -544,7 +546,13 @@ foreach my $file (@ARGV) {
     if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
 	warn "$P: file '$file' not found in version control $!\n";
     }
-    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
+    if ($from_filename) {
+	if (!$silence_file_warning) {
+	    warn "$P: WARNING: Prefer running the script on patches as "
+	        . "generated by git format-patch. Selecting paths is known "
+		. "to miss recipients!\n";
+	}
+
 	$file =~ s/^\Q${cur_path}\E//;	#strip any absolute path
 	$file =~ s/^\Q${lk_path}\E//;	#or the path to the lk tree
 	push(@files, $file);
@@ -1081,6 +1089,7 @@ version: $V
   --mailmap => use .mailmap file (default: $email_use_mailmap)
   --no-tree => run without a kernel tree
   --self-test => show potential issues with MAINTAINERS file content
+  --silence-file-warning => silence the warning about -f being used (see Notes)
   --version => show version
   --help => show this help information
 
@@ -1089,6 +1098,10 @@ version: $V
    --pattern-depth=0 --remove-duplicates --rolestats]
 
 Notes:
+  Using "-f file" is generally discouraged, running the script on a filepatch
+      (as generated by git format-patch) is usually the right thing to do.
+      Commit message is an integral part of the change and $P
+      will extract additional information from it (keywords, Fixes tags etc.)
   Using "-f directory" may give unexpected results:
       Used with "--git", git signators for _all_ files in and below
           directory are examined as git recurses directories.
-- 
2.41.0

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Greg KH]

On Tue, Jul 25, 2023 at 08:59:26AM -0700, Jakub Kicinski wrote:
> We repeatedly see noobs misuse get_maintainer by running it on
> the file paths rather than the patchfile. This leads to authors
> of changes (quoted commits and commits under Fixes) not getting
> CCed. These are usually the best reviewers!
> 
> The file option should really not be used by noobs, unless
> they are just trying to find a maintainer to manually contact.
> 
> Print a warning when someone tries to use -f and remove
> the "auto-guessing" of file paths.
> 
> This script may break people's "scripts on top of get_maintainer"
> if they are using -f... but that's the point.

Ok, I'll go fix up my local scripts, but you should change your subject
line to say "get_maintainer", not "checkpatch" :)

thanks,

greg k-h

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Krzysztof Kozlowski]

On 25/07/2023 17:59, Jakub Kicinski wrote:
> We repeatedly see noobs misuse get_maintainer by running it on
> the file paths rather than the patchfile. This leads to authors
> of changes (quoted commits and commits under Fixes) not getting
> CCed. These are usually the best reviewers!
> 
> The file option should really not be used by noobs, unless
> they are just trying to find a maintainer to manually contact.
> 
> Print a warning when someone tries to use -f and remove
> the "auto-guessing" of file paths.
> 
> This script may break people's "scripts on top of get_maintainer"
> if they are using -f... but that's the point.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> This is what I had in mind.

"Anything that can go wrong will go wrong."
https://en.wikipedia.org/wiki/Murphy%27s_law

Therefore:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Jakub Kicinski]

On Tue, 25 Jul 2023 18:53:48 +0200 Greg KH wrote:
> > This script may break people's "scripts on top of get_maintainer"
> > if they are using -f... but that's the point.  
> 
> Ok, I'll go fix up my local scripts,

Which one? I spotted this in your repo but it already seems
to use patches:

https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list

How do you use this, BTW?

> but you should change your subject
> line to say "get_maintainer", not "checkpatch" :)

Ugh, will do :)

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Greg KH]

On Tue, Jul 25, 2023 at 10:10:51AM -0700, Jakub Kicinski wrote:
> On Tue, 25 Jul 2023 18:53:48 +0200 Greg KH wrote:
> > > This script may break people's "scripts on top of get_maintainer"
> > > if they are using -f... but that's the point.  
> > 
> > Ok, I'll go fix up my local scripts,
> 
> Which one? I spotted this in your repo but it already seems
> to use patches:
> 
> https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list

Oh yeah, it does work on patches.  Nevermind, I think I just use the -f
version manually when trying to figure out who to blame for a bug report
in a specific file :)

> How do you use this, BTW?

I do:
	- git format-patch to generate the patch series.
	- run the generate_cc_list script which creates XXXX.info files
	  (the XXXX being the patch number) that contain the
	  people/lists to cc: on the patch
	- git rebase -i on the patch series and edit the changelog
	  description and paste in the XXXX.info file for that specific
	  patch.

Yeah, it's a lot of manual steps, I should use b4 for it, one of these
days...

thanks,

greg k-h

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Jakub Kicinski]

On Tue, 25 Jul 2023 19:25:09 +0200 Greg KH wrote:
> I do:
> 	- git format-patch to generate the patch series.
> 	- run the generate_cc_list script which creates XXXX.info files
> 	  (the XXXX being the patch number) that contain the
> 	  people/lists to cc: on the patch
> 	- git rebase -i on the patch series and edit the changelog
> 	  description and paste in the XXXX.info file for that specific
> 	  patch.
> 
> Yeah, it's a lot of manual steps, I should use b4 for it, one of these
> days...

Oh, neat! I do a similar thing. Modulo the number of steps:

  git rebase --exec './ccer.py --inline'

I was wondering if I was the only one who pastes the Cc: person
into the patch, because I'd love to teach get_maintainer to do
the --inline thing, instead of carrying my own wrapper...

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Joe Perches]

On Tue, 2023-07-25 at 12:52 -0700, Jakub Kicinski wrote:
> On Tue, 25 Jul 2023 19:25:09 +0200 Greg KH wrote:
> > I do:
> > 	- git format-patch to generate the patch series.
> > 	- run the generate_cc_list script which creates XXXX.info files
> > 	  (the XXXX being the patch number) that contain the
> > 	  people/lists to cc: on the patch
> > 	- git rebase -i on the patch series and edit the changelog
> > 	  description and paste in the XXXX.info file for that specific
> > 	  patch.
> > 
> > Yeah, it's a lot of manual steps, I should use b4 for it, one of these
> > days...
> 
> Oh, neat! I do a similar thing. Modulo the number of steps:
> 
>   git rebase --exec './ccer.py --inline'
> 
> I was wondering if I was the only one who pastes the Cc: person
> into the patch, because I'd love to teach get_maintainer to do
> the --inline thing, instead of carrying my own wrapper...

Now for that, you'd want checkpatch or yet another script to do it.

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Joe Perches]

On Tue, 2023-07-25 at 08:59 -0700, Jakub Kicinski wrote:
> We repeatedly see noobs misuse get_maintainer by running it on
> the file paths rather than the patchfile. This leads to authors
> of changes (quoted commits and commits under Fixes) not getting
> CCed. These are usually the best reviewers!
> 
> The file option should really not be used by noobs, unless
> they are just trying to find a maintainer to manually contact.

noobs is not an appropriate use IMO for a commit message.

> This is what I had in mind.

<shrug>  I think it's unnecessary and this content should be
better described in some process doc.

> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
[]
> @@ -544,7 +546,13 @@ foreach my $file (@ARGV) {
>      if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
>  	warn "$P: file '$file' not found in version control $!\n";
>      }
> -    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
> +    if ($from_filename) {
> +	if (!$silence_file_warning) {
> +	    warn "$P: WARNING: Prefer running the script on patches as "
> +	        . "generated by git format-patch. Selecting paths is known "
> +		. "to miss recipients!\n";

Don't separate a single output message into multiple lines.
Coalesce the string elements.

Also, this should show some reason why this isn't appropriate
as a patch to a single file would not have this issue.

e.g.:	When a patch series touches multiple files, showing all maintainers is useful. see:  <some process doc>

> @@ -1089,6 +1098,10 @@ version: $V
>     --pattern-depth=0 --remove-duplicates --rolestats]
>  
>  Notes:
> +  Using "-f file" is generally discouraged, running the script on a filepatch
> +      (as generated by git format-patch) is usually the right thing to do.
> +      Commit message is an integral part of the change and $P
> +      will extract additional information from it (keywords, Fixes tags etc.)

-f <file>

"filepatch" doesn't appear in the kernel at all. Use "patch file".

grammar: The commit message is...

may instead of will

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Jakub Kicinski]

On Tue, 25 Jul 2023 14:18:15 -0700 Joe Perches wrote:
> > @@ -544,7 +546,13 @@ foreach my $file (@ARGV) {
> >      if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
> >  	warn "$P: file '$file' not found in version control $!\n";
> >      }
> > -    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
> > +    if ($from_filename) {
> > +	if (!$silence_file_warning) {
> > +	    warn "$P: WARNING: Prefer running the script on patches as "
> > +	        . "generated by git format-patch. Selecting paths is known "
> > +		. "to miss recipients!\n";  
> 
> Don't separate a single output message into multiple lines.
> Coalesce the string elements.
> 
> Also, this should show some reason why this isn't appropriate
> as a patch to a single file would not have this issue.
> 
> e.g.:	When a patch series touches multiple files, showing all maintainers is useful. see:  <some process doc>

I tried to do that in --help. Added the "multiple files" one there, too.

> > @@ -1089,6 +1098,10 @@ version: $V
> >     --pattern-depth=0 --remove-duplicates --rolestats]
> >  
> >  Notes:
> > +  Using "-f file" is generally discouraged, running the script on a filepatch
> > +      (as generated by git format-patch) is usually the right thing to do.
> > +      Commit message is an integral part of the change and $P
> > +      will extract additional information from it (keywords, Fixes tags etc.)  
> 
> "filepatch" doesn't appear in the kernel at all. Use "patch file".

I got it the wrong way around. I'll use patchfile (no space) for v2
since that's what's what get_maintainer uses in two other places.

Re: [PATCH] scripts: checkpatch: steer people away from using file paths

[Joe Perches]

On Tue, 2023-07-25 at 15:15 -0700, Jakub Kicinski wrote:
> On Tue, 25 Jul 2023 14:18:15 -0700 Joe Perches wrote:
> > > @@ -544,7 +546,13 @@ foreach my $file (@ARGV) {
> > >      if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
> > >  	warn "$P: file '$file' not found in version control $!\n";
> > >      }
> > > -    if ($from_filename || ($file ne "&STDIN" && vcs_file_exists($file))) {
> > > +    if ($from_filename) {
> > > +	if (!$silence_file_warning) {
> > > +	    warn "$P: WARNING: Prefer running the script on patches as "
> > > +	        . "generated by git format-patch. Selecting paths is known "
> > > +		. "to miss recipients!\n";  
> > 
> > Don't separate a single output message into multiple lines.
> > Coalesce the string elements.
> > 
> > Also, this should show some reason why this isn't appropriate
> > as a patch to a single file would not have this issue.
> > 
> > e.g.:	When a patch series touches multiple files, showing all maintainers is useful. see:  <some process doc>
> 
> I tried to do that in --help. Added the "multiple files" one there, too.

It'd be more useful in the warning message.
Hardly anyone reads help.

> 
> > > @@ -1089,6 +1098,10 @@ version: $V
> > >     --pattern-depth=0 --remove-duplicates --rolestats]
> > >  
> > >  Notes:
> > > +  Using "-f file" is generally discouraged, running the script on a filepatch
> > > +      (as generated by git format-patch) is usually the right thing to do.
> > > +      Commit message is an integral part of the change and $P
> > > +      will extract additional information from it (keywords, Fixes tags etc.)  
> > 
> > "filepatch" doesn't appear in the kernel at all. Use "patch file".
> 
> I got it the wrong way around. I'll use patchfile (no space) for v2
> since that's what's what get_maintainer uses in two other places.