Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support

[Ardelean, Alexandru]

On Tue, 2019-08-13 at 08:48 +0300, Alexandru Ardelean wrote:
> On Mon, 2019-08-12 at 16:33 +0200, Andrew Lunn wrote:
> > [External]
> > 
> > > +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> > > +				   struct adin_hw_stat *stat,
> > > +				   u32 *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val = (ret & 0xffff);
> > > +
> > > +	if (stat->reg2 == 0)
> > > +		return 0;
> > > +
> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val <<= 16;
> > > +	*val |= (ret & 0xffff);
> > > +
> > > +	return 0;
> > > +}
> > 
> > It still looks like you have not dealt with overflow from the LSB into
> > the MSB between the two reads.
> 
> Apologies for forgetting to address this.
> I did not intentionally leave it out; this item got lost after V1 [which had the most remarks].
> Changelog V1 -> V2 was quite bulky, and I did not look at V1 remarks after I finished V2.
> 
> Thanks for snippet.

So, I have to apologize again here.
I guess I was an idiot/n00b about this.

The PHY stats do support snapshot, and I sync-ed with someone from the chip-team to confirm.

Also, from the datasheet[1] (page 29 - FRAME GENERATOR AND CHECKER - 5th paragraph):
--------------------------------------------------------------
The frame checker counts the number of CRC errors and these 
are reported in the receive error counter register (RxErrCnt 
register,  address 0x0014). To ensure synchronization between 
the frame checker error counter and frame checker frame counters,
all of the counters are latched once the receive error counter
register is read. Hence when using the frame checker, the
receive error counter should be read first and then all the other
frame counters and error counters should be read. A latched copy
of the receive frame counter register is available in the
FcFrmCntH and FcFrmCntL registers (register addresses 0x1E.0x940A
and 0x1E.0x940B).
-------------------------------------------------------------

Then in the description of these regs, it mentions (repeteadly):
-------------------------------------------------------------
This register is a latched copy of bits 31:16 of the 32-bit
receive frame counter register. When the receive error counter
(RxErrCnt register address 0x0014) is read, the receive
frame
counter register is latched. A copy of the receive frame counter
register is latched when RxErrCnt is read so that
the error count
and receive frame count are synchronized
-------------------------------------------------------------

I'll re-spin this with the rename of the strings, and maybe do a minor polish of the code.

Thanks & sorry for the noise/trouble
Alex

[1] https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf


> 
> > 	do {
> > 		hi1 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > 		if (hi1 < 0)
> > 			return hi1;
> > 		
> > 		low = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > 		if (low < 0)
> > 			return low;
> > 
> > 		hi2 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > 		if (hi2 < 0)
> > 			return hi1;
> > 	} while (hi1 != hi2)
> > 
> > 	return low | (hi << 16);
> > 
> > 	Andrew