On Thu, Nov 16, 2017 at 06:42:50PM -0800, [email protected] wrote:
> From: Tristram Ha <[email protected]>
>
> Add MIB counter reading support.
Hi Tristram
Some more details would be good here.
> +static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
> + u64 *dropped, u64 *cnt)
> +{
> + addr = ksz9477_mib_names[addr].index;
> + ksz9477_r_mib_cnt(dev, port, addr, cnt);
> +}
dropped is unused here. Which seems to make dropped in general unused.
> +static void ksz_mib_read_work(struct work_struct *work)
> +{
> + struct ksz_device *dev =
> + container_of(work, struct ksz_device, mib_read);
> + struct ksz_port *p;
> + struct ksz_port_mib *mib;
> + int i;
> +
> + for (i = 0; i < dev->mib_port_cnt; i++) {
> + p = &dev->ports[i];
> + if (!p->on)
> + continue;
> + mib = &p->mib;
> + mutex_lock(&mib->cnt_mutex);
> +
> + /* read only dropped counters when link is not up */
> + if (p->link_down)
> + p->link_down = 0;
It is not obvious to me what this is doing.
> + else if (!p->link_up)
> + mib->cnt_ptr = dev->reg_mib_cnt;
> + port_r_cnt(dev, i);
> + mutex_unlock(&mib->cnt_mutex);
> + }
> +}
> +
> +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);
> + memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
Should the memcpy be made while holding the mutex? As soon as you
release it, the timer code can start updating the values in
mib->counters. That then makes the freeze pointless.
> void ksz_switch_remove(struct ksz_device *dev)
> {
> + /* timer started */
> + if (dev->mib_read_timer.expires) {
> + del_timer_sync(&dev->mib_read_timer);
> + flush_work(&dev->mib_read);
> + }
Is this race free?
Andrew
