netdev - Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support

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: <20181205175347.GF12484@lunn.ch>
Date:   Wed, 5 Dec 2018 18:53:47 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Tristram.Ha@...rochip.com
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Pavel Machek <pavel@....cz>, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading
 support

Hi Tristan

> +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	u32 data;
> +	int timeout;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	/* retain the flush/freeze bit */
> +	data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> +	data |= MIB_COUNTER_READ;
> +	data |= (addr << MIB_COUNTER_INDEX_S);
> +	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
> +
> +	timeout = 1000;
> +	do {
> +		ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
> +			    &data);
> +		usleep_range(1, 10);
> +		if (!(data & MIB_COUNTER_READ))
> +			break;
> +	} while (timeout-- > 0);

Could you use readx_poll_timeout() here?

> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +
> +	/* freeze MIB counters if supported */
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, true);
> +	mutex_lock(&mib->cnt_mutex);
> +	port_r_cnt(dev, port);
> +	mutex_unlock(&mib->cnt_mutex);
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, false);

Should the freeze be protected by the mutex as well?

> +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));

I wonder if this memcpy should also be protected by the mutex. As soon
as the mutex is dropped, the scheduled work could start updating
mib->counters in non-atomic ways?

> +}
> +
>  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>  			 struct net_device *br)
>  {
> @@ -255,6 +349,7 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
>  	/* setup slave port */
>  	dev->dev_ops->port_setup(dev, port, false);
>  	dev->dev_ops->phy_setup(dev, port, phy);
> +	dev->dev_ops->port_init_cnt(dev, port);

This is probably not the correct place to do this. MIB counters should
not be cleared by an ifdown/ifup cycle. They should only be cleared
when the driver is probed.