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

Powered by blists - more mailing lists