On Thu, Aug 25, 2011 at 01:26:37AM -0700, Joe Perches wrote:
> On Thu, 2011-08-25 at 13:00 +0530, Vasanthakumar Thiagarajan wrote:
> > Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@xxxxxxxxxxxxxxxx>
> []
> > 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.
It is well enough, needed is only around 110 bytes.
>
> > + 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?
>
Well, right now the size of cred_dist_list can be maximum of 9.
The maximum memory required would be around 2.5K, I think for
this size kmalloc is fine.
> > +
> > + 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?
Right, totally unaligned is not readable either ;). Ok, the number
of digits that needed to represent the number of credits are not
going to exceed 3. I just tried to have it displayed around at the center
below the respective tag.
>
> 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
>
It is very unlikely to have an output like above in real time.
Sample output that I got is
Epid Flags Cred_norm Cred_min Credits Cred_assngd Seek_cred Cred_sz Cred_per_msg Cred_to_dist qdepth
0 0 0 0 0 0 0 1664 1 0 0
1 80000000 1 1 1 1 0 1664 1 0 0
5 0 5 1 0 0 0 1664 1 0 0
4 0 5 1 0 0 0 1664 1 0 0
2 80000000 5 1 2 3 0 1664 1 0 0
3 80000000 5 1 1 1 0 1664 1 0 0
> > + 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
Makes sense.
Thanks for the review
Vasanth
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
