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

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

[David Miller]

From: Andrew Lunn 
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]

> 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

[Florian Fainelli]

On 10/23/2017 02:26 PM, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn  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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  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

[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