| Message ID | yw1x8rud4cux.fsf@mansr.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA using cpsw and lan9303 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
+others, netdev is a high volume list, you should probably copy directly the people involved with the code you are working with. On 2/14/22 8:44 AM, Måns Rullgård wrote: > The hardware I'm working on has a LAN9303 switch connected to the > Ethernet port of an AM335x (ZCE package). In trying to make DSA work > with this combination, I have encountered two problems. > > Firstly, the cpsw driver configures the hardware to filter out frames > with unknown VLAN tags. To make it accept the tagged frames coming from > the LAN9303, I had to modify the latter driver like this: > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index 2de67708bbd2..460c998c0c33 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -1078,20 +1079,28 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, > struct phy_device *phy) > { > struct lan9303 *chip = ds->priv; > + struct net_device *master; > > if (!dsa_is_user_port(ds, port)) > return 0; > > + master = dsa_to_port(chip->ds, 0)->master; > + vlan_vid_add(master, htons(ETH_P_8021Q), port); That looks about right given that net/dsa/tag_lan9303.c appears to be a quasi DSA_TAG_PROTO_8021Q implementation AFAICT. > + > return lan9303_enable_processing_port(chip, port); > } > > Secondly, the cpsw driver strips VLAN tags from incoming frames, and > this prevents the DSA parsing from working. As a dirty workaround, I > did this: > > diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c > index 424e644724e4..e15f42ece8bf 100644 > --- a/drivers/net/ethernet/ti/cpsw_priv.c > +++ b/drivers/net/ethernet/ti/cpsw_priv.c > @@ -235,6 +235,7 @@ void cpsw_rx_vlan_encap(struct sk_buff *skb) > > /* Remove VLAN header encapsulation word */ > skb_pull(skb, CPSW_RX_VLAN_ENCAP_HDR_SIZE); > + return; > > pkt_type = (rx_vlan_encap_hdr >> > CPSW_RX_VLAN_ENCAP_HDR_PKT_TYPE_SHIFT) & > > With these changes, everything seems to work as expected. > > Now I'd appreciate if someone could tell me how I should have done this. > Please don't make me send an actual patch.
Hi Måns, On Mon, Feb 14, 2022 at 09:16:10AM -0800, Florian Fainelli wrote: > +others, > > netdev is a high volume list, you should probably copy directly the > people involved with the code you are working with. Thanks, Florian. > > Secondly, the cpsw driver strips VLAN tags from incoming frames, and > > this prevents the DSA parsing from working. As a dirty workaround, I > > did this: > > > > diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c > > index 424e644724e4..e15f42ece8bf 100644 > > --- a/drivers/net/ethernet/ti/cpsw_priv.c > > +++ b/drivers/net/ethernet/ti/cpsw_priv.c > > @@ -235,6 +235,7 @@ void cpsw_rx_vlan_encap(struct sk_buff *skb) > > > > /* Remove VLAN header encapsulation word */ > > skb_pull(skb, CPSW_RX_VLAN_ENCAP_HDR_SIZE); > > + return; > > > > pkt_type = (rx_vlan_encap_hdr >> > > CPSW_RX_VLAN_ENCAP_HDR_PKT_TYPE_SHIFT) & > > > > With these changes, everything seems to work as expected. > > > > Now I'd appreciate if someone could tell me how I should have done this. Assuming cpsw_rx_vlan_encap() doesn't just eat the VLAN, but puts it in the skb hwaccel area. The tag_lan9303.c tagger must deal with both variants of VLANs. For example, dsa_8021q_rcv() has: skb_push_rcsum(skb, ETH_HLEN); if (skb_vlan_tag_present(skb)) { tci = skb_vlan_tag_get(skb); __vlan_hwaccel_clear_tag(skb); } else { __skb_vlan_pop(skb, &tci); } skb_pull_rcsum(skb, ETH_HLEN); vid = tci & VLAN_VID_MASK; (process @vid here) which should give you a head start. > > Please don't make me send an actual patch. So what is your plan otherwise? :)
Vladimir Oltean <olteanv@gmail.com> writes: > Hi Måns, > > On Mon, Feb 14, 2022 at 09:16:10AM -0800, Florian Fainelli wrote: >> +others, >> >> netdev is a high volume list, you should probably copy directly the >> people involved with the code you are working with. > > Thanks, Florian. > >> > Secondly, the cpsw driver strips VLAN tags from incoming frames, and >> > this prevents the DSA parsing from working. As a dirty workaround, I >> > did this: >> > >> > diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c >> > index 424e644724e4..e15f42ece8bf 100644 >> > --- a/drivers/net/ethernet/ti/cpsw_priv.c >> > +++ b/drivers/net/ethernet/ti/cpsw_priv.c >> > @@ -235,6 +235,7 @@ void cpsw_rx_vlan_encap(struct sk_buff *skb) >> > >> > /* Remove VLAN header encapsulation word */ >> > skb_pull(skb, CPSW_RX_VLAN_ENCAP_HDR_SIZE); >> > + return; >> > >> > pkt_type = (rx_vlan_encap_hdr >> >> > CPSW_RX_VLAN_ENCAP_HDR_PKT_TYPE_SHIFT) & >> > >> > With these changes, everything seems to work as expected. >> > >> > Now I'd appreciate if someone could tell me how I should have done this. > > Assuming cpsw_rx_vlan_encap() doesn't just eat the VLAN, but puts it in > the skb hwaccel area. The tag_lan9303.c tagger must deal with both > variants of VLANs. > > For example, dsa_8021q_rcv() has: > > skb_push_rcsum(skb, ETH_HLEN); > if (skb_vlan_tag_present(skb)) { > tci = skb_vlan_tag_get(skb); > __vlan_hwaccel_clear_tag(skb); > } else { > __skb_vlan_pop(skb, &tci); > } > skb_pull_rcsum(skb, ETH_HLEN); > > vid = tci & VLAN_VID_MASK; > > (process @vid here) > > which should give you a head start. Thanks, that looks promising. >> > Please don't make me send an actual patch. > > So what is your plan otherwise? :) I meant I didn't want to send a patch I know to be broken only to provoke a discussion.
Hi Måns, On Mon, Feb 14, 2022 at 09:16:10AM -0800, Florian Fainelli wrote: > +others, > > netdev is a high volume list, you should probably copy directly the > people involved with the code you are working with. > > On 2/14/22 8:44 AM, Måns Rullgård wrote: > > The hardware I'm working on has a LAN9303 switch connected to the > > Ethernet port of an AM335x (ZCE package). In trying to make DSA work > > with this combination, I have encountered two problems. > > > > Firstly, the cpsw driver configures the hardware to filter out frames > > with unknown VLAN tags. To make it accept the tagged frames coming from > > the LAN9303, I had to modify the latter driver like this: > > > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > > index 2de67708bbd2..460c998c0c33 100644 > > --- a/drivers/net/dsa/lan9303-core.c > > +++ b/drivers/net/dsa/lan9303-core.c > > @@ -1078,20 +1079,28 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, > > struct phy_device *phy) > > { > > struct lan9303 *chip = ds->priv; > > + struct net_device *master; > > > > if (!dsa_is_user_port(ds, port)) > > return 0; > > > > + master = dsa_to_port(chip->ds, 0)->master; > > + vlan_vid_add(master, htons(ETH_P_8021Q), port); > > That looks about right given that net/dsa/tag_lan9303.c appears to be a > quasi DSA_TAG_PROTO_8021Q implementation AFAICT. In case it was not clear, I agree with Florian that this looks "about right", I just thought that mentioning it wouldn't bring much to the table. But I noticed you submitted a patch for the other issue and not for this. Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), but it's not the first place in this driver where that is done. dsa_tag_8021q_port_setup() also has: /* Add @rx_vid to the master's RX filter. */ vlan_vid_add(master, ctx->proto, rx_vid); which is an indication that other switches with VLAN-based tagging protocols should handle this somehow, somewhere. Note, though, that vlan_vid_add() allocates memory, so it would be good to have a vlan_vid_del() too at some point.
Vladimir Oltean <olteanv@gmail.com> writes: > Hi Måns, > > On Mon, Feb 14, 2022 at 09:16:10AM -0800, Florian Fainelli wrote: >> +others, >> >> netdev is a high volume list, you should probably copy directly the >> people involved with the code you are working with. >> >> On 2/14/22 8:44 AM, Måns Rullgård wrote: >> > The hardware I'm working on has a LAN9303 switch connected to the >> > Ethernet port of an AM335x (ZCE package). In trying to make DSA work >> > with this combination, I have encountered two problems. >> > >> > Firstly, the cpsw driver configures the hardware to filter out frames >> > with unknown VLAN tags. To make it accept the tagged frames coming from >> > the LAN9303, I had to modify the latter driver like this: >> > >> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c >> > index 2de67708bbd2..460c998c0c33 100644 >> > --- a/drivers/net/dsa/lan9303-core.c >> > +++ b/drivers/net/dsa/lan9303-core.c >> > @@ -1078,20 +1079,28 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, >> > struct phy_device *phy) >> > { >> > struct lan9303 *chip = ds->priv; >> > + struct net_device *master; >> > >> > if (!dsa_is_user_port(ds, port)) >> > return 0; >> > >> > + master = dsa_to_port(chip->ds, 0)->master; >> > + vlan_vid_add(master, htons(ETH_P_8021Q), port); >> >> That looks about right given that net/dsa/tag_lan9303.c appears to be a >> quasi DSA_TAG_PROTO_8021Q implementation AFAICT. > > In case it was not clear, I agree with Florian that this looks "about right", > I just thought that mentioning it wouldn't bring much to the table. > But I noticed you submitted a patch for the other issue and not for this. > > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), > but it's not the first place in this driver where that is done. What would be the proper way to do it? > dsa_tag_8021q_port_setup() also has: > > /* Add @rx_vid to the master's RX filter. */ > vlan_vid_add(master, ctx->proto, rx_vid); > > which is an indication that other switches with VLAN-based tagging > protocols should handle this somehow, somewhere. > > Note, though, that vlan_vid_add() allocates memory, so it would be good > to have a vlan_vid_del() too at some point. Yes, I just didn't include that bit in the initial query.
On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote: > > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), > > but it's not the first place in this driver where that is done. > > What would be the proper way to do it? Generally speaking: struct dsa_port *cpu_dp; dsa_switch_for_each_cpu_port(cpu_dp, ds) break; // use cpu_dp If your code runs after dsa_tree_setup_default_cpu(), which contains the "DSA: tree %d has no CPU port\n" check, you don't even need to check whether cpu_dp was found or not - it surely was. Everything that runs after dsa_register_switch() has completed successfully - for example the DSA ->setup() method - qualifies here.
Vladimir Oltean <olteanv@gmail.com> writes: > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote: >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), >> > but it's not the first place in this driver where that is done. >> >> What would be the proper way to do it? > > Generally speaking: > > struct dsa_port *cpu_dp; > > dsa_switch_for_each_cpu_port(cpu_dp, ds) > break; > > // use cpu_dp > > If your code runs after dsa_tree_setup_default_cpu(), which contains the > "DSA: tree %d has no CPU port\n" check, you don't even need to check > whether cpu_dp was found or not - it surely was. Everything that runs > after dsa_register_switch() has completed successfully - for example the > DSA ->setup() method - qualifies here. In this particular driver, the setup function contains this: /* Make sure that port 0 is the cpu port */ if (!dsa_is_cpu_port(ds, 0)) { dev_err(chip->dev, "port 0 is not the CPU port\n"); return -EINVAL; } I take this to mean that port 0 is guaranteed to be the cpu port. Of course, it can't hurt to be thorough just in case that check is ever removed.
On Wed, Feb 16, 2022 at 02:23:24PM +0000, Måns Rullgård wrote: > Vladimir Oltean <olteanv@gmail.com> writes: > > > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote: > >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), > >> > but it's not the first place in this driver where that is done. > >> > >> What would be the proper way to do it? > > > > Generally speaking: > > > > struct dsa_port *cpu_dp; > > > > dsa_switch_for_each_cpu_port(cpu_dp, ds) > > break; > > > > // use cpu_dp > > > > If your code runs after dsa_tree_setup_default_cpu(), which contains the > > "DSA: tree %d has no CPU port\n" check, you don't even need to check > > whether cpu_dp was found or not - it surely was. Everything that runs > > after dsa_register_switch() has completed successfully - for example the > > DSA ->setup() method - qualifies here. > > In this particular driver, the setup function contains this: > > /* Make sure that port 0 is the cpu port */ > if (!dsa_is_cpu_port(ds, 0)) { > dev_err(chip->dev, "port 0 is not the CPU port\n"); > return -EINVAL; > } > > I take this to mean that port 0 is guaranteed to be the cpu port. Of > course, it can't hurt to be thorough just in case that check is ever > removed. Yes, I saw that, and I said that there are other places in the driver that assume port 0 is the CPU port. Although I don't know why that is, if the switch can only operate like that, etc. I just pointed out how it would be preferable to get a hold of the CPU port in a regular DSA driver without any special constraints.
On Wed, Feb 16, 2022 at 04:26:34PM +0200, Vladimir Oltean wrote: > On Wed, Feb 16, 2022 at 02:23:24PM +0000, Måns Rullgård wrote: > > Vladimir Oltean <olteanv@gmail.com> writes: > > > > > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote: > > >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), > > >> > but it's not the first place in this driver where that is done. > > >> > > >> What would be the proper way to do it? > > > > > > Generally speaking: > > > > > > struct dsa_port *cpu_dp; > > > > > > dsa_switch_for_each_cpu_port(cpu_dp, ds) > > > break; > > > > > > // use cpu_dp > > > > > > If your code runs after dsa_tree_setup_default_cpu(), which contains the > > > "DSA: tree %d has no CPU port\n" check, you don't even need to check > > > whether cpu_dp was found or not - it surely was. Everything that runs > > > after dsa_register_switch() has completed successfully - for example the > > > DSA ->setup() method - qualifies here. > > > > In this particular driver, the setup function contains this: > > > > /* Make sure that port 0 is the cpu port */ > > if (!dsa_is_cpu_port(ds, 0)) { > > dev_err(chip->dev, "port 0 is not the CPU port\n"); > > return -EINVAL; > > } > > > > I take this to mean that port 0 is guaranteed to be the cpu port. Of > > course, it can't hurt to be thorough just in case that check is ever > > removed. > > Yes, I saw that, and I said that there are other places in the driver > that assume port 0 is the CPU port. Although I don't know why that is, > if the switch can only operate like that, etc. I just pointed out how it > would be preferable to get a hold of the CPU port in a regular DSA > driver without any special constraints. Ah, silly me, I should have paid more attention on where you're actually inserting the code. You could have done: static int lan9303_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy) { struct dsa_port *dp = dsa_to_port(ds, port); struct lan9303 *chip = ds->priv; if (!dsa_port_is_user(dp)) return 0; vlan_vid_add(dp->cpu_dp->master, htons(ETH_P_8021Q), port); return lan9303_enable_processing_port(chip, port); } the advantage being that if this driver ever supports the remapping of the CPU port, or multiple CPU ports, this logic wouldn't need to be changed, as it also conveys the user-to-CPU port affinity. Anyway, doesn't really matter, and you certainly don't need to resend for this. Sorry again for not paying too much attention.
Vladimir Oltean <olteanv@gmail.com> writes: > On Wed, Feb 16, 2022 at 04:26:34PM +0200, Vladimir Oltean wrote: >> On Wed, Feb 16, 2022 at 02:23:24PM +0000, Måns Rullgård wrote: >> > Vladimir Oltean <olteanv@gmail.com> writes: >> > >> > > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote: >> > >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0), >> > >> > but it's not the first place in this driver where that is done. >> > >> >> > >> What would be the proper way to do it? >> > > >> > > Generally speaking: >> > > >> > > struct dsa_port *cpu_dp; >> > > >> > > dsa_switch_for_each_cpu_port(cpu_dp, ds) >> > > break; >> > > >> > > // use cpu_dp >> > > >> > > If your code runs after dsa_tree_setup_default_cpu(), which contains the >> > > "DSA: tree %d has no CPU port\n" check, you don't even need to check >> > > whether cpu_dp was found or not - it surely was. Everything that runs >> > > after dsa_register_switch() has completed successfully - for example the >> > > DSA ->setup() method - qualifies here. >> > >> > In this particular driver, the setup function contains this: >> > >> > /* Make sure that port 0 is the cpu port */ >> > if (!dsa_is_cpu_port(ds, 0)) { >> > dev_err(chip->dev, "port 0 is not the CPU port\n"); >> > return -EINVAL; >> > } >> > >> > I take this to mean that port 0 is guaranteed to be the cpu port. Of >> > course, it can't hurt to be thorough just in case that check is ever >> > removed. >> >> Yes, I saw that, and I said that there are other places in the driver >> that assume port 0 is the CPU port. Although I don't know why that is, >> if the switch can only operate like that, etc. I just pointed out how it >> would be preferable to get a hold of the CPU port in a regular DSA >> driver without any special constraints. > > Ah, silly me, I should have paid more attention on where you're actually > inserting the code. You could have done: > > static int lan9303_port_enable(struct dsa_switch *ds, int port, > struct phy_device *phy) > { > struct dsa_port *dp = dsa_to_port(ds, port); > struct lan9303 *chip = ds->priv; > > if (!dsa_port_is_user(dp)) > return 0; > > vlan_vid_add(dp->cpu_dp->master, htons(ETH_P_8021Q), port); > > return lan9303_enable_processing_port(chip, port); > } > > the advantage being that if this driver ever supports the remapping of > the CPU port, or multiple CPU ports, this logic wouldn't need to be > changed, as it also conveys the user-to-CPU port affinity. The LAN9303 has (R)MII for port 0 and internal PHYs for ports 1/2, so there's really only one sensible way to connect it, even though the switch core has identical functionality for all ports.
On Wed, Feb 16, 2022 at 05:47:32PM +0000, Måns Rullgård wrote: > The LAN9303 has (R)MII for port 0 and internal PHYs for ports 1/2, so > there's really only one sensible way to connect it, even though the > switch core has identical functionality for all ports. As strange as it may seem to you, people are connecting other switches to a Beaglebone Black and using one of the internal PHY ports as a CPU port. That driver did not need any modification for that particular aspect (the port number), even though that use case was not directly planned: https://patchwork.kernel.org/project/netdevbpf/patch/20210814025003.2449143-11-colin.foster@in-advantage.com/#24380929
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 2de67708bbd2..460c998c0c33 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -1078,20 +1079,28 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy) { struct lan9303 *chip = ds->priv; + struct net_device *master; if (!dsa_is_user_port(ds, port)) return 0; + master = dsa_to_port(chip->ds, 0)->master; + vlan_vid_add(master, htons(ETH_P_8021Q), port); + return lan9303_enable_processing_port(chip, port); } Secondly, the cpsw driver strips VLAN tags from incoming frames, and this prevents the DSA parsing from working. As a dirty workaround, I did this: diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c index 424e644724e4..e15f42ece8bf 100644 --- a/drivers/net/ethernet/ti/cpsw_priv.c +++ b/drivers/net/ethernet/ti/cpsw_priv.c @@ -235,6 +235,7 @@ void cpsw_rx_vlan_encap(struct sk_buff *skb) /* Remove VLAN header encapsulation word */ skb_pull(skb, CPSW_RX_VLAN_ENCAP_HDR_SIZE); + return; pkt_type = (rx_vlan_encap_hdr >>
The hardware I'm working on has a LAN9303 switch connected to the Ethernet port of an AM335x (ZCE package). In trying to make DSA work with this combination, I have encountered two problems. Firstly, the cpsw driver configures the hardware to filter out frames with unknown VLAN tags. To make it accept the tagged frames coming from the LAN9303, I had to modify the latter driver like this: CPSW_RX_VLAN_ENCAP_HDR_PKT_TYPE_SHIFT) & With these changes, everything seems to work as expected. Now I'd appreciate if someone could tell me how I should have done this. Please don't make me send an actual patch.