[PATCH 2/2] enhance sysfs rfkill interface
flo...@mickler.org
/sys/class/rfkill/rfkill[0-9]+/hard: (ro)
hardblock kill state
/sys/class/rfkill/rfkill[0-9]+/soft: (rw)
softblock kill state
Signed-off-by: Florian Mickler <flo...@mickler.org>
---
Documentation/ABI/stable/sysfs-class-rfkill | 25 ++++++++++++
net/rfkill/core.c | 57 +++++++++++++++++++++++++++
2 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-class-rfkill b/Documentation/ABI/stable/sysfs-class-rfkill
index 3884344..097f522 100644
--- a/Documentation/ABI/stable/sysfs-class-rfkill
+++ b/Documentation/ABI/stable/sysfs-class-rfkill
@@ -40,3 +40,28 @@ Description: Whether the soft blocked state is initialised from non-volatile
Values: A numeric value.
0: false
1: true
+
+
+What: /sys/class/rfkill/rfkill[0-9]+/hard
+Date: 12-March-2010
+KernelVersion v2.6.34
+Contact: linux-w...@vger.kernel.org
+Description: Current hardblock state. This file is read only.
+Values: A numeric value.
+ 0: inactive
+ The transmitter is (potentially) active.
+ 1: active
+ The transmitter is forced off by something outside of
+ the driver's control.
+
+
+What: /sys/class/rfkill/rfkill[0-9]+/soft
+Date: 12-March-2010
+KernelVersion v2.6.34
+Contact: linux-w...@vger.kernel.org
+Description: Current softblock state. This file is read and write.
+Values: A numeric value.
+ 0: inactive
+ The transmitter is (potentially) active.
+ 1: active
+ The transmitter is turned off by software.
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index c218e07..38a0358 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -628,6 +628,61 @@ static ssize_t rfkill_persistent_show(struct device *dev,
return sprintf(buf, "%d\n", rfkill->persistent);
}
+static ssize_t rfkill_hard_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct rfkill *rfkill = to_rfkill(dev);
+ unsigned long flags;
+ u32 state;
+
+ spin_lock_irqsave(&rfkill->lock, flags);
+ state = rfkill->state;
+ spin_unlock_irqrestore(&rfkill->lock, flags);
+
+ return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_HW) ? 1 : 0 );
+}
+
+static ssize_t rfkill_soft_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct rfkill *rfkill = to_rfkill(dev);
+ unsigned long flags;
+ u32 state;
+
+ spin_lock_irqsave(&rfkill->lock, flags);
+ state = rfkill->state;
+ spin_unlock_irqrestore(&rfkill->lock, flags);
+
+ return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_SW) ? 1 : 0 );
+}
+
+static ssize_t rfkill_soft_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct rfkill *rfkill = to_rfkill(dev);
+ unsigned long state;
+ int err;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ err = strict_strtoul(buf, 0, &state);
+ if (err)
+ return err;
+
+ if (state > 1 )
+ return -EINVAL;
+
+ mutex_lock(&rfkill_global_mutex);
+ rfkill_set_block(rfkill, state);
+ mutex_unlock(&rfkill_global_mutex);
+
+ return err ?: count;
+}
+
static u8 user_state_from_blocked(unsigned long state)
{
if (state & RFKILL_BLOCK_HW)
@@ -700,6 +755,8 @@ static struct device_attribute rfkill_dev_attrs[] = {
__ATTR(persistent, S_IRUGO, rfkill_persistent_show, NULL),
__ATTR(state, S_IRUGO|S_IWUSR, rfkill_state_show, rfkill_state_store),
__ATTR(claim, S_IRUGO|S_IWUSR, rfkill_claim_show, rfkill_claim_store),
+ __ATTR(soft, S_IRUGO|S_IWUSR, rfkill_soft_show, rfkill_soft_store),
+ __ATTR(hard, S_IRUGO, rfkill_hard_show, NULL),
__ATTR_NULL
};
--
1.6.6.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Dmitry Torokhov
>
> +static ssize_t rfkill_hard_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct rfkill *rfkill = to_rfkill(dev);
> + unsigned long flags;
> + u32 state;
> +
> + spin_lock_irqsave(&rfkill->lock, flags);
> + state = rfkill->state;
> + spin_unlock_irqrestore(&rfkill->lock, flags);
Why exactly is this lock needed?
> +
> + return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_HW) ? 1 : 0 );
> +}
> +
> +static ssize_t rfkill_soft_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct rfkill *rfkill = to_rfkill(dev);
> + unsigned long flags;
> + u32 state;
> +
> + spin_lock_irqsave(&rfkill->lock, flags);
> + state = rfkill->state;
> + spin_unlock_irqrestore(&rfkill->lock, flags);
And here as well...
--
Dmitry
Florian Mickler
Dmitry Torokhov <dmitry....@gmail.com> wrote:
> On Fri, Mar 12, 2010 at 07:03:08PM +0100, flo...@mickler.org wrote:
> >
> > +static ssize_t rfkill_hard_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct rfkill *rfkill = to_rfkill(dev);
> > + unsigned long flags;
> > + u32 state;
> > +
> > + spin_lock_irqsave(&rfkill->lock, flags);
> > + state = rfkill->state;
> > + spin_unlock_irqrestore(&rfkill->lock, flags);
>
> Why exactly is this lock needed?
The rfkill state is updated from multiple contexts... Am I overlooking
smth obvious here?
cheers,
Flo
Dmitry Torokhov
> On Fri, 12 Mar 2010 10:22:09 -0800
> Dmitry Torokhov <dmitry....@gmail.com> wrote:
>
> > On Fri, Mar 12, 2010 at 07:03:08PM +0100, flo...@mickler.org wrote:
> > >
> > > +static ssize_t rfkill_hard_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct rfkill *rfkill = to_rfkill(dev);
> > > + unsigned long flags;
> > > + u32 state;
> > > +
> > > + spin_lock_irqsave(&rfkill->lock, flags);
> > > + state = rfkill->state;
> > > + spin_unlock_irqrestore(&rfkill->lock, flags);
> >
> > Why exactly is this lock needed?
>
> The rfkill state is updated from multiple contexts... Am I overlooking
> smth obvious here?
>
You are not updating but reading... Are you concerned about seeing
a partial write to u32? It does not happen.
--
Dmitry
Florian Mickler
Dmitry Torokhov <dmitry....@gmail.com> wrote:
> On Fri, Mar 12, 2010 at 09:57:43PM +0100, Florian Mickler wrote:
> > On Fri, 12 Mar 2010 10:22:09 -0800
> > Dmitry Torokhov <dmitry....@gmail.com> wrote:
> >
> > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, flo...@mickler.org wrote:
> > > >
> > > > +static ssize_t rfkill_hard_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + struct rfkill *rfkill = to_rfkill(dev);
> > > > + unsigned long flags;
> > > > + u32 state;
> > > > +
> > > > + spin_lock_irqsave(&rfkill->lock, flags);
> > > > + state = rfkill->state;
> > > > + spin_unlock_irqrestore(&rfkill->lock, flags);
> > >
> > > Why exactly is this lock needed?
> >
> > The rfkill state is updated from multiple contexts... Am I overlooking
> > smth obvious here?
> >
>
> You are not updating but reading... Are you concerned about seeing
> a partial write to u32? It does not happen.
>
Hm.. You shure? On every arch that supports wireless drivers?
I've just copied that code from the old sysfs state-file handler.
So I assumed that reading partial updated state can happen... Also I
just searched a little but did not find anything, cause i didn't know
where to look. Who garantees this? Is it a gcc thing?
Dmitry Torokhov
None of the arches would do byte-by-byte writes to a u32, they'd write
dword at once. Also, even if they could, you are interested in a single
flag (bit). You do realize that once you leave spinlock whatever you
fetched is stale data and may not be trusted?
--
Dmitry
Valdis.K...@vt.edu
> On Fri, 12 Mar 2010 13:20:26 -0800
> Dmitry Torokhov <dmitry....@gmail.com> wrote:
> > You are not updating but reading... Are you concerned about seeing
> > a partial write to u32? It does not happen.
> >
> Hm.. You shure? On every arch that supports wireless drivers?
If a u32 load or store from memory isn't atomic, the Linux kernel is screwed
anyhow. Hint - imagine if every 32-bit reference had to be treated the way
we currently treat 64-bit references on a 32-bit system.
Florian Mickler
Dmitry Torokhov <dmitry....@gmail.com> wrote:
On Fri, 12 Mar 2010 18:48:19 -0500
Valdis.K...@vt.edu wrote:
> If a u32 load or store from memory isn't atomic, the Linux kernel is screwed
> anyhow. Hint - imagine if every 32-bit reference had to be treated the way
> we currently treat 64-bit references on a 32-bit system.
i presume, there is no way any digital device could write _one bit_
partial :)
so this _may_ actually be safe *g*
how about the write in the _store() function? there we
read,update and write back the whole 32 bit which then potentially
overwrites some other flag concurrently set by an driver interrupt on
another cpu? i think the lock there is needed.
cheers,
Flo
flo...@mickler.org
"blocked_hw" and "blocked_sw" to "hard" and "soft", as the hardware vs
software conotation is misleading.
It also gets rid of not needed locks around u32-read-access.
Signed-off-by: Florian Mickler <flo...@mickler.org>
---
Documentation/ABI/stable/sysfs-class-rfkill | 10 +++++-----
net/rfkill/core.c | 17 +++++------------
2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-class-rfkill b/Documentation/ABI/stable/sysfs-class-rfkill
index b91c3f3..097f522 100644
--- a/Documentation/ABI/stable/sysfs-class-rfkill
+++ b/Documentation/ABI/stable/sysfs-class-rfkill
@@ -4,7 +4,7 @@ For details to this subsystem look at Documentation/rfkill.txt.
For the deprecated /sys/class/rfkill/*/state and
/sys/class/rfkill/*/claim knobs of this interface look in
-Documentation/ABI/obsolte/sysfs-class-rfkill.
+Documentation/ABI/obsolete/sysfs-class-rfkill.
What: /sys/class/rfkill
Date: 09-Jul-2007
@@ -42,8 +42,8 @@ Values: A numeric value.
1: true
-What: /sys/class/rfkill/rfkill[0-9]+/blocked_hw
-Date: 23-Feb-2010
+What: /sys/class/rfkill/rfkill[0-9]+/hard
+Date: 12-March-2010
KernelVersion v2.6.34
Contact: linux-w...@vger.kernel.org
Description: Current hardblock state. This file is read only.
@@ -55,8 +55,8 @@ Values: A numeric value.
the driver's control.
-What: /sys/class/rfkill/rfkill[0-9]+/blocked_sw
-Date: 23-Feb-2010
+What: /sys/class/rfkill/rfkill[0-9]+/soft
+Date: 12-March-2010
KernelVersion v2.6.34
Contact: linux-w...@vger.kernel.org
Description: Current softblock state. This file is read and write.
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 5f33151..60830f6 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -628,7 +628,7 @@ static ssize_t rfkill_persistent_show(struct device *dev,
return sprintf(buf, "%d\n", rfkill->persistent);
}
-static ssize_t rfkill_blocked_hw_show(struct device *dev,
+static ssize_t rfkill_hard_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
@@ -636,14 +636,12 @@ static ssize_t rfkill_blocked_hw_show(struct device *dev,
unsigned long flags;
u32 state;
- spin_lock_irqsave(&rfkill->lock, flags);
state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_HW) ? 1 : 0 );
}
-static ssize_t rfkill_blocked_sw_show(struct device *dev,
+static ssize_t rfkill_soft_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
@@ -651,14 +649,12 @@ static ssize_t rfkill_blocked_sw_show(struct device *dev,
unsigned long flags;
u32 state;
- spin_lock_irqsave(&rfkill->lock, flags);
state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_SW) ? 1 : 0 );
}
-static ssize_t rfkill_blocked_sw_store(struct device *dev,
+static ssize_t rfkill_soft_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -701,9 +697,7 @@ static ssize_t rfkill_state_show(struct device *dev,
unsigned long flags;
u32 state;
- spin_lock_irqsave(&rfkill->lock, flags);
state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
return sprintf(buf, "%d\n", user_state_from_blocked(state));
}
@@ -755,9 +749,8 @@ static struct device_attribute rfkill_dev_attrs[] = {
__ATTR(persistent, S_IRUGO, rfkill_persistent_show, NULL),
__ATTR(state, S_IRUGO|S_IWUSR, rfkill_state_show, rfkill_state_store),
__ATTR(claim, S_IRUGO|S_IWUSR, rfkill_claim_show, rfkill_claim_store),
- __ATTR(sw, S_IRUGO|S_IWUSR, rfkill_blocked_sw_show,
- rfkill_blocked_sw_store),
- __ATTR(hw, S_IRUGO, rfkill_blocked_hw_show, NULL),
+ __ATTR(soft, S_IRUGO|S_IWUSR, rfkill_soft_show, rfkill_soft_store),
+ __ATTR(hard, S_IRUGO, rfkill_hard_show, NULL),
__ATTR_NULL
};
--
1.6.6.1
--
Dmitry Torokhov
> This patch renames the (never officially released) sysfs-knobs
> "blocked_hw" and "blocked_sw" to "hard" and "soft", as the hardware vs
> software conotation is misleading.
>
> It also gets rid of not needed locks around u32-read-access.
You could also drop temporaries as well.
--
Dmitry
Dmitry Torokhov
Well, right now it is mutex so it will not protect if something happens
in interrupt context. Takeing the global rfkill mutex seems pretty heavy
but there does not seem to be a per-device mutex. There also some
muching with spinlock inside rfkill_set_state but it is dropped when we
actually carry out the operation. I am afraid the locking in rfkill
needs some reviewing...
--
Dmitry
flo...@mickler.org
"blocked_hw" and "blocked_sw" to "hard" and "soft", as the hardware vs
software conotation is misleading.
It also gets rid of not needed locks around u32-read-access.
Signed-off-by: Florian Mickler <flo...@mickler.org>
---
This version also gets rid of unnecessary local vars...
Documentation/ABI/stable/sysfs-class-rfkill | 10 ++++----
net/rfkill/core.c | 35 ++++++--------------------
2 files changed, 13 insertions(+), 32 deletions(-)
index 5f33151..7ae58b5 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -628,37 +628,25 @@ static ssize_t rfkill_persistent_show(struct device *dev,
return sprintf(buf, "%d\n", rfkill->persistent);
}
-static ssize_t rfkill_blocked_hw_show(struct device *dev,
+static ssize_t rfkill_hard_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct rfkill *rfkill = to_rfkill(dev);
- unsigned long flags;
- u32 state;
- spin_lock_irqsave(&rfkill->lock, flags);
- state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
-
- return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_HW) ? 1 : 0 );
+ return sprintf(buf, "%d\n", (rfkill->state & RFKILL_BLOCK_HW) ? 1 : 0 );
}
-static ssize_t rfkill_blocked_sw_show(struct device *dev,
+static ssize_t rfkill_soft_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct rfkill *rfkill = to_rfkill(dev);
- unsigned long flags;
- u32 state;
-
- spin_lock_irqsave(&rfkill->lock, flags);
- state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
- return sprintf(buf, "%d\n", (state & RFKILL_BLOCK_SW) ? 1 : 0 );
+ return sprintf(buf, "%d\n", (rfkill->state & RFKILL_BLOCK_SW) ? 1 : 0 );
}
-static ssize_t rfkill_blocked_sw_store(struct device *dev,
+static ssize_t rfkill_soft_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -698,14 +686,8 @@ static ssize_t rfkill_state_show(struct device *dev,
char *buf)
{
struct rfkill *rfkill = to_rfkill(dev);
- unsigned long flags;
- u32 state;
-
- spin_lock_irqsave(&rfkill->lock, flags);
- state = rfkill->state;
- spin_unlock_irqrestore(&rfkill->lock, flags);
- return sprintf(buf, "%d\n", user_state_from_blocked(state));
+ return sprintf(buf, "%d\n", user_state_from_blocked(rfkill->state));
}
static ssize_t rfkill_state_store(struct device *dev,
@@ -755,9 +737,8 @@ static struct device_attribute rfkill_dev_attrs[] = {
__ATTR(persistent, S_IRUGO, rfkill_persistent_show, NULL),
__ATTR(state, S_IRUGO|S_IWUSR, rfkill_state_show, rfkill_state_store),
__ATTR(claim, S_IRUGO|S_IWUSR, rfkill_claim_show, rfkill_claim_store),
- __ATTR(sw, S_IRUGO|S_IWUSR, rfkill_blocked_sw_show,
- rfkill_blocked_sw_store),
- __ATTR(hw, S_IRUGO, rfkill_blocked_hw_show, NULL),
+ __ATTR(soft, S_IRUGO|S_IWUSR, rfkill_soft_show, rfkill_soft_store),
+ __ATTR(hard, S_IRUGO, rfkill_hard_show, NULL),
__ATTR_NULL
};
--
1.6.6.1
--
Florian Mickler
Dmitry Torokhov <dmitry....@gmail.com> wrote:
> Well, right now it is mutex so it will not protect if something happens
> in interrupt context. Takeing the global rfkill mutex seems pretty heavy
> but there does not seem to be a per-device mutex. There also some
> muching with spinlock inside rfkill_set_state but it is dropped when we
> actually carry out the operation. I am afraid the locking in rfkill
> needs some reviewing...
>
I _think_ this is ok...
as far as i can see everything which writes to rfkill->state in
net/rfkill/core.c takes the rfkill->lock spinlock... the mutex
probably protects other global rfkill data... as long as drivers only
use the rfkill.h interface and not acess the rfkill->state
themselves this should be ok...
Flo
Florian Mickler
flo...@mickler.org wrote:
> This patch renames the (never officially released) sysfs-knobs
> "blocked_hw" and "blocked_sw" to "hard" and "soft", as the hardware vs
> software conotation is misleading.
>
> It also gets rid of not needed locks around u32-read-access.
>
> Signed-off-by: Florian Mickler <flo...@mickler.org>
Is this ok?
cheers,
Flo
John W. Linville
> On Sat, 13 Mar 2010 13:31:05 +0100
> flo...@mickler.org wrote:
>
> > This patch renames the (never officially released) sysfs-knobs
> > "blocked_hw" and "blocked_sw" to "hard" and "soft", as the hardware vs
> > software conotation is misleading.
> >
> > It also gets rid of not needed locks around u32-read-access.
> >
> > Signed-off-by: Florian Mickler <flo...@mickler.org>
>
> Is this ok?
Yes, fine. Sorry, I got distracted by some other things.
Thanks!
John
--
John W. Linville Someday the world will need a hero, and you
linv...@tuxdriver.com might be all we have. Be ready.