| Message ID | 1398327250-12923-1-git-send-email-yeohchunyeow@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Chun-Yeow Yeoh <yeohchunyeow@gmail.com> writes: > Firmware 999.999.0.636 does not allow stand alone monitor > mode. This means that bridging the STA mode and put it into > promiscuous mode will also cause the firmware to crash. Avoid > this. > > Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com> [...] > @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar) > > static int ath10k_monitor_start(struct ath10k *ar) > { > - int ret; > + int ret = -1; I prefer to avoid initialising ret variables. And -1 is not a proper error value. > lockdep_assert_held(&ar->conf_mutex); > > + if (ar->fw_version_build == 636) { Checking for firmware version in ath10k is a big no. For a functinality change like this you should add a new feature flag to enum ath10k_fw_features (and I need to then recreate the firmware image). > + ath10k_warn("stand alone monitor mode is not supported\n"); I would prefer not to print a warning for a situation like this. Can't we instead return an error value back to the caller? > + return ret; return -EOPNOTSUPP or similar is better approach than initialising ret to -1.
On Thu, Apr 24, 2014 at 4:22 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Chun-Yeow Yeoh <yeohchunyeow@gmail.com> writes: > >> Firmware 999.999.0.636 does not allow stand alone monitor >> mode. This means that bridging the STA mode and put it into >> promiscuous mode will also cause the firmware to crash. Avoid >> this. >> >> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com> > > [...] > >> @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar) >> >> static int ath10k_monitor_start(struct ath10k *ar) >> { >> - int ret; >> + int ret = -1; > > I prefer to avoid initialising ret variables. And -1 is not a proper > error value. > Ok. >> lockdep_assert_held(&ar->conf_mutex); >> >> + if (ar->fw_version_build == 636) { > > Checking for firmware version in ath10k is a big no. For a functinality > change like this you should add a new feature flag to enum > ath10k_fw_features (and I need to then recreate the firmware image). > Should we just use the ATH10K_FW_FEATURE_WMI_10X? >> + ath10k_warn("stand alone monitor mode is not supported\n"); > > I would prefer not to print a warning for a situation like this. Can't > we instead return an error value back to the caller? > Yes. >> + return ret; > > return -EOPNOTSUPP or similar is better approach than initialising ret > to -1. Sure. ---- Chun-Yeow -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 April 2014 10:14, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> wrote: > Firmware 999.999.0.636 does not allow stand alone monitor > mode. This means that bridging the STA mode and put it into > promiscuous mode will also cause the firmware to crash. Avoid > this. > > Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com> > --- > drivers/net/wireless/ath/ath10k/mac.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index e2c01dc..f640328 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar) > > static int ath10k_monitor_start(struct ath10k *ar) > { > - int ret; > + int ret = -1; > > lockdep_assert_held(&ar->conf_mutex); > > + if (ar->fw_version_build == 636) { > + ath10k_warn("stand alone monitor mode is not supported\n"); > + return ret; > + } I think Monitor operation should be performed on a best effort basis. This means monitor_start/stop should be attempted once number of non-monitor vdevs changes. We should probably introduce a ath10k_recalc_monitor() for that purpose. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes: >>> + if (ar->fw_version_build == 636) { >> >> Checking for firmware version in ath10k is a big no. For a functinality >> change like this you should add a new feature flag to enum >> ath10k_fw_features (and I need to then recreate the firmware image). >> > Should we just use the ATH10K_FW_FEATURE_WMI_10X? That's a bit dangerous if in the future there's a new firmware which doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand alone monitor mode.
> I think Monitor operation should be performed on a best effort basis. > This means monitor_start/stop should be attempted once number of > non-monitor vdevs changes. I am too clear about this. Do you mean that actually we can have monitor mode on non-monitor vdevs in firmware 636? ---- Chun-Yeow -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-04-24 at 10:44 +0200, Michal Kazior wrote: > I think Monitor operation should be performed on a best effort basis. > This means monitor_start/stop should be attempted once number of > non-monitor vdevs changes. > > We should probably introduce a ath10k_recalc_monitor() for that purpose. Doesn't mac80211 do that for you? See IEEE80211_HW_WANT_MONITOR_VIF. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 April 2014 10:50, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote: >> I think Monitor operation should be performed on a best effort basis. >> This means monitor_start/stop should be attempted once number of >> non-monitor vdevs changes. > > I am too clear about this. Do you mean that actually we can have > monitor mode on non-monitor vdevs in firmware 636? No. What I mean is we should attempt to start monitor vdev once at least 1 non-monitor vdev is present and stop monitor vdev is last non-monitor vdev is about to be stopped on 636. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 April 2014 10:50, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-04-24 at 10:44 +0200, Michal Kazior wrote: > >> I think Monitor operation should be performed on a best effort basis. >> This means monitor_start/stop should be attempted once number of >> non-monitor vdevs changes. >> >> We should probably introduce a ath10k_recalc_monitor() for that purpose. > > Doesn't mac80211 do that for you? > > See IEEE80211_HW_WANT_MONITOR_VIF. This is not sufficient in this case. E.g. If you add a disconnected 4addr sta interface to bridge the interface enters promisc mode. This attempts to start monitor vdev in ath10k before sta vdev is started internally. We could probably make it start earlier (in add_interface) but there still remains a problem when you stop last non-monitor interface (monitor vif will be created _after_ last non-monitor is removed which is too late). Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 24, 2014 at 4:53 PM, Michal Kazior <michal.kazior@tieto.com> wrote: > On 24 April 2014 10:50, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote: >>> I think Monitor operation should be performed on a best effort basis. >>> This means monitor_start/stop should be attempted once number of >>> non-monitor vdevs changes. >> >> I am too clear about this. Do you mean that actually we can have >> monitor mode on non-monitor vdevs in firmware 636? > > No. What I mean is we should attempt to start monitor vdev once at > least 1 non-monitor vdev is present and stop monitor vdev is last > non-monitor vdev is about to be stopped on 636. > Got it. But my investigation on firmware 636 shows that the firmware crashes even though 1 non-monitor vdev is present. So can we conclude actually monitor mode is not allowed in firmware 636. --- Chun-Yeow -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes: > >>>> + if (ar->fw_version_build == 636) { >>> >>> Checking for firmware version in ath10k is a big no. For a functinality >>> change like this you should add a new feature flag to enum >>> ath10k_fw_features (and I need to then recreate the firmware image). >>> >> Should we just use the ATH10K_FW_FEATURE_WMI_10X? > > That's a bit dangerous if in the future there's a new firmware which > doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand > alone monitor mode. > Then, we may need to introduce the new feature flag. But then I just wondering if the firmware 636 claimed to support STA mode "well" but then not allowed to be bridged. This may cause confusion to end user which is the best firmware for STA mode. FYI, AP firmware has no such issue if using as STA mode and put into promiscuous mode. ---- Chun-Yeow -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes: > On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes: >> >>>>> + if (ar->fw_version_build == 636) { >>>> >>>> Checking for firmware version in ath10k is a big no. For a functinality >>>> change like this you should add a new feature flag to enum >>>> ath10k_fw_features (and I need to then recreate the firmware image). >>>> >>> Should we just use the ATH10K_FW_FEATURE_WMI_10X? >> >> That's a bit dangerous if in the future there's a new firmware which >> doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand >> alone monitor mode. > > Then, we may need to introduce the new feature flag. And that will create other problems. It's better to bite the bullet now than trying to postpone it. Besides, adding the feature flag is trivial. > But then I just wondering if the firmware 636 claimed to support STA > mode "well" but then not allowed to be bridged. This may cause > confusion to end user which is the best firmware for STA mode. FYI, AP > firmware has no such issue if using as STA mode and put into > promiscuous mode. Yeah, maybe should change the documentation to recommend using 10.1 branch for AP, STA and monitor modes? And recommended main branch only for Ad-Hoc and P2P?
> > Yeah, maybe should change the documentation to recommend using 10.1 > branch for AP, STA and monitor modes? And recommended main branch only > for Ad-Hoc and P2P? > Eventually, we need a single firmware that can support all the modes, including mesh. For the 10.1, it cited " STA specific features might not work". Can someone comment what working and what not working? ---- Chun-Yeow -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index e2c01dc..f640328 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar) static int ath10k_monitor_start(struct ath10k *ar) { - int ret; + int ret = -1; lockdep_assert_held(&ar->conf_mutex); + if (ar->fw_version_build == 636) { + ath10k_warn("stand alone monitor mode is not supported\n"); + return ret; + } + if (!ath10k_monitor_is_enabled(ar)) { ath10k_warn("trying to start monitor with no references\n"); return 0;
Firmware 999.999.0.636 does not allow stand alone monitor mode. This means that bridging the STA mode and put it into promiscuous mode will also cause the firmware to crash. Avoid this. Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com> --- drivers/net/wireless/ath/ath10k/mac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)