Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function

From: Vladimir Oltean <olteanv@gmail.com>
To: Richard Cochran <richardcochran@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	"David S. Miller" <davem@davemloft.net>,
	John Stultz <john.stultz@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Stephen Boyd <sboyd@kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Let taggers specify a can_timestamp function
Date: Wed, 29 May 2019 23:33:31 +0300	[thread overview]
Message-ID: <CA+h21hoNrhcpAONTvJra5Ekk+yJ6xP0VAaPSygaLOw31qsGPTg@mail.gmail.com> (raw)
In-Reply-To: <20190529044912.cyg44rqvdo73oeiu@localhost>

On Wed, 29 May 2019 at 07:49, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Wed, May 29, 2019 at 02:56:25AM +0300, Vladimir Oltean wrote:
> > The newly introduced function is called on both the RX and TX paths.
>
> NAK on this patch.
>
> > The boolean returned by port_txtstamp should only return false if the
> > driver tried to timestamp the skb but failed.
>
> So you say.
>
> > Currently there is some logic in the mv88e6xxx driver that determines
> > whether it should timestamp frames or not.
> >
> > This is wasteful, because if the decision is to not timestamp them, then
> > DSA will have cloned an skb and freed it immediately afterwards.
>
> No, it isn't wasteful.  Look at the tests in that driver to see why.
>
> > Additionally other drivers (sja1105) may have other hardware criteria
> > for timestamping frames on RX, and the default conditions for
> > timestamping a frame are too restrictive.
>
> I'm sorry, but we won't change the frame just for one device that has
> design issues.
>
> Please put device specific workarounds into its driver.
>
> Thanks,
> Richard

Hi Richard,

I removed this patch and my RX timestamping path still works, apparently.
It's just that now I'm not holding up in the RX timestamping queue
anything else except what the PTP classifier requested me to.
What I'm concerned about is that I'm using the skb->cb as a
communication space between the tagger waiting for the meta frame, and
the RX timestamping queue waiting to be notified that the meta frame
arrived.
If I'm not holding up all frames that I know will have a follow-up
come after them, then I'm letting them free up the stack, and who
knows whose skb->cb the tagger will overwrite.
By asking me to remove this patch you're basically asking the state
machine inside the tagger to guess whether the previous frame is one
that DSA cares about w.r.t. RX timestamping, and based on that info
write to its skb->cb or not.
I would like to avoid keeping meta frames in their own RX queue,
because then I'm complicating (or rather put, making impossible) the
association between a meta frame and the frame it holds a timestamp
of.

Regards,
-Vladimir