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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180914130156.GB14865@lunn.ch>
Date:   Fri, 14 Sep 2018 15:01:56 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Quentin Schulz <quentin.schulz@...tlin.com>
Cc:     davem@...emloft.net, f.fainelli@...il.com,
        allan.nielsen@...rochip.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, thomas.petazzoni@...tlin.com,
        Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>
Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics
 counters

Hi Quentin

> +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {

You could add a const to that.

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

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

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

> +	if (!vsc8531->stats)
> +		return -ENOMEM;
>  
>  	return vsc85xx_dt_led_modes_get(phydev, default_mode);
>  }

   Andrew