* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Peter Zijlstra @ 2011-02-24 20:27 UTC (permalink / raw)
To: Alan Cox
Cc: Vincent Guittot, linux-kernel, linux-hotplug, Frederic Weisbecker,
Steven Rostedt, amit.kucheria, Rusty Russell, Ingo Molnar,
Thomas Gleixner
In-Reply-To: <20110224201124.138311ba@lxorguk.ukuu.org.uk>
On Thu, 2011-02-24 at 20:11 +0000, Alan Cox wrote:
> > Anybody who is interested in the latency of cpu hotplug is deluding
> > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > rest of your justification just disappeared as well.
>
> Actually CPU hotplug is a power management feature on some devices where
> you need to shutdown one of the cores to enter low power modes.
Aren't we confusing things here? Surely simply idling a core is good
enough? Why would we want to go through the whole CPU hotplug dance
simply to enter a low power state?
> Remember we use it as part of the suspend paths and various processors
> nowdays drop into a suspend to RAM type state on CPU idling.
Which would illustrate the above point. CPU hotplug is a terribly
expensive op, and doing so from idle is really utterly ridiculous (nor
can we, idle is not supposed to schedule and cpu-hotplug needs to
schedule)
Why can't we do these things from the normal idle path, presumably these
state transitions are 'fast', so we can implement them as normal idle
modes.
The scheduler has (due to power7 support) the ability to favour lower
cpu nrs when placing tasks, so idle !bsp (assuming cpu0 is the bsp) can
drop into their special state, and then when the bsp goes idle it can do
whatever it needs to do.
All that needs is to make sure smp_send_reschedule() can wake !bsp cores
from their special sleep state, but that's all arch code anyway.
I really see no reason to conflate cpu-hotplug and idle/power-states.
^ permalink raw reply
* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Nicolas Pitre @ 2011-02-24 20:24 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Alan Cox, Peter Zijlstra, Vincent Guittot, lkml, linux-hotplug,
Frederic Weisbecker, Steven Rostedt, amit.kucheria, Rusty Russell,
Ingo Molnar
In-Reply-To: <alpine.LFD.2.00.1102242115011.2701@localhost6.localdomain6>
On Thu, 24 Feb 2011, Thomas Gleixner wrote:
> On Thu, 24 Feb 2011, Alan Cox wrote:
>
> > > Anybody who is interested in the latency of cpu hotplug is deluding
> > > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > > rest of your justification just disappeared as well.
> >
> > Actually CPU hotplug is a power management feature on some devices where
> > you need to shutdown one of the cores to enter low power modes.
>
> Right.
>
> > Remember we use it as part of the suspend paths and various processors
> > nowdays drop into a suspend to RAM type state on CPU idling.
>
> The suspend path is using it, but the cpu idle one is hardly using the
> hotplug muck.
Most SMP ARM processors are going to use it soon. Powering down idle
cores provides substantial power saving.
Nicolas
^ permalink raw reply
* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Thomas Gleixner @ 2011-02-24 20:16 UTC (permalink / raw)
To: Alan Cox
Cc: Peter Zijlstra, Vincent Guittot, linux-kernel, linux-hotplug,
Frederic Weisbecker, Steven Rostedt, amit.kucheria, Rusty Russell,
Ingo Molnar
In-Reply-To: <20110224201124.138311ba@lxorguk.ukuu.org.uk>
On Thu, 24 Feb 2011, Alan Cox wrote:
> > Anybody who is interested in the latency of cpu hotplug is deluding
> > himself, also cpu hotplug is _NOT_ a power management feature, so the
> > rest of your justification just disappeared as well.
>
> Actually CPU hotplug is a power management feature on some devices where
> you need to shutdown one of the cores to enter low power modes.
Right.
> Remember we use it as part of the suspend paths and various processors
> nowdays drop into a suspend to RAM type state on CPU idling.
The suspend path is using it, but the cpu idle one is hardly using the
hotplug muck.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Alan Cox @ 2011-02-24 20:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vincent Guittot, linux-kernel, linux-hotplug, Frederic Weisbecker,
Steven Rostedt, amit.kucheria, Rusty Russell, Ingo Molnar,
Thomas Gleixner
In-Reply-To: <1298573197.2428.457.camel@twins>
> Anybody who is interested in the latency of cpu hotplug is deluding
> himself, also cpu hotplug is _NOT_ a power management feature, so the
> rest of your justification just disappeared as well.
Actually CPU hotplug is a power management feature on some devices where
you need to shutdown one of the cores to enter low power modes.
Remember we use it as part of the suspend paths and various processors
nowdays drop into a suspend to RAM type state on CPU idling.
Alan
^ permalink raw reply
* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Peter Zijlstra @ 2011-02-24 18:46 UTC (permalink / raw)
To: Vincent Guittot
Cc: linux-kernel, linux-hotplug, Frederic Weisbecker, Steven Rostedt,
amit.kucheria, Rusty Russell, Ingo Molnar, Thomas Gleixner
In-Reply-To: <AANLkTinkctQ6Nek98JV7vaos34gD4Qn8nsuBzt=_Jj6T@mail.gmail.com>
On Thu, 2011-02-24 at 18:33 +0100, Vincent Guittot wrote:
> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.
> this patch adds new events for cpu hotplug tracing
> * plug/unplug sequence
> * core and architecture latency measurements
Aside of the points tglx made, I do have to ask _WHY_!?
Anybody who is interested in the latency of cpu hotplug is deluding
himself, also cpu hotplug is _NOT_ a power management feature, so the
rest of your justification just disappeared as well.
^ permalink raw reply
* Re: [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Thomas Gleixner @ 2011-02-24 18:40 UTC (permalink / raw)
To: Vincent Guittot
Cc: linux-kernel, linux-hotplug, Frederic Weisbecker, Steven Rostedt,
amit.kucheria, Rusty Russell, Ingo Molnar
In-Reply-To: <AANLkTinkctQ6Nek98JV7vaos34gD4Qn8nsuBzt=_Jj6T@mail.gmail.com>
On Thu, 24 Feb 2011, Vincent Guittot wrote:
Please send full patch series not a single V5 2/2 which lacks any
references to 0/2 1/2.
> Please find below a new proposal for adding trace events for cpu hotplug:
Either it's a patch or a proposal. Darn, why think people that
proposal is such a important word? It's just useless. You don't have
to sell anything to your manager. You provide a patch which is judged
on it's technical merits and correctness. Nothing else.
> -the lock/unlock of cpu_add_remove_lock mutex is now outside the trace
>
> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.
This belongs into a cover mail [0/2] not into the patch itself
> Subject: [PATCH 2/2] add hotplug tracepoint
While your mail subject is correct, this is not.
If you would have sent a [0/2] cover mail with all the above blurb in
it then this extra subject line would be not needed at all.
> this patch adds new events for cpu hotplug tracing
Sentences start with an upper case letter.
Also we already know that this is a patch. Where is the value of this
changelog? It does not tell more than the subject line.
> * plug/unplug sequence
How surprising.
> * core and architecture latency measurements
No it does not. It does not add latency measurements. It merily adds
tracepoints which allow you to compute the time spent in the various
steps of the hotplug state machine and the overall time.
> #ifdef CONFIG_SMP
> /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> static DEFINE_MUTEX(cpu_add_remove_lock);
> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
> static int __ref take_cpu_down(void *_param)
> {
> struct take_cpu_down_param *param = _param;
> + unsigned int cpu = (unsigned int)(param->hcpu);
> int err;
>
> /* Ensure this CPU doesn't handle any more interrupts. */
> + trace_cpu_hotplug_disable_start(cpu);
> err = __cpu_disable();
> + trace_cpu_hotplug_disable_end(cpu);
How useful. What about recording the return code of __cpu_disable()?
> if (err < 0)
> return err;
> + trace_cpu_hotplug_down_start(cpu);
> +
What's the point of this tracepoint _BEFORE_ the cpu_hotplug_disabled
check without recording cpu_hotplug_disabled ?
> if (cpu_hotplug_disabled) {
> err = -EBUSY;
> goto out;
> @@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
> err = _cpu_down(cpu, 0);
>
> out:
> + trace_cpu_hotplug_down_end(cpu);
And this one is misplaced as well. It wants to be only called when we
actually called _cpu_down() and it wants to record the return code as
well.
> +
> cpu_maps_update_done();
> return err;
> }
> @@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
> tasks_frozen)
> }
>
> /* Arch-specific enabling code. */
> + trace_cpu_hotplug_arch_up_start(cpu);
> ret = __cpu_up(cpu);
> + trace_cpu_hotplug_arch_up_end(cpu);
See above.
> if (ret != 0)
> goto out_notify;
> BUG_ON(!cpu_online(cpu));
> @@ -369,6 +383,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>
> cpu_maps_update_begin();
>
> + trace_cpu_hotplug_up_start(cpu);
> +
Ditto
> if (cpu_hotplug_disabled) {
> err = -EBUSY;
> goto out;
> @@ -377,6 +393,8 @@ int __cpuinit cpu_up(unsigned int cpu)
> err = _cpu_up(cpu, 0);
>
> out:
> + trace_cpu_hotplug_up_end(cpu);
> +
Sigh.
> cpu_maps_update_done();
> return err;
Thanks,
tglx
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set of
From: Vilius Benetis @ 2011-02-24 17:36 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
>> KERNEL!="sd[a-z][0-9]", GOTO="end_usb_key_filter"
>
> Nope, way too late, you want to catch things _before_ the usb-storage
> driver binds to it, right?
>
> So match on a usb interfaces, and look at the class values to match the
> usb storage ones. Â If they are a match, then go up a level and disable
> the device by writing a 0 to the "authorized" file.
>
>> SUBSYSTEM="usb", ATTRS{serial}="xx1", GOTO="end_usb_key_filter"
>> SUBSYSTEM="usb", ATTRS{serial}="xx2", GOTO="end_usb_key_filter"
>> SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
>>
>> or just:
>>
>> SUBSYSTEM="usb", ATTRS{serial}="xx1|xx2|xx3", GOTO="end_usb_key_filter"
>> SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
>> LABEL="end_usb_key_filter"
>
> That might work, but watch out that you don't deactivate your USB
> keyboards :)
finally I found this - explaining what this USB authorisation is:
http://www.mjmwired.net/kernel/Documentation/usb/authorization.txt
tomorrow I will play with the code.
Vilius
^ permalink raw reply
* [PATCH V5 2/2] tracing, perf : add cpu hotplug trace events
From: Vincent Guittot @ 2011-02-24 17:33 UTC (permalink / raw)
To: linux-kernel, linux-hotplug
Cc: Frederic Weisbecker, Steven Rostedt, amit.kucheria, Rusty Russell,
Ingo Molnar
Please find below a new proposal for adding trace events for cpu hotplug:
-the lock/unlock of cpu_add_remove_lock mutex is now outside the trace
The goal is to measure the latency of each part (kernel, architecture)
and also to trace the cpu hotplug activity with other power events. I
have tested these traces events on an arm platform.
Subject: [PATCH 2/2] add hotplug tracepoint
this patch adds new events for cpu hotplug tracing
* plug/unplug sequence
* core and architecture latency measurements
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
kernel/cpu.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 156cc55..8abb84b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -16,6 +16,9 @@
#include <linux/mutex.h>
#include <linux/gfp.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/cpu_hotplug.h>
+
#ifdef CONFIG_SMP
/* Serializes the updates to cpu_online_mask, cpu_present_mask */
static DEFINE_MUTEX(cpu_add_remove_lock);
@@ -197,10 +200,13 @@ struct take_cpu_down_param {
static int __ref take_cpu_down(void *_param)
{
struct take_cpu_down_param *param = _param;
+ unsigned int cpu = (unsigned int)(param->hcpu);
int err;
/* Ensure this CPU doesn't handle any more interrupts. */
+ trace_cpu_hotplug_disable_start(cpu);
err = __cpu_disable();
+ trace_cpu_hotplug_disable_end(cpu);
if (err < 0)
return err;
@@ -256,7 +262,9 @@ static int __ref _cpu_down(unsigned int cpu, int
tasks_frozen)
cpu_relax();
/* This actually kills the CPU. */
+ trace_cpu_hotplug_die_start(cpu);
__cpu_die(cpu);
+ trace_cpu_hotplug_die_end(cpu);
/* CPU is completely dead: tell everyone. Too late to complain. */
cpu_notify_nofail(CPU_DEAD | mod, hcpu);
@@ -276,6 +284,8 @@ int __ref cpu_down(unsigned int cpu)
cpu_maps_update_begin();
+ trace_cpu_hotplug_down_start(cpu);
+
if (cpu_hotplug_disabled) {
err = -EBUSY;
goto out;
@@ -284,6 +294,8 @@ int __ref cpu_down(unsigned int cpu)
err = _cpu_down(cpu, 0);
out:
+ trace_cpu_hotplug_down_end(cpu);
+
cpu_maps_update_done();
return err;
}
@@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
tasks_frozen)
}
/* Arch-specific enabling code. */
+ trace_cpu_hotplug_arch_up_start(cpu);
ret = __cpu_up(cpu);
+ trace_cpu_hotplug_arch_up_end(cpu);
if (ret != 0)
goto out_notify;
BUG_ON(!cpu_online(cpu));
@@ -369,6 +383,8 @@ int __cpuinit cpu_up(unsigned int cpu)
cpu_maps_update_begin();
+ trace_cpu_hotplug_up_start(cpu);
+
if (cpu_hotplug_disabled) {
err = -EBUSY;
goto out;
@@ -377,6 +393,8 @@ int __cpuinit cpu_up(unsigned int cpu)
err = _cpu_up(cpu, 0);
out:
+ trace_cpu_hotplug_up_end(cpu);
+
cpu_maps_update_done();
return err;
}
--
1.7.1
^ permalink raw reply related
* Re: How to use Udev to restrict USB access only to particular set of
From: Greg KH @ 2011-02-24 17:01 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Thu, Feb 24, 2011 at 06:36:19PM +0200, Vilius Benetis wrote:
> On Thu, Feb 24, 2011 at 5:46 PM, Greg KH <greg@kroah.com> wrote:
> >>[vilius]
> >> we tried to disable USB storage sticks with the following command:
> >>
> >> SUBSYSTEMS="usb" DRIVERS="usb-storage" OPTIONS:="ignore_device"
> >>
> >> but we failed to make it work.
> >
> > Ignoring the device still makes it "active" in the system, especially as
> > you just tested that the usb-storage device was bound to your device
> > (which wouldn't be true that early in the process, which is one reason
> > why this failed).
> >
> > you need to write a 0 to the "authorized" file in sysfs which will
> > disable the whole USB device entirely if it meets your "list of devices
> > to reject". You also need to test not for driver binding, which again
> > will not have happened, and you don't want to have happen, but that it
> > is a usb storage device type (by virtue of the correct class config
> > options as shown by sysfs) and that it doesn't pass your list of valid
> > serial numbers.
> >
> > Note, all of that might be easier to do in a script than in a udev rule
> > alone, but it should be possible.
>
> I can follow the logic, but I think I am not able to convert the
> guidance to the actions.
>
> do you mean (am not sure what is this "authorised" file in sysfs):
>
> KERNEL!="sd[a-z][0-9]", GOTO="end_usb_key_filter"
Nope, way too late, you want to catch things _before_ the usb-storage
driver binds to it, right?
So match on a usb interfaces, and look at the class values to match the
usb storage ones. If they are a match, then go up a level and disable
the device by writing a 0 to the "authorized" file.
> SUBSYSTEM="usb", ATTRS{serial}="xx1", GOTO="end_usb_key_filter"
> SUBSYSTEM="usb", ATTRS{serial}="xx2", GOTO="end_usb_key_filter"
> SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
>
> or just:
>
> SUBSYSTEM="usb", ATTRS{serial}="xx1|xx2|xx3", GOTO="end_usb_key_filter"
> SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
> LABEL="end_usb_key_filter"
That might work, but watch out that you don't deactivate your USB
keyboards :)
thanks,
greg k-h
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set of
From: Vilius Benetis @ 2011-02-24 16:36 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Thu, Feb 24, 2011 at 5:46 PM, Greg KH <greg@kroah.com> wrote:
>>[vilius]
>> we tried to disable USB storage sticks with the following command:
>>
>> SUBSYSTEMS="usb" DRIVERS="usb-storage" OPTIONS:="ignore_device"
>>
>> but we failed to make it work.
>
> Ignoring the device still makes it "active" in the system, especially as
> you just tested that the usb-storage device was bound to your device
> (which wouldn't be true that early in the process, which is one reason
> why this failed).
>
> you need to write a 0 to the "authorized" file in sysfs which will
> disable the whole USB device entirely if it meets your "list of devices
> to reject". You also need to test not for driver binding, which again
> will not have happened, and you don't want to have happen, but that it
> is a usb storage device type (by virtue of the correct class config
> options as shown by sysfs) and that it doesn't pass your list of valid
> serial numbers.
>
> Note, all of that might be easier to do in a script than in a udev rule
> alone, but it should be possible.
I can follow the logic, but I think I am not able to convert the
guidance to the actions.
do you mean (am not sure what is this "authorised" file in sysfs):
KERNEL!="sd[a-z][0-9]", GOTO="end_usb_key_filter"
SUBSYSTEM="usb", ATTRS{serial}="xx1", GOTO="end_usb_key_filter"
SUBSYSTEM="usb", ATTRS{serial}="xx2", GOTO="end_usb_key_filter"
SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
or just:
SUBSYSTEM="usb", ATTRS{serial}="xx1|xx2|xx3", GOTO="end_usb_key_filter"
SUBSYSTEM="usb", RUN="echo 0 >/sys/xxx"
LABEL="end_usb_key_filter"
--
/Vilius
^ permalink raw reply
* Re: [PATCH V4 2/2] tracing, perf : add cpu hotplug trace events
From: Vincent Guittot @ 2011-02-24 16:15 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: linux-kernel, linux-hotplug, rostedt, amit.kucheria,
Rusty Russell, Ingo Molnar
In-Reply-To: <20110224150050.GA1840@nowhere>
On 24 February 2011 16:00, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Mon, Jan 31, 2011 at 11:23:59AM +0100, Vincent Guittot wrote:
>> Please find below a new proposal for adding trace events for cpu hotplug.
>> The goal is to measure the latency of each part (kernel, architecture)
>> and also to trace the cpu hotplug activity with other power events. I
>> have tested these traces events on an arm platform.
>>
>> Subject: [PATCH 2/2] add hotplug tracepoint
>>
>> this patch adds new events for cpu hotplug tracing
>> * plug/unplug sequence
>> * core and architecture latency measurements
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>> kernel/cpu.c | 18 ++++++++++++++++++
>> 1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 156cc55..692e819 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -16,6 +16,9 @@
>> #include <linux/mutex.h>
>> #include <linux/gfp.h>
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/cpu_hotplug.h>
>> +
>> #ifdef CONFIG_SMP
>> /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>> static DEFINE_MUTEX(cpu_add_remove_lock);
>> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
>> static int __ref take_cpu_down(void *_param)
>> {
>> struct take_cpu_down_param *param = _param;
>> + unsigned int cpu = (unsigned int)(param->hcpu);
>> int err;
>>
>> /* Ensure this CPU doesn't handle any more interrupts. */
>> + trace_cpu_hotplug_disable_start(cpu);
>> err = __cpu_disable();
>> + trace_cpu_hotplug_disable_end(cpu);
>> if (err < 0)
>> return err;
>>
>> @@ -256,7 +262,9 @@ static int __ref _cpu_down(unsigned int cpu, int
>> tasks_frozen)
>> cpu_relax();
>>
>> /* This actually kills the CPU. */
>> + trace_cpu_hotplug_die_start(cpu);
>> __cpu_die(cpu);
>> + trace_cpu_hotplug_die_end(cpu);
>>
>> /* CPU is completely dead: tell everyone. Too late to complain. */
>> cpu_notify_nofail(CPU_DEAD | mod, hcpu);
>> @@ -274,6 +282,8 @@ int __ref cpu_down(unsigned int cpu)
>> {
>> int err;
>>
>> + trace_cpu_hotplug_down_start(cpu);
>> +
>> cpu_maps_update_begin();
>>
>> if (cpu_hotplug_disabled) {
>> @@ -285,6 +295,8 @@ int __ref cpu_down(unsigned int cpu)
>>
>> out:
>> cpu_maps_update_done();
>> +
>> + trace_cpu_hotplug_down_end(cpu);
>> return err;
>> }
>> EXPORT_SYMBOL(cpu_down);
>> @@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
>> tasks_frozen)
>> }
>>
>> /* Arch-specific enabling code. */
>> + trace_cpu_hotplug_arch_up_start(cpu);
>> ret = __cpu_up(cpu);
>> + trace_cpu_hotplug_arch_up_end(cpu);
>> if (ret != 0)
>> goto out_notify;
>> BUG_ON(!cpu_online(cpu));
>> @@ -335,6 +349,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>> pg_data_t *pgdat;
>> #endif
>>
>> + trace_cpu_hotplug_up_start(cpu);
>> +
>> if (!cpu_possible(cpu)) {
>> printk(KERN_ERR "can't online cpu %d because it is not "
>> "configured as may-hotadd at boot time\n", cpu);
>> @@ -376,6 +392,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>>
>> err = _cpu_up(cpu, 0);
>>
>> + trace_cpu_hotplug_up_end(cpu);
>
> You should probably have this call after cpu_maps_update_done(),
> because you put the start before the mutex is locked.
> Just to stay symetric with lock events.
>
> In fact I think it may be better not to include the hotplug lock/unlock
> in the cpu down/up tracing, but trace start once it is locked and trace
> stop before we release it.
>
> It's just that I think you're not interested in including cpu_add_remove_lock
> mutex contention in cpu hotplug traces. That's rather something to be measured
> with lock events if needed.
>
you're right, it's not symetric, I'm going to correct that point and
exclude the mutex from the trace
> It's a detail, for the rest I'm fine the patches. As Steve said though, it would
> be nice to get feedback from cpu hotplug maintainers (who I'm adding in Cc
> here again).
>
Thanks for your comments
> Thanks.
>
>
>> +
>> out:
>> cpu_maps_update_done();
>> return err;
>> --
>> 1.7.1
>
^ permalink raw reply
* Re: [PATCH] cdrom_id: cd_media_toc() extend toc size to 65536
From: Martin Pitt @ 2011-02-24 15:59 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <1298563025-7118-1-git-send-email-harald@redhat.com>
Hello Harald,
harald@redhat.com [2011-02-24 16:57 +0100]:
> Seems like an iDRAC reports a lot of toc entries.
>
> "For cd_media_toc() will have to be modified to handle larger
> tables right now it has an "unsigned char toc[2048]" but the toc
> can be up to 65536 bytes long . I got a TOC length of 4610 bytes,
> causing cd_media_toc() to fail."
>
> https://bugzilla.redhat.com/show_bug.cgi?idf0367
Thanks! Pushed.
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
^ permalink raw reply
* [PATCH] cdrom_id: cd_media_toc() extend toc size to 65536
From: harald @ 2011-02-24 15:57 UTC (permalink / raw)
To: linux-hotplug
From: Harald Hoyer <harald@redhat.com>
Seems like an iDRAC reports a lot of toc entries.
"For cd_media_toc() will have to be modified to handle larger
tables right now it has an "unsigned char toc[2048]" but the toc
can be up to 65536 bytes long . I got a TOC length of 4610 bytes,
causing cd_media_toc() to fail."
https://bugzilla.redhat.com/show_bug.cgi?idf0367
---
| 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
--git a/extras/cdrom_id/cdrom_id.c b/extras/cdrom_id/cdrom_id.c
index f0e1cbb..4a58a49 100644
--- a/extras/cdrom_id/cdrom_id.c
+++ b/extras/cdrom_id/cdrom_id.c
@@ -737,7 +737,7 @@ static int cd_media_toc(struct udev *udev, int fd)
{
struct scsi_cmd sc;
unsigned char header[12];
- unsigned char toc[2048];
+ unsigned char toc[65536];
unsigned int len, i, num_tracks;
unsigned char *p;
int err;
--
1.7.3.4
^ permalink raw reply related
* Re: How to use Udev to restrict USB access only to particular set of
From: Greg KH @ 2011-02-24 15:46 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Thu, Feb 24, 2011 at 03:52:16PM +0200, Vilius Benetis wrote:
> On Tue, Feb 22, 2011 at 4:38 PM, Greg KH <greg@kroah.com> wrote:
> > Ok, what is the udev rule that you tried and did not work?
>
> we tried to disable USB storage sticks with the following command:
>
> SUBSYSTEMS="usb" DRIVERS="usb-storage" OPTIONS:="ignore_device"
>
> but we failed to make it work.
Ignoring the device still makes it "active" in the system, especially as
you just tested that the usb-storage device was bound to your device
(which wouldn't be true that early in the process, which is one reason
why this failed).
you need to write a 0 to the "authorized" file in sysfs which will
disable the whole USB device entirely if it meets your "list of devices
to reject". You also need to test not for driver binding, which again
will not have happened, and you don't want to have happen, but that it
is a usb storage device type (by virtue of the correct class config
options as shown by sysfs) and that it doesn't pass your list of valid
serial numbers.
Note, all of that might be easier to do in a script than in a udev rule
alone, but it should be possible.
Hope this helps,
greg k-h
^ permalink raw reply
* Re: [PATCH V4 2/2] tracing, perf : add cpu hotplug trace events
From: Frederic Weisbecker @ 2011-02-24 15:00 UTC (permalink / raw)
To: Vincent Guittot
Cc: linux-kernel, linux-hotplug, rostedt, amit.kucheria,
Rusty Russell, Ingo Molnar
In-Reply-To: <AANLkTi=z8GjFEK5bUfrfiSikETFywyAcT5ZdnWaJ6ZzA@mail.gmail.com>
On Mon, Jan 31, 2011 at 11:23:59AM +0100, Vincent Guittot wrote:
> Please find below a new proposal for adding trace events for cpu hotplug.
> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.
>
> Subject: [PATCH 2/2] add hotplug tracepoint
>
> this patch adds new events for cpu hotplug tracing
> * plug/unplug sequence
> * core and architecture latency measurements
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> kernel/cpu.c | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 156cc55..692e819 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -16,6 +16,9 @@
> #include <linux/mutex.h>
> #include <linux/gfp.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/cpu_hotplug.h>
> +
> #ifdef CONFIG_SMP
> /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> static DEFINE_MUTEX(cpu_add_remove_lock);
> @@ -197,10 +200,13 @@ struct take_cpu_down_param {
> static int __ref take_cpu_down(void *_param)
> {
> struct take_cpu_down_param *param = _param;
> + unsigned int cpu = (unsigned int)(param->hcpu);
> int err;
>
> /* Ensure this CPU doesn't handle any more interrupts. */
> + trace_cpu_hotplug_disable_start(cpu);
> err = __cpu_disable();
> + trace_cpu_hotplug_disable_end(cpu);
> if (err < 0)
> return err;
>
> @@ -256,7 +262,9 @@ static int __ref _cpu_down(unsigned int cpu, int
> tasks_frozen)
> cpu_relax();
>
> /* This actually kills the CPU. */
> + trace_cpu_hotplug_die_start(cpu);
> __cpu_die(cpu);
> + trace_cpu_hotplug_die_end(cpu);
>
> /* CPU is completely dead: tell everyone. Too late to complain. */
> cpu_notify_nofail(CPU_DEAD | mod, hcpu);
> @@ -274,6 +282,8 @@ int __ref cpu_down(unsigned int cpu)
> {
> int err;
>
> + trace_cpu_hotplug_down_start(cpu);
> +
> cpu_maps_update_begin();
>
> if (cpu_hotplug_disabled) {
> @@ -285,6 +295,8 @@ int __ref cpu_down(unsigned int cpu)
>
> out:
> cpu_maps_update_done();
> +
> + trace_cpu_hotplug_down_end(cpu);
> return err;
> }
> EXPORT_SYMBOL(cpu_down);
> @@ -310,7 +322,9 @@ static int __cpuinit _cpu_up(unsigned int cpu, int
> tasks_frozen)
> }
>
> /* Arch-specific enabling code. */
> + trace_cpu_hotplug_arch_up_start(cpu);
> ret = __cpu_up(cpu);
> + trace_cpu_hotplug_arch_up_end(cpu);
> if (ret != 0)
> goto out_notify;
> BUG_ON(!cpu_online(cpu));
> @@ -335,6 +349,8 @@ int __cpuinit cpu_up(unsigned int cpu)
> pg_data_t *pgdat;
> #endif
>
> + trace_cpu_hotplug_up_start(cpu);
> +
> if (!cpu_possible(cpu)) {
> printk(KERN_ERR "can't online cpu %d because it is not "
> "configured as may-hotadd at boot time\n", cpu);
> @@ -376,6 +392,8 @@ int __cpuinit cpu_up(unsigned int cpu)
>
> err = _cpu_up(cpu, 0);
>
> + trace_cpu_hotplug_up_end(cpu);
You should probably have this call after cpu_maps_update_done(),
because you put the start before the mutex is locked.
Just to stay symetric with lock events.
In fact I think it may be better not to include the hotplug lock/unlock
in the cpu down/up tracing, but trace start once it is locked and trace
stop before we release it.
It's just that I think you're not interested in including cpu_add_remove_lock
mutex contention in cpu hotplug traces. That's rather something to be measured
with lock events if needed.
It's a detail, for the rest I'm fine the patches. As Steve said though, it would
be nice to get feedback from cpu hotplug maintainers (who I'm adding in Cc
here again).
Thanks.
> +
> out:
> cpu_maps_update_done();
> return err;
> --
> 1.7.1
^ permalink raw reply
* Re: [PATCH V4 1/2] tracing, perf : add cpu hotplug trace events
From: Steven Rostedt @ 2011-02-24 14:36 UTC (permalink / raw)
To: Vincent Guittot
Cc: linux-kernel, linux-hotplug, fweisbec, amit.kucheria, Ingo Molnar,
Rusty Russell
In-Reply-To: <AANLkTinvZmoXQV3Kb7uyuHsCnDNNGfed0BfRfLaE_CdZ@mail.gmail.com>
[ Added Ingo and Rusty ]
On Thu, 2011-02-24 at 15:26 +0100, Vincent Guittot wrote:
> Hi,
>
> Any comments or feedbacks about these both patches ?
I'm fine with these patches and I believe Frederic said he was too. But
as these patches trace cpu hotplug, it is up to the maintainers of cpu
hotplug to make the final decision.
-- Steve
>
> Thanks,
> Vincent
>
> On 31 January 2011 11:22, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > Please find below a new proposal for adding trace events for cpu hotplug.
> > The goal is to measure the latency of each part (kernel, architecture)
> > and also to trace the cpu hotplug activity with other power events. I
> > have tested these traces events on an arm platform.
> >
> > Subject: [PATCH 1/2] cpu hotplug tracepoint
> >
> > this patch adds new events for cpu hotplug tracing
> > * plug/unplug sequence
> > * core and architecture latency measurements
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> > include/trace/events/cpu_hotplug.h | 103 ++++++++++++++++++++++++++++++++++++
> > 1 files changed, 103 insertions(+), 0 deletions(-)
> > create mode 100644 include/trace/events/cpu_hotplug.h
> >
> > diff --git a/include/trace/events/cpu_hotplug.h
> > b/include/trace/events/cpu_hotplug.h
> > new file mode 100644
> > index 0000000..af05775
> > --- /dev/null
> > +++ b/include/trace/events/cpu_hotplug.h
> > @@ -0,0 +1,103 @@
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM cpu_hotplug
> > +
> > +#if !defined(_TRACE_CPU_HOTPLUG_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_CPU_HOTPLUG_H
> > +
> > +#include <linux/tracepoint.h>
> > +
> > +DECLARE_EVENT_CLASS(cpu_hotplug,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid),
> > +
> > + TP_STRUCT__entry(
> > + __field( unsigned int, cpuid )
> > + ),
> > +
> > + TP_fast_assign(
> > + __entry->cpuid = cpuid;
> > + ),
> > +
> > + TP_printk("cpuid=%u", __entry->cpuid)
> > +);
> > +
> > +/* Core function of cpu hotplug */
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_down_start,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_down_end,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_up_start,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_up_end,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +/* Architecture function for cpu hotplug */
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_disable_start,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_disable_end,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_die_start,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_die_end,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_arch_up_start,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_arch_up_end,
> > +
> > + TP_PROTO(unsigned int cpuid),
> > +
> > + TP_ARGS(cpuid)
> > +);
> > +
> > +#endif /* _TRACE_CPU_HOTPLUG_H */
> > +
> > +/* This part must be outside protection */
> > +#include <trace/define_trace.h>
> > --
> > 1.7.1
> >
^ permalink raw reply
* Re: [PATCH V4 1/2] tracing, perf : add cpu hotplug trace events
From: Vincent Guittot @ 2011-02-24 14:26 UTC (permalink / raw)
To: linux-kernel, linux-hotplug, fweisbec, rostedt, amit.kucheria
In-Reply-To: <AANLkTi=V8xPzo-T=DMpf-FMF141SrMnb9J9mXqE9pW0w@mail.gmail.com>
Hi,
Any comments or feedbacks about these both patches ?
Thanks,
Vincent
On 31 January 2011 11:22, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> Please find below a new proposal for adding trace events for cpu hotplug.
> The goal is to measure the latency of each part (kernel, architecture)
> and also to trace the cpu hotplug activity with other power events. I
> have tested these traces events on an arm platform.
>
> Subject: [PATCH 1/2] cpu hotplug tracepoint
>
> this patch adds new events for cpu hotplug tracing
> * plug/unplug sequence
> * core and architecture latency measurements
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> include/trace/events/cpu_hotplug.h | 103 ++++++++++++++++++++++++++++++++++++
> 1 files changed, 103 insertions(+), 0 deletions(-)
> create mode 100644 include/trace/events/cpu_hotplug.h
>
> diff --git a/include/trace/events/cpu_hotplug.h
> b/include/trace/events/cpu_hotplug.h
> new file mode 100644
> index 0000000..af05775
> --- /dev/null
> +++ b/include/trace/events/cpu_hotplug.h
> @@ -0,0 +1,103 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM cpu_hotplug
> +
> +#if !defined(_TRACE_CPU_HOTPLUG_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_CPU_HOTPLUG_H
> +
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(cpu_hotplug,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid),
> +
> + TP_STRUCT__entry(
> + __field( unsigned int, cpuid )
> + ),
> +
> + TP_fast_assign(
> + __entry->cpuid = cpuid;
> + ),
> +
> + TP_printk("cpuid=%u", __entry->cpuid)
> +);
> +
> +/* Core function of cpu hotplug */
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_down_start,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_down_end,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_up_start,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_up_end,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +/* Architecture function for cpu hotplug */
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_disable_start,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_disable_end,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_die_start,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_die_end,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_arch_up_start,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +DEFINE_EVENT(cpu_hotplug, cpu_hotplug_arch_up_end,
> +
> + TP_PROTO(unsigned int cpuid),
> +
> + TP_ARGS(cpuid)
> +);
> +
> +#endif /* _TRACE_CPU_HOTPLUG_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> --
> 1.7.1
>
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set of
From: Vilius Benetis @ 2011-02-24 13:52 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Tue, Feb 22, 2011 at 4:38 PM, Greg KH <greg@kroah.com> wrote:
> Ok, what is the udev rule that you tried and did not work?
we tried to disable USB storage sticks with the following command:
SUBSYSTEMS="usb" DRIVERS="usb-storage" OPTIONS:="ignore_device"
but we failed to make it work.
in general, I would say, that the sequence should be reversed - at
first ATTR(serial) to be checked for positives with ":=" (to terminate
the next checking), and at the end to place
SUBSYSTEMS="usb" DRIVERS="usb-storage" OPTIONS:="ignore_device"
Any guidance is very appreciated.
--Vilius
^ permalink raw reply
* libudev, a question about the enumerate API.
From: Antonio Ospite @ 2011-02-23 22:50 UTC (permalink / raw)
To: linux-hotplug; +Cc: linux-input, Alan Ott
[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]
Hi,
in my attempts to handle leds setting and usb pairing for the Sixaxis
controller I had to fight with libudev a little bit, eventually I
succeeded in what I wanted to achieve but I still have some questions
about the libudev API, so here I am.
This is basically the layout for the device:
4-1:1.0/
|-- 0003:054C:0268.000C (hid device)
| |-- hidraw
| | `-- hidraw1
| `-- uevent (which tells me HID_PHYS)
|
`-- input
`-- input17
|-- js0
`-- phys
I needed to get the hidraw device node, and the js device number, the
two sibling devices (from hid and input subsystem) can be matched using
the 'phys' property.
This is what I do now:
0. Monitor with a filter matching the "hidraw" subsystem.
1. When a matching device is connected we get the "hidraw" device.
2. Go up to its "hid" parent device and check it is actually a
Sixaxis (using HID_NAME), if not GOTO 1.
3. Store the hidraw device node for later use
4. Store the HID_PHYS value in order to look for the matching joystick.
5. Enumerate the joystick devices with the sysname filter "js*"
6. For each joystick:
a. Go up to its input parent device
b. Check that the phys attribute matches HID_PHYS,
if so, store the joystick device number.
7. Set leds
8. If the Sixaxis is connected via USB do the cable pairing.
My doubt is about 5. and 6a.: I have go deep in the hierarchy down to
the js0 leaf device and then I have to go up one level to get the
input device, naively I would have done the contrary:
5'. Enumerate the input devices such that phys == HID_PHYS
6'. Enumerate the devices _below_ the input device from 5'. looking
for the jsX device.
But this is not possible because the enumerate API is designed to start
always from the root of /sys.
Finally the question: why the enumerate API in libudev does not allow
enumerating only from a _subtree_? Has that been designed this way to
keep the API simple, or because it is not considered useful?
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH V3] Export ACPI _DSM provided firmware instance number
From: Narendra_K @ 2011-02-23 13:12 UTC (permalink / raw)
To: mjg59
Cc: linux-pci, linux-hotplug, netdev, Matt_Domsch, Charles_Rose,
Jordan_Hargrave, Shyam_Iyer
In-Reply-To: <20110223124419.GA5695@srcf.ucam.org>
On Wed, Feb 23, 2011 at 06:14:19PM +0530, Matthew Garrett wrote:
> I think this version will still break the build. You need to depend on
> CONFIG_NLS.
Matthew,
Thanks. I posted a patch to linux-next to fix the build failure.
With regards,
Narendra K
^ permalink raw reply
* [PATCH V3] Export ACPI _DSM provided firmware instance number and
From: Narendra_K @ 2011-02-23 12:48 UTC (permalink / raw)
To: linux-pci, linux-hotplug
Cc: netdev, mjg, Matt_Domsch, Charles_Rose, Jordan_Hargrave,
Shyam_Iyer
In-Reply-To: <20110223124419.GA5695@srcf.ucam.org>
Hello,
This patch exports ACPI _DSM provided firmware instance number and
string name to sysfs.
V1 -> V2:
The attribute 'index' is changed to 'acpi_index' as the semantics of
SMBIOS provided device type instance and ACPI _DSM provided firmware
instance number are different.
V2 -> V3:
Matthew Garrett pointed out that 'sysfs_create_groups' does return an
error when there are no ACPI _DSM attributes available and because of
that the fallback to SMBIOS will not happen. As a result SMBIOS provided
attributes are not created. This version of the patch addresses the issue.
Please consider this patch for inclusion.
From: Narendra K <narendra_k@dell.com>
Subject: [PATCH] Export ACPI _DSM provided firmware instance number and string to sysfs
This patch exports ACPI _DSM (Device Specific Method) provided firmware
instance number and string name of PCI devices as defined by
'PCI Firmware Specification Revision 3.1' section 4.6.7.( DSM for Naming
a PCI or PCI Express Device Under Operating Systems) to sysfs.
New files created are:
/sys/bus/pci/devices/.../label which contains the firmware name for
the device in question, and
/sys/bus/pci/devices/.../acpi_index which contains the firmware device type
instance for the given device.
cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/acpi_index
1
cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/label
Embedded Broadcom 5709C NIC 1
cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.1/acpi_index
2
cat /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.1/label
Embedded Broadcom 5709C NIC 2
The ACPI _DSM provided firmware 'instance number' and 'string name' will
be given priority if the firmware also provides 'SMBIOS type 41 device
type instance and string'.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Jordan Hargrave <jordan_hargrave@dell.com>
Signed-off-by: Narendra K <narendra_k@dell.com>
---
Documentation/ABI/testing/sysfs-bus-pci | 31 +++-
drivers/pci/Makefile | 3 +-
drivers/pci/pci-label.c | 247 ++++++++++++++++++++++++++++++-
drivers/pci/pci.h | 2 +-
4 files changed, 268 insertions(+), 15 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index f979d82..36bf454 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -145,9 +145,11 @@ Date: July 2010
Contact: Narendra K <narendra_k@dell.com>, linux-bugs@dell.com
Description:
Reading this attribute will provide the firmware
- given name(SMBIOS type 41 string) of the PCI device.
- The attribute will be created only if the firmware
- has given a name to the PCI device.
+ given name (SMBIOS type 41 string or ACPI _DSM string) of
+ the PCI device. The attribute will be created only
+ if the firmware has given a name to the PCI device.
+ ACPI _DSM string name will be given priority if the
+ system firmware provides SMBIOS type 41 string also.
Users:
Userspace applications interested in knowing the
firmware assigned name of the PCI device.
@@ -157,12 +159,27 @@ Date: July 2010
Contact: Narendra K <narendra_k@dell.com>, linux-bugs@dell.com
Description:
Reading this attribute will provide the firmware
- given instance(SMBIOS type 41 device type instance)
- of the PCI device. The attribute will be created
- only if the firmware has given a device type instance
- to the PCI device.
+ given instance (SMBIOS type 41 device type instance) of the
+ PCI device. The attribute will be created only if the firmware
+ has given an instance number to the PCI device.
Users:
Userspace applications interested in knowing the
firmware assigned device type instance of the PCI
device that can help in understanding the firmware
intended order of the PCI device.
+
+What: /sys/bus/pci/devices/.../acpi_index
+Date: July 2010
+Contact: Narendra K <narendra_k@dell.com>, linux-bugs@dell.com
+Description:
+ Reading this attribute will provide the firmware
+ given instance (ACPI _DSM instance number) of the PCI device.
+ The attribute will be created only if the firmware has given
+ an instance number to the PCI device. ACPI _DSM instance number
+ will be given priority if the system firmware provides SMBIOS
+ type 41 device type instance also.
+Users:
+ Userspace applications interested in knowing the
+ firmware assigned instance number of the PCI
+ device that can help in understanding the firmware
+ intended order of the PCI device.
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 98e6fdf..bb1d3b2 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -53,8 +53,9 @@ obj-$(CONFIG_TILE) += setup-bus.o setup-irq.o
#
# ACPI Related PCI FW Functions
+# ACPI _DSM provided firmware instance and string name
#
-obj-$(CONFIG_ACPI) += pci-acpi.o
+obj-$(CONFIG_ACPI) += pci-acpi.o pci-label.o
# SMBIOS provided firmware instance and labels
obj-$(CONFIG_DMI) += pci-label.o
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 90c0a72..824e247 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -5,6 +5,13 @@
* by Narendra K <Narendra_K@dell.com>,
* Jordan Hargrave <Jordan_Hargrave@dell.com>
*
+ * PCI Firmware Specification Revision 3.1 section 4.6.7 (DSM for Naming a
+ * PCI or PCI Express Device Under Operating Systems) defines an instance
+ * number and string name. This code retrieves them and exports them to sysfs.
+ * If the system firmware does not provide the ACPI _DSM (Device Specific
+ * Method), then the SMBIOS type 41 instance number and string is exported to
+ * sysfs.
+ *
* SMBIOS defines type 41 for onboard pci devices. This code retrieves
* the instance number and string from the type 41 record and exports
* it to sysfs.
@@ -19,8 +26,30 @@
#include <linux/pci_ids.h>
#include <linux/module.h>
#include <linux/device.h>
+#include <linux/nls.h>
+#include <linux/acpi.h>
+#include <linux/pci-acpi.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acpi_bus.h>
#include "pci.h"
+#define DEVICE_LABEL_DSM 0x07
+
+#ifndef CONFIG_DMI
+
+static inline int
+pci_create_smbiosname_file(struct pci_dev *pdev)
+{
+ return -1;
+}
+
+static inline void
+pci_remove_smbiosname_file(struct pci_dev *pdev)
+{
+}
+
+#else
+
enum smbios_attr_enum {
SMBIOS_ATTR_NONE = 0,
SMBIOS_ATTR_LABEL_SHOW,
@@ -120,9 +149,7 @@ static struct attribute_group smbios_attr_group = {
static int
pci_create_smbiosname_file(struct pci_dev *pdev)
{
- if (!sysfs_create_group(&pdev->dev.kobj, &smbios_attr_group))
- return 0;
- return -ENODEV;
+ return sysfs_create_group(&pdev->dev.kobj, &smbios_attr_group);
}
static void
@@ -131,13 +158,221 @@ pci_remove_smbiosname_file(struct pci_dev *pdev)
sysfs_remove_group(&pdev->dev.kobj, &smbios_attr_group);
}
+#endif
+
+#ifndef CONFIG_ACPI
+
+static inline int
+pci_create_acpi_index_label_files(struct pci_dev *pdev)
+{
+ return -1;
+}
+
+static inline int
+pci_remove_acpi_index_label_files(struct pci_dev *pdev)
+{
+ return -1;
+}
+
+#else
+
+static const char device_label_dsm_uuid[] = {
+ 0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
+ 0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
+};
+
+enum acpi_attr_enum {
+ ACPI_ATTR_NONE = 0,
+ ACPI_ATTR_LABEL_SHOW,
+ ACPI_ATTR_INDEX_SHOW,
+};
+
+static void dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
+{
+ int len;
+ len = utf16s_to_utf8s((const wchar_t *)obj->
+ package.elements[1].string.pointer,
+ obj->package.elements[1].string.length,
+ UTF16_LITTLE_ENDIAN,
+ buf, PAGE_SIZE);
+ buf[len] = '\n';
+}
+
+static int
+dsm_get_label(acpi_handle handle, int func,
+ struct acpi_buffer *output,
+ char *buf, enum acpi_attr_enum attribute)
+{
+ struct acpi_object_list input;
+ union acpi_object params[4];
+ union acpi_object *obj;
+ int len = 0;
+
+ int err;
+
+ input.count = 4;
+ input.pointer = params;
+ params[0].type = ACPI_TYPE_BUFFER;
+ params[0].buffer.length = sizeof(device_label_dsm_uuid);
+ params[0].buffer.pointer = (char *)device_label_dsm_uuid;
+ params[1].type = ACPI_TYPE_INTEGER;
+ params[1].integer.value = 0x02;
+ params[2].type = ACPI_TYPE_INTEGER;
+ params[2].integer.value = func;
+ params[3].type = ACPI_TYPE_PACKAGE;
+ params[3].package.count = 0;
+ params[3].package.elements = NULL;
+
+ err = acpi_evaluate_object(handle, "_DSM", &input, output);
+ if (err)
+ return -1;
+
+ obj = (union acpi_object *)output->pointer;
+
+ switch (obj->type) {
+ case ACPI_TYPE_PACKAGE:
+ if (obj->package.count != 2)
+ break;
+ len = obj->package.elements[0].integer.value;
+ if (buf) {
+ if (attribute = ACPI_ATTR_INDEX_SHOW)
+ scnprintf(buf, PAGE_SIZE, "%llu\n",
+ obj->package.elements[0].integer.value);
+ else if (attribute = ACPI_ATTR_LABEL_SHOW)
+ dsm_label_utf16s_to_utf8s(obj, buf);
+ kfree(output->pointer);
+ return strlen(buf);
+ }
+ kfree(output->pointer);
+ return len;
+ break;
+ default:
+ kfree(output->pointer);
+ }
+ return -1;
+}
+
+static bool
+device_has_dsm(struct device *dev)
+{
+ acpi_handle handle;
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+
+ handle = DEVICE_ACPI_HANDLE(dev);
+
+ if (!handle)
+ return FALSE;
+
+ if (dsm_get_label(handle, DEVICE_LABEL_DSM, &output, NULL,
+ ACPI_ATTR_NONE) > 0)
+ return TRUE;
+
+ return FALSE;
+}
+
+static mode_t
+acpi_index_string_exist(struct kobject *kobj, struct attribute *attr, int n)
+{
+ struct device *dev;
+
+ dev = container_of(kobj, struct device, kobj);
+
+ if (device_has_dsm(dev))
+ return S_IRUGO;
+
+ return 0;
+}
+
+static ssize_t
+acpilabel_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ acpi_handle handle;
+ int length;
+
+ handle = DEVICE_ACPI_HANDLE(dev);
+
+ if (!handle)
+ return -1;
+
+ length = dsm_get_label(handle, DEVICE_LABEL_DSM,
+ &output, buf, ACPI_ATTR_LABEL_SHOW);
+
+ if (length < 1)
+ return -1;
+
+ return length;
+}
+
+static ssize_t
+acpiindex_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ acpi_handle handle;
+ int length;
+
+ handle = DEVICE_ACPI_HANDLE(dev);
+
+ if (!handle)
+ return -1;
+
+ length = dsm_get_label(handle, DEVICE_LABEL_DSM,
+ &output, buf, ACPI_ATTR_INDEX_SHOW);
+
+ if (length < 0)
+ return -1;
+
+ return length;
+
+}
+
+static struct device_attribute acpi_attr_label = {
+ .attr = {.name = "label", .mode = 0444},
+ .show = acpilabel_show,
+};
+
+static struct device_attribute acpi_attr_index = {
+ .attr = {.name = "acpi_index", .mode = 0444},
+ .show = acpiindex_show,
+};
+
+static struct attribute *acpi_attributes[] = {
+ &acpi_attr_label.attr,
+ &acpi_attr_index.attr,
+ NULL,
+};
+
+static struct attribute_group acpi_attr_group = {
+ .attrs = acpi_attributes,
+ .is_visible = acpi_index_string_exist,
+};
+
+static int
+pci_create_acpi_index_label_files(struct pci_dev *pdev)
+{
+ return sysfs_create_group(&pdev->dev.kobj, &acpi_attr_group);
+}
+
+static int
+pci_remove_acpi_index_label_files(struct pci_dev *pdev)
+{
+ sysfs_remove_group(&pdev->dev.kobj, &acpi_attr_group);
+ return 0;
+}
+#endif
+
void pci_create_firmware_label_files(struct pci_dev *pdev)
{
- if (!pci_create_smbiosname_file(pdev))
- ;
+ if (device_has_dsm(&pdev->dev))
+ pci_create_acpi_index_label_files(pdev);
+ else
+ pci_create_smbiosname_file(pdev);
}
void pci_remove_firmware_label_files(struct pci_dev *pdev)
{
- pci_remove_smbiosname_file(pdev);
+ if (device_has_dsm(&pdev->dev))
+ pci_remove_acpi_index_label_files(pdev);
+ else
+ pci_remove_smbiosname_file(pdev);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f69d6e0..a6ec200 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -11,7 +11,7 @@
extern int pci_uevent(struct device *dev, struct kobj_uevent_env *env);
extern int pci_create_sysfs_dev_files(struct pci_dev *pdev);
extern void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
-#ifndef CONFIG_DMI
+#if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI)
static inline void pci_create_firmware_label_files(struct pci_dev *pdev)
{ return; }
static inline void pci_remove_firmware_label_files(struct pci_dev *pdev)
--
1.7.3.1
With regards,
Narendra K
^ permalink raw reply related
* Re: [PATCH V3] Export ACPI _DSM provided firmware instance number
From: Matthew Garrett @ 2011-02-23 12:44 UTC (permalink / raw)
To: Narendra_K
Cc: linux-pci, linux-hotplug, netdev, Matt_Domsch, Charles_Rose,
Jordan_Hargrave, Shyam_Iyer
In-Reply-To: <20110223125741.GA16473@fedora14-r610.blr.amer.dell.com>
I think this version will still break the build. You need to depend on
CONFIG_NLS.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set
From: Greg KH @ 2011-02-22 14:38 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Tue, Feb 22, 2011 at 04:01:25PM +0200, Vilius Benetis wrote:
> On Tue, Feb 22, 2011 at 3:54 PM, Greg KH <greg@kroah.com> wrote:
> > Can you post what you tried and the errors you got from that?
> > You should also look at the archives for this list, as this topic comes
> > up every 6 months or so, and has been solved numerous times in the past.
>
> ok, to save the ether, I will search the archives. I looked at them
> initially - before posting my question, but could not spot the
> mentioned discussions.
>
> There are no errors, just functionality does not work, as we think it should.
Ok, what is the udev rule that you tried and did not work?
thanks,
greg k-h
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set of
From: Vilius Benetis @ 2011-02-22 14:28 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
[greg]
> Yes, add a udev rule to not "enable" any usb device that is a mass
> storage device that does not fall in your list of "valid" devices.
> There is a single sysfs file to write to which would prevent any access
> to that device, use that.
Greg I searched the documentation and the archives again, and failed
to find how exactly you set a rule
"to not "enable" any usb device that is a mass storage device that
does not fall in your list of "valid" devices."
Could you please clarify this for me?
Thank you,
--
/Vilius
^ permalink raw reply
* Re: How to use Udev to restrict USB access only to particular set of
From: Vilius Benetis @ 2011-02-22 14:01 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <AANLkTinqeg=o_bmdR0CKuVmP14x71kkrE3=zdaxx2AE=@mail.gmail.com>
On Tue, Feb 22, 2011 at 3:54 PM, Greg KH <greg@kroah.com> wrote:
> Can you post what you tried and the errors you got from that?
> You should also look at the archives for this list, as this topic comes
> up every 6 months or so, and has been solved numerous times in the past.
ok, to save the ether, I will search the archives. I looked at them
initially - before posting my question, but could not spot the
mentioned discussions.
There are no errors, just functionality does not work, as we think it should.
Greetings,
vilius
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox