Re: [PATCH v2] ath9k: Gather and report IRQ sync_cause errors.

[Joe Perches <joe@perches.com>]

(added Luis R and the current ath9k-devel list to cc's)

On Wed, 2012-04-04 at 11:25 -0700, Ben Greear wrote:
> On 04/04/2012 09:16 AM, Joe Perches wrote:
> > On Tue, 2012-04-03 at 21:20 -0700, greearb@candelatech.com wrote:
> >> From: Ben Greear<greearb@candelatech.com>
> >>
> >> Report all defined sync_cause errors in debugfs
> >> to aid with debugging.
> >
> > just trivia:
> >
> >> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
> > []
> >> @@ -385,63 +385,130 @@ static ssize_t read_file_interrupt(struct file *file, char __user *user_buf,
> >>   				   size_t count, loff_t *ppos)
> >>   {
> >>   	struct ath_softc *sc = file->private_data;
> > []
> >>   	if (sc->sc_ah->caps.hw_caps&  ATH9K_HW_CAP_EDMA) {
> > []
> >> +		len += snprintf(buf + len, mxlen - len,
> >> +			"%21s: %10u\n", "RXLP", sc->debug.stats.istats.rxlp);
> >
> > Alignment is overrated.
> >
> > I wouldn't change any of the original block
> > though perhaps changing the sc pointer to an
> > istats pointer so these dereferences become
> > similar to istats->rxlp;
> 
> Other files in this driver are aligned, so it seems
> nice to continue the tradition.  debugfs files are
> for human consumption, so making them readable is
> a virtue.

It'd still be readable, it would just be a different block
separated by a new header line.

> If someone wants more efficient access to this data,
> then we can use ethtool stats (I'll add ethtool stats
> support for these counters if/when my first ethtool
> patches make it in...)

That'd be a good idea.

> >> +void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause)
> >> +{
> >> +	struct ath_softc *sc = common->priv;
> >> +	if (sync_cause)
> >> +		sc->debug.stats.istats.sync_cause_all++;
> >> +	if (sync_cause&  AR_INTR_SYNC_RTC_IRQ)
> >> +		sc->debug.stats.istats.sync_rtc_irq++;
> >
> > And nicer here to use a pointer to istats too.
> 
> Hopefully the compiler is smart enough that this
> doesn't really matter.

It is, but it's really for reader clarity not
compiler output.

> If the maintainers care, I'll respin the patch, but
> otherwise I'm not sure it's worth the effort.

The patch doesn't apply cleanly against next anyway.
There's a trivial #include mismatch.

cheers, Joe