netdev - RE: [PATCH RFC 5/6] net: dsa: microchip: Update tag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Dec 2018 20:00:26 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <f.fainelli@...il.com>, <andrew@...n.ch>
CC:     <pavel@....cz>, <UNGLinuxDriver@...rochip.com>,
        <netdev@...r.kernel.org>
Subject: RE: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access
 switch driver

> >>> Update tag_ksz.c to access switch driver's tail tagging operations.
> >>
> >> Hi Tristram
> >>
> >> Humm, i'm not sure we want this, the tagging spit into two places.  I
> >> need to take a closer look at the previous patch, to see why it cannot
> >> be done here.
> >
> > O.K, i think i get what is going on.
> >
> > I would however implement it differently.
> >
> > One net/dsa/tag_X.c file can export two dsa_device_ops structures,
> > allowing you to share common code for the two taggers. You could call
> > these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE,
> and the
> > .get_tag_protocol call would then return the correct one for the
> > switch.
> 
> Agreed, that is what is done by net/dsa/tag_brcm.c because there are two
> formats for the Broadcom tag:
> 
> - TAG_BRCM: the 4-bytes Broadcom tag is between MAC SA and Ethertype
> - TAG_BRCM_PREPEND: the 4-bytes Broadcom tag is before the MAC DA
>

I did try to implement this way.  But the other switches do not have the same
format even though the length is the same.  Then I need to change the following
files for any new KSZ switch: include/linux/dsa.h, net/dsa/dsa.c, net/dsa/dsa_priv.h,
and finally net/dsa/tag_ksz.c.

Even then it will not work if Microchip wants to add 1588 PTP capability to the switches.

For KSZ9477 the length of the tail tag changes when the PTP function is enabled.
Typically this function is either enabled or disabled all the time, but if users want to
change that during normal operation to see how the switch behaves, the transmit
function completely stops working correctly.

Older driver implementation is to monitor that register change and adjust the length
dynamically.

Another problem is the tail tag needs to include the timestamp for the 1-step
Pdelay_Resp to have accurate turnaround time when that message is sent out by the
switch.  This will require access to the main switch driver which will keep track of those
PTP messages.

PTP handles transmit timestamp in skb_tx_timestamp, which is typically called after the
frame is sent, so it is too late.  DSA calls dsa_skb_tx_timestamp before sending, but it
only provides a clone to the driver that supports port_txstamp and so the switch driver
may not be able to do anything.

> And the code to process them is basically using relative offsets from
> the start of the frame to access correct data.
> 
> This is done largely for performance reasons because we have 1/2
> Gigabit/secs capable CPU ports and so we want to avoid as little cache
> trashing as possible and immediately get the right rcv() function to
> process the packets.
> 

The SoC I used for this driver development actually has problem sending
Gigabit traffic so I do not see the effect of any slowdown, and the updated
MAC driver change for a hardware problem does not help and greatly
degrades the transmit performance.

> >
> > It might also be possible to merge in tag_trailer, or at least share
> > some code.
> >

Actually in previous old DSA implementation I just hijacked this file to
add the tail tag operations without creating a new file like tag_ksz.c.

> > What i don't yet understand is how you are passing PTP information
> > around. The commit messages need to explain that, since it is not
> > obvious, and it is the first time we have needed PTP info in a tag
> > driver.

It seems the official 1588 PTP timestamp API for a PHY driver is only implemented
in only PHY driver, net/phy/dp83640.c, in the whole kernel.  DSA uses similar
mechanism to support 1588 PTP.  In dsa_switch_rcv() the CPU receive function is called
first before dsa_skb_defer_rx_timestamp().  That means the receive tail tag operation
has to be done first to retrieve the receive timestamp so that it can be passed later.

It is probably not good to change the socket buffer length inside the port_rxtstamp
function, and I do not see any other way to insert that transmit timestamp.

A customer has already inquired about implementing 1588 PTP in the DSA driver.  I hope
this mechanism is approved so that I can start doing that.