netdev - Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: global2: Expose ATU stats register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 7 Nov 2019 01:12:41 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>, jiri@...lanox.com
Subject: Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: global2: Expose ATU
 stats register

> > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip)
> > +{
> > +	int err;
> > +	u16 val;
> > +
> > +	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	return val & MV88E6XXX_G2_ATU_STATS_MASK;
> > +}
> 
> I would use the logic commonly used in the mv88e6xxx driver for
> functions that may fail, returning only an error code and taking a
> pointer to the correct type as argument, so that we do as usual:
> 
>     err = mv88e6xxx_g2_atu_stats_get(chip, &val);
>     if (err)
>         return err;

Well, actually. Take a look at the next patch. This function gets
changed there. After you reviewed the first devlink patchset adding
control of the ATU algorithm, i went through this patchset and applied
the same comments to it. So i change the implementation to how you
actually wanted it. But i messed up my git commands and that changed
ended up in the next patch :-(

> >  /* Offset 0x0E: ATU Stats Register */
> > -#define MV88E6XXX_G2_ATU_STATS		0x0e
> > +#define MV88E6XXX_G2_ATU_STATS				0x0e
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_0			(0x0 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_1			(0x1 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_2			(0x2 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_BIN_3			(0x3 << 14)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL			(0x0 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC		(0x1 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL		(0x2 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC	(0x3 << 12)
> > +#define MV88E6XXX_G2_ATU_STATS_MASK			0x0fff
> 
> Please use the 0x1234 format for these 16-bit registers.

Ug, no. That is a lot less readable. The datasheet describes there
fields in terms of bit offsets in the registers. Bit 14 and 15 for the
bin, bits 12 and 13 for the mode. You can clearly see that when using
value and shift representation. 0x0200 is much harder to read, and
much more error prone.

      Andrew