linux-kernel - Re: [PATCH v2 net-next 1/5] net: dsa: microchip: add rmon grouping for ethtool statistics

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]

Date:   Fri, 17 Feb 2023 15:59:12 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Rakesh Sankaranarayanan <rakesh.sankaranarayanan@...rochip.com>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <woojung.huh@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
        <andrew@...n.ch>, <f.fainelli@...il.com>, <olteanv@...il.com>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>,
        Thangaraj Samynathan <Thangaraj.S@...rochip.com>
Subject: Re: [PATCH v2 net-next 1/5] net: dsa: microchip: add rmon grouping
 for ethtool statistics

From: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@...rochip.com>
Date: Fri, 17 Feb 2023 16:32:07 +0530

>     Add support for ethtool standard device statistics grouping. Support rmon
>     statistics grouping using rmon groups parameter in ethtool command. rmon
>     provides packet size based range grouping. Common mib parameters are used
>     across all KSZ series swtches for packet size statistics, except for
>     KSZ8830. KSZ series have mib counters for packets with size:

[...]

> +void ksz8_get_rmon_stats(struct ksz_device *dev, int port,
> +			 struct ethtool_rmon_stats *rmon_stats,
> +			 const struct ethtool_rmon_hist_range **ranges)
> +{
> +	struct ksz_port_mib *mib;
> +	u64 *cnt;

Nit: I guess it can be const since you only read it (in every such
callback)?

> +	u8 i;
> +
> +	mib = &dev->ports[port].mib;
> +
> +	mutex_lock(&mib->cnt_mutex);
> +
> +	cnt = &mib->counters[KSZ8_RX_UNDERSIZE];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_UNDERSIZE, NULL, cnt);
> +	rmon_stats->undersize_pkts = *cnt;
> +
> +	cnt = &mib->counters[KSZ8_RX_OVERSIZE];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_OVERSIZE, NULL, cnt);
> +	rmon_stats->oversize_pkts = *cnt;
> +
> +	cnt = &mib->counters[KSZ8_RX_FRAGMENTS];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_FRAGMENTS, NULL, cnt);
> +	rmon_stats->fragments = *cnt;
> +
> +	cnt = &mib->counters[KSZ8_RX_JABBERS];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ8_RX_JABBERS, NULL, cnt);
> +	rmon_stats->jabbers = *cnt;
> +
> +	for (i = 0; i < KSZ8_HIST_LEN; i++) {
> +		cnt = &mib->counters[KSZ8_RX_64_OR_LESS + i];
> +		dev->dev_ops->r_mib_pkt(dev, port,
> +				(KSZ8_RX_64_OR_LESS + i), NULL, cnt);

Weird linewrap. Please align the following lines with the opening brace
of the first one, e.g.

		dev->dev_ops->r_mib_pkt(dev, port,
					KSZ8_RX_64_OR_LESS + i, NULL,
					cnt);

BUT I don't see why you need those braces around `macro + i` and without
them you can fit it into the previous line I believe.

> +		rmon_stats->hist[i] = *cnt;
> +	}
> +
> +	mutex_unlock(&mib->cnt_mutex);
> +
> +	*ranges = ksz_rmon_ranges;
> +}
> +
> +void ksz9477_get_rmon_stats(struct ksz_device *dev, int port,
> +			    struct ethtool_rmon_stats *rmon_stats,
> +			    const struct ethtool_rmon_hist_range **ranges)
> +{
> +	struct ksz_port_mib *mib;
> +	u64 *cnt;
> +	u8 i;
> +
> +	mib = &dev->ports[port].mib;
> +
> +	mutex_lock(&mib->cnt_mutex);
> +
> +	cnt = &mib->counters[KSZ9477_RX_UNDERSIZE];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_UNDERSIZE, NULL, cnt);
> +	rmon_stats->undersize_pkts = *cnt;
> +
> +	cnt = &mib->counters[KSZ9477_RX_OVERSIZE];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_OVERSIZE, NULL, cnt);
> +	rmon_stats->oversize_pkts = *cnt;
> +
> +	cnt = &mib->counters[KSZ9477_RX_FRAGMENTS];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_FRAGMENTS, NULL, cnt);
> +	rmon_stats->fragments = *cnt;
> +
> +	cnt = &mib->counters[KSZ9477_RX_JABBERS];
> +	dev->dev_ops->r_mib_pkt(dev, port, KSZ9477_RX_JABBERS, NULL, cnt);
> +	rmon_stats->jabbers = *cnt;
> +
> +	for (i = 0; i < KSZ9477_HIST_LEN; i++) {
> +		cnt = &mib->counters[KSZ9477_RX_64_OR_LESS + i];
> +		dev->dev_ops->r_mib_pkt(dev, port,
> +				(KSZ9477_RX_64_OR_LESS + i), NULL, cnt);

(same, and please check all other places in the series)

> +		rmon_stats->hist[i] = *cnt;
> +	}
> +
> +	mutex_unlock(&mib->cnt_mutex);
> +
> +	*ranges = ksz_rmon_ranges;
> +}
[...]

Thanks,
Olek