RE: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver

[Y.b. Lu]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@xxxxxxxxx>
> Sent: 2021年4月27日 2:17
> To: Y.b. Lu <yangbo.lu@xxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; Richard Cochran <richardcochran@xxxxxxxxx>;
> Vladimir Oltean <vladimir.oltean@xxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Jonathan Corbet
> <corbet@xxxxxxx>; Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>; Andrew Lunn
> <andrew@xxxxxxx>; Vivien Didelot <vivien.didelot@xxxxxxxxx>; Florian
> Fainelli <f.fainelli@xxxxxxxxx>; Claudiu Manoil <claudiu.manoil@xxxxxxx>;
> Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>;
> UNGLinuxDriver@xxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver
> 
> On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote:
> > Free skb->cb usage in core driver and let device drivers decide to use
> > or not. The reason having a DSA_SKB_CB(skb)->clone was because
> > dsa_skb_tx_timestamp() which may set the clone pointer was called
> > before p->xmit() which would use the clone if any, and the device
> > driver has no way to initialize the clone pointer.
> >
> > Although for now putting memset(skb->cb, 0, 48) at beginning of
> > dsa_slave_xmit() by this patch is not very good, there is still way to
> > improve this. Otherwise, some other new features, like one-step
> > timestamp which needs a flag of skb marked in dsa_skb_tx_timestamp(),
> > and handles as one-step timestamp in p->xmit() will face same
> > situation.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@xxxxxxx>
> > ---
> > Changes for v2:
> > 	- Added this patch.
> > ---
> >  drivers/net/dsa/ocelot/felix.c         |  1 +
> >  drivers/net/dsa/sja1105/sja1105_main.c |  2 +-
> > drivers/net/dsa/sja1105/sja1105_ptp.c  |  4 +++-
> >  drivers/net/ethernet/mscc/ocelot.c     |  6 +++---
> >  drivers/net/ethernet/mscc/ocelot_net.c |  2 +-
> >  include/linux/dsa/sja1105.h            |  3 ++-
> >  include/net/dsa.h                      | 14 --------------
> >  include/soc/mscc/ocelot.h              |  8 ++++++++
> >  net/dsa/slave.c                        |  3 +--
> >  net/dsa/tag_ocelot.c                   |  8 ++++----
> >  net/dsa/tag_ocelot_8021q.c             |  8 ++++----
> >  11 files changed, 28 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/dsa/ocelot/felix.c
> > b/drivers/net/dsa/ocelot/felix.c index d679f023dc00..8980d56ee793
> > 100644
> > --- a/drivers/net/dsa/ocelot/felix.c
> > +++ b/drivers/net/dsa/ocelot/felix.c
> > @@ -1403,6 +1403,7 @@ static bool felix_txtstamp(struct dsa_switch
> > *ds, int port,
> >
> >  	if (ocelot->ptp && ocelot_port->ptp_cmd ==
> IFH_REW_OP_TWO_STEP_PTP) {
> >  		ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> > +		OCELOT_SKB_CB(skb)->clone = clone;
> >  		return true;
> >  	}
> >
> 
> Uh-oh, this patch fails to build:
> 
> In file included from ./include/soc/mscc/ocelot_vcap.h:9:0,
>                  from drivers/net/dsa/ocelot/felix.c:9:
> drivers/net/dsa/ocelot/felix.c: In function ‘felix_txtstamp’:
> drivers/net/dsa/ocelot/felix.c:1406:17: error: ‘skb’ undeclared (first use in
> this function)
>    OCELOT_SKB_CB(skb)->clone = clone;
>                  ^
> ./include/soc/mscc/ocelot.h:698:29: note: in definition of macro
> ‘OCELOT_SKB_CB’
>   ((struct ocelot_skb_cb *)((skb)->cb))
>                              ^~~
> drivers/net/dsa/ocelot/felix.c:1406:17: note: each undeclared identifier is
> reported only once for each function it appears in
>    OCELOT_SKB_CB(skb)->clone = clone;
>                  ^
> ./include/soc/mscc/ocelot.h:698:29: note: in definition of macro
> ‘OCELOT_SKB_CB’
>   ((struct ocelot_skb_cb *)((skb)->cb))
>                              ^~~
> 
> It depends on changes made in patch 3.

Oh.. That's a sequence problem.
I switched patch #3 and patch #4 with rebasing to fix up in v3 version.
Thanks.