| Message ID | 20190927163911.11179-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: dsa: rtl8366: Check VLAN ID and not ports | expand |
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
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
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
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
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,
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(-) -- 2.21.0