Re: [net-next] net: dsa: qca8k: only change the MIB_EN bit in MODULE_EN register

On Mon, Nov 08, 2021 at 10:39:27PM +0100, Robert Marko wrote:

On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:

quoted

On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean [off-list ref] wrote:

quoted

Timed out waiting for ACK/NACK from John.

On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:

quoted

From: Gabor Juhos <redacted>

The MIB module needs to be enabled in the MODULE_EN register in
order to make it to counting. This is done in the qca8k_mib_init()
function. However instead of only changing the MIB module enable
bit, the function writes the whole register. As a side effect other
internal modules gets disabled.

Please be more specific.
The MODULE_EN register contains these other bits:
BIT(0): MIB_EN
BIT(1): ACL_EN (ACL module enable)
BIT(2): L3_EN (Layer 3 offload enable)
BIT(10): SPECIAL_DIP_EN (Enable special DIP (224.0.0.x or ff02::1) broadcast
0 = Use multicast DP
1 = Use broadcast DP)

quoted

Fix up the code to only change the MIB module specific bit.

Clearing which one of the above bits bothers you? The driver for the
qca8k switch supports neither layer 3 offloading nor ACLs, and I don't
really know what this special DIP packet/header is).

Generally the assumption for OF-based drivers is that one should not
rely on any configuration done by prior boot stages, so please explain
what should have worked but doesn't.

Hi,
I think that the commit message wasn't clear enough and that's my fault for not
fixing it up before sending.

Yes, it is not. If things turn out to need changing, you should resend
with an updated commit message.

quoted

MODULE_EN register has 3 more bits that aren't documented in the QCA8337
datasheet but only in the IPQ4019 one but they are there.
Those are:
BIT(31) S17C_INT (This one is IPQ4019 specific)
BIT(9) LOOKUP_ERR_RST_EN
BIT(10) QM_ERR_RST_EN

Are you sure that BIT(10) is QM_ERR_RST_EN on IPQ4019? Because in the
QCA8334 document I'm looking at, it is SPECIAL_DIP_EN.

Sorry, QM_ERR_RST_EN is BIT(8) and it as well as LOOKUP_ERR_RST_EN should
be exactly the same on QCA833x switches as well as IPQ4019 uses a
variant of QCA8337N.

quoted

quoted

Lookup and QM bits as well as the DIP default to 1 while the INT bit is 0.

Clearing the QM and Lookup bits is what is bothering me, why should we clear HW
default bits without mentioning that they are being cleared and for what reason?

To be fair, BIT(9) is marked as RESERVED and documented as being set to 1,
so writing a zero is probably not very smart.

quoted

We aren't depending on the bootloader or whatever configuring the switch, we are
even invoking the HW reset before doing anything to make sure that the
whole networking
subsystem in IPQ4019 is back to HW defaults to get rid of various
bootloader hackery.

Gabor found this while working on IPQ4019 support and to him and to me it looks
like a bug.

A bug with what impact? I don't have a description of those bits that
get unset. What do they do, what doesn't work?

LOOKUP_ERR_RST_EN:
1b1:Enableautomatic software reset by hardware due to
lookup error.

QM_ERR_RST_EN:
1b1:enableautomatic software reset by hardware due to qm
error.

So clearing these 2 disables the built-in error recovery essentially.

To me clearing the bits even if they are not breaking something now
should at least have a comment in the code that indicates that it's intentional
for some reason.
I wish John would explain the logic behind this.

That sounds... aggressive. Have you or Gabor exercised this error path?
What is supposed to happen? Is software prepared for the hardware to
automatically reset?