dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Salvatore Mesoraca <[email protected]>
---
net/dsa/switch.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index b935117..78e9897 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -136,7 +136,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
{
const struct switchdev_obj_port_mdb *mdb = info->mdb;
struct switchdev_trans *trans = info->trans;
- DECLARE_BITMAP(group, ds->num_ports);
+ DECLARE_BITMAP(group, DSA_MAX_PORTS);
int port;
/* Build a mask of Multicast group members */
@@ -204,7 +204,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
{
const struct switchdev_obj_port_vlan *vlan = info->vlan;
struct switchdev_trans *trans = info->trans;
- DECLARE_BITMAP(members, ds->num_ports);
+ DECLARE_BITMAP(members, DSA_MAX_PORTS);
int port;
/* Build a mask of VLAN members */
--
1.9.1
Hi Salvatore,
Salvatore Mesoraca <[email protected]> writes:
> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Salvatore Mesoraca <[email protected]>
NAK.
We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
Thanks,
Vivien
On 03/13/2018 12:58 PM, Vivien Didelot wrote:
> Hi Salvatore,
>
> Salvatore Mesoraca <[email protected]> writes:
>
>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <[email protected]>
>
> NAK.
>
> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
Then this means that we need to allocate a bitmap from the heap, which
sounds a bit superfluous and could theoretically fail... not sure which
way is better, but bumping the size to DSA_MAX_PORTS definitively does
help people working on enabling -Wvla.
--
Florian
2018-03-13 20:58 GMT+01:00 Vivien Didelot <[email protected]>:
> Hi Salvatore,
Hi Vivien,
> Salvatore Mesoraca <[email protected]> writes:
>
>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Salvatore Mesoraca <[email protected]>
>
> NAK.
>
> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
I can rewrite the patch using kmalloc.
Although, if ds->num_ports will always be less than or equal to 12, it
should be better to
just use DSA_MAX_PORTS.
Thank you,
Salvatore
From: Salvatore Mesoraca
> Sent: 13 March 2018 22:01
> 2018-03-13 20:58 GMT+01:00 Vivien Didelot <[email protected]>:
> > Hi Salvatore,
>
> Hi Vivien,
>
> > Salvatore Mesoraca <[email protected]> writes:
> >
> >> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> >> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
> >>
> >> [1] https://lkml.org/lkml/2018/3/7/621
> >>
> >> Signed-off-by: Salvatore Mesoraca <[email protected]>
> >
> > NAK.
> >
> > We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> > and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>
> I can rewrite the patch using kmalloc.
> Although, if ds->num_ports will always be less than or equal to 12, it
> should be better to
> just use DSA_MAX_PORTS.
Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
than the number of bits in a word?
David
2018-03-14 12:24 GMT+01:00 David Laight <[email protected]>:
> Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
> than the number of bits in a word?
It allocates ceiling(size/8) "unsigned long"s, so yes.
2018-03-14 13:48 GMT+01:00 Salvatore Mesoraca <[email protected]>:
> 2018-03-14 12:24 GMT+01:00 David Laight <[email protected]>:
>> Isn't using DECLARE_BITMAP() completely OTT when the maximum size is less
>> than the number of bits in a word?
>
> It allocates ceiling(size/8) "unsigned long"s, so yes.
Actually I meant ceiling(size/8/sizeof(unsigned long))
I'm sorry for the typo.
Salvatore
2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:
> On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> Hi Salvatore,
>>
>> Salvatore Mesoraca <[email protected]> writes:
>>
>>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>>
>>> [1] https://lkml.org/lkml/2018/3/7/621
>>>
>>> Signed-off-by: Salvatore Mesoraca <[email protected]>
>>
>> NAK.
>>
>> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
>> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>
> Then this means that we need to allocate a bitmap from the heap, which
> sounds a bit superfluous and could theoretically fail... not sure which
> way is better, but bumping the size to DSA_MAX_PORTS definitively does
> help people working on enabling -Wvla.
Hi Florian,
Should I consider this patch still NAKed or not?
Should I resend the patch with some modifications?
Thank you,
Salvatore
On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote:
> 2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:
> > On 03/13/2018 12:58 PM, Vivien Didelot wrote:
> >> Hi Salvatore,
> >>
> >> Salvatore Mesoraca <[email protected]> writes:
> >>
> >>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
> >>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
> >>>
> >>> [1] https://lkml.org/lkml/2018/3/7/621
> >>>
> >>> Signed-off-by: Salvatore Mesoraca <[email protected]>
> >>
> >> NAK.
> >>
> >> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
> >> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
> >
> > Then this means that we need to allocate a bitmap from the heap, which
> > sounds a bit superfluous and could theoretically fail... not sure which
> > way is better, but bumping the size to DSA_MAX_PORTS definitively does
> > help people working on enabling -Wvla.
>
> Hi Florian,
>
> Should I consider this patch still NAKed or not?
> Should I resend the patch with some modifications?
Hi Salvatore
We have been removing all uses of DSA_MAX_PORTS. I don't particularly
like arbitrary limits on how many ports a switch can have, or how many
switches a board can have.
So i would prefer to not use DSA_MAX_PORTS here.
You could make the bitmap part of the dsa_switch structure. This is
allocated by dsa_switch_alloc() and is passed the number of ports.
Doing the allocation there means you don't need to worry about it
failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().
Andrew
On Sat, May 5, 2018 at 8:39 AM, Andrew Lunn <[email protected]> wrote:
> On Sat, May 05, 2018 at 12:36:36PM +0200, Salvatore Mesoraca wrote:
>> 2018-03-13 21:06 GMT+01:00 Florian Fainelli <[email protected]>:
>> > On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> >> Hi Salvatore,
>> >>
>> >> Salvatore Mesoraca <[email protected]> writes:
>> >>
>> >>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>> >>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>> >>>
>> >>> [1] https://lkml.org/lkml/2018/3/7/621
>> >>>
>> >>> Signed-off-by: Salvatore Mesoraca <[email protected]>
>> >>
>> >> NAK.
>> >>
>> >> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
>> >> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>> >
>> > Then this means that we need to allocate a bitmap from the heap, which
>> > sounds a bit superfluous and could theoretically fail... not sure which
>> > way is better, but bumping the size to DSA_MAX_PORTS definitively does
>> > help people working on enabling -Wvla.
>>
>> Hi Florian,
>>
>> Should I consider this patch still NAKed or not?
>> Should I resend the patch with some modifications?
>
> Hi Salvatore
>
> We have been removing all uses of DSA_MAX_PORTS. I don't particularly
> like arbitrary limits on how many ports a switch can have, or how many
> switches a board can have.
>
> So i would prefer to not use DSA_MAX_PORTS here.
>
> You could make the bitmap part of the dsa_switch structure. This is
> allocated by dsa_switch_alloc() and is passed the number of ports.
> Doing the allocation there means you don't need to worry about it
> failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().
Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be
single-threaded?
-Kees
--
Kees Cook
Pixel Security
> > You could make the bitmap part of the dsa_switch structure. This is
> > allocated by dsa_switch_alloc() and is passed the number of ports.
> > Doing the allocation there means you don't need to worry about it
> > failing in dsa_switch_mdb_add() or dsa_switch_vlan_add().
>
> Are dsa_switch_mdb_add() and dsa_switch_vlan_add() guaranteed to be
> single-threaded?
Yes, that is the interesting question here.... against each other, or
themselves?
They are called from a notifier chain. It is the same notifier chain
for both dsa_switch_mdb_add() and dsa_switch_vlan_add().
notifier_call_chain() itself appears to not provide any guarantees
about the same handler being called in parallel.
It is dsa_port_notify() which is calling the notifier_call_chain().
This is being called by both dsa_port_vlan_add() and
dsa_port_mdb_add() in dsa_slave_port_obj_add(). This is a switchdev
op. switchdev_port_obj_add_now() does have ASSERT_RTNL(); So that
should serialize everything.
Andrew