Hi Stanislaw,
Thank you for your patience ...
On Tue, 2009-08-11 at 07:09 -0700, Stanislaw Gruszka wrote:
> On Mon, Aug 10, 2009 at 09:44:52AM -0700, reinette chatre wrote:
> > Yes. I assume that what happens here is that rfkill notifies user that
> > state changes to RFKILL_STATE_UNBLOCKED. In your new patch the driver
> > will now clear STATUS_RF_KILL_SW, with STATUS_RF_KILL_HW still being
> > set. So, in this run, after iwl_rfkill_soft_rf_kill is called there will
> > be a state mismatch with rfkill thinking the system is unblocked while
> > the driver has it as hard blocked. This is not right.
>
> In such case we return -EBUSY from iwl_rfkill_soft_rf_kill() - rfkill
> state not change.
oh - right - sorry
> I made a comment it will be HARD_BLOCKED, this
> is not true anymore, it can be also in state SOFT_BLOCKED .
How so? Isn't the idea behind toggle_radio that the SOFT_BLOCKED state
changes? In this case when we get a new state of UNBLOCKED then I do not
understand how SOFT_BLOCKED can be true also.
> However
> comment was true with previous version of the patch for 2.6.29, where
> there was no HARD -> SOFT downgrade and that part was called only when
> rfkill state was HARD_BLOCKED.
>
> > Can this be fixed by adding a iwl_rfkill_set_hw_state in this run?
>
> We can not call iwl_rfkill_set_hw_state in iwl_rfkill_soft_rt_kill
> as rfkill->muttex is taken. We eventually can force state in the same ugly
> way as is done for case RFKILL_STATE_SOFT_BLOCKED and I think, this is good
> idea :) , below not tested delta patch:
>
This just seems to mess with the rfkill internals even more. Can this
not be avoided?
> diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> index d6b6098..636c04a 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
> @@ -35,6 +35,19 @@
> #include "iwl-dev.h"
> #include "iwl-core.h"
>
> +static void iwl_force_rfkill_state(struct iwl_priv *priv,
> + enum rfkill_state state)
> +{
> + enum rfkill_state oldstate;
> +
> + oldstate = priv->rfkill->state;
> + priv->rfkill->state = state;
> +
> + /* rfkill_uevent() */
> + if (oldstate != state)
> + kobject_uevent(&priv->rfkill->dev.kobj, KOBJ_CHANGE);
> +}
> +
> /* software rf-kill from user */
> static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> {
> @@ -54,8 +67,9 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> case RFKILL_STATE_UNBLOCKED:
> if (iwl_is_rfkill_hw(priv)) {
> err = -EBUSY;
> - /* pass error to rfkill core to make it state HARD
> + /* pass error to rfkill core, make it state HARD
> * BLOCKED and disable software kill switch */
> + iwl_force_rfkill_state(priv, RFKILL_STATE_HARD_BLOCKED);
> }
> iwl_radio_kill_sw_enable_radio(priv);
> break;
> @@ -63,10 +77,10 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
> iwl_radio_kill_sw_disable_radio(priv);
> /* rfkill->mutex lock is taken */
> if (priv->rfkill->state == RFKILL_STATE_HARD_BLOCKED) {
> - /* force rfkill core state to be SOFT BLOCKED,
> + /* force rfkill core state to be in SOFT BLOCKED,
> * otherwise core will be unable to disable software
> * kill switch */
> - priv->rfkill->state = RFKILL_STATE_SOFT_BLOCKED;
> + iwl_force_rfkill_state(priv, RFKILL_STATE_SOFT_BLOCKED);
> }
> break;
> default:
> > >From what I can tell this patch introduced a disagreement of rfkill
> > state between driver and rfkill system.
>
> In driver we have no states, but separate bits for each killswitch. Situation
> gets better after rfkill rewrite where also rfkill core become to have separate
> bits, but with 2.6.30 we have no such luck.
>
> Currently we have "states" like below:
>
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED
>
> Patch is intended to work like that:
>
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=1 <-> RFKILL_STATUS_SOFT_BLOCKED
> STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_HARD_BLOCKED
> STATUS_RF_KILL_HW=0 STATUS_RF_KILL_SW=0 <-> RFKILL_STATUS_UNBLOCKED
I can see that this is what the last hunk of the patch accomplishes -
but I do not see why it is needed.
>
> STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
>
> driver HW on
>
> STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
>
> rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
>
> STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
>
> rfkill SW off (HARD_BLOCKED not clearing STATUS_RF_KILL_SW)
>
> STATUS_RF_KILL_HW=1, STATUS_RF_KILL_SW=1, RFKILL_STATE_HARD_BLOCKED
>
> driver HW off (called from iwl_bg_rf_kill())
>
> STATUS_RF_KILL_HW=0, STATUS_RF_KILL_SW=1, RFKILL_STATE_SOFT_BLOCKED
>
> rfkill core no longer wants to turn radio on
>From what I understand what you are describing above should be addressed
by this hunk of your patch:
case RFKILL_STATE_UNBLOCKED:
if (iwl_is_rfkill_hw(priv)) {
err = -EBUSY;
- goto out_unlock;
+ /* pass error to rfkill core to make it state HARD
+ * BLOCKED and disable software kill switch */
}
This should make these new transitions possible:
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
driver HW on
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
rfkill SW on ( -> rfkill_epo() -> rfkill_toggle_radio() with force = 1)
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
rfkill SW off
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
driver HW off (called from iwl_bg_rf_kill())
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
Looking further I tried to see how other combinations would be treated. Here is how I see the potential scenarios:
Case1 (considered above):
driver HW on -> rfkill SW on -> rfkill SW off -> driver HW off
Case2:
driver HW on -> rfkill SW on -> driver HW off -> rfkill SW off
Case3:
rfkill SW on -> driver HW on -> rfkill SW off -> driver HW off
Case4:
rfkill SW on -> driver HW on -> driver HW off -> rfkill SW off
Looking at the rest of the cases I do not see the problem addressed by the other hunks.
I see:
Case 2:
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
driver HW on
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
rfkill SW on
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
driver HW off
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
Case3:
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
rfkill SW on
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
driver HW on
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
rfkill SW off
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 0, RFKILL_STATE_HARD_BLOCKED
driver HW off
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
Case4:
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
rfkill SW on
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
driver HW on
STATUS_RF_KILL_HW = 1, STATUS_RF_KILL_SW = 1, RFKILL_STATE_HARD_BLOCKED
driver HW off
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 1, RFKILL_STATE_SOFT_BLOCKED
rfkill SW off
STATUS_RF_KILL_HW = 0, STATUS_RF_KILL_SW = 0, RFKILL_STATE_UNBLOCKED
I understand that one hunk of your patch accomplishes the mapping of
"STATUS_RF_KILL_HW=1 STATUS_RF_KILL_SW=1 <->
RFKILL_STATUS_SOFT_BLOCKED" - but I do not understand why it is needed. Could you please explain?
I also do not understand the need to modify rfkill's internal state.
Reinette
--
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
