On Thursday 05 December 2013 22:56:01 you wrote:
> On 2013-12-05 22:41, Tijs Van Buggenhout wrote:
> > Hi Felix,
> >
> > I have a question about https://dev.openwrt.org/changeset/38486. While
> > backporting the aforementioned patch from trunk to attitude adjustment, I
> > noticed that the patch does not include a gettor function for the adc
> > entropy of the ar9002 chip, only those of ar5008 and ar9003.
> >
> > static void ar9003_hw_get_adc_entropy(struct ath_hw *ah, u8 *buf, size_t
> > len) static void ar5008_hw_get_adc_entropy(struct ath_hw *ah, u8 *buf,
> > size_t len)
> >
> > but no
> >
> > static void ar9002_hw_get_adc_entropy(struct ath_hw *ah, u8 *buf, size_t
> > len)
> >
> > Furthermore, when the phy_ops are constructed in ar9002_hw_attach_phy_ops,
> > the get_adc_entropy function pointer is never assigned.
> >
> > I can be wrong ofcourse, but am under the impression this will create a
> > problem when the driver is probed for an ar9002 chip and possibly result
> > in a kernel oops, because "ath9k_hw_get_adc_entropy" expects the ath_hw
> > struct to have a valid (non-NULL) function pointer for get_adc_entropy.
> > See below:
> >
> > static inline void ath9k_hw_get_adc_entropy(struct ath_hw *ah,
> >
> > u8 *buf, size_t len)
> >
> > {
> >
> > ath9k_hw_ops(ah)->get_adc_entropy(ah, buf, len);
> >
> > }
> >
> > Did changes in ar9002_phy.c somehow miss inclusion in the patch?
>
> ar5008_hw_attach_phy_ops is called for all chips older than AR9003, and
> it is generic enough to work across that range of chips.
Ah yes, I see what you mean in ar9002_hw_attach_ops
ret = ar5008_hw_attach_phy_ops(ah);
if (ret)
return ret;
if (AR_SREV_9280_20_OR_LATER(ah))
ar9002_hw_attach_phy_ops(ah);
Missed that.. I assumed hw_attach_phy_ops was specific for every chip.
> > When testing the backported patch for my ar9003 chip, I was confronted
> > with a kernel oops at initialisation, which I narrowed down to
> > ath9k_hw_reset function, called from ath_get_initial_entropy, where the
> > channels in ieee80211_conf data field in common hardware config are not
> > yet initialised it seems. I changed the condition to verify for a valid
> > pointer before dereferencing it, which seems to solve the problem.
> > This is different in the mac82011 version from trunk (compat-
> > wireless-2013-11-05), where channelFlags from channel are used in the
> > condition, which is an u32, and thus can not result in a null pointer
> > dereference. My question now is, although the fix seems sufficient, is it
> > also correct/expected behaviour?
>
> The patch assumes that ah->curchan is left initializes after the tx
> power limit test.
> In ath9k_init_txpower_limits, the new version has this code:
>
> if (curchan)
> ah->curchan = curchan;
>
> I think the old version probably overwrites ah->curchan without checking
> for NULL.
This is part of the entropy patch, which I included in the backport aswell..
diff -u -Naur a/drivers/net/wireless/ath/ath9k/init.c
b/drivers/net/wireless/ath/ath9k/init.c
--- a/drivers/net/wireless/ath/ath9k/init.c 2013-10-28 18:00:58.000000000
+0100
+++ b/drivers/net/wireless/ath/ath9k/init.c 2013-12-05 21:09:55.000000000
+0100
@@ -735,7 +735,8 @@
if (ah->caps.hw_caps & ATH9K_HW_CAP_5GHZ)
ath9k_init_band_txpower(sc, IEEE80211_BAND_5GHZ);
- ah->curchan = curchan;
+ if (curchan)
+ ah->curchan = curchan;
}
void ath9k_reload_chainmask_settings(struct ath_softc *sc)
@@ -880,6 +881,19 @@
but the problem is in struct ieee80211_conf, part of hw common config, which
is not yet initialised when ath_get_initial_entropy is called from
ath9k_init_device..
--- a/drivers/net/wireless/ath/ath9k/hw.c 2013-10-28 18:00:58.000000000
+0100
+++ b/drivers/net/wireless/ath/ath9k/hw.c 2013-12-05 21:05:25.000000000
+0100
@@ -131,8 +131,8 @@
static void ath9k_hw_set_clockrate(struct ath_hw *ah)
{
- struct ieee80211_conf *conf = &ath9k_hw_common(ah)->hw->conf;
struct ath_common *common = ath9k_hw_common(ah);
+ struct ieee80211_conf *conf = &common->hw->conf;
unsigned int clockrate;
/* AR9287 v1.3+ uses async FIFO and runs the MAC at 117 MHz */
@@ -140,7 +140,7 @@
clockrate = 117;
else if (!ah->curchan) /* should really check for CCK instead */
clockrate = ATH9K_CLOCK_RATE_CCK;
- else if (conf->chandef.chan->band == IEEE80211_BAND_2GHZ)
+ else if (conf->chandef.chan && conf->chandef.chan->band ==
IEEE80211_BAND_2GHZ)
In ath9k_hw_reset, I experienced the problem with ath9k_hw_set_clockrate(ah)
at line 1998, a few lines later (2008) in ath9k_hw_init_global_settings(ah) I
see the same kind of null condition check I introduced in
ath9k_hw_set_clockrate:
/*
* Workaround for early ACK timeouts, add an offset to match the
* initval's 64us ack timeout value. Use 48us for the CTS timeout.
* This was initially only meant to work around an issue with delayed
* BA frames in some implementations, but it has been found to fix ACK
* timeout issues in other cases as well.
*/
if (conf->chandef.chan &&
conf->chandef.chan->band == IEEE80211_BAND_2GHZ &&
!IS_CHAN_HALF_RATE(chan) && !IS_CHAN_QUARTER_RATE(chan)) {
acktimeout += 64 - sifstime - ah->slottime;
ctstimeout += 48 - sifstime - ah->slottime;
}
So it is probably safe to assume this can happen.
When I move the call to ath_get_initial_entropy in ath9k_init_device, which is
in between ath9k_init_txpower_limits and ieee80211_register_hw, down a bit
until after ieee80211_register_hw, the kernel oops is no more, like so:
@@ -918,30 +932,32 @@ int ath9k_init_device(u16 devid, struct
ath9k_init_txpower_limits(sc);
#ifdef CPTCFG_MAC80211_LEDS
/* must be initialized before ieee80211_register_hw */
sc->led_default_trigger = ieee80211_create_tpt_led_trigger(sc->hw,
IEEE80211_TPT_LEDTRIG_FL_RADIO, ath9k_tpt_blink,
ARRAY_SIZE(ath9k_tpt_blink));
#endif
/* Register with mac80211 */
error = ieee80211_register_hw(hw);
if (error)
goto rx_cleanup;
+ ath_get_initial_entropy(sc);
+
error = ath9k_init_debug(ah);
if (error) {
ath_err(common, "Unable to create debugfs files\n");
goto unregister;
}
Is there anything opposed this change?
> - Felix
Thanks,
Tijs
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
