netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
@ 2019-10-01 14:28 Linus Walleij
  2019-10-02 16:10 ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2019-10-01 14:28 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli; +Cc: netdev, Linus Walleij

There has been some confusion between the port number and
the VLAN ID in this driver. What we need to check for
validity is the VLAN ID, nothing else.

The current confusion came from assigning a few default
VLANs for default routing and we need to rewrite that
properly.

Instead of checking if the port number is a valid VLAN
ID, check the actual VLAN IDs passed in to the callback
one by one as expected.

Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix the iterator to actually check the VLAN ID.
- Use "vid" as variable name.
---
 drivers/net/dsa/rtl8366.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index ca3d17e43ed8..ac88caca5ad4 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -339,10 +339,12 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
 			 const struct switchdev_obj_port_vlan *vlan)
 {
 	struct realtek_smi *smi = ds->priv;
+	u16 vid;
 	int ret;
 
-	if (!smi->ops->is_vlan_valid(smi, port))
-		return -EINVAL;
+	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
+		if (!smi->ops->is_vlan_valid(smi, vid))
+			return -EINVAL;
 
 	dev_info(smi->dev, "prepare VLANs %04x..%04x\n",
 		 vlan->vid_begin, vlan->vid_end);
@@ -370,8 +372,9 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 	u16 vid;
 	int ret;
 
-	if (!smi->ops->is_vlan_valid(smi, port))
-		return;
+	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
+		if (!smi->ops->is_vlan_valid(smi, vid))
+			return;
 
 	dev_info(smi->dev, "add VLAN on port %d, %s, %s\n",
 		 port,
-- 
2.21.0


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

* Re: [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
  2019-10-01 14:28 [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports Linus Walleij
@ 2019-10-02 16:10 ` David Miller
  0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2019-10-02 16:10 UTC (permalink / raw)
  To: linus.walleij; +Cc: andrew, vivien.didelot, f.fainelli, netdev

From: Linus Walleij <linus.walleij@linaro.org>
Date: Tue,  1 Oct 2019 16:28:43 +0200

> There has been some confusion between the port number and
> the VLAN ID in this driver. What we need to check for
> validity is the VLAN ID, nothing else.
> 
> The current confusion came from assigning a few default
> VLANs for default routing and we need to rewrite that
> properly.
> 
> Instead of checking if the port number is a valid VLAN
> ID, check the actual VLAN IDs passed in to the callback
> one by one as expected.
> 
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Applied and queued up for -stable, thanks.

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

* Re: [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
  2019-09-28 20:36     ` Florian Fainelli
@ 2019-09-28 20:43       ` Linus Walleij
  0 siblings, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2019-09-28 20:43 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: Andrew Lunn, Vivien Didelot, netdev

On Sat, Sep 28, 2019 at 10:36 PM Florian Fainelli <f.fainelli@gmail.com> wrote:

> Do we need to duplicate the same is_vlan_valid() check in both the
> vlan_prepare and vlan_add callbacks?

I'm unsure of the semantics of these calls, the check was in the
OpenWrt driver that I started from.

Is it guaranteed that .vlan_prepare() and .vlan_add() are
always called in succession? Then .vlan_prepare() should
be enough.

Yours,
Linus Walleij

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

* Re: [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
  2019-09-28 20:26   ` Linus Walleij
@ 2019-09-28 20:36     ` Florian Fainelli
  2019-09-28 20:43       ` Linus Walleij
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2019-09-28 20:36 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Andrew Lunn, Vivien Didelot, netdev



On 9/28/2019 1:26 PM, Linus Walleij wrote:
> On Fri, Sep 27, 2019 at 6:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 9/27/19 9:39 AM, Linus Walleij wrote:
>>> There has been some confusion between the port number and
>>> the VLAN ID in this driver. What we need to check for
>>> validity is the VLAN ID, nothing else.
>>>
>>> The current confusion came from assigning a few default
>>> VLANs for default routing and we need to rewrite that
>>> properly.
>>>
>>> Instead of checking if the port number is a valid VLAN
>>> ID, check the actual VLAN IDs passed in to the callback
>>> one by one as expected.
>>>
>>> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>  drivers/net/dsa/rtl8366.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
>>> index ca3d17e43ed8..e2c91b75e843 100644
>>> --- a/drivers/net/dsa/rtl8366.c
>>> +++ b/drivers/net/dsa/rtl8366.c
>>> @@ -340,9 +340,11 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
>>>  {
>>>       struct realtek_smi *smi = ds->priv;
>>>       int ret;
>>> +     int i;
>>>
>>> -     if (!smi->ops->is_vlan_valid(smi, port))
>>> -             return -EINVAL;
>>> +     for (i = vlan->vid_begin; i < vlan->vid_end; i++)
>>> +             if (!smi->ops->is_vlan_valid(smi, port))
>>> +                     return -EINVAL;
>>
>> You are still checking the port and not the "i" (VLAN ID) argument here,
>> is there something I am missing?
> 
> No you're right, just correcting old mistakes by making
> new mistakes .. I'll fix, thanks!
> 
Do we need to duplicate the same is_vlan_valid() check in both the
vlan_prepare and vlan_add callbacks?
-- 
Florian

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

* Re: [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
  2019-09-27 16:40 ` Florian Fainelli
@ 2019-09-28 20:26   ` Linus Walleij
  2019-09-28 20:36     ` Florian Fainelli
  0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2019-09-28 20:26 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: Andrew Lunn, Vivien Didelot, netdev

On Fri, Sep 27, 2019 at 6:40 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 9/27/19 9:39 AM, Linus Walleij wrote:
> > There has been some confusion between the port number and
> > the VLAN ID in this driver. What we need to check for
> > validity is the VLAN ID, nothing else.
> >
> > The current confusion came from assigning a few default
> > VLANs for default routing and we need to rewrite that
> > properly.
> >
> > Instead of checking if the port number is a valid VLAN
> > ID, check the actual VLAN IDs passed in to the callback
> > one by one as expected.
> >
> > Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/net/dsa/rtl8366.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> > index ca3d17e43ed8..e2c91b75e843 100644
> > --- a/drivers/net/dsa/rtl8366.c
> > +++ b/drivers/net/dsa/rtl8366.c
> > @@ -340,9 +340,11 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
> >  {
> >       struct realtek_smi *smi = ds->priv;
> >       int ret;
> > +     int i;
> >
> > -     if (!smi->ops->is_vlan_valid(smi, port))
> > -             return -EINVAL;
> > +     for (i = vlan->vid_begin; i < vlan->vid_end; i++)
> > +             if (!smi->ops->is_vlan_valid(smi, port))
> > +                     return -EINVAL;
>
> You are still checking the port and not the "i" (VLAN ID) argument here,
> is there something I am missing?

No you're right, just correcting old mistakes by making
new mistakes .. I'll fix, thanks!

Yours,
Linus Walleij

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

* Re: [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
  2019-09-27 16:39 Linus Walleij
@ 2019-09-27 16:40 ` Florian Fainelli
  2019-09-28 20:26   ` Linus Walleij
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2019-09-27 16:40 UTC (permalink / raw)
  To: Linus Walleij, Andrew Lunn, Vivien Didelot; +Cc: netdev

On 9/27/19 9:39 AM, Linus Walleij wrote:
> There has been some confusion between the port number and
> the VLAN ID in this driver. What we need to check for
> validity is the VLAN ID, nothing else.
> 
> The current confusion came from assigning a few default
> VLANs for default routing and we need to rewrite that
> properly.
> 
> Instead of checking if the port number is a valid VLAN
> ID, check the actual VLAN IDs passed in to the callback
> one by one as expected.
> 
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/dsa/rtl8366.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index ca3d17e43ed8..e2c91b75e843 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -340,9 +340,11 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
>  {
>  	struct realtek_smi *smi = ds->priv;
>  	int ret;
> +	int i;
>  
> -	if (!smi->ops->is_vlan_valid(smi, port))
> -		return -EINVAL;
> +	for (i = vlan->vid_begin; i < vlan->vid_end; i++)
> +		if (!smi->ops->is_vlan_valid(smi, port))
> +			return -EINVAL;

You are still checking the port and not the "i" (VLAN ID) argument here,
is there something I am missing?
-- 
Florian

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

* [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports
@ 2019-09-27 16:39 Linus Walleij
  2019-09-27 16:40 ` Florian Fainelli
  0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2019-09-27 16:39 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli; +Cc: netdev, Linus Walleij

There has been some confusion between the port number and
the VLAN ID in this driver. What we need to check for
validity is the VLAN ID, nothing else.

The current confusion came from assigning a few default
VLANs for default routing and we need to rewrite that
properly.

Instead of checking if the port number is a valid VLAN
ID, check the actual VLAN IDs passed in to the callback
one by one as expected.

Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/rtl8366.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index ca3d17e43ed8..e2c91b75e843 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -340,9 +340,11 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
 {
 	struct realtek_smi *smi = ds->priv;
 	int ret;
+	int i;
 
-	if (!smi->ops->is_vlan_valid(smi, port))
-		return -EINVAL;
+	for (i = vlan->vid_begin; i < vlan->vid_end; i++)
+		if (!smi->ops->is_vlan_valid(smi, port))
+			return -EINVAL;
 
 	dev_info(smi->dev, "prepare VLANs %04x..%04x\n",
 		 vlan->vid_begin, vlan->vid_end);
@@ -369,9 +371,11 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 	u32 untag = 0;
 	u16 vid;
 	int ret;
+	int i;
 
-	if (!smi->ops->is_vlan_valid(smi, port))
-		return;
+	for (i = vlan->vid_begin; i < vlan->vid_end; i++)
+		if (!smi->ops->is_vlan_valid(smi, port))
+			return;
 
 	dev_info(smi->dev, "add VLAN on port %d, %s, %s\n",
 		 port,
-- 
2.21.0


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

end of thread, other threads:[~2019-10-02 16:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 14:28 [PATCH] net: dsa: rtl8366: Check VLAN ID and not ports Linus Walleij
2019-10-02 16:10 ` David Miller
  -- strict thread matches above, loose matches on Subject: below --
2019-09-27 16:39 Linus Walleij
2019-09-27 16:40 ` Florian Fainelli
2019-09-28 20:26   ` Linus Walleij
2019-09-28 20:36     ` Florian Fainelli
2019-09-28 20:43       ` Linus Walleij

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