Re: [PATCH V2 3/3] ath6kl: Add debugfs file entry to dump credit distribution stats

[Joe Perches <joe@perches.com>]

On Thu, 2011-08-25 at 13:00 +0530, Vasanthakumar Thiagarajan wrote:
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
[]
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
[]
> +static ssize_t read_file_credit_dist_stats(struct file *file,
> +					   char __user *user_buf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct ath6kl *ar = file->private_data;
> +	struct htc_target *target = ar->htc_target;
> +	struct htc_endpoint_credit_dist *ep_list;
> +	char *buf;
> +	unsigned int buf_len = 128, len = 0;
> +	ssize_t ret_cnt;
> +
> +	buf_len += get_queue_depth(&target->cred_dist_list) * 128;

Doesn't look like 128 is the right size.

> +	buf = kzalloc(buf_len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;

kmalloc can run out of space pretty quickly if cred_dist_list
is large.  Maybe vmalloc?

> +
> +	len += scnprintf(buf + len, buf_len - len,
> +			 " Epid  Flags    Cred_norm  Cred_min  Credits  Cred_assngd"
> +			 "  Seek_cred  Cred_sz  Cred_per_msg  Cred_to_dist"
> +			 "  qdepth\n");
> +
> +	list_for_each_entry(ep_list, &target->cred_dist_list, list) {
> +		len += scnprintf(buf + len, buf_len - len, "  %2d",
> +				 ep_list->endpoint);
> +		len += scnprintf(buf + len, buf_len - len, "%10x",
> +				 ep_list->dist_flags);

I think this is nearly unreadable.
It doesn't line up well with your header.

Why not align them correctly or otherwise make no
attempt to align them at all?

Your current header and field widths:

          1         2         3         4         5         6         7         8         9        10        11        12 
 123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
	len += scnprintf(buf + len, buf_len - len,
" Epid  Flags    Cred_norm  Cred_min  Credits  Cred_assngd  Seek_cred  Cred_sz  Cred_per_msg  Cred_to_dist  qdepth\n");
   12          12345678         123456789          1234567890123            123456789              123456789012
     1234567890        123456789         1234567890             123456789012         12345678901234

> +	list_for_each_entry(ep_list, &target->cred_dist_list, list) {
> +		len += scnprintf(buf + len, buf_len - len, "  %2d",
> +				 ep_list->endpoint);
> +		len += scnprintf(buf + len, buf_len - len, "%10x",
> +				 ep_list->dist_flags);

maybe use a macro:
#define ep_list_field(fmt, field)	\ 
	len += scnprint(buf + len, buf_len - len, fmt, ep_list->field)

		ep_list_field("  %2d", endpoint);
		ep_list_field("%9d", cred_min);
		ep_list_field("%9d", credits);
etc

> +		len += scnprintf(buf + len, buf_len - len, "%12d\n",
> +				 get_queue_depth(&((struct htc_endpoint *)
> +						 ep_list->htc_rsvd)->txq));
> +	}