netdev - Re: [PATCH v3 net-next 2/4] 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] [day] [month] [year] [list]

Message-ID: <c9f89cff-88d9-9e04-b4dd-7b5300988d21@gmail.com>
Date:   Thu, 21 Feb 2019 15:01:43 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tristram.Ha@...rochip.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Pavel Machek <pavel@....cz>
Cc:     UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 2/4] net: dsa: microchip: add MIB counter
 reading support

On 2/21/19 2:03 PM, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Add background MIB counter reading support.
> 
> Port MIB counters should only be read when there is link.  Otherwise it is
> a waste of time as hardware never increases those counters.  There are
> exceptions as some switches keep track of dropped counts no matter what.

Some minor comments below, none of them need to be addressed right now,
but it might be a good idea to do it at a later time.

[snip]

> +
> +static void mib_monitor(struct timer_list *t)
> +{
> +	struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
> +	const struct dsa_port *dp;
> +	struct net_device *netdev;
> +	struct ksz_port_mib *mib;
> +	struct ksz_port *p;
> +	int i;
> +
> +	mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
> +
> +	/* Check which port needs to read MIB counters. */
> +	for (i = 0; i < dev->mib_port_cnt; i++) {
> +		p = &dev->ports[i];
> +		if (!p->on)
> +			continue;

Would not using dsa_port_is_unused() accomplish the same thing here?
Your setup function should not have enabled non-existent/connected to
the outside world ports anyway.

> +		dp = dsa_to_port(dev->ds, i);
> +		netdev = dp->slave;
> +
> +		mib = &p->mib;
> +		mutex_lock(&mib->cnt_mutex);

Have you built this with CONFIG_DEBUG_SLEEP_ATOMIC? timer executes in BH
context (atomic), you cannot hold a mutex here which would be allowed to
sleep.

> +
> +		/* Read only dropped counters when link is not up. */
> +		if (netdev && !netif_carrier_ok(netdev))> +			mib->cnt_ptr = dev->reg_mib_cnt;
> +		mutex_unlock(&mib->cnt_mutex);
> +		p->read = true;
> +	}
> +	schedule_work(&dev->mib_read);
> +}

[snip]

> +
>  int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg)
>  {
>  	struct ksz_device *dev = ds->priv;
> @@ -72,6 +167,26 @@ int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
>  }
>  EXPORT_SYMBOL_GPL(ksz_sset_count);
>  
> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> +	const struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct ksz_device *dev = ds->priv;
> +	struct net_device *netdev;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +	mutex_lock(&mib->cnt_mutex);
> +
> +	/* Only read dropped counters if no link. */
> +	netdev = dp->slave;
> +	if (netdev && !netif_carrier_ok(netdev))
> +		mib->cnt_ptr = dev->reg_mib_cnt;

The netdev is guaranteed to exist, otherwise you would not be in this
function, and we are executing with RTNL held, so the device can't
vanish under your feet.

[snip]
>  
> +/* Modify from readx_poll_timeout in iopoll.h. */
> +#define ksz_pread_poll_timeout(op, dev, p, addr, val, cond, sleep_us, \
> +			       timeout_us) \
> +({ \
> +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> +	might_sleep_if(sleep_us); \
> +	for (;;) { \
> +		op(dev, p, addr, &(val)); \

I don't think you need to create your own custom macro, which you seem
to need such that you can  pass dev and p as arguments, instead you can
create a specific helper function, pass it down to op() as the "addr"
argument, since that is a macro that does not type checking at all. So
something like this:

struct ksz_read_ctx {
	struct ksz_device *dev;
	struct ksz_port *p;
	u16 reg;
};

static int ksz_pread32_poll(struct ksz_read_ctx *ctx)
{
	return ksz_pread32(ctx->dev, ctx->p, ctx->reg);
}

static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
			      u64 *cnt)
{
	struct ksz_port *p = &dev->ports[port];
	struct ksz_read_ctx ctx = {
		.dev = dev,
		.p = &dev->ports[port],
		.reg = REG_PORT_MIB_CTRL_STAT__4
	};
	u32 data;
	int ret;

	/* 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);

	ret = readx_poll_timeout(ksz_pread32_poll, &ctx, data, !(data &
MIB_COUNTER_READ), 10, 1000);

Completely not compile tested, but you get the idea.
-- 
Florian