diff mbox series

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

Message ID 20211104124927.364683-1-robert.marko@sartura.hr (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: dsa: qca8k: only change the MIB_EN bit in MODULE_EN register | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Robert Marko Nov. 4, 2021, 12:49 p.m. UTC 
From: Gabor Juhos <j4g8y7@gmail.com>

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.

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

Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/net/dsa/qca8k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vladimir Oltean Nov. 4, 2021, 12:52 p.m. UTC | #1 
On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> From: Gabor Juhos <j4g8y7@gmail.com>
> 
> 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.
> 
> Fix up the code to only change the MIB module specific bit.
> 
> Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/net/dsa/qca8k.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a984f06f6f04..a229776924f8 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
>  	if (ret)
>  		goto exit;
>  
> -	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> +	ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
>  
>  exit:
>  	mutex_unlock(&priv->reg_mutex);
> -- 
> 2.33.1
> 

You should have copied the original patch author too. Adding him now.
Vladimir Oltean Nov. 8, 2021, 8:20 p.m. UTC | #2 
Timed out waiting for ACK/NACK from John.

On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> From: Gabor Juhos <j4g8y7@gmail.com>
> 
> 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)

> 
> 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.

> 
> Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/net/dsa/qca8k.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a984f06f6f04..a229776924f8 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
>  	if (ret)
>  		goto exit;
>  
> -	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> +	ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
>  
>  exit:
>  	mutex_unlock(&priv->reg_mutex);
> -- 
> 2.33.1
>
Robert Marko Nov. 8, 2021, 9:10 p.m. UTC | #3 
On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Timed out waiting for ACK/NACK from John.
>
> On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > From: Gabor Juhos <j4g8y7@gmail.com>
> >
> > 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)
>
> >
> > 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.
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
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?

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.

I hope this clears up things a bit.
Regards,
Robert
>
> >
> > Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> >  drivers/net/dsa/qca8k.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a984f06f6f04..a229776924f8 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
> >       if (ret)
> >               goto exit;
> >
> > -     ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > +     ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> >
> >  exit:
> >       mutex_unlock(&priv->reg_mutex);
> > --
> > 2.33.1
> >
Vladimir Oltean Nov. 8, 2021, 9:18 p.m. UTC | #4 
On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Timed out waiting for ACK/NACK from John.
> >
> > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > From: Gabor Juhos <j4g8y7@gmail.com>
> > >
> > > 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)
> >
> > >
> > > 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.

> 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.

> 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.

> 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?

> I hope this clears up things a bit.
> Regards,
> Robert
> >
> > >
> > > Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > >  drivers/net/dsa/qca8k.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > index a984f06f6f04..a229776924f8 100644
> > > --- a/drivers/net/dsa/qca8k.c
> > > +++ b/drivers/net/dsa/qca8k.c
> > > @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
> > >       if (ret)
> > >               goto exit;
> > >
> > > -     ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > > +     ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > >
> > >  exit:
> > >       mutex_unlock(&priv->reg_mutex);
> > > --
> > > 2.33.1
> > >
> 
> 
> 
> -- 
> Robert Marko
> Staff Embedded Linux Engineer
> Sartura Ltd.
> Lendavska ulica 16a
> 10000 Zagreb, Croatia
> Email: robert.marko@sartura.hr
> Web: www.sartura.hr
Robert Marko Nov. 8, 2021, 9:39 p.m. UTC | #5 
On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> > On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Timed out waiting for ACK/NACK from John.
> > >
> > > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > > From: Gabor Juhos <j4g8y7@gmail.com>
> > > >
> > > > 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)
> > >
> > > >
> > > > 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.
>
> > 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.
>
> > 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.
>
> > 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.

Regards,
Robert
>
> > I hope this clears up things a bit.
> > Regards,
> > Robert
> > >
> > > >
> > > > Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")
> > > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > ---
> > > >  drivers/net/dsa/qca8k.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > > index a984f06f6f04..a229776924f8 100644
> > > > --- a/drivers/net/dsa/qca8k.c
> > > > +++ b/drivers/net/dsa/qca8k.c
> > > > @@ -583,7 +583,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
> > > >       if (ret)
> > > >               goto exit;
> > > >
> > > > -     ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > > > +     ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> > > >
> > > >  exit:
> > > >       mutex_unlock(&priv->reg_mutex);
> > > > --
> > > > 2.33.1
> > > >
> >
> >
> >
> > --
> > Robert Marko
> > Staff Embedded Linux Engineer
> > Sartura Ltd.
> > Lendavska ulica 16a
> > 10000 Zagreb, Croatia
> > Email: robert.marko@sartura.hr
> > Web: www.sartura.hr
Vladimir Oltean Nov. 8, 2021, 9:46 p.m. UTC | #6 
On Mon, Nov 08, 2021 at 10:39:27PM +0100, Robert Marko wrote:
> On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> > > On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > Timed out waiting for ACK/NACK from John.
> > > >
> > > > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > > > From: Gabor Juhos <j4g8y7@gmail.com>
> > > > >
> > > > > 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)
> > > >
> > > > >
> > > > > 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.
> >
> > > 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.
> >
> > > 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.
> >
> > > 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?
Robert Marko Nov. 8, 2021, 9:56 p.m. UTC | #7 
On Mon, Nov 8, 2021 at 10:46 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Nov 08, 2021 at 10:39:27PM +0100, Robert Marko wrote:
> > On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> > > > On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > Timed out waiting for ACK/NACK from John.
> > > > >
> > > > > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > > > > From: Gabor Juhos <j4g8y7@gmail.com>
> > > > > >
> > > > > > 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)
> > > > >
> > > > > >
> > > > > > 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.
> > >
> > > > 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.
> > >
> > > > 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.
> > >
> > > > 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?

I am not trying to be aggressive, but to me, this is either a bug or they are
intentionally cleaned but it's not documented.
Have tried triggering the QM error, but couldn't hit it even when
doing crazy stuff.
It should be nearly impossible to hit it, but it's there to prevent
the switch from just locking up
under extreme conditions (At least that is how I understand it).

I don't think the driver currently even monitors the QM registers at all.
I can understand clearing these bits intentionally, but it's gotta be
documented otherwise
somebody else is gonna think is a bug/mistake/whatever in the code.

Regards,
Robert
--
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr
Vladimir Oltean Nov. 8, 2021, 9:59 p.m. UTC | #8 
On Mon, Nov 08, 2021 at 10:56:51PM +0100, Robert Marko wrote:
> On Mon, Nov 8, 2021 at 10:46 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Mon, Nov 08, 2021 at 10:39:27PM +0100, Robert Marko wrote:
> > > On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> > > > > On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > > >
> > > > > > Timed out waiting for ACK/NACK from John.
> > > > > >
> > > > > > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > > > > > From: Gabor Juhos <j4g8y7@gmail.com>
> > > > > > >
> > > > > > > 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)
> > > > > >
> > > > > > >
> > > > > > > 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.
> > > >
> > > > > 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.
> > > >
> > > > > 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.
> > > >
> > > > > 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?
> 
> I am not trying to be aggressive, but to me, this is either a bug or they are
> intentionally cleaned but it's not documented.
> Have tried triggering the QM error, but couldn't hit it even when
> doing crazy stuff.
> It should be nearly impossible to hit it, but it's there to prevent
> the switch from just locking up
> under extreme conditions (At least that is how I understand it).
> 
> I don't think the driver currently even monitors the QM registers at all.
> I can understand clearing these bits intentionally, but it's gotta be
> documented otherwise
> somebody else is gonna think is a bug/mistake/whatever in the code.

Oh no no, I'm not saying that you're aggressive, but the hardware
behavior of automatically performing a software reset.

The driver keeps state. If the switch just resets by itself, what do you
think will continue to work fine afterwards? The code path needs testing.
I am not convinced that a desynchronized software state is any better
than a lockup.
Robert Marko Nov. 8, 2021, 10:13 p.m. UTC | #9 
On Mon, Nov 8, 2021 at 10:59 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Nov 08, 2021 at 10:56:51PM +0100, Robert Marko wrote:
> > On Mon, Nov 8, 2021 at 10:46 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Mon, Nov 08, 2021 at 10:39:27PM +0100, Robert Marko wrote:
> > > > On Mon, Nov 8, 2021 at 10:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > On Mon, Nov 08, 2021 at 10:10:19PM +0100, Robert Marko wrote:
> > > > > > On Mon, Nov 8, 2021 at 9:21 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > > > >
> > > > > > > Timed out waiting for ACK/NACK from John.
> > > > > > >
> > > > > > > On Thu, Nov 04, 2021 at 01:49:27PM +0100, Robert Marko wrote:
> > > > > > > > From: Gabor Juhos <j4g8y7@gmail.com>
> > > > > > > >
> > > > > > > > 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)
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > >
> > > > > > 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.
> > > > >
> > > > > > 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.
> > > > >
> > > > > > 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?
> >
> > I am not trying to be aggressive, but to me, this is either a bug or they are
> > intentionally cleaned but it's not documented.
> > Have tried triggering the QM error, but couldn't hit it even when
> > doing crazy stuff.
> > It should be nearly impossible to hit it, but it's there to prevent
> > the switch from just locking up
> > under extreme conditions (At least that is how I understand it).
> >
> > I don't think the driver currently even monitors the QM registers at all.
> > I can understand clearing these bits intentionally, but it's gotta be
> > documented otherwise
> > somebody else is gonna think is a bug/mistake/whatever in the code.
>
> Oh no no, I'm not saying that you're aggressive, but the hardware
> behavior of automatically performing a software reset.
>
> The driver keeps state. If the switch just resets by itself, what do you
> think will continue to work fine afterwards? The code path needs testing.
> I am not convinced that a desynchronized software state is any better
> than a lockup.

It's really unpredictable, as QCA doesn't specify what does the software reset
actually does, as I doubt that they are completely resetting the
switch to HW defaults.
But since I was not able to trigger the QM error and the resulting
reset, it's hard to tell.
Phylink would probably see the ports going down and trigger the MAC
configuration again,
this should at least allow using the ports and forwarding to CPU again.
However, it may also reset the forwarding config to basically flooding
all ports which is the default
which is not great.

But I do agree that it may not be a lot better than a lockup.

Regards,
Robert
Vladimir Oltean Nov. 8, 2021, 11:38 p.m. UTC | #10 
On Mon, Nov 08, 2021 at 11:13:30PM +0100, Robert Marko wrote:
> > The driver keeps state. If the switch just resets by itself, what do you
> > think will continue to work fine afterwards? The code path needs testing.
> > I am not convinced that a desynchronized software state is any better
> > than a lockup.
> 
> It's really unpredictable, as QCA doesn't specify what does the software reset
> actually does, as I doubt that they are completely resetting the
> switch to HW defaults.
> But since I was not able to trigger the QM error and the resulting
> reset, it's hard to tell.
> Phylink would probably see the ports going down and trigger the MAC
> configuration again,
> this should at least allow using the ports and forwarding to CPU again.
> However, it may also reset the forwarding config to basically flooding
> all ports which is the default
> which is not great.
> 
> But I do agree that it may not be a lot better than a lockup.

I'm not sure what you expect going forward. You haven't proven an issue
with the actual code structure, or an improvement brought by your change.
Allowing the hardware to autonomously reconfigure itself, even if
partially, is out of the question (of course, that's if and only if I
understand correctly the info that you've presented).
Robert Marko Nov. 8, 2021, 11:52 p.m. UTC | #11 
On Tue, Nov 9, 2021 at 12:38 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Nov 08, 2021 at 11:13:30PM +0100, Robert Marko wrote:
> > > The driver keeps state. If the switch just resets by itself, what do you
> > > think will continue to work fine afterwards? The code path needs testing.
> > > I am not convinced that a desynchronized software state is any better
> > > than a lockup.
> >
> > It's really unpredictable, as QCA doesn't specify what does the software reset
> > actually does, as I doubt that they are completely resetting the
> > switch to HW defaults.
> > But since I was not able to trigger the QM error and the resulting
> > reset, it's hard to tell.
> > Phylink would probably see the ports going down and trigger the MAC
> > configuration again,
> > this should at least allow using the ports and forwarding to CPU again.
> > However, it may also reset the forwarding config to basically flooding
> > all ports which is the default
> > which is not great.
> >
> > But I do agree that it may not be a lot better than a lockup.
>
> I'm not sure what you expect going forward. You haven't proven an issue
> with the actual code structure, or an improvement brought by your change.
> Allowing the hardware to autonomously reconfigure itself, even if
> partially, is out of the question (of course, that's if and only if I
> understand correctly the info that you've presented).

After this discussion, I think that John clears the bits intentionally,
I still don't think its really the best practice to do so in the MIB enablement
without documenting it.
However, since this doesn't seem to be hurting anyone I will drop it and rather
focus on IPQ4019 support which is slowly shaping into something upstreamable.

Regards,
Robert
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a984f06f6f04..a229776924f8 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -583,7 +583,7 @@  qca8k_mib_init(struct qca8k_priv *priv)
 	if (ret)
 		goto exit;
 
-	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+	ret = qca8k_reg_set(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
 
 exit:
 	mutex_unlock(&priv->reg_mutex);