Hi all!
The first patch moves the rfkill sysfs ABI Documentation to Documentation/ABI
and deprecates the state and claim file.
The second patch creates a replacement for the state file. Instead of exporting
some made-up state we just export the state of the 2 kill lines.
The first patch should probably go into 2.6.34, as to warn users
(if there are any) early about this removal.
If there is no intent to remove the broken files, the feature-removal-schedule hunk
should probably be skipped.
Cheers,
Flo
p.s.: first discussion of this: http://lkml.org/lkml/2010/2/22/127
Florian Mickler (2):
Document the rfkill sysfs ABI
enhance sysfs rfkill interface
Documentation/ABI/obsolete/sysfs-class-rfkill | 29 +++++++++++
Documentation/ABI/stable/sysfs-class-rfkill | 67 +++++++++++++++++++++++++
Documentation/feature-removal-schedule.txt | 18 +++++++
Documentation/rfkill.txt | 44 +++++-----------
net/rfkill/core.c | 57 +++++++++++++++++++++
5 files changed, 184 insertions(+), 31 deletions(-)
create mode 100644 Documentation/ABI/obsolete/sysfs-class-rfkill
create mode 100644 Documentation/ABI/stable/sysfs-class-rfkill
On Fri, Mar 12, 2010 at 09:57:43PM +0100, Florian Mickler wrote:
> On Fri, 12 Mar 2010 10:22:09 -0800
> Dmitry Torokhov <[email protected]> wrote:
>
> > On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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
This moves sysfs ABI info from Documentation/rfkill.txt to the
ABI subfolder and reformats it.
This also schedules the deprecated sysfs parts to be removed in
2012 (claim file) and 2014 (state file).
Signed-off-by: Florian Mickler <[email protected]>
---
Documentation/ABI/obsolete/sysfs-class-rfkill | 29 ++++++++++++++++
Documentation/ABI/stable/sysfs-class-rfkill | 42 +++++++++++++++++++++++
Documentation/feature-removal-schedule.txt | 18 ++++++++++
Documentation/rfkill.txt | 44 +++++++-----------------
4 files changed, 102 insertions(+), 31 deletions(-)
create mode 100644 Documentation/ABI/obsolete/sysfs-class-rfkill
create mode 100644 Documentation/ABI/stable/sysfs-class-rfkill
diff --git a/Documentation/ABI/obsolete/sysfs-class-rfkill b/Documentation/ABI/obsolete/sysfs-class-rfkill
new file mode 100644
index 0000000..4201d5b
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-class-rfkill
@@ -0,0 +1,29 @@
+rfkill - radio frequency (RF) connector kill switch support
+
+For details to this subsystem look at Documentation/rfkill.txt.
+
+What: /sys/class/rfkill/rfkill[0-9]+/state
+Date: 09-Jul-2007
+KernelVersion v2.6.22
+Contact: [email protected]
+Description: Current state of the transmitter.
+ This file is deprecated and sheduled to be removed in 2014,
+ because its not possible to express the 'soft and hard block'
+ state of the rfkill driver.
+Values: A numeric value.
+ 0: RFKILL_STATE_SOFT_BLOCKED
+ transmitter is turned off by software
+ 1: RFKILL_STATE_UNBLOCKED
+ transmitter is (potentially) active
+ 2: RFKILL_STATE_HARD_BLOCKED
+ transmitter is forced off by something outside of
+ the driver's control.
+
+What: /sys/class/rfkill/rfkill[0-9]+/claim
+Date: 09-Jul-2007
+KernelVersion v2.6.22
+Contact: [email protected]
+Description: This file is deprecated because there no longer is a way to
+ claim just control over a single rfkill instance.
+ This file is scheduled to be removed in 2012.
+Values: 0: Kernel handles events
diff --git a/Documentation/ABI/stable/sysfs-class-rfkill b/Documentation/ABI/stable/sysfs-class-rfkill
new file mode 100644
index 0000000..3884344
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-class-rfkill
@@ -0,0 +1,42 @@
+rfkill - radio frequency (RF) connector kill switch support
+
+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/obsolete/sysfs-class-rfkill.
+
+What: /sys/class/rfkill
+Date: 09-Jul-2007
+KernelVersion: v2.6.22
+Contact: [email protected],
+Description: The rfkill class subsystem folder.
+ Each registered rfkill driver is represented by an rfkillX
+ subfolder (X being an integer > 0).
+
+
+What: /sys/class/rfkill/rfkill[0-9]+/name
+Date: 09-Jul-2007
+KernelVersion v2.6.22
+Contact: [email protected]
+Description: Name assigned by driver to this key (interface or driver name).
+Values: arbitrary string.
+
+
+What: /sys/class/rfkill/rfkill[0-9]+/type
+Date: 09-Jul-2007
+KernelVersion v2.6.22
+Contact: [email protected]
+Description: Driver type string ("wlan", "bluetooth", etc).
+Values: See include/linux/rfkill.h.
+
+
+What: /sys/class/rfkill/rfkill[0-9]+/persistent
+Date: 09-Jul-2007
+KernelVersion v2.6.22
+Contact: [email protected]
+Description: Whether the soft blocked state is initialised from non-volatile
+ storage at startup.
+Values: A numeric value.
+ 0: false
+ 1: true
diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index a5cc0db..9b450d7 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -520,6 +520,24 @@ Who: Hans de Goede <[email protected]>
----------------------------
+What: sysfs-class-rfkill state file
+When: Feb 2014
+Files: net/rfkill/core.c
+Why: Documented as obsolete since Feb 2010. This file is limited to 3
+ states while the rfkill drivers can have 4 states.
+Who: anybody or Florian Mickler <[email protected]>
+
+----------------------------
+
+What: sysfs-class-rfkill claim file
+When: Feb 2012
+Files: net/rfkill/core.c
+Why: It is not possible to claim an rfkill driver since 2007. This is
+ Documented as obsolete since Feb 2010.
+Who: anybody or Florian Mickler <[email protected]>
+
+----------------------------
+
What: corgikbd, spitzkbd, tosakbd driver
When: 2.6.35
Files: drivers/input/keyboard/{corgi,spitz,tosa}kbd.c
diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index b486050..83668e5 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -99,37 +99,15 @@ system. Also, it is possible to switch all rfkill drivers (or all drivers of
a specified type) into a state which also updates the default state for
hotplugged devices.
-After an application opens /dev/rfkill, it can read the current state of
-all devices, and afterwards can poll the descriptor for hotplug or state
-change events.
-
-Applications must ignore operations (the "op" field) they do not handle,
-this allows the API to be extended in the future.
-
-Additionally, each rfkill device is registered in sysfs and there has the
-following attributes:
-
- name: Name assigned by driver to this key (interface or driver name).
- type: Driver type string ("wlan", "bluetooth", etc).
- persistent: Whether the soft blocked state is initialised from
- non-volatile storage at startup.
- state: Current state of the transmitter
- 0: RFKILL_STATE_SOFT_BLOCKED
- transmitter is turned off by software
- 1: RFKILL_STATE_UNBLOCKED
- transmitter is (potentially) active
- 2: RFKILL_STATE_HARD_BLOCKED
- transmitter is forced off by something outside of
- the driver's control.
- This file is deprecated because it can only properly show
- three of the four possible states, soft-and-hard-blocked is
- missing.
- claim: 0: Kernel handles events
- This file is deprecated because there no longer is a way to
- claim just control over a single rfkill instance.
-
-rfkill devices also issue uevents (with an action of "change"), with the
-following environment variables set:
+After an application opens /dev/rfkill, it can read the current state of all
+devices. Changes can be either obtained by either polling the descriptor for
+hotplug or state change events or by listening for uevents emitted by the
+rfkill core framework.
+
+Additionally, each rfkill device is registered in sysfs and emits uevents.
+
+rfkill devices issue uevents (with an action of "change"), with the following
+environment variables set:
RFKILL_NAME
RFKILL_STATE
@@ -137,3 +115,7 @@ RFKILL_TYPE
The contents of these variables corresponds to the "name", "state" and
"type" sysfs files explained above.
+
+
+For further details consult Documentation/ABI/stable/dev-rfkill and
+Documentation/ABI/stable/sysfs-class-rfkill.
--
1.6.6.1
On Thu, Mar 18, 2010 at 11:59:17AM +0100, Florian Mickler wrote:
> On Sat, 13 Mar 2010 13:31:05 +0100
> [email protected] 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 <[email protected]>
>
> 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
[email protected] might be all we have. Be ready.
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 <[email protected]>
---
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: [email protected]
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: [email protected]
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
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 <[email protected]>
---
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(-)
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: [email protected]
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: [email protected]
Description: Current softblock state. This file is read and write.
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
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
On Sat, Mar 13, 2010 at 10:41:28AM +0100, Florian Mickler wrote:
> On Fri, 12 Mar 2010 14:48:28 -0800
> Dmitry Torokhov <[email protected]> wrote:
>
> > On Fri, Mar 12, 2010 at 11:39:25PM +0100, Florian Mickler wrote:
> > > On Fri, 12 Mar 2010 13:20:26 -0800
> > > Dmitry Torokhov <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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?
> > >
> >
> > 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?
>
> On Fri, 12 Mar 2010 18:48:19 -0500
> [email protected] 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.
>
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
On Sat, 13 Mar 2010 13:31:05 +0100
[email protected] 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 <[email protected]>
Is this ok?
cheers,
Flo
On Fri, 12 Mar 2010 13:20:26 -0800
Dmitry Torokhov <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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?
On Fri, 12 Mar 2010 10:22:09 -0800
Dmitry Torokhov <[email protected]> wrote:
> On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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
On Sat, Mar 13, 2010 at 10:44:07AM +0100, [email protected] 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.
You could also drop temporaries as well.
--
Dmitry
This commit introduces two new sysfs knobs.
/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 <[email protected]>
---
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: [email protected]
+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: [email protected]
+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
On Fri, Mar 12, 2010 at 07:18:19PM +0100, Florian Mickler wrote:
> sorry! I just realised that you already have the blocked_sw / blocked_hw one's
> queued.
>
> This just changes the name of blocked_sw and blocked_hw to "soft" and
> "hard".
>
> I don't know what you wanna do. I'm fine with blocked_hw and
> blocked_sw as it's referenced as such (hw/sw) in the code, but Marcel
> wanted to get away from the "hard blocked by hardware" notation, as it
> suggests that the transmitter could only be blocked by hardware...
Could you just submit a third patch that makes the requested name changes?
Thanks!
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Sat, 13 Mar 2010 01:55:54 -0800
Dmitry Torokhov <[email protected]> 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
On Fri, 12 Mar 2010 19:03:06 +0100
[email protected] wrote:
> Hi all!
>
> The first patch moves the rfkill sysfs ABI Documentation to Documentation/ABI
> and deprecates the state and claim file.
>
> The second patch creates a replacement for the state file. Instead of exporting
> some made-up state we just export the state of the 2 kill lines.
>
> The first patch should probably go into 2.6.34, as to warn users
> (if there are any) early about this removal.
> If there is no intent to remove the broken files, the feature-removal-schedule hunk
> should probably be skipped.
>
> Cheers,
> Flo
>
> p.s.: first discussion of this: http://lkml.org/lkml/2010/2/22/127
>
Hi,
sorry! I just realised that you already have the blocked_sw / blocked_hw one's
queued.
This just changes the name of blocked_sw and blocked_hw to "soft" and
"hard".
I don't know what you wanna do. I'm fine with blocked_hw and
blocked_sw as it's referenced as such (hw/sw) in the code, but Marcel
wanted to get away from the "hard blocked by hardware" notation, as it
suggests that the transmitter could only be blocked by hardware...
cheers,
Flo
On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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?
> +
> + 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
On Fri, 12 Mar 2010 23:39:25 +0100, Florian Mickler said:
> On Fri, 12 Mar 2010 13:20:26 -0800
> Dmitry Torokhov <[email protected]> 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.
On Fri, 12 Mar 2010 14:48:28 -0800
Dmitry Torokhov <[email protected]> wrote:
> On Fri, Mar 12, 2010 at 11:39:25PM +0100, Florian Mickler wrote:
> > On Fri, 12 Mar 2010 13:20:26 -0800
> > Dmitry Torokhov <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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?
> >
>
> 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?
On Fri, 12 Mar 2010 18:48:19 -0500
[email protected] 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
On Fri, Mar 12, 2010 at 11:39:25PM +0100, Florian Mickler wrote:
> On Fri, 12 Mar 2010 13:20:26 -0800
> Dmitry Torokhov <[email protected]> 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 <[email protected]> wrote:
> > >
> > > > On Fri, Mar 12, 2010 at 07:03:08PM +0100, [email protected] 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?
>
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