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

[Jie Luo]

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.