Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

[Vladimir Oltean]

On Tue, Nov 09, 2021 at 10:20:25AM -0800, Florian Fainelli wrote:
> On 11/9/21 10:15 AM, Vladimir Oltean wrote:
> > On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> >>
> >> On 11/9/21 10:05 AM, Florian Fainelli wrote:
> >>> On 11/9/21 1:50 AM, Martin Kaistra wrote:
> >>>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> >>>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> >>>> include/linux/dsa/b53.h.
> >>>>
> >>>> Signed-off-by: Martin Kaistra <martin.kaistra@xxxxxxxxxxxxx>
> >>>> ---
> >>>>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
> >>>>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 101 insertions(+), 89 deletions(-)
> >>>>  create mode 100644 include/linux/dsa/b53.h
> >>>
> >>> All you really access is the b53_port_hwtstamp structure within the
> >>> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
> >>
> >> You do access b53_dev in the TX part, still, I would like to find a more
> >> elegant solution to exposing everything here, can you create a
> >> b53_timecounter_cyc2time() function that is exported to modules but does
> >> not require exposing the b53_device to net/dsa/tag_brcm.c?
> >> --
> >> Florian
> > 
> > Switch drivers can't export symbols to tagging protocol drivers, remember?
> > https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
> 
> I do now :) How about a function pointer in dsa_switch_ops that driver
> can hook onto?

IMHO, the honest answer to this is to admit that it's not quite okay to
timestamp a single packet at a time and simply not timestamp any packet
that might be concurrent with that, no warning or questions asked. We
should queue that second packet if it cannot be marked for timestamping
right away.

Which brings me to my second point, there used to be a generic deferred
xmit mechanism in DSA that also happened to solve this problem because
yes, there was a function pointer in dsa_switch_ops for it.
But you and Richard didn't quite like it, so I removed it.
https://patchwork.ozlabs.org/cover/1215617/