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: <SN1PR11MB04462D90B223D5F74BC8BE80ECA90@SN1PR11MB0446.namprd11.prod.outlook.com>
Date:   Thu, 6 Dec 2018 20:16:48 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <andrew@...n.ch>
CC:     <f.fainelli@...il.com>, <pavel@....cz>,
        <UNGLinuxDriver@...rochip.com>, <netdev@...r.kernel.org>
Subject: RE: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading
 support

> > +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?
>

I will update as suggested.
 
> > +}
> > +
> >  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.

I wonder what is the official way to clear the counters.  For network debugging
It is good to clear the counters to start fresh to see which frame is not
being sent or received.  Typically the device is reset when it is shutdown as there
are hardware problems.  I would think it is the job of applications like SNMP Manager
to keep track of MIB counters throughout the life of a running system.