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