Message-ID: <03dedec9-2383-b0bd-eb2e-4d3b334e8c0b@intel.com>
Date:   Fri, 17 Feb 2023 15:59:12 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 net-next 1/5] net: dsa: microchip: add rmon grouping
 for ethtool statistics
Content-Language: en-US
To:     Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <woojung.huh@microchip.com>, <UNGLinuxDriver@microchip.com>,
        <andrew@lunn.ch>, <f.fainelli@gmail.com>, <olteanv@gmail.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>,
        Thangaraj Samynathan <Thangaraj.S@microchip.com>
References: <20230217110211.433505-1-rakesh.sankaranarayanan@microchip.com>
 <20230217110211.433505-2-rakesh.sankaranarayanan@microchip.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230217110211.433505-2-rakesh.sankaranarayanan@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bnFyTWVVRFM1TDJXVjZ5L2RsUS9mamJWNFpMcFdKOTRvVnVyVFQ1ODZ3SllV?=
 =?utf-8?B?VDlDWEwxenUzOE1lb2FBM2FIanRTTGlRY1Vac2hjTFpnLy9jVEFtZk9zcmx6?=
 =?utf-8?B?bHJweEJsWUJSQi8zN1F0MHhNUkt3ZTluWnVneTg4dTFTVWhCaEU3aFFEMEY3?=
 =?utf-8?B?ZzcxbnZWVnQwZmlNZldtcWoyNEQ0eS84VkxUMGtodThkMy9nVWdpaWRtc1JC?=
 =?utf-8?B?NUw1T0xqdDJQdEJLMUVPVUJJbjh0ZjNrdDNEZzY5b212emVsL2krUUZTZk9r?=
 =?utf-8?B?SU5KSXRHVG00cXo2S1JXVDFSU1VHSUk0di9SNXNwSmh0RTY3TDhzbzhsb0lm?=
 =?utf-8?B?eHZqbVc3NHA3SDFRcVphVHpHdjZYWmdaRHlTS2ZlSGdyOFg2eWcyWmRlVktT?=
 =?utf-8?B?S3hsOUdhK2l2OFY3akZ3dkRtazVvZVBxS0xQTGMrOWZPVHNIOVFoTFJ6cURw?=
 =?utf-8?B?TFZYOU1SREkxY3hXajhMSmlrL1BrZWFFZHR4YUcyUERFQ1lKaEVZM3BDVTg2?=
 =?utf-8?B?L00vTTRsc1d3dlVqWWhicGdIYm10cThvTjVTcE96OU5aYlJRVWFzVGErVG9Y?=
 =?utf-8?B?aXdxVEM3eERablJ1dWVjRTRlL2F4eHI4LzU1dmcvaUNFSVQySXpkS25KeHRt?=
 =?utf-8?B?b3M4djlhazZrbVlLL25FQ2RGRkRqcFZxMUMwVCtIN1MvbXFINnBuelN1SUFw?=
 =?utf-8?B?c3BHNnVDQ2pJQms2a1ErYjdGTWQ0UmxkTFlMa3hxMFV0bkNaVEcyZkhGQzBH?=
 =?utf-8?B?cC9mVjQ5Zm9hU0I0OVJOTE5tMko2L3hCOWdFV25idEVWREJnUWZNblB5UzVo?=
 =?utf-8?B?bzBhNUs3T1BzTmtNa3Y2SmFCYUhFNmVpV1cwQ0xLVW9kems5NUtmbXVZblY5?=
 =?utf-8?B?MkN6dnh2VVJlSTQwSDYvdHp0Rzl5WW1MK3RWVCtMTnRwUkdYWVdYalhzWklT?=
 =?utf-8?B?QWZPQVJ6RWtsQ28rZTVPaExmTnU0V1ZlTDh5SmxvcjU2c0hLNmVWYUN4bkhT?=
 =?utf-8?B?SmJ3Z3BEYTVpNVhBZjhDTjJHMTRueEtRUnJ4TjU0cDd0dHlUcWJ6bnBoemN0?=
 =?utf-8?B?MTl6MEVaU0RYT1JVM2Rxc3oxS29SRktFUnFVMm9vZnFHSHlhQThGb3NjUE1G?=
 =?utf-8?B?TUZ6RXRMTkFMUjBoUVZadzZpbVhXQm9ta3g1ZVozVTdCKzlSVFVLMUtPSTdK?=
 =?utf-8?B?eFc3TEg1dHlMbU80VWJXQ056d3FtM0JHT1J3TXNnT2srUklqOWNBOUI4VFN4?=
 =?utf-8?B?dERjR2R3bkdabUJ5M0dNN2k5T2JDeTJqNU1IYzFRcHRvZ2RHUjRqcmFYZDlo?=
 =?utf-8?B?QldRMFhUVXlrck5ma3htOHNyV055V1dBT0g2UUFPbW1iL3pkNGpsUkFzYkpS?=
 =?utf-8?B?c0IyalhCUTN2WUMya2RvazltdlRQMWc1K0Nrc0FoQlhBcnRJMXVpa3VrNU5r?=
 =?utf-8?B?WWJNNytiazN2NElLSDZHVE5XNllTMDJBa1drR2FITmNUZmlweStIY0N2NnZB?=
 =?utf-8?B?Wm9PeFRRMVpqbklNR21DL3BXci9QZUpSZklNVTBpWHFMWjFybDF2WEVTQzBj?=
 =?utf-8?B?Z0VHSXdkNm02TlpONXhZdW5Icm9LYmpDNktMZkkrMEc2Nk1NN1A2UzJqalAv?=
 =?utf-8?B?NmVSVC82ZWJNSDlWakp5ZjZwSnJCU1pyNEgxWHBVRnpLMkJpNGFMNmY1VEpO?=
 =?utf-8?B?TEdGMVAxSHltczNoS3ZLekpBWHRnalhrWE1YbUpwMlJ2QW5pUmk3YXlwakQ5?=
 =?utf-8?B?UldWbGlXdWtyRDV2T1UwdTBQS2NkalZVdTNOQXp4ZjlhdWt1U1JmcGdiQm1p?=
 =?utf-8?B?YWtid0swWXRiUXRaRjJWc3g5ZHNWaTFXSkhZcGRmRHp5cGxDK05FcXIyQzhw?=
 =?utf-8?B?MVN3QTN0WDZkcHVSc0dwb1lpSVU1bVM1WWdyaDRmajJqSW82TjAwNFJhYTZ0?=
 =?utf-8?B?S1pKZ3IzU2JTNVZEU0k2bnRaMnRaUThBNU9vOTZ6TzZjSGhIQTN2anZsQUFV?=
 =?utf-8?B?WndLaTdkOFBPY3NKQlBLVEc2ZXBqWWF5bjNud1hXZzN5RXdTME0zbmc4ZXc1?=
 =?utf-8?B?N3VzUDZPNnA3RlpNK2RwWXFHZFA1WndtTHFRQW5SVW01QWNoeVZZb0ozOG5P?=
 =?utf-8?B?d3lGMjdoVzZsd3g5VVFXbEI4OHovSU9GQ1ZTOGkwb1ozd1Jpa2hpZ3RUNkVp?=
 =?utf-8?Q?FWz9l25a+4QEZso0xpXOBig=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0c5d41f3-041b-49b7-3d38-08db10f7c1c3
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2023 15:00:49.9633
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wcxzLTOfO0jz0FiXmuqMkDFP5+ffBNH0RrVW2PO2KrEnniPt+33YGjqHrR1e7th2baJxhpKP+L7gFx3A8zIoCuXOcwKuN/D4mv5nmt+zFq4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4734
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.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