netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* DSA using cpsw and lan9303
@ 2022-02-14 16:44 Måns Rullgård
  2022-02-14 17:16 ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Måns Rullgård @ 2022-02-14 16:44 UTC (permalink / raw)
  To: netdev

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);
+
        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.

-- 
Måns Rullgård

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-14 16:44 DSA using cpsw and lan9303 Måns Rullgård
@ 2022-02-14 17:16 ` Florian Fainelli
  2022-02-14 17:43   ` Vladimir Oltean
  2022-02-15 20:54   ` Vladimir Oltean
  0 siblings, 2 replies; 12+ messages in thread
From: Florian Fainelli @ 2022-02-14 17:16 UTC (permalink / raw)
  To: Måns Rullgård, netdev, Egil Hjelmeland, Vladimir Oltean,
	Andrew Lunn, Juergen Borleis, Grygorii Strashko, lorenzo

+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.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-14 17:16 ` Florian Fainelli
@ 2022-02-14 17:43   ` Vladimir Oltean
  2022-02-14 19:17     ` Måns Rullgård
  2022-02-15 20:54   ` Vladimir Oltean
  1 sibling, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-14 17:43 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Måns Rullgård, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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? :)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-14 17:43   ` Vladimir Oltean
@ 2022-02-14 19:17     ` Måns Rullgård
  0 siblings, 0 replies; 12+ messages in thread
From: Måns Rullgård @ 2022-02-14 19:17 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-14 17:16 ` Florian Fainelli
  2022-02-14 17:43   ` Vladimir Oltean
@ 2022-02-15 20:54   ` Vladimir Oltean
  2022-02-16 13:17     ` Måns Rullgård
  1 sibling, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-15 20:54 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Måns Rullgård, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-15 20:54   ` Vladimir Oltean
@ 2022-02-16 13:17     ` Måns Rullgård
  2022-02-16 14:15       ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Måns Rullgård @ 2022-02-16 13:17 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 13:17     ` Måns Rullgård
@ 2022-02-16 14:15       ` Vladimir Oltean
  2022-02-16 14:23         ` Måns Rullgård
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-16 14:15 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 14:15       ` Vladimir Oltean
@ 2022-02-16 14:23         ` Måns Rullgård
  2022-02-16 14:26           ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Måns Rullgård @ 2022-02-16 14:23 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 14:23         ` Måns Rullgård
@ 2022-02-16 14:26           ` Vladimir Oltean
  2022-02-16 17:00             ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-16 14:26 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 14:26           ` Vladimir Oltean
@ 2022-02-16 17:00             ` Vladimir Oltean
  2022-02-16 17:47               ` Måns Rullgård
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-16 17:00 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 17:00             ` Vladimir Oltean
@ 2022-02-16 17:47               ` Måns Rullgård
  2022-02-16 17:55                 ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Måns Rullgård @ 2022-02-16 17:47 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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.

-- 
Måns Rullgård

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: DSA using cpsw and lan9303
  2022-02-16 17:47               ` Måns Rullgård
@ 2022-02-16 17:55                 ` Vladimir Oltean
  0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2022-02-16 17:55 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: Florian Fainelli, netdev, Egil Hjelmeland, Andrew Lunn,
	Juergen Borleis, Grygorii Strashko, lorenzo

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-02-16 17:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-14 16:44 DSA using cpsw and lan9303 Måns Rullgård
2022-02-14 17:16 ` Florian Fainelli
2022-02-14 17:43   ` Vladimir Oltean
2022-02-14 19:17     ` Måns Rullgård
2022-02-15 20:54   ` Vladimir Oltean
2022-02-16 13:17     ` Måns Rullgård
2022-02-16 14:15       ` Vladimir Oltean
2022-02-16 14:23         ` Måns Rullgård
2022-02-16 14:26           ` Vladimir Oltean
2022-02-16 17:00             ` Vladimir Oltean
2022-02-16 17:47               ` Måns Rullgård
2022-02-16 17:55                 ` Vladimir Oltean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).