[PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

This patch merges the 'wiphy' and 'misc' debugfs files
and consolidates the information.

Information about the current channel and other HT parameters
can be obtained from both mac80211 and iw. Remove such
redundant data.

The reset statistics have been removed, they will be re-added in
a subsequent patch (in a new debugfs file).

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/debug.c |  224 +++++++++-----------------------
 1 files changed, 64 insertions(+), 160 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index dc45cf9..b2c6069 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -451,109 +451,6 @@ static const struct file_operations fops_interrupt = {
 	.llseek = default_llseek,
 };
 
-static const char *channel_type_str(enum nl80211_channel_type t)
-{
-	switch (t) {
-	case NL80211_CHAN_NO_HT:
-		return "no ht";
-	case NL80211_CHAN_HT20:
-		return "ht20";
-	case NL80211_CHAN_HT40MINUS:
-		return "ht40-";
-	case NL80211_CHAN_HT40PLUS:
-		return "ht40+";
-	default:
-		return "???";
-	}
-}
-
-static ssize_t read_file_wiphy(struct file *file, char __user *user_buf,
-			       size_t count, loff_t *ppos)
-{
-	struct ath_softc *sc = file->private_data;
-	struct ieee80211_channel *chan = sc->hw->conf.channel;
-	struct ieee80211_conf *conf = &(sc->hw->conf);
-	char buf[512];
-	unsigned int len = 0;
-	u8 addr[ETH_ALEN];
-	u32 tmp;
-
-	len += snprintf(buf + len, sizeof(buf) - len,
-			"%s (chan=%d  center-freq: %d MHz  channel-type: %d (%s))\n",
-			wiphy_name(sc->hw->wiphy),
-			ieee80211_frequency_to_channel(chan->center_freq),
-			chan->center_freq,
-			conf->channel_type,
-			channel_type_str(conf->channel_type));
-
-	ath9k_ps_wakeup(sc);
-	put_unaligned_le32(REG_READ_D(sc->sc_ah, AR_STA_ID0), addr);
-	put_unaligned_le16(REG_READ_D(sc->sc_ah, AR_STA_ID1) & 0xffff, addr + 4);
-	len += snprintf(buf + len, sizeof(buf) - len,
-			"addr: %pM\n", addr);
-	put_unaligned_le32(REG_READ_D(sc->sc_ah, AR_BSSMSKL), addr);
-	put_unaligned_le16(REG_READ_D(sc->sc_ah, AR_BSSMSKU) & 0xffff, addr + 4);
-	len += snprintf(buf + len, sizeof(buf) - len,
-			"addrmask: %pM\n", addr);
-	tmp = ath9k_hw_getrxfilter(sc->sc_ah);
-	ath9k_ps_restore(sc);
-	len += snprintf(buf + len, sizeof(buf) - len,
-			"rfilt: 0x%x", tmp);
-	if (tmp & ATH9K_RX_FILTER_UCAST)
-		len += snprintf(buf + len, sizeof(buf) - len, " UCAST");
-	if (tmp & ATH9K_RX_FILTER_MCAST)
-		len += snprintf(buf + len, sizeof(buf) - len, " MCAST");
-	if (tmp & ATH9K_RX_FILTER_BCAST)
-		len += snprintf(buf + len, sizeof(buf) - len, " BCAST");
-	if (tmp & ATH9K_RX_FILTER_CONTROL)
-		len += snprintf(buf + len, sizeof(buf) - len, " CONTROL");
-	if (tmp & ATH9K_RX_FILTER_BEACON)
-		len += snprintf(buf + len, sizeof(buf) - len, " BEACON");
-	if (tmp & ATH9K_RX_FILTER_PROM)
-		len += snprintf(buf + len, sizeof(buf) - len, " PROM");
-	if (tmp & ATH9K_RX_FILTER_PROBEREQ)
-		len += snprintf(buf + len, sizeof(buf) - len, " PROBEREQ");
-	if (tmp & ATH9K_RX_FILTER_PHYERR)
-		len += snprintf(buf + len, sizeof(buf) - len, " PHYERR");
-	if (tmp & ATH9K_RX_FILTER_MYBEACON)
-		len += snprintf(buf + len, sizeof(buf) - len, " MYBEACON");
-	if (tmp & ATH9K_RX_FILTER_COMP_BAR)
-		len += snprintf(buf + len, sizeof(buf) - len, " COMP_BAR");
-	if (tmp & ATH9K_RX_FILTER_PSPOLL)
-		len += snprintf(buf + len, sizeof(buf) - len, " PSPOLL");
-	if (tmp & ATH9K_RX_FILTER_PHYRADAR)
-		len += snprintf(buf + len, sizeof(buf) - len, " PHYRADAR");
-	if (tmp & ATH9K_RX_FILTER_MCAST_BCAST_ALL)
-		len += snprintf(buf + len, sizeof(buf) - len, " MCAST_BCAST_ALL");
-
-	len += snprintf(buf + len, sizeof(buf) - len,
-		       "\n\nReset causes:\n"
-		       "  baseband hang: %d\n"
-		       "  baseband watchdog: %d\n"
-		       "  fatal hardware error interrupt: %d\n"
-		       "  tx hardware error: %d\n"
-		       "  tx path hang: %d\n"
-		       "  pll rx hang: %d\n",
-		       sc->debug.stats.reset[RESET_TYPE_BB_HANG],
-		       sc->debug.stats.reset[RESET_TYPE_BB_WATCHDOG],
-		       sc->debug.stats.reset[RESET_TYPE_FATAL_INT],
-		       sc->debug.stats.reset[RESET_TYPE_TX_ERROR],
-		       sc->debug.stats.reset[RESET_TYPE_TX_HANG],
-		       sc->debug.stats.reset[RESET_TYPE_PLL_HANG]);
-
-	if (len > sizeof(buf))
-		len = sizeof(buf);
-
-	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
-}
-
-static const struct file_operations fops_wiphy = {
-	.read = read_file_wiphy,
-	.open = ath9k_debugfs_open,
-	.owner = THIS_MODULE,
-	.llseek = default_llseek,
-};
-
 #define PR_QNUM(_n) sc->tx.txq_map[_n]->axq_qnum
 #define PR(str, elem)							\
 	do {								\
@@ -763,84 +660,93 @@ static ssize_t read_file_misc(struct file *file, char __user *user_buf,
 {
 	struct ath_softc *sc = file->private_data;
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
-	struct ath_hw *ah = sc->sc_ah;
 	struct ieee80211_hw *hw = sc->hw;
-	char *buf;
-	unsigned int len = 0, size = 8000;
+	struct ath9k_vif_iter_data iter_data;
+	char buf[512];
+	unsigned int len = 0;
 	ssize_t retval = 0;
 	unsigned int reg;
-	struct ath9k_vif_iter_data iter_data;
+	u32 rxfilter;
 
-	ath9k_calculate_iter_data(hw, NULL, &iter_data);
-	
-	buf = kzalloc(size, GFP_KERNEL);
-	if (buf == NULL)
-		return -ENOMEM;
+	len += snprintf(buf + len, sizeof(buf) - len,
+			"BSSID: %pM\n", common->curbssid);
+	len += snprintf(buf + len, sizeof(buf) - len,
+			"BSSID-MASK: %pM\n", common->bssidmask);
+	len += snprintf(buf + len, sizeof(buf) - len,
+			"OPMODE: %s\n", ath_opmode_to_string(sc->sc_ah->opmode));
 
 	ath9k_ps_wakeup(sc);
-	len += snprintf(buf + len, size - len,
-			"curbssid: %pM\n"
-			"OP-Mode: %s(%i)\n"
-			"Beacon-Timer-Register: 0x%x\n",
-			common->curbssid,
-			ath_opmode_to_string(sc->sc_ah->opmode),
-			(int)(sc->sc_ah->opmode),
-			REG_READ(ah, AR_BEACON_PERIOD));
-
-	reg = REG_READ(ah, AR_TIMER_MODE);
+	rxfilter = ath9k_hw_getrxfilter(sc->sc_ah);
 	ath9k_ps_restore(sc);
-	len += snprintf(buf + len, size - len, "Timer-Mode-Register: 0x%x (",
-			reg);
-	if (reg & AR_TBTT_TIMER_EN)
-		len += snprintf(buf + len, size - len, "TBTT ");
-	if (reg & AR_DBA_TIMER_EN)
-		len += snprintf(buf + len, size - len, "DBA ");
-	if (reg & AR_SWBA_TIMER_EN)
-		len += snprintf(buf + len, size - len, "SWBA ");
-	if (reg & AR_HCF_TIMER_EN)
-		len += snprintf(buf + len, size - len, "HCF ");
-	if (reg & AR_TIM_TIMER_EN)
-		len += snprintf(buf + len, size - len, "TIM ");
-	if (reg & AR_DTIM_TIMER_EN)
-		len += snprintf(buf + len, size - len, "DTIM ");
-	len += snprintf(buf + len, size - len, ")\n");
+
+	len += snprintf(buf + len, sizeof(buf) - len,
+			"RXFILTER: 0x%x", rxfilter);
+
+	if (rxfilter & ATH9K_RX_FILTER_UCAST)
+		len += snprintf(buf + len, sizeof(buf) - len, " UCAST");
+	if (rxfilter & ATH9K_RX_FILTER_MCAST)
+		len += snprintf(buf + len, sizeof(buf) - len, " MCAST");
+	if (rxfilter & ATH9K_RX_FILTER_BCAST)
+		len += snprintf(buf + len, sizeof(buf) - len, " BCAST");
+	if (rxfilter & ATH9K_RX_FILTER_CONTROL)
+		len += snprintf(buf + len, sizeof(buf) - len, " CONTROL");
+	if (rxfilter & ATH9K_RX_FILTER_BEACON)
+		len += snprintf(buf + len, sizeof(buf) - len, " BEACON");
+	if (rxfilter & ATH9K_RX_FILTER_PROM)
+		len += snprintf(buf + len, sizeof(buf) - len, " PROM");
+	if (rxfilter & ATH9K_RX_FILTER_PROBEREQ)
+		len += snprintf(buf + len, sizeof(buf) - len, " PROBEREQ");
+	if (rxfilter & ATH9K_RX_FILTER_PHYERR)
+		len += snprintf(buf + len, sizeof(buf) - len, " PHYERR");
+	if (rxfilter & ATH9K_RX_FILTER_MYBEACON)
+		len += snprintf(buf + len, sizeof(buf) - len, " MYBEACON");
+	if (rxfilter & ATH9K_RX_FILTER_COMP_BAR)
+		len += snprintf(buf + len, sizeof(buf) - len, " COMP_BAR");
+	if (rxfilter & ATH9K_RX_FILTER_PSPOLL)
+		len += snprintf(buf + len, sizeof(buf) - len, " PSPOLL");
+	if (rxfilter & ATH9K_RX_FILTER_PHYRADAR)
+		len += snprintf(buf + len, sizeof(buf) - len, " PHYRADAR");
+	if (rxfilter & ATH9K_RX_FILTER_MCAST_BCAST_ALL)
+		len += snprintf(buf + len, sizeof(buf) - len, " MCAST_BCAST_ALL");
+	if (rxfilter & ATH9K_RX_FILTER_CONTROL_WRAPPER)
+		len += snprintf(buf + len, sizeof(buf) - len, " CONTROL_WRAPPER");
+
+	len += snprintf(buf + len, sizeof(buf) - len, "\n");
 
 	reg = sc->sc_ah->imask;
-	len += snprintf(buf + len, size - len, "imask: 0x%x (", reg);
+
+	len += snprintf(buf + len, sizeof(buf) - len, "INTERRUPT-MASK: 0x%x", reg);
+
 	if (reg & ATH9K_INT_SWBA)
-		len += snprintf(buf + len, size - len, "SWBA ");
+		len += snprintf(buf + len, sizeof(buf) - len, " SWBA");
 	if (reg & ATH9K_INT_BMISS)
-		len += snprintf(buf + len, size - len, "BMISS ");
+		len += snprintf(buf + len, sizeof(buf) - len, " BMISS");
 	if (reg & ATH9K_INT_CST)
-		len += snprintf(buf + len, size - len, "CST ");
+		len += snprintf(buf + len, sizeof(buf) - len, " CST");
 	if (reg & ATH9K_INT_RX)
-		len += snprintf(buf + len, size - len, "RX ");
+		len += snprintf(buf + len, sizeof(buf) - len, " RX");
 	if (reg & ATH9K_INT_RXHP)
-		len += snprintf(buf + len, size - len, "RXHP ");
+		len += snprintf(buf + len, sizeof(buf) - len, " RXHP");
 	if (reg & ATH9K_INT_RXLP)
-		len += snprintf(buf + len, size - len, "RXLP ");
+		len += snprintf(buf + len, sizeof(buf) - len, " RXLP");
 	if (reg & ATH9K_INT_BB_WATCHDOG)
-		len += snprintf(buf + len, size - len, "BB_WATCHDOG ");
-	/* there are other IRQs if one wanted to add them. */
-	len += snprintf(buf + len, size - len, ")\n");
+		len += snprintf(buf + len, sizeof(buf) - len, " BB_WATCHDOG");
 
-	len += snprintf(buf + len, size - len,
-			"VIF Counts: AP: %i STA: %i MESH: %i WDS: %i"
-			" ADHOC: %i OTHER: %i nvifs: %hi beacon-vifs: %hi\n",
+	len += snprintf(buf + len, sizeof(buf) - len, "\n");
+
+	ath9k_calculate_iter_data(hw, NULL, &iter_data);
+
+	len += snprintf(buf + len, sizeof(buf) - len,
+			"VIF-COUNTS: AP: %i STA: %i MESH: %i WDS: %i"
+			" ADHOC: %i OTHER: %i TOTAL: %hi BEACON-VIF: %hi\n",
 			iter_data.naps, iter_data.nstations, iter_data.nmeshes,
 			iter_data.nwds, iter_data.nadhocs, iter_data.nothers,
 			sc->nvifs, sc->nbcnvifs);
 
-	len += snprintf(buf + len, size - len,
-			"Calculated-BSSID-Mask: %pM\n",
-			iter_data.mask);
-
-	if (len > size)
-		len = size;
+	if (len > sizeof(buf))
+		len = sizeof(buf);
 
 	retval = simple_read_from_buffer(user_buf, count, ppos, buf, len);
-	kfree(buf);
-
 	return retval;
 }
 
@@ -1637,8 +1543,6 @@ int ath9k_init_debug(struct ath_hw *ah)
 			    &fops_dma);
 	debugfs_create_file("interrupt", S_IRUSR, sc->debug.debugfs_phy, sc,
 			    &fops_interrupt);
-	debugfs_create_file("wiphy", S_IRUSR | S_IWUSR, sc->debug.debugfs_phy,
-			    sc, &fops_wiphy);
 	debugfs_create_file("xmit", S_IRUSR, sc->debug.debugfs_phy, sc,
 			    &fops_xmit);
 	debugfs_create_file("stations", S_IRUSR, sc->debug.debugfs_phy, sc,
-- 
1.7.9

Re: [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Ben Greear]

On 02/12/2012 10:20 PM, Sujith Manoharan wrote:
> This patch merges the 'wiphy' and 'misc' debugfs files
> and consolidates the information.
>
> Information about the current channel and other HT parameters
> can be obtained from both mac80211 and iw. Remove such
> redundant data.

It is nice to be able to see what the driver/NIC thinks
it is set to as well.  It is always possible that the
stack and the NIC are out of sync for some reason.
I'd rather those values be left in the debugfs unless
there is a good reason for getting rid of them.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

Ben Greear wrote:
> It is nice to be able to see what the driver/NIC thinks
> it is set to as well.  It is always possible that the
> stack and the NIC are out of sync for some reason.
> I'd rather those values be left in the debugfs unless
> there is a good reason for getting rid of them.

The 'wiphy' file is a remnant from the old virtual HW code.
To know the channel parameters of multiple, virtual PHYs, it
was useful. Since the operating parameters, channel type, HT
capabilites etc. are all exposed by mac80211's debugfs, we might
as well use that. Most of the other information - BSSID, RXfilter etc.
have been retained.

Sujith

Re: [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Ben Greear]

On 02/13/2012 05:48 PM, Sujith Manoharan wrote:
> Ben Greear wrote:
>> It is nice to be able to see what the driver/NIC thinks
>> it is set to as well.  It is always possible that the
>> stack and the NIC are out of sync for some reason.
>> I'd rather those values be left in the debugfs unless
>> there is a good reason for getting rid of them.
>
> The 'wiphy' file is a remnant from the old virtual HW code.
> To know the channel parameters of multiple, virtual PHYs, it
> was useful. Since the operating parameters, channel type, HT
> capabilites etc. are all exposed by mac80211's debugfs, we might

What if there is some bug that causes the NIC to be in a
state different than mac80211 thinks it is?  If you leave
this in, then maybe someone will notice.

Ath9k is a tricky beast to debug, and I think you should
not remove anything that is remotely useful to debugging
unless removing it has a real benefit other than just
decreasing lines of (conditionally compiled) code.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

Ben Greear wrote:
> What if there is some bug that causes the NIC to be in a
> state different than mac80211 thinks it is?  If you leave
> this in, then maybe someone will notice.

Well, if we are not operating in the channel that mac80211
tells us, then we are screwed in a major, non-obvious way. :)

> Ath9k is a tricky beast to debug, and I think you should
> not remove anything that is remotely useful to debugging
> unless removing it has a real benefit other than just
> decreasing lines of (conditionally compiled) code.

I am aware that you have rather unique requirements regarding ath9k,
which is why I left 'stations', 'xmit', etc.. untouched. :)

The only data points that have been removed are the current operating
channel parameters. And removing redundant code will make a difference
in memory-constrained environments like APs. (OpenWRT has DEBUGFS enabled
by default).

Sujith

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Peter Stuge]

Sujith Manoharan wrote:
> > Ath9k is a tricky beast to debug, and I think you should
> > not remove anything that is remotely useful to debugging
> > unless removing it has a real benefit other than just
> > decreasing lines of (conditionally compiled) code.
> 
> I am aware that you have rather unique requirements regarding ath9k,

Not really. He just wants to have HALF a chance to find issues. I'm
sure you understand that ath9k is far from bug free. Until that has
changed (don't hold your breath) the driver can not have too much
debug information!


> removing redundant code will make a difference in
> memory-constrained environments like APs. (OpenWRT has DEBUGFS
> enabled by default).

It is not very wise for ath9k to reduce debug output with the
motivation that some users leave it enabled even if they deploy
into limited systems and they would actually prefer to have less
debugging.

If ath9k users have an issue with debugging being too big then THEY
can disable it. It seems senseless to delete debugging when the
driver is clearly still buggy.

It also seems senseless to try to punt to mac80211. It is amazing
that Ben's point is not absolutely obvious.

One purpose of debugging is to allow and enable consistency checking.
Redundant information is by definition neccessary for that.


As a compromise, you could suggest a move to a two-level compile time
option for debugging. You would enable only what you believe to be
adequate for debugging in production systems (wait, what did I just
write? oh well..) with the first option, and you would allow the rest
of the world to enable complete debugging with the second option.

I would support such a scheme.


//Peter

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

Peter Stuge wrote:
> It is not very wise for ath9k to reduce debug output with the
> motivation that some users leave it enabled even if they deploy
> into limited systems and they would actually prefer to have less
> debugging.
> 
> If ath9k users have an issue with debugging being too big then THEY
> can disable it. It seems senseless to delete debugging when the
> driver is clearly still buggy.
> 
> It also seems senseless to try to punt to mac80211. It is amazing
> that Ben's point is not absolutely obvious.
> 
> One purpose of debugging is to allow and enable consistency checking.
> Redundant information is by definition neccessary for that.
> 
> 
> As a compromise, you could suggest a move to a two-level compile time
> option for debugging. You would enable only what you believe to be
> adequate for debugging in production systems (wait, what did I just
> write? oh well..) with the first option, and you would allow the rest
> of the world to enable complete debugging with the second option.

/me disregards all the nerd-rage and the general snarkiness.

Having multiple levels of debugging makes things complex. If the general
consensus is to keep the information, I can rework the patch.

Sujith

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Ben Greear]

On 02/14/2012 12:30 AM, Sujith Manoharan wrote:
> Peter Stuge wrote:

>> As a compromise, you could suggest a move to a two-level compile time
>> option for debugging. You would enable only what you believe to be

> Having multiple levels of debugging makes things complex. If the general
> consensus is to keep the information, I can rework the patch.

Actually, I think it might be useful to have a second level of debugging.
I hope to soon have time & resources to add some logic to dump lots of register
info and such in human-readable format, (like, when DMA times out).  That is going to be a lot
of strings added to the driver, so the compile size will definitely
increase.  If keeping the size small is important, then this sort of verbose thing
could be hidden behind a second level of debugging...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith]

Ben Greear wrote:
> Actually, I think it might be useful to have a second level of debugging.
> I hope to soon have time & resources to add some logic to dump lots of register
> info and such in human-readable format, (like, when DMA times out).  That is going to be a lot
> of strings added to the driver, so the compile size will definitely
> increase.  If keeping the size small is important, then this sort of verbose thing
> could be hidden behind a second level of debugging...

That could be implemented similar to what usbmon does. A debugfs file that could
be read and redirected to a file. And there would be no overhead to the
driver, I think. We could call it the 'event log'. :)

Sujith

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Ben Greear]

On 02/14/2012 10:29 AM, Sujith wrote:
> Ben Greear wrote:
>> Actually, I think it might be useful to have a second level of debugging.
>> I hope to soon have time&  resources to add some logic to dump lots of register
>> info and such in human-readable format, (like, when DMA times out).  That is going to be a lot
>> of strings added to the driver, so the compile size will definitely
>> increase.  If keeping the size small is important, then this sort of verbose thing
>> could be hidden behind a second level of debugging...
>
> That could be implemented similar to what usbmon does. A debugfs file that could
> be read and redirected to a file. And there would be no overhead to the
> driver, I think. We could call it the 'event log'. :)

I was thinking about adding a method that grabbed as many registers
as I have info for and dumping them with printk when DMA errors
hit.  This would make kernel splats more useful.

And also have a debugfs file called 'registers' or similar that one
could cat out and get similar info.  And this can let folks look
at steady-state or whatever.

But, the logic to turn the register bit values into strings would
be in the driver (and thus add some code size bloat).

My hope is that this would allow a better chance of understanding
the stop-DMA errors that some people get reliably (but which I can never reliably
reproduce).

I'm not sure how that plays into your 'event log' idea, but maybe
one will help the other.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Felix Fietkau]

On 2012-02-14 7:43 PM, Ben Greear wrote:
> On 02/14/2012 10:29 AM, Sujith wrote:
>> Ben Greear wrote:
>>> Actually, I think it might be useful to have a second level of debugging.
>>> I hope to soon have time&  resources to add some logic to dump lots of register
>>> info and such in human-readable format, (like, when DMA times out).  That is going to be a lot
>>> of strings added to the driver, so the compile size will definitely
>>> increase.  If keeping the size small is important, then this sort of verbose thing
>>> could be hidden behind a second level of debugging...
>>
>> That could be implemented similar to what usbmon does. A debugfs file that could
>> be read and redirected to a file. And there would be no overhead to the
>> driver, I think. We could call it the 'event log'. :)
> 
> I was thinking about adding a method that grabbed as many registers
> as I have info for and dumping them with printk when DMA errors
> hit.  This would make kernel splats more useful.
> 
> And also have a debugfs file called 'registers' or similar that one
> could cat out and get similar info.  And this can let folks look
> at steady-state or whatever.
> 
> But, the logic to turn the register bit values into strings would
> be in the driver (and thus add some code size bloat).
> 
> My hope is that this would allow a better chance of understanding
> the stop-DMA errors that some people get reliably (but which I can never reliably
> reproduce).
> 
> I'm not sure how that plays into your 'event log' idea, but maybe
> one will help the other.
I think the 'let's dump all kinds of random crap when the issue occurs
until we find somebody that can parse it' approach won't work here, and
I really think it's not a good idea in general.

In the past the stop-DMA crap has been a symptom with a wild variety of
different causes, most of which were actually *software* race
conditions, e.g. dma tx or rx enable during reset, locking issues, etc.

In addition to those software causes, there was one actual hardware
condition that also triggered this error, and even this wouldn't have
showed up in a normal register dump, because it required setting up the
MAC observation bus in a particular way.

That hardware trigger for this issue was analyzed not by dumping random
data, but by actually talking to hardware designers that could look
through the code and guide the debugging process.

Let's not carpet-bomb the driver with lots of debug crap that probably
won't ever lead anybody to any good solution for the remaining issues,
let's fix stuff the old-fashioned way: by reading the code,
understanding what's going on, analyzing problems in a systematic way,
rather than clouding the whole process with assumptions based on old
bugs that have since been fixed.

- Felix

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Ben Greear]

On 02/14/2012 02:29 PM, Felix Fietkau wrote:
> On 2012-02-14 7:43 PM, Ben Greear wrote:
>> On 02/14/2012 10:29 AM, Sujith wrote:
>>> Ben Greear wrote:
>>>> Actually, I think it might be useful to have a second level of debugging.
>>>> I hope to soon have time&   resources to add some logic to dump lots of register
>>>> info and such in human-readable format, (like, when DMA times out).  That is going to be a lot
>>>> of strings added to the driver, so the compile size will definitely
>>>> increase.  If keeping the size small is important, then this sort of verbose thing
>>>> could be hidden behind a second level of debugging...
>>>
>>> That could be implemented similar to what usbmon does. A debugfs file that could
>>> be read and redirected to a file. And there would be no overhead to the
>>> driver, I think. We could call it the 'event log'. :)
>>
>> I was thinking about adding a method that grabbed as many registers
>> as I have info for and dumping them with printk when DMA errors
>> hit.  This would make kernel splats more useful.
>>
>> And also have a debugfs file called 'registers' or similar that one
>> could cat out and get similar info.  And this can let folks look
>> at steady-state or whatever.
>>
>> But, the logic to turn the register bit values into strings would
>> be in the driver (and thus add some code size bloat).
>>
>> My hope is that this would allow a better chance of understanding
>> the stop-DMA errors that some people get reliably (but which I can never reliably
>> reproduce).
>>
>> I'm not sure how that plays into your 'event log' idea, but maybe
>> one will help the other.
> I think the 'let's dump all kinds of random crap when the issue occurs
> until we find somebody that can parse it' approach won't work here, and
> I really think it's not a good idea in general.
>
> In the past the stop-DMA crap has been a symptom with a wild variety of
> different causes, most of which were actually *software* race
> conditions, e.g. dma tx or rx enable during reset, locking issues, etc.

I'm interested in parsing it.  There are folks that can reproduce
this bug every time, and it seems none of the developers can reproduce
it.  So, the only thing I can think to do is to try to get more info
from the folks that see the problem.

The good news is that some of them are desperate enough to run my
hacked kernel, so if such patches are not wanted upstream,
I'll just put it in my tree and see if I can get any useful
info from them that way...

> Let's not carpet-bomb the driver with lots of debug crap that probably
> won't ever lead anybody to any good solution for the remaining issues,
> let's fix stuff the old-fashioned way: by reading the code,
> understanding what's going on, analyzing problems in a systematic way,
> rather than clouding the whole process with assumptions based on old
> bugs that have since been fixed.

That sounds good, and I hope folks are doing that.  But as for me,
unless I can reproduce a problem I don't think I'll be able to
do much, as I don't understand the code that well
and I don't have any access to folks who know the details
of the hardware and such...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

Ben Greear wrote:
> I was thinking about adding a method that grabbed as many registers
> as I have info for and dumping them with printk when DMA errors
> hit.  This would make kernel splats more useful.
> 
> And also have a debugfs file called 'registers' or similar that one
> could cat out and get similar info.  And this can let folks look
> at steady-state or whatever.
> 
> But, the logic to turn the register bit values into strings would
> be in the driver (and thus add some code size bloat).
> 
> My hope is that this would allow a better chance of understanding
> the stop-DMA errors that some people get reliably (but which I can never reliably
> reproduce).
> 
> I'm not sure how that plays into your 'event log' idea, but maybe
> one will help the other.

I was thinking more on the lines of instrumentation, but what you are
describing here looks like a VERBOSE_DEBUG option is needed.

Sujith

Re: [ath9k-devel] [PATCH 3/7] ath9k: Merge wiphy and misc debugfs files

[Sujith Manoharan]

Sujith Manoharan wrote:
> Ben Greear wrote:
> > I was thinking about adding a method that grabbed as many registers
> > as I have info for and dumping them with printk when DMA errors
> > hit.  This would make kernel splats more useful.
> > 
> > And also have a debugfs file called 'registers' or similar that one
> > could cat out and get similar info.  And this can let folks look
> > at steady-state or whatever.
> > 
> > But, the logic to turn the register bit values into strings would
> > be in the driver (and thus add some code size bloat).
> > 
> > My hope is that this would allow a better chance of understanding
> > the stop-DMA errors that some people get reliably (but which I can never reliably
> > reproduce).
> > 
> > I'm not sure how that plays into your 'event log' idea, but maybe
> > one will help the other.
> 
> I was thinking more on the lines of instrumentation, but what you are
> describing here looks like a VERBOSE_DEBUG option is needed.

Huh.

This discussion amounts to nothing. The stuff that was removed was dumping
the information present in mac80211.

Sujith