| Message ID | 20220826063816.948397-2-mattias.forsblad@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add RMU support | expand |
On Fri, Aug 26, 2022 at 08:38:14AM +0200, Mattias Forsblad wrote: > Support handling of layer 2 part for RMU frames which is > handled in-band with other DSA traffic. > > 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); > 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); > }; Hi Mattias Vladimer pointed you towards the qca driver, in a comment for your RFC. qca already has support for switch commands via Ethernet frames. The point he was trying to make is that you should look at that code. The concept of executing a command via an Ethernet frame, and expecting a reply via an Ethernet frame is generic. The format of those frames is specific to the switch. We want the generic parts to look the same for all switches. If possible, we want to implement it once in the dsa core, so all switch drivers share it. Less code, better tested code, less bugs, easier maintenance. Take a look at qca_tagger_data. Please use the same mechanism with mv88e6xxx. But also look deeper. What else can be shared? You need a buffer to put the request in, you need to send it, you need to wait for the reply, you need to pass the results to the driver, you need to free that buffer afterwards. That should all be common. Look at these parts in the qca driver. Can you make them generic, move them into the DSA core? Are there other parts which could be shared? Andrew
On 2022-08-26 21:27, Andrew Lunn wrote: > On Fri, Aug 26, 2022 at 08:38:14AM +0200, Mattias Forsblad wrote: >> Support handling of layer 2 part for RMU frames which is >> handled in-band with other DSA traffic. >> >> 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); >> 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); >> }; > > Hi Mattias > > Vladimer pointed you towards the qca driver, in a comment for your > RFC. qca already has support for switch commands via Ethernet frames. > > The point he was trying to make is that you should look at that > code. The concept of executing a command via an Ethernet frame, and > expecting a reply via an Ethernet frame is generic. The format of > those frames is specific to the switch. We want the generic parts to > look the same for all switches. If possible, we want to implement it > once in the dsa core, so all switch drivers share it. Less code, > better tested code, less bugs, easier maintenance. > > Take a look at qca_tagger_data. Please use the same mechanism with This I can do which makes sense. > mv88e6xxx. But also look deeper. What else can be shared? You need a I can also make a generic dsa_eth_tx_timeout routine which handles the sending and receiving of cmd frames. > buffer to put the request in, you need to send it, you need to wait The skb is the buffer and it's up to the driver to decode it properly? I've looked into the qca driver and that uses a small static buffer for replies, no buffer lifetime cycle. > for the reply, you need to pass the results to the driver, you need to > free that buffer afterwards. That should all be common. Look at these > parts in the qca driver. Can you make them generic, move them into the > DSA core? Are there other parts which could be shared? I cannot change the qca code as I have no way of verifying that the resulting code works. > > Andrew > > /Mattias
> > Hi Mattias > > > > Vladimer pointed you towards the qca driver, in a comment for your > > RFC. qca already has support for switch commands via Ethernet frames. > > > > The point he was trying to make is that you should look at that > > code. The concept of executing a command via an Ethernet frame, and > > expecting a reply via an Ethernet frame is generic. The format of > > those frames is specific to the switch. We want the generic parts to > > look the same for all switches. If possible, we want to implement it > > once in the dsa core, so all switch drivers share it. Less code, > > better tested code, less bugs, easier maintenance. > > > > Take a look at qca_tagger_data. Please use the same mechanism with > > This I can do which makes sense. > > > mv88e6xxx. But also look deeper. What else can be shared? You need a > > I can also make a generic dsa_eth_tx_timeout routine which > handles the sending and receiving of cmd frames. > > > buffer to put the request in, you need to send it, you need to wait > > The skb is the buffer and it's up to the driver to decode it properly? Yes, the tagger layer passes the skb, which contains the complete Ethernet frame, plus additional meta data. But you need to watch out for the life cycle of that skb: /* Ethernet mgmt read/write packet */ if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) { if (likely(tagger_data->rw_reg_ack_handler)) tagger_data->rw_reg_ack_handler(ds, skb); return NULL; } returning NULL means the DSA core will free the skb when the call to qca_tag_rcv() returns. So either you need to take a copy of the data, clone the skb, or change this code somehow. See what makes most sense for generic code. > I've looked into the qca driver and that uses a small static buffer > for replies, no buffer lifetime cycle. > > > for the reply, you need to pass the results to the driver, you need to > > free that buffer afterwards. That should all be common. Look at these > > parts in the qca driver. Can you make them generic, move them into the > > DSA core? Are there other parts which could be shared? > > I cannot change the qca code as I have no way of verifying that the > resulting code works. You can change it. You can at least compile test your change. The QCA developers are also quite active, and so will probably help out testing whatever you change. And ideally, you want lots of simple, obviously correct changes, which i can review and say look correct. Changing code which you cannot test is very normal in Linux, do your best, and ask for testing. Andrew
On Fri, Aug 26, 2022 at 08:38:14AM +0200, Mattias Forsblad wrote: > Support handling of layer 2 part for RMU frames which is > handled in-band with other DSA traffic. Great explanation, everything is clear! > > 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. > 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. > + > + 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(). > + > + 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. > + dev_dbg_ratelimited(ds->dev, "DSA inband: error decoding packet"); > + ret = -EIO; > + } > + > + return ret;
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;
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); 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 */ /* * 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); + + 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; + + ds = dev->dsa_ptr->ds; + if (!ds) + return 0; + + dsa_header = skb->data - 2; + + 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)) { + dev_dbg_ratelimited(ds->dev, "DSA inband: error decoding packet"); + ret = -EIO; + } + + return ret; +} + static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, u8 extra) { @@ -218,9 +302,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, switch (code) { case DSA_CODE_FRAME2REG: - /* Remote management is not implemented yet, - * drop. - */ + dsa_inband_rcv_ll(skb, dev); return NULL; case DSA_CODE_ARP_MIRROR: case DSA_CODE_POLICY_MIRROR: @@ -325,6 +407,12 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA) +static int dsa_inband_xmit(struct sk_buff *skb, struct net_device *dev, + int seq_no) +{ + return dsa_inband_xmit_ll(skb, dev, NULL, 0, seq_no); +} + static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev) { return dsa_xmit_ll(skb, dev, 0); @@ -343,6 +431,7 @@ static const struct dsa_device_ops dsa_netdev_ops = { .proto = DSA_TAG_PROTO_DSA, .xmit = dsa_xmit, .rcv = dsa_rcv, + .inband_xmit = dsa_inband_xmit, .needed_headroom = DSA_HLEN, }; @@ -354,6 +443,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_DSA); #define EDSA_HLEN 8 +static int edsa_inband_xmit(struct sk_buff *skb, struct net_device *dev, + int seq_no) +{ + u8 edsa_header[4]; + + edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff; + edsa_header[1] = ETH_P_EDSA & 0xff; + edsa_header[2] = 0x00; + edsa_header[3] = 0x00; + + return dsa_inband_xmit_ll(skb, dev, edsa_header, 4, seq_no); +} + static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev) { u8 *edsa_header; @@ -385,6 +487,7 @@ static const struct dsa_device_ops edsa_netdev_ops = { .proto = DSA_TAG_PROTO_EDSA, .xmit = edsa_xmit, .rcv = edsa_rcv, + .inband_xmit = edsa_inband_xmit, .needed_headroom = EDSA_HLEN, };
Support handling of layer 2 part for RMU frames which is handled in-band with other DSA traffic. 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(-)