On 2/21/19 2:03 PM, tristram...@microchip.com wrote:
> From: Tristram Ha <tristram...@microchip.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
