Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz <quentin.schulz@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

Hi Andrew,

On Fri, Sep 14, 2018 at 03:01:56PM +0200, Andrew Lunn wrote:
> Hi Quentin
> 
> > +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {
> 
> You could add a const to that.
> 

ACK.

> > +	{
> > +		.string	= "phy_receive_errors",
> > +		.reg	= MSCC_PHY_ERR_RX_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_false_carrier",
> > +		.reg	= MSCC_PHY_ERR_FALSE_CARRIER_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_link_disconnect",
> > +		.reg	= MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
> > +		.page	= MSCC_PHY_PAGE_STANDARD,
> > +		.mask	= ERR_CNT_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_crc_good_count",
> > +		.reg	= MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
> > +		.page	= MSCC_PHY_PAGE_EXTENDED,
> > +		.mask	= VALID_CRC_CNT_CRC_MASK,
> > +	}, {
> > +		.string	= "phy_cu_media_crc_error_count",
> > +		.reg	= MSCC_PHY_EXT_PHY_CNTL_4,
> > +		.page	= MSCC_PHY_PAGE_EXTENDED,
> > +		.mask	= ERR_CNT_MASK,
> > +	},
> > +};
> 
> > +static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
> > +{
> > +	struct vsc8531_private *priv = phydev->priv;
> > +	int val;
> > +	u64 ret;
> > +
> > +	vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);
> 
> I might of asked this before...
> 
> Does changing the page effect registers in the lower range? It is
> possible for other operations to happen at the same time, and you
> don't want for example a status read to happen from some other
> extended page register because a statistics read is happening.
> 

When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.

> phy_read_page() and phy_write_page() will do the needed locking if
> this is an issue.
> 

That's awesome! Didn't know it existed. Thanks a ton!

Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.

That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations. Couldn't we use the
phy_device mutex instead (as it's currently done in the whole driver)?
Or is it worse/comparable in performance to the suggested solution?

> > @@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
> >  	vsc8531->rate_magic = rate_magic;
> >  	vsc8531->nleds = 2;
> >  	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> > +	vsc8531->hw_stats = vsc85xx_hw_stats;
> > +	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
> > +	vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
> > +				      sizeof(u64) * vsc8531->nstats,
> > +				      GFP_KERNEL);
> 
> devm_kmalloc_array()? The security people prefer that.
> 

ACK.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]