lkml.org 
[lkml]   [2021]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet
On Tue, Dec 14, 2021 at 11:43:53PM +0100, Ansuel Smith wrote:
> Hi, this is ready but require some additional test on a wider userbase.
>
> The main reason for this is that we notice some routing problem in the
> switch and it seems assisted learning is needed. Considering mdio is
> quite slow due to the indirect write using this Ethernet alternative way
> seems to be quicker.
>
> The qca8k switch supports a special way to pass mdio read/write request
> using specially crafted Ethernet packet.
> This works by putting some defined data in the Ethernet header where the
> mac source and dst should be placed. The Ethernet type header is set to qca
> header and is set to a mdio read/write type.
> This is used to communicate to the switch that this is a special packet
> and should be parsed differently.
>
> Currently we use Ethernet packet for
> - MIB counter
> - mdio read/write configuration
> - phy read/write for each port
>
> Current implementation of this use completion API to wait for the packet
> to be processed by the tagger and has a timeout that fallback to the
> legacy mdio way and mutex to enforce one transaction at time.
>
> We now have connect()/disconnect() ops for the tagger. They are used to
> allocate priv data in the dsa priv. The header still has to be put in
> global include to make it usable by a dsa driver.
> They are called when the tag is connect to the dst and the data is freed
> using discconect on tagger change.
>
> (if someone wonder why the bind function is put at in the general setup
> function it's because tag is set in the cpu port where the notifier is
> still not available and we require the notifier to sen the
> tag_proto_connect() event.
>
> We now have a tag_proto_connect() for the dsa driver used to put
> additional data in the tagger priv (that is actually the dsa priv).
> This is called using a switch event DSA_NOTIFIER_TAG_PROTO_CONNECT.
> Current use for this is adding handler for the Ethernet packet to keep
> the tagger code as dumb as possible.
>
> The tagger priv implement only the handler for the special packet. All the
> other stuff is placed in the qca8k_priv and the tagger has to access
> it under lock.
>
> We use the new API from Vladimir to track if the master port is
> operational or not. We had to track many thing to reach a usable state.
> Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
> also not enough since it use also for other task. The correct way was
> both track for interface UP and if a qdisc was assigned to the
> interface. That tells us the port (and the tagger indirectly) is ready
> to accept and process packet.
>
> I tested this with multicpu port and with port6 set as the unique port and
> it's sad.
> It seems they implemented this feature in a bad way and this is only
> supported with cpu port0. When cpu port6 is the unique port, the switch
> doesn't send ack packet. With multicpu port, packet ack are not duplicated
> and only cpu port0 sends them. This is the same for the MIB counter.
> For this reason this feature is enabled only when cpu port0 is enabled and
> operational.
>
> Current concern are:
> - Any hint about the naming? Is calling this mdio Ethernet correct?
> Should we use a more ""standard""/significant name? (considering also
> other switch will implement this)
>
> v6:
> - Fix some error in ethtool handler caused by rebase/cleanup
> v5:
> - Adapt to new API fixes
> - Fix a wrong logic for noop
> - Add additional lock for master_state change
> - Limit mdio Ethernet to cpu port0 (switch limitation)
> - Add priority to these special packet
> - Move mdio cache to qca8k_priv
> v4:
> - Remove duplicate patch sent by mistake.
> v3:
> - Include MIB with Ethernet packet.
> - Include phy read/write with Ethernet packet.
> - Reorganize code with new API.
> - Introuce master tracking by Vladimir
> v2:
> - Address all suggestion from Vladimir.
> Try to generilize this with connect/disconnect function from the
> tagger and tag_proto_connect for the driver.
>
> Ansuel Smith (12):
> net: dsa: tag_qca: convert to FIELD macro
> net: dsa: tag_qca: move define to include linux/dsa
> net: dsa: tag_qca: enable promisc_on_master flag
> net: dsa: tag_qca: add define for handling mdio Ethernet packet
> net: dsa: tag_qca: add define for handling MIB packet
> net: dsa: tag_qca: add support for handling mdio Ethernet and MIB
> packet
> net: dsa: qca8k: add tracking state of master port
> net: dsa: qca8k: add support for mdio read/write in Ethernet packet
> net: dsa: qca8k: add support for mib autocast in Ethernet packet
> net: dsa: qca8k: add support for phy read/write with mdio Ethernet
> net: dsa: qca8k: move page cache to driver priv
> net: dsa: qca8k: cache lo and hi for mdio write
>
> Vladimir Oltean (4):
> net: dsa: provide switch operations for tracking the master state
> net: dsa: stop updating master MTU from master.c
> net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> net: dsa: replay master state events in
> dsa_tree_{setup,teardown}_master
>
> drivers/net/dsa/qca8k.c | 600 ++++++++++++++++++++++++++++++++++--
> drivers/net/dsa/qca8k.h | 46 ++-
> include/linux/dsa/tag_qca.h | 79 +++++
> include/net/dsa.h | 17 +
> net/dsa/dsa2.c | 81 ++++-
> net/dsa/dsa_priv.h | 13 +
> net/dsa/master.c | 29 +-
> net/dsa/slave.c | 32 ++
> net/dsa/switch.c | 15 +
> net/dsa/tag_qca.c | 81 +++--
> 10 files changed, 901 insertions(+), 92 deletions(-)
> create mode 100644 include/linux/dsa/tag_qca.h
>
> --
> 2.33.1
>

I just tested if the Documentation is correct about this alternative way
being able to read/write 16byte of data at times (instead of 4).

I confirm this work but I need to understand how and if this can be
used. I can see I should use the regmap bulk api... But I assume I
should change the val_bits. (think that is not acceptable if regmap is
already init and would be problematic for the fallback...)

Wonder if I should register a separate regmap for eth and use that...
(and create an helper to switch between the mdio and the ethernet one?)

This would be very useful for fib read/write that require 4 read/write
to put data in the db. (1 lock, 1 packet instead of 4 lock 4 packet)

Would love any hint if we have other way to handle this but I think the
double regmap and the helper seems to be the cleaner way for this.

(obviously this will come later and won't be part of this already big
patch)

--
Ansuel

\
 
 \ /
  Last update: 2021-12-15 00:35    [W:1.694 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site