Re: [PATCH net-next v2 1/3] dsa: Implement RMU layer in DSA

Mattias Forsblad
2022-08-31T05:55:26Z

On 2022-08-30 17:47, Vladimir Oltean wrote:

>> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
>> ---
>> include/net/dsa.h | 7 +++
>> include/uapi/linux/if_ether.h | 1 +
>> net/dsa/tag_dsa.c | 109 +++++++++++++++++++++++++++++++++-
>> 3 files changed, 114 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index f2ce12860546..54f7f3494f84 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -92,6 +92,7 @@ struct dsa_switch;
>> struct dsa_device_ops {
>> struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
>> struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
>> + int (*inband_xmit)(struct sk_buff *skb, struct net_device *dev, int seq_no);
>
> There isn't a reason that I can see to expand the DSA tagger ops with an
> inband_xmit(). DSA tagging protocol ops are reactive hooks to modify
> packets belonging to slave interfaces, rather than initiators of traffic.
>
> Here you are calling tag_ops->inband_xmit() from mv88e6xxx_rmu_tx(),
> i.e. this operation is never invoked from the DSA core, but from a code
> path fully in control of the hardware driver. We don't usually (ever?)
> use DSA ops in this way, but rather just a way for the framework to
> invoke driver-specific code.
>
> Is there a reason why dsa_inband_xmit_ll() cannot simply live within the
> mv88e6xxx driver (the direct caller) rather than within the dsa/edsa tagger?
>
> Tagging protocols can be changed at driver runtime, but only while the
> DSA master is down. So when the master goes up, you can also check which
> tagging protocol is in use, and cache/use that to construct the skb.
>
> Furthermore, there is no slave interface associated with RMU traffic,
> although in your proposed implementation here, there is (that's what
> "struct net_device *dev" passed here is).
>
> Which slave @dev are you passing? That's right, dev_get_by_name(&init_net, "chan0").
> I think it's pretty obvious there is a systematic problem with your approach.
> Not everyone has a slave net device called chan0 in the main netns.
>
> The qca8k implementation, as opposed to yours, calls dev_queue_xmit()
> with skb->dev directly on the DSA master, and forgoes the DSA tagger on
> TX. I don't see a problem with that approach, on the contrary, I think
> it's better and simpler.
>

Yes, I agree and will rework it more in line with qca8k.

>> void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
>> int *offset);
>> int (*connect)(struct dsa_switch *ds);
>> @@ -1193,6 +1194,12 @@ struct dsa_switch_ops {
>> void (*master_state_change)(struct dsa_switch *ds,
>> const struct net_device *master,
>> bool operational);
>> +
>> + /*
>> + * RMU operations
>> + */
>> + int (*inband_receive)(struct dsa_switch *ds, struct sk_buff *skb,
>> + int seq_no);
>> };
>>
>> #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes) \
>> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
>> index d370165bc621..9de1bdc7cccc 100644
>> --- a/include/uapi/linux/if_ether.h
>> +++ b/include/uapi/linux/if_ether.h
>> @@ -158,6 +158,7 @@
>> #define ETH_P_MCTP 0x00FA /* Management component transport
>> * protocol packets
>> */
>> +#define ETH_P_RMU_DSA 0x00FB /* RMU DSA protocol */
>
> I think it's more normal to set skb->protocol = eth->h_proto = htons(the actual EtherType),
> rather than introducing a new skb->protocol which won't be used anywhere.
>
>>
>> /*
>> * This is an Ethernet frame header.
>> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
>> index e4b6e3f2a3db..36f02e7dd3c3 100644
>> --- a/net/dsa/tag_dsa.c
>> +++ b/net/dsa/tag_dsa.c
>> @@ -123,6 +123,90 @@ enum dsa_code {
>> DSA_CODE_RESERVED_7 = 7
>> };
>>
>> +#define DSA_RMU_RESV1 0x3e
>> +#define DSA_RMU 1
>> +#define DSA_RMU_PRIO 6
>> +#define DSA_RMU_RESV2 0xf
>> +
>> +static int dsa_inband_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>> + const u8 *header, int header_len, int seq_no)
>> +{
>> + static const u8 dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
>> + struct dsa_port *dp;
>> + struct ethhdr *eth;
>> + u8 *data;
>> +
>> + if (!dev)
>> + return -ENODEV;
>> +
>> + dp = dsa_slave_to_port(dev);
>> + if (!dp)
>> + return -ENODEV;
>> +
>> + /* Create RMU L2 header */
>> + data = skb_push(skb, 6);
>> + data[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
>> + data[1] = DSA_RMU_RESV1 << 2 | DSA_RMU << 1;
>> + data[2] = DSA_RMU_PRIO << 5 | DSA_RMU_RESV2;
>> + data[3] = seq_no;
>> + data[4] = 0;
>> + data[5] = 0;
>> +
>> + /* Add header if any */
>> + if (header) {
>> + data = skb_push(skb, header_len);
>> + memcpy(data, header, header_len);
>> + }
>> +
>> + /* Create MAC header */
>> + eth = (struct ethhdr *)skb_push(skb, 2 * ETH_ALEN);
>> + memcpy(eth->h_source, dev->dev_addr, ETH_ALEN);
>> + memcpy(eth->h_dest, dest_addr, ETH_ALEN);
>> +
>> + skb->protocol = htons(ETH_P_RMU_DSA);
>> +
>> + dev_queue_xmit(skb);
>
> Just for things to be 100% clear for everyone. Per your design, we have
> dsa_inband_xmit() which gets called by the driver with a slave @dev, and
> this constructs an skb without the DSA/EDSA header. Then dev_queue_xmit()
> will invoke the ndo_start_xmit of DSA, dsa_slave_xmit(). In turn this
> will enter the tagging protocol driver a second time, through p->xmit() ->
> dsa_xmit_ll(). The second time is when the DSA/EDSA header is actually
> introduced.
>
> This is way more complicated than it needs to be.
>

See comment above.

>> +
>> + return 0;
>> +}
>> +
>> +static int dsa_inband_rcv_ll(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct dsa_switch *ds;
>> + int source_device;
>> + u8 *dsa_header;
>> + int rcv_seqno;
>> + int ret = 0;
>> +
>> + if (!dev || !dev->dsa_ptr)
>> + return 0;
>
> Way too defensive programming which wastes CPU cycles for nothing. The
> DSA rcv hook runs as an ETH_P_XDSA packet_type handler on the DSA master,
> based on eth_type_trans() which had set skb->protocol to ETH_P_XDSA
> based on the presence of dev->dsa_ptr. So yes, the DSA master is not NULL,
> and yes, the DSA master's dev->dsa_ptr is not NULL either.
>
>> +
>> + ds = dev->dsa_ptr->ds;
>> + if (!ds)
>> + return 0;
>
> We don't have NULL pointers in cpu_dp->ds lying around. dp->ds is set by
> dsa_port_touch(), which runs earlier than dsa_master_setup() sets
> dev->dsa_ptr in such a way that we start processing RX packets.
>
>> +
>> + dsa_header = skb->data - 2;
>
> Please use dsa_etype_header_pos_rx(). In fact, this pointer was already
> available in dsa_rcv_ll().
>

I did see it, thanks.

>> +
>> + source_device = dsa_header[0] & 0x1f;
>> + ds = dsa_switch_find(ds->dst->index, source_device);
>> + if (!ds) {
>> + net_dbg_ratelimited("DSA inband: Didn't find switch with index %d", source_device);
>> + return -EINVAL;
>> + }
>> +
>> + /* Get rcv seqno */
>> + rcv_seqno = dsa_header[3];
>> +
>> + skb_pull(skb, DSA_HLEN);
>> +
>> + if (ds->ops && ds->ops->inband_receive(ds, skb, rcv_seqno)) {
>
> I think the reason why Andrew is asking you to find common aspects with
> qca8k which can be further generalized is because your proposed code is
> very ambitious, introducing a generic ds->ops->inband_receive() like this.
>
> I personally wouldn't be so ambitious for myself. The way the qca8k
> driver and tagging protocol work together is that they set up a group of
> driver-specific function pointers, rw_reg_ack_handler and mib_autocast_handler,
> through which the switch driver connected to the tagger may subscribe as
> a consumer to the received Ethernet management packets. Whereas you are
> proposing a one-size-fits-all ds->ops->inband_receive with no effort to
> see if it fits even the single other user of this concept, qca8k.
>
> What I would do is introduce one more case of driver-specific consumer
> of RMU packets, specific to the dsa/edsa tagger <-> mv88e6xxx driver pair.
> I'd let things sit for a while, maybe wait for a third user, then see
> how/if the prototype for consuming Ethernet management packets can be
> made generic.
>
> But in general, we need drivers to handle non-data RX packets coming
> from the switch for all sorts of reasons. For example SJA1110 delivers
> back TX timestamps as Ethernet packets, and I wouldn't consider
> expanding ds->ops for that. This driver-specific hook mechanism
> ("tagger owned storage" as I named it) is flexible enough to allow each
> driver to respond to the needs of its hardware, without needing
> everybody else to follow suit or suffer of ops bloat because of it.
> I wouldn't rush.
>

I'll come back with a new version to discuss.

>> + dev_dbg_ratelimited(ds->dev, "DSA inband: error decoding packet");
>> + ret = -EIO;
>> + }
>> +
>> + return ret;