netdev - Re: [PATCH net-next v3 13/14] net: ethernet: qualcomm: Add PPE debugfs support for PPE counters

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]

Message-ID: <a455a2f6-ca0b-43e0-b18c-53f73344981f@quicinc.com>
Date: Fri, 14 Feb 2025 15:53:41 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Lei Wei <quic_leiwei@...cinc.com>,
        Suruchi Agarwal
	<quic_suruchia@...cinc.com>,
        Pavithra R <quic_pavir@...cinc.com>,
        "Simon
 Horman" <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
        Kees Cook
	<kees@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        "Philipp
 Zabel" <p.zabel@...gutronix.de>,
        <linux-arm-msm@...r.kernel.org>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-hardening@...r.kernel.org>,
        <quic_kkumarcs@...cinc.com>, <quic_linchen@...cinc.com>,
        <srinivas.kandagatla@...aro.org>, <bartosz.golaszewski@...aro.org>,
        <john@...ozen.org>
Subject: Re: [PATCH net-next v3 13/14] net: ethernet: qualcomm: Add PPE
 debugfs support for PPE counters



On 2/11/2025 9:55 PM, Andrew Lunn wrote:
>> +#define PRINT_COUNTER_PREFIX(desc, cnt_type)		\
>> +	seq_printf(seq, "%-16s %16s", desc, cnt_type)
>> +
>> +#define PRINT_CPU_CODE_COUNTER(cnt, code)		\
>> +	seq_printf(seq, "%10u(cpucode:%d)", cnt, code)
>> +
>> +#define PRINT_DROP_CODE_COUNTER(cnt, port, code)	\
>> +	seq_printf(seq, "%10u(port=%d),dropcode:%d", cnt, port, code)
>> +
>> +#define PRINT_SINGLE_COUNTER(tag, cnt, str, index)			\
>> +do {									\
>> +	if (!((tag) % 4))							\
>> +		seq_printf(seq, "\n%-16s %16s", "", "");		\
>> +	seq_printf(seq, "%10u(%s=%04d)", cnt, str, index);		\
>> +} while (0)
>> +
>> +#define PRINT_TWO_COUNTERS(tag, cnt0, cnt1, str, index)			\
>> +do {									\
>> +	if (!((tag) % 4))							\
>> +		seq_printf(seq, "\n%-16s %16s", "", "");		\
>> +	seq_printf(seq, "%10u/%u(%s=%04d)", cnt0, cnt1, str, index);	\
>> +} while (0)
> 
> I don't think these make the code any more readable. Just inline it.

OK.

> 
>> +/* The number of packets dropped because of no buffer available, no PPE
>> + * buffer assigned to these packets.
>> + */
>> +static void ppe_port_rx_drop_counter_get(struct ppe_device *ppe_dev,
>> +					 struct seq_file *seq)
>> +{
>> +	u32 reg, drop_cnt = 0;
>> +	int ret, i, tag = 0;
>> +
>> +	PRINT_COUNTER_PREFIX("PRX_DROP_CNT", "SILENT_DROP:");
>> +	for (i = 0; i < PPE_DROP_CNT_TBL_ENTRIES; i++) {
>> +		reg = PPE_DROP_CNT_TBL_ADDR + i * PPE_DROP_CNT_TBL_INC;
>> +		ret = ppe_pkt_cnt_get(ppe_dev, reg, PPE_PKT_CNT_SIZE_1WORD,
>> +				      &drop_cnt, NULL);
>> +		if (ret) {
>> +			seq_printf(seq, "ERROR %d\n", ret);
>> +			return;
>> +		}
> 
> This is an error getting the value from the hardware? You should not
> put that into the debugfs itself, you want the read() call to return
> it.
> 

Yes, this error code is returned by regmap read functions in
ppe_pkt_cnt_get() when the hardware counter read fails. I will
remove it from debugfs file and instead log the error to the
console (dev_info).

>> +/* Display the various packet counters of PPE. */
>> +static int ppe_packet_counter_show(struct seq_file *seq, void *v)
>> +{
>> +	struct ppe_device *ppe_dev = seq->private;
>> +
>> +	ppe_port_rx_drop_counter_get(ppe_dev, seq);
>> +	ppe_port_rx_bm_drop_counter_get(ppe_dev, seq);
>> +	ppe_port_rx_bm_port_counter_get(ppe_dev, seq);
>> +	ppe_parse_pkt_counter_get(ppe_dev, seq);
>> +	ppe_port_rx_counter_get(ppe_dev, seq);
>> +	ppe_vp_rx_counter_get(ppe_dev, seq);
>> +	ppe_pre_l2_counter_get(ppe_dev, seq);
>> +	ppe_vlan_counter_get(ppe_dev, seq);
>> +	ppe_cpu_code_counter_get(ppe_dev, seq);
>> +	ppe_eg_vsi_counter_get(ppe_dev, seq);
>> +	ppe_vp_tx_counter_get(ppe_dev, seq);
>> +	ppe_port_tx_counter_get(ppe_dev, seq);
>> +	ppe_queue_tx_counter_get(ppe_dev, seq);
> 
> It would be more normal to have one debugfs file per group of
> counters.
> 
> 	Andrew

Sure. We used a single file as it may be more convenient to display
these counters all together in one go, since dumping single group
counter has no help on tracing packet drops inside the PPE path.
But perhaps, a script can be used as well on top of the segregated
files to dump all the counters.