[PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[Hans Schultz]

For convenience the function mv88e6xxx_g1_atu_op() has been used to read
ATU violations, but the function has other purposes and does not enable
the possibility to read the FID when reading ATU violations.

The FID is needed to get hold of which VID was involved in the violation,
thus the need for future purposes to be able to read the FID.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 40bd67a5c8e9..5d120d53823c 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -114,6 +114,19 @@ static int mv88e6xxx_g1_atu_op_wait(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_ATU_OP, bit, 0);
 }
 
+static int mv88e6xxx_g1_read_atu_violation(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_ATU_OP,
+				 MV88E6XXX_G1_ATU_OP_BUSY |
+				 MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	if (err)
+		return err;
+
+	return mv88e6xxx_g1_atu_op_wait(chip);
+}
+
 static int mv88e6xxx_g1_atu_op(struct mv88e6xxx_chip *chip, u16 fid, u16 op)
 {
 	u16 val;
@@ -359,8 +372,7 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 
 	mv88e6xxx_reg_lock(chip);
 
-	err = mv88e6xxx_g1_atu_op(chip, 0,
-				  MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	err = mv88e6xxx_g1_read_atu_violation(chip);
 	if (err)
 		goto out;
 
-- 
2.30.2

Re: [PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[Vladimir Oltean]

On Wed, Jul 06, 2022 at 02:25:02PM +0200, Hans Schultz wrote:
> For convenience the function mv88e6xxx_g1_atu_op() has been used to read
> ATU violations, but the function has other purposes and does not enable
> the possibility to read the FID when reading ATU violations.
> 
> The FID is needed to get hold of which VID was involved in the violation,
> thus the need for future purposes to be able to read the FID.

Make no mistake, the existing code doesn't disallow reading back the FID
during an ATU Get/Clear Violation operation, and your patch isn't
"allowing" something that wasn't disallowed.

The documentation for the ATU FID register says that its contents is
ignored before the operation starts, and it contains the returned ATU
entry's FID after the operation completes.

So the change simply says: don't bother to write the ATU FID register
with zero, it doesn't matter what this contains. This is probably true,
but the patch needs to do what's written on the box.

Please note that this only even matters at all for switches with
mv88e6xxx_num_databases(chip) > 256, where MV88E6352_G1_ATU_FID is a
dedicated register which this patch avoids writing. For other switches,
the FID is embedded within MV88E6XXX_G1_ATU_CTL or MV88E6XXX_G1_ATU_OP.
So _practically_, for those switches, you are still emitting the
GET_CLR_VIOLATION ATU op with a FID of 0 whether you like it or not, and
this patch introduces a (most likely irrelevant) discrepancy between the
access methods for various switches.

Please note that this observation is relevant for your future changes to
read back the FID too. As I said here:
https://patchwork.kernel.org/project/netdevbpf/patch/20220524152144.40527-4-schultz.hans+netdev@gmail.com/#24912482
you can't just assume that the FID lies within the MV88E6352_G1_ATU_FID
register, just look at the way it is packed within mv88e6xxx_g1_atu_op().
You'll need to unpack it in the same way.

Re: [PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[Jakub Kicinski]

On Thu, 7 Jul 2022 13:28:36 +0300 Vladimir Oltean wrote:
> Make no mistake, the existing code doesn't disallow reading back the FID
> during an ATU Get/Clear Violation operation, and your patch isn't
> "allowing" something that wasn't disallowed.
> 
> The documentation for the ATU FID register says that its contents is
> ignored before the operation starts, and it contains the returned ATU
> entry's FID after the operation completes.
> 
> So the change simply says: don't bother to write the ATU FID register
> with zero, it doesn't matter what this contains. This is probably true,
> but the patch needs to do what's written on the box.
> 
> Please note that this only even matters at all for switches with
> mv88e6xxx_num_databases(chip) > 256, where MV88E6352_G1_ATU_FID is a
> dedicated register which this patch avoids writing. For other switches,
> the FID is embedded within MV88E6XXX_G1_ATU_CTL or MV88E6XXX_G1_ATU_OP.
> So _practically_, for those switches, you are still emitting the
> GET_CLR_VIOLATION ATU op with a FID of 0 whether you like it or not, and
> this patch introduces a (most likely irrelevant) discrepancy between the
> access methods for various switches.
> 
> Please note that this observation is relevant for your future changes to
> read back the FID too. As I said here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20220524152144.40527-4-schultz.hans+netdev@gmail.com/#24912482
> you can't just assume that the FID lies within the MV88E6352_G1_ATU_FID
> register, just look at the way it is packed within mv88e6xxx_g1_atu_op().
> You'll need to unpack it in the same way.

I reckon it'll be useful to render some of this info into the commit
message and adjust the subject so marking Changes Requested.

Re: [PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[netdev]

On 2022-07-07 12:28, Vladimir Oltean wrote:
> On Wed, Jul 06, 2022 at 02:25:02PM +0200, Hans Schultz wrote:
>> For convenience the function mv88e6xxx_g1_atu_op() has been used to 
>> read
>> ATU violations, but the function has other purposes and does not 
>> enable
>> the possibility to read the FID when reading ATU violations.
>> 
>> The FID is needed to get hold of which VID was involved in the 
>> violation,
>> thus the need for future purposes to be able to read the FID.
> 
> Make no mistake, the existing code doesn't disallow reading back the 
> FID
> during an ATU Get/Clear Violation operation, and your patch isn't
> "allowing" something that wasn't disallowed.

It would only read 0 the way it worked. And I don't understand why
mv88e6xxx_g1_atu_op() writes the FID?

> 
> The documentation for the ATU FID register says that its contents is
> ignored before the operation starts, and it contains the returned ATU
> entry's FID after the operation completes.
> 
> So the change simply says: don't bother to write the ATU FID register
> with zero, it doesn't matter what this contains. This is probably true,
> but the patch needs to do what's written on the box.

Writing 0 to the ATU fID register resulted in a read giving zero of 
course.

> 
> Please note that this only even matters at all for switches with
> mv88e6xxx_num_databases(chip) > 256, where MV88E6352_G1_ATU_FID is a
> dedicated register which this patch avoids writing. For other switches,
> the FID is embedded within MV88E6XXX_G1_ATU_CTL or MV88E6XXX_G1_ATU_OP.
> So _practically_, for those switches, you are still emitting the
> GET_CLR_VIOLATION ATU op with a FID of 0 whether you like it or not, 
> and
> this patch introduces a (most likely irrelevant) discrepancy between 
> the
> access methods for various switches.
> 
> Please note that this observation is relevant for your future changes 
> to
> read back the FID too. As I said here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20220524152144.40527-4-schultz.hans+netdev@gmail.com/#24912482
> you can't just assume that the FID lies within the MV88E6352_G1_ATU_FID
> register, just look at the way it is packed within 
> mv88e6xxx_g1_atu_op().
> You'll need to unpack it in the same way.

So I need a new function to read the FID that mimics 
mv88e6xxx_g1_atu_op()
as far as I understand?