[PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Vivien Didelot]

DSA has several bitmaps to store the type of ports: cpu_port_mask,
dsa_port_mask and enabled_port_mask. But the code is inconsistently
unmasking them.

The legacy code tries to unmask cpu_port_mask and dsa_port_mask but
skips enabled_port_mask.

The new bindings unmasks cpu_port_mask and enabled_port_mask but skips
dsa_port_mask.

In fact there is no need to unmask them because we are in the error
path, and they won't be used after. Instead of fixing the unmasking,
simply remove them.

Vivien Didelot (2):
  net: dsa: legacy: don't unmask port bitmaps
  net: dsa: don't unmask port bitmaps

 net/dsa/dsa2.c   | 4 ----
 net/dsa/legacy.c | 4 ----
 2 files changed, 8 deletions(-)

-- 
2.14.2

[PATCH net-next 1/2] net: dsa: legacy: don't unmask port bitmaps

[Vivien Didelot]

The legacy code does not unmask the cpu_port_mask and dsa_port_mask as
stated. But this is done on the error path and those masks won't be used
after that. So instead of fixing the bit operation, simply remove it.

Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/legacy.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index b6c88fd33d4f..93c1c43bcc58 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -272,10 +272,6 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
 			continue;
 		dsa_cpu_dsa_destroy(&ds->ports[port]);
-
-		/* Clearing a bit which is not set does no harm */
-		ds->cpu_port_mask |= ~(1 << port);
-		ds->dsa_port_mask |= ~(1 << port);
 	}
 
 	if (ds->slave_mii_bus && ds->ops->phy_read)
-- 
2.14.2

[PATCH net-next 2/2] net: dsa: don't unmask port bitmaps

[Vivien Didelot]

The unapply functions are called on the error path.

As for dsa_port_mask, enabled_port_mask and cpu_port_mask won't be used
after so there's no need to unmask the corresponding port bit from them.

This makes dsa_cpu_port_unapply() and dsa_dsa_port_unapply() identical,
which can be factorized later.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa2.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 9e8b8aab049d..62485a57dbfc 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -260,8 +260,6 @@ static void dsa_cpu_port_unapply(struct dsa_port *port)
 {
 	devlink_port_unregister(&port->devlink_port);
 	dsa_cpu_dsa_destroy(port);
-	port->ds->cpu_port_mask &= ~BIT(port->index);
-
 }
 
 static int dsa_user_port_apply(struct dsa_port *port)
@@ -300,7 +298,6 @@ static void dsa_user_port_unapply(struct dsa_port *port)
 	if (port->slave) {
 		dsa_slave_destroy(port->slave);
 		port->slave = NULL;
-		port->ds->enabled_port_mask &= ~(1 << port->index);
 	}
 }
 
@@ -512,7 +509,6 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	tag_ops = dsa_resolve_tag_protocol(tag_protocol);
 	if (IS_ERR(tag_ops)) {
 		dev_warn(ds->dev, "No tagger for this switch\n");
-		ds->cpu_port_mask &= ~BIT(index);
 		return PTR_ERR(tag_ops);
 	}
 
-- 
2.14.2

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Andrew Lunn]

On Mon, Oct 23, 2017 at 02:17:29PM -0400, Vivien Didelot wrote:
> DSA has several bitmaps to store the type of ports: cpu_port_mask,
> dsa_port_mask and enabled_port_mask. But the code is inconsistently
> unmasking them.
> 
> The legacy code tries to unmask cpu_port_mask and dsa_port_mask but
> skips enabled_port_mask.
> 
> The new bindings unmasks cpu_port_mask and enabled_port_mask but skips
> dsa_port_mask.
> 
> In fact there is no need to unmask them because we are in the error
> path, and they won't be used after. Instead of fixing the unmasking,
> simply remove them.

Hi Vivien

I'm not looked at the code, travelling and don't have time.

What happens if the failure is -PROBE_DEFERRED, and it tried again
later. Will these masks be set back to 0? Or will they retain the old
values? I think there is supposed to be symmetry here, so that we undo
what we did, and so the next time we try again, we start from a good
state.

	Andrew

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Oct 23, 2017 at 02:17:29PM -0400, Vivien Didelot wrote:
>> DSA has several bitmaps to store the type of ports: cpu_port_mask,
>> dsa_port_mask and enabled_port_mask. But the code is inconsistently
>> unmasking them.
>> 
>> The legacy code tries to unmask cpu_port_mask and dsa_port_mask but
>> skips enabled_port_mask.
>> 
>> The new bindings unmasks cpu_port_mask and enabled_port_mask but skips
>> dsa_port_mask.
>> 
>> In fact there is no need to unmask them because we are in the error
>> path, and they won't be used after. Instead of fixing the unmasking,
>> simply remove them.
>
> I'm not looked at the code, travelling and don't have time.

heu, ok.

> What happens if the failure is -PROBE_DEFERRED, and it tried again
> later. Will these masks be set back to 0? Or will they retain the old
> values? I think there is supposed to be symmetry here, so that we undo
> what we did, and so the next time we try again, we start from a good
> state.

The type of a port is a static information parsed either from device
tree or from platform data. Thus there is no symmetry needed here.

The fact that the unmasking of these bitmaps is currently erroneous also
shows that it is unnecessary.


Thanks,

        Vivien

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Florian Fainelli]

On 10/23/2017 02:26 PM, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
>> On Mon, Oct 23, 2017 at 02:17:29PM -0400, Vivien Didelot wrote:
>>> DSA has several bitmaps to store the type of ports: cpu_port_mask,
>>> dsa_port_mask and enabled_port_mask. But the code is inconsistently
>>> unmasking them.
>>>
>>> The legacy code tries to unmask cpu_port_mask and dsa_port_mask but
>>> skips enabled_port_mask.
>>>
>>> The new bindings unmasks cpu_port_mask and enabled_port_mask but skips
>>> dsa_port_mask.
>>>
>>> In fact there is no need to unmask them because we are in the error
>>> path, and they won't be used after. Instead of fixing the unmasking,
>>> simply remove them.
>>
>> I'm not looked at the code, travelling and don't have time.
> 
> heu, ok.
> 
>> What happens if the failure is -PROBE_DEFERRED, and it tried again
>> later. Will these masks be set back to 0? Or will they retain the old
>> values? I think there is supposed to be symmetry here, so that we undo
>> what we did, and so the next time we try again, we start from a good
>> state.

In case of probe deferral, you get the full probe function to exit with
an error, and that usually involves freeing the recently allocated
dsa_switch instance, and then allocating a new one when probe is
re-entered, so that should not be a problem.

> 
> The type of a port is a static information parsed either from device
> tree or from platform data. Thus there is no symmetry needed here.
> 
> The fact that the unmasking of these bitmaps is currently erroneous also
> shows that it is unnecessary.

As much as I would like to see this being symmetrical here, as Vivien
points out, this does not appear to be the case because of missing code,
and it does not seem to solve a particular problem in being symmetrical.
-- 
Florian

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Andrew Lunn]

> In case of probe deferral, you get the full probe function to exit with
> an error, and that usually involves freeing the recently allocated
> dsa_switch instance, and then allocating a new one when probe is
> re-entered, so that should not be a problem.

Hi Florian

That is the simple case. I remember having problems with more complex
cases, D in DSA. Switches 1 and 2 probe O.K, switch 3 fail with
EPROBE_DEFER. Switch 3, as you say, releases its dsa_switch instance,
so will get a freshly zero'ed new instance when it probes
again. However, switches 1 and 2 only experience the unwind at the DSA
level. The devices are not removed and later probed again. They have a
'dirty' dsa_switch structure the next time they are applied.

I just think there might be potential for regressions here. But i've
not yet looked at the details to really know if there actually is.

    Andrew

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 24 Oct 2017 11:22:34 +0200

>> In case of probe deferral, you get the full probe function to exit with
>> an error, and that usually involves freeing the recently allocated
>> dsa_switch instance, and then allocating a new one when probe is
>> re-entered, so that should not be a problem.
> 
> Hi Florian
> 
> That is the simple case. I remember having problems with more complex
> cases, D in DSA. Switches 1 and 2 probe O.K, switch 3 fail with
> EPROBE_DEFER. Switch 3, as you say, releases its dsa_switch instance,
> so will get a freshly zero'ed new instance when it probes
> again. However, switches 1 and 2 only experience the unwind at the DSA
> level. The devices are not removed and later probed again. They have a
> 'dirty' dsa_switch structure the next time they are applied.
> 
> I just think there might be potential for regressions here. But i've
> not yet looked at the details to really know if there actually is.

I realize there is still some trepidation about these patches, but it
seems like the only real way to find out if it causes regressions is
to apply them and watch carefully.

So I've applied this and if anything pops up we can easily revert.

Thanks.

Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

[Andrew Lunn]

> > I just think there might be potential for regressions here. But i've
> > not yet looked at the details to really know if there actually is.
> 
> I realize there is still some trepidation about these patches, but it
> seems like the only real way to find out if it causes regressions is
> to apply them and watch carefully.
> 
> So I've applied this and if anything pops up we can easily revert.

Hi David

Yes, this is good. I will try to test this next week with the platform
i had issues with. We are only going to hit problems with D in DSA
platforms, and there are not many of them. So even if it is broken, it
will not affect many people.

	   Andrew