diff mbox series

[net,2/4] net: mvpp2: Remove unneeded Kconfig dependency.

Message ID 1069fecd4b7e13485839e1c66696c5a6c70f6144.1611198584.git.richardcochran@gmail.com (mailing list archive)
State New, archived
Headers show
Series Remove unneeded PHY time stamping option. | expand

Commit Message

Richard Cochran Jan. 21, 2021, 4:06 a.m. UTC
The mvpp2 is an Ethernet driver, and it implements MAC style time
stamping of PTP frames.  It has no need of the expensive option to
enable PHY time stamping.  Remove the incorrect dependency.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")
---
 drivers/net/ethernet/marvell/Kconfig | 1 -
 1 file changed, 1 deletion(-)

Comments

Russell King - ARM Linux admin Jan. 21, 2021, 10:27 a.m. UTC | #1
On Wed, Jan 20, 2021 at 08:06:01PM -0800, Richard Cochran wrote:
> The mvpp2 is an Ethernet driver, and it implements MAC style time
> stamping of PTP frames.  It has no need of the expensive option to
> enable PHY time stamping.  Remove the incorrect dependency.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")

NAK.

> ---
>  drivers/net/ethernet/marvell/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
> index 41815b609569..7fe15a3286f4 100644
> --- a/drivers/net/ethernet/marvell/Kconfig
> +++ b/drivers/net/ethernet/marvell/Kconfig
> @@ -94,7 +94,6 @@ config MVPP2
>  
>  config MVPP2_PTP
>  	bool "Marvell Armada 8K Enable PTP support"
> -	depends on NETWORK_PHY_TIMESTAMPING
>  	depends on (PTP_1588_CLOCK = y && MVPP2 = y) || \
>  		   (PTP_1588_CLOCK && MVPP2 = m)
>  
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Richard Cochran Jan. 21, 2021, 3:08 p.m. UTC | #2
On Thu, Jan 21, 2021 at 10:27:54AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 20, 2021 at 08:06:01PM -0800, Richard Cochran wrote:
> > The mvpp2 is an Ethernet driver, and it implements MAC style time
> > stamping of PTP frames.  It has no need of the expensive option to
> > enable PHY time stamping.  Remove the incorrect dependency.
> > 
> > Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> > Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")
> 
> NAK.

Can you please explain why mvpp2 requires NETWORK_PHY_TIMESTAMING?

Thanks,
Richard
Jakub Kicinski Jan. 23, 2021, 2:14 a.m. UTC | #3
On Thu, 21 Jan 2021 07:08:02 -0800 Richard Cochran wrote:
> On Thu, Jan 21, 2021 at 10:27:54AM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Jan 20, 2021 at 08:06:01PM -0800, Richard Cochran wrote:  
> > > The mvpp2 is an Ethernet driver, and it implements MAC style time
> > > stamping of PTP frames.  It has no need of the expensive option to
> > > enable PHY time stamping.  Remove the incorrect dependency.
> > > 
> > > Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> > > Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")  
> > 
> > NAK.  
> 
> Can you please explain why mvpp2 requires NETWORK_PHY_TIMESTAMING?

Russell, I think we all agree now this is not the solution to the
problem of which entity should provide the timestamp, but the series
doesn't seem objectionable in itself.

Please LMK if you think otherwise.

(I would put it in net-next tho, given the above this at most a space
optimization.)
Russell King - ARM Linux admin Jan. 23, 2021, 9:39 a.m. UTC | #4
On Fri, Jan 22, 2021 at 06:14:44PM -0800, Jakub Kicinski wrote:
> On Thu, 21 Jan 2021 07:08:02 -0800 Richard Cochran wrote:
> > On Thu, Jan 21, 2021 at 10:27:54AM +0000, Russell King - ARM Linux admin wrote:
> > > On Wed, Jan 20, 2021 at 08:06:01PM -0800, Richard Cochran wrote:  
> > > > The mvpp2 is an Ethernet driver, and it implements MAC style time
> > > > stamping of PTP frames.  It has no need of the expensive option to
> > > > enable PHY time stamping.  Remove the incorrect dependency.
> > > > 
> > > > Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> > > > Fixes: 91dd71950bd7 ("net: mvpp2: ptp: add TAI support")  
> > > 
> > > NAK.  
> > 
> > Can you please explain why mvpp2 requires NETWORK_PHY_TIMESTAMING?
> 
> Russell, I think we all agree now this is not the solution to the
> problem of which entity should provide the timestamp, but the series
> doesn't seem objectionable in itself.
> 
> Please LMK if you think otherwise.
> 
> (I would put it in net-next tho, given the above this at most a space
> optimization.)

Correct - my NAK is on the basis that this series was put forward
as solving the issue I had raised, but in reality it does little
to achieve that.

It is, as you say, just a space optimisation, and I have no issue
with it being merged on that basis.
Richard Cochran Jan. 23, 2021, 1:26 p.m. UTC | #5
On Fri, Jan 22, 2021 at 06:14:44PM -0800, Jakub Kicinski wrote:

> (I would put it in net-next tho, given the above this at most a space
> optimization.)

It isn't just about space but also time.  The reason why I targeted
net and not net-next was that NETWORK_PHY_TIMESTAMPING activates a
function call to skb_clone_tx_timestamp() for every transmitted frame.

	static inline void skb_tx_timestamp(struct sk_buff *skb)
	{
		skb_clone_tx_timestamp(skb);
		if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
			skb_tstamp_tx(skb, NULL);
	}

In the abscence of a PHY time stamping device, the check for its
presence inside of skb_clone_tx_timestamp() will of course fail, but
this still incurs the cost of the call on every transmitted skb.

Similarly netif_receive_skb() futilely calls skb_defer_rx_timestamp()
on every received skb.

I would argue that most users don't want this option activated by
accident.

(And yes, we could avoid the functions call by moving the check
outside of the global functions and inline to the call sites.  I'll be
sure to have that in the shiny new improved scheme under discussion.)

Thanks,
Richard
Jakub Kicinski Jan. 23, 2021, 8:12 p.m. UTC | #6
On Sat, 23 Jan 2021 05:26:26 -0800 Richard Cochran wrote:
> On Fri, Jan 22, 2021 at 06:14:44PM -0800, Jakub Kicinski wrote:
> 
> > (I would put it in net-next tho, given the above this at most a space
> > optimization.)  
> 
> It isn't just about space but also time.  The reason why I targeted
> net and not net-next was that NETWORK_PHY_TIMESTAMPING activates a
> function call to skb_clone_tx_timestamp() for every transmitted frame.
> 
> 	static inline void skb_tx_timestamp(struct sk_buff *skb)
> 	{
> 		skb_clone_tx_timestamp(skb);
> 		if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
> 			skb_tstamp_tx(skb, NULL);
> 	}
> 
> In the abscence of a PHY time stamping device, the check for its
> presence inside of skb_clone_tx_timestamp() will of course fail, but
> this still incurs the cost of the call on every transmitted skb.
> 
> Similarly netif_receive_skb() futilely calls skb_defer_rx_timestamp()
> on every received skb.
> 
> I would argue that most users don't want this option activated by
> accident.

I see. The only thing I'm worried about then is the churn in patch 3.
This would land in Linus's tree shortly before rc6, kinda late to be
taking chances in the name of minor optimizations :S
 
> (And yes, we could avoid the functions call by moving the check
> outside of the global functions and inline to the call sites.  I'll be
> sure to have that in the shiny new improved scheme under discussion.)
Richard Cochran Jan. 23, 2021, 9:14 p.m. UTC | #7
On Sat, Jan 23, 2021 at 12:12:27PM -0800, Jakub Kicinski wrote:
> I see. The only thing I'm worried about then is the churn in patch 3.
> This would land in Linus's tree shortly before rc6, kinda late to be
> taking chances in the name of minor optimizations :S

;^)

Yeah, by all means, avoid ARM churn... I remember Bad Things there...

Maybe you could take #1 and #2 for net-next?

I should probably submit 3-4 throught the SoC tree anyhow.

Thanks,
Richard
Jakub Kicinski Jan. 23, 2021, 9:38 p.m. UTC | #8
On Sat, 23 Jan 2021 13:14:00 -0800 Richard Cochran wrote:
> On Sat, Jan 23, 2021 at 12:12:27PM -0800, Jakub Kicinski wrote:
> > I see. The only thing I'm worried about then is the churn in patch 3.
> > This would land in Linus's tree shortly before rc6, kinda late to be
> > taking chances in the name of minor optimizations :S  
> 
> ;^)
> 
> Yeah, by all means, avoid ARM churn... I remember Bad Things there...
> 
> Maybe you could take #1 and #2 for net-next?

Done, thanks!

> I should probably submit 3-4 throught the SoC tree anyhow.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 41815b609569..7fe15a3286f4 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -94,7 +94,6 @@  config MVPP2
 
 config MVPP2_PTP
 	bool "Marvell Armada 8K Enable PTP support"
-	depends on NETWORK_PHY_TIMESTAMPING
 	depends on (PTP_1588_CLOCK = y && MVPP2 = y) || \
 		   (PTP_1588_CLOCK && MVPP2 = m)