Re: [PATCH v3] printk: Have printk() never buffer its data
On Tue, Jul 10, 2012 at 12:29 AM, Joe Perches wrote:
> On Tue, 2012-07-10 at 00:10 +0200, Kay Sievers wrote:
>> On Mon, Jul 9, 2012 at 11:42 PM, Joe Perches wrote:
>> > On Sun, 2012-07-08 at 19:55 +0200, Kay Sievers wrote:
>> >
>> >> At the same time the CPU#2 prints the same warning with a continuation
>> >> line, but the buffer from CPU#1 can not be flushed to the console, nor
>> >> can the continuation line printk()s from CPU#2 be merged at this point.
>> >> The consoles are still locked and busy with replaying the old log
>> >> messages, so the new continuation data is just stored away in the record
>> >> buffer as it is coming in.
>> >> If the console would be registered a bit earlier, or the warning would
>> >> happen a bit later, we would probably not see any of this.
>> >>
>> >> I can fake something like this just by holding the console semaphore
>> >> over a longer time and printing continuation lines with different CPUs
>> >> in a row.
>> >>
>> >> The patch below seems to work for me. It is also here:
>> >>
>> >> http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-merge-cont.patch;hb=HEAD
>> >>
>> >> It only applies cleanly on top of this patch:
>> >>
>> >> http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-syslog-1-byte-read.patch;hb=HEAD
>> >>
>> >
>> > Hi Kay.
>> >
>> > I just ran a test with what's in Greg's driver-core -for-linus branch.
>> >
>> > One of the differences in dmesg is timestamping of consecutive
>> > pr_("foo...)
>> > followed directly by
>> > pr_cont("bar...")
>> >
>> > For instance: (dmesg is 3.4, dmesg.0 is 3.5-rc6+)
>> >
>> > # grep MAP /var/log/dm* -A1
>> > dmesg:[0.781687] ata_piix :00:1f.2: MAP [ P0 P2 P1 P3 ]
>> > dmesg-[0.781707] ata2: port disabled--ignoring
>> > --
>> > dmesg.0:[0.948881] ata_piix :00:1f.2: MAP [
>> > dmesg.0-[0.948883] P0 P2 P1 P3 ]
>> >
>> > These messages originate starting at
>> > drivers/ata/ata_piix.c:1354
>> >
>> > All the continuations are emitted with pr_cont.
>> >
>> > I think this output should still be coalesced without
>> > timestamp deltas. Perhaps the timestamping code can
>> > still be reworked to avoid too small a delta producing
>> > a new timestamp and another dmesg line.
>>
>> Hmm, I don't see that.
>>
>> If I do:
>> pr_info("[");
>> for (i = 0; i < 4; i++)
>>pr_cont("%i ", i);
>> pr_cont("]\n");
>>
>> I get:
>> 6,173,0;[0 1 2 3 ]
>>
>> And if I fill the cont buffer and forcefully hold the console sem
>> during all that, and we can't merge anymore, I get:
>> 6,167,0;[
>> 4,168,0;0
>> 4,169,0;1
>> 4,170,0;2
>> 4,171,0;3
>> 4,172,0;]
>>
>> But the output is still all fine for both lines:
>> [0.00] [0 1 2 3 ]
>> [0.00] [0 1 2 3 ]
>>
>> What do I miss?
>
> In this case the initial line is dev_info not pr_info
> so there are the additional dict descriptors output to
> /dev/kmsg as well.
>
> Maybe that interferes with continuations. Dunno.
Yes, it does. Annotated records dev_printk() must be reliable in the
data storage and for the consumers. We can not expose them to the racy
continuation printk()s. We need to be able to trust the data they
print and not possibly merge unrelated things into it.
If it's needed, we can try to set the flags accordingly, that they
*look* like a line in the classic byte-stream output, but the
interface in /dev/kmsg must not export them that way, because
continuation lines can never be trusted to be correct.
Kay
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Mon, Jul 9, 2012 at 11:42 PM, Joe Perches wrote:
> On Sun, 2012-07-08 at 19:55 +0200, Kay Sievers wrote:
>
>> At the same time the CPU#2 prints the same warning with a continuation
>> line, but the buffer from CPU#1 can not be flushed to the console, nor
>> can the continuation line printk()s from CPU#2 be merged at this point.
>> The consoles are still locked and busy with replaying the old log
>> messages, so the new continuation data is just stored away in the record
>> buffer as it is coming in.
>> If the console would be registered a bit earlier, or the warning would
>> happen a bit later, we would probably not see any of this.
>>
>> I can fake something like this just by holding the console semaphore
>> over a longer time and printing continuation lines with different CPUs
>> in a row.
>>
>> The patch below seems to work for me. It is also here:
>>
>> http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-merge-cont.patch;hb=HEAD
>>
>> It only applies cleanly on top of this patch:
>>
>> http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-syslog-1-byte-read.patch;hb=HEAD
>>
>
> Hi Kay.
>
> I just ran a test with what's in Greg's driver-core -for-linus branch.
>
> One of the differences in dmesg is timestamping of consecutive
> pr_("foo...)
> followed directly by
> pr_cont("bar...")
>
> For instance: (dmesg is 3.4, dmesg.0 is 3.5-rc6+)
>
> # grep MAP /var/log/dm* -A1
> dmesg:[0.781687] ata_piix :00:1f.2: MAP [ P0 P2 P1 P3 ]
> dmesg-[0.781707] ata2: port disabled--ignoring
> --
> dmesg.0:[0.948881] ata_piix :00:1f.2: MAP [
> dmesg.0-[0.948883] P0 P2 P1 P3 ]
>
> These messages originate starting at
> drivers/ata/ata_piix.c:1354
>
> All the continuations are emitted with pr_cont.
>
> I think this output should still be coalesced without
> timestamp deltas. Perhaps the timestamping code can
> still be reworked to avoid too small a delta producing
> a new timestamp and another dmesg line.
Hmm, I don't see that.
If I do:
pr_info("[");
for (i = 0; i < 4; i++)
pr_cont("%i ", i);
pr_cont("]\n");
I get:
6,173,0;[0 1 2 3 ]
And if I fill the cont buffer and forcefully hold the console sem
during all that, and we can't merge anymore, I get:
6,167,0;[
4,168,0;0
4,169,0;1
4,170,0;2
4,171,0;3
4,172,0;]
But the output is still all fine for both lines:
[0.00] [0 1 2 3 ]
[0.00] [0 1 2 3 ]
What do I miss?
Kay
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Sat, 2012-07-07 at 07:04 +1000, Michael Neuling wrote:
> Whole kmsg below.
I guess I have an idea now what's going on.
> 4,47,0;WARNING: at
> /scratch/mikey/src/linux-ozlabs/arch/powerpc/sysdev/xics/xics-common.c:105
> 4,51,0;MSR: 90021032 CR: 2442 XER: 2200
> 4,54,0;TASK = c0b2dd80[0] 'swapper/0' THREAD: c0c24000 CPU: 0
This is the warning on CPU#1, all fine, all in one line.
> 6,74,0;console [tty0] enabled
> 6,75,0;console [hvc0] enabled
Now the boot consoles are registered, which replays the whole buffer
that was collected up to this point. During the entire time the console
semaphore needs to be held, and this can be quite a while.
> 4,87,24545;WARNING: at
> /scratch/mikey/src/linux-ozlabs/arch/powerpc/sysdev/xics/xics-common.c:105
> \4,91,24586;MSR: 90021032
> 4,92,24590;<
> 4,93,24594;SF
> 4,94,24599;,HV
> 4,95,24604;,ME
> 4,96,24609;,IR
> 4,97,24614;,DR
> 4,98,24619;,RI
> 4,99,24623;>
> 4,104,24661; CPU: 1
At the same time the CPU#2 prints the same warning with a continuation
line, but the buffer from CPU#1 can not be flushed to the console, nor
can the continuation line printk()s from CPU#2 be merged at this point.
The consoles are still locked and busy with replaying the old log
messages, so the new continuation data is just stored away in the record
buffer as it is coming in.
If the console would be registered a bit earlier, or the warning would
happen a bit later, we would probably not see any of this.
I can fake something like this just by holding the console semaphore
over a longer time and printing continuation lines with different CPUs
in a row.
The patch below seems to work for me. It is also here:
http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-merge-cont.patch;hb=HEAD
It only applies cleanly on top of this patch:
http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=kmsg-syslog-1-byte-read.patch;hb=HEAD
Thanks,
Kay
Subject: kmsg: merge continuation records while printing
In (the unlikely) case our continuation merge buffer is busy, we unfortunately
can not merge further continuation printk()s into a single record and have to
store them separately, which leads to split-up output of these lines when they
are printed.
Add some flags about newlines and prefix existence to these records and try to
reconstruct the full line again, when the separated records are printed.
---
kernel/printk.c | 119
1 file changed, 77 insertions(+), 42 deletions(-)
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -194,8 +194,10 @@ static int console_may_schedule;
*/
enum log_flags {
- LOG_DEFAULT = 0,
- LOG_NOCONS = 1, /* already flushed, do not print to console */
+ LOG_NOCONS = 1,/* already flushed, do not print to console */
+ LOG_NEWLINE = 2,/* text ended with a newline */
+ LOG_PREFIX = 4,/* text started with a prefix */
+ LOG_CONT= 8,/* text is a fragment of a continuation line */
};
struct log {
@@ -217,6 +219,7 @@ static DEFINE_RAW_SPINLOCK(logbuf_lock);
/* the next printk record to read by syslog(READ) or /proc/kmsg */
static u64 syslog_seq;
static u32 syslog_idx;
+static enum log_flags syslog_prev;
static size_t syslog_partial;
/* index and sequence number of the first record stored in the buffer */
@@ -839,8 +842,8 @@ static size_t print_prefix(const struct
return len;
}
-static size_t msg_print_text(const struct log *msg, bool syslog,
-char *buf, size_t size)
+static size_t msg_print_text(const struct log *msg, enum log_flags prev,
+bool syslog, char *buf, size_t size)
{
const char *text = log_text(msg);
size_t text_size = msg->text_len;
@@ -849,6 +852,8 @@ static size_t msg_print_text(const struc
do {
const char *next = memchr(text, '\n', text_size);
size_t text_len;
+ bool prefix = true;
+ bool newline = true;
if (next) {
text_len = next - text;
@@ -858,19 +863,35 @@ static size_t msg_print_text(const struc
text_len = text_size;
}
+ if ((prev & LOG_CONT) && !(msg->flags & LOG_PREFIX))
+ prefix = false;
+
+ if (msg->flags & LOG_CONT) {
+ if ((prev & LOG_CONT) && !(prev & LOG_NEWLINE))
+ prefix = false;
+
+ if (!(msg->flags & LOG_NEWLINE))
+ newline = false;
+ }
+
if (buf) {
if (print_prefix(msg, syslog, NULL) +
text_len + 1>= size - len)
break;
- len += print_prefix(msg, syslog, buf + len);
+ if (prefix)
Re: [PATCH v3] printk: Have printk() never buffer its data
On Fri, Jul 6, 2012 at 12:46 PM, Kay Sievers wrote: > On Fri, Jul 6, 2012 at 5:47 AM, Michael Neuling wrote: > >>> 4,89,24561;NIP: c0048164 LR: c0048160 CTR: >>> 4,90,24576;REGS: c0007e59fb50 TRAP: 0700 Tainted: GW >>> (3.5.0-rc4-mikey) >>> 4,91,24583;MSR: 90021032 >>> 4,92,24586;< >>> 4,93,24591;SF >>> 4,94,24596;,HV >>> 4,95,24601;,ME >>> 4,96,24606;,IR >>> 4,97,24611;,DR >>> 4,98,24616;,RI >>> 4,99,24619;> >>> 4,100,24628; CR: 2842 XER: 2200 >> >> FWIW, compiling with the parent commit gives this: >> >> 4,89,1712;NIP: c0048164 LR: c0048160 CTR: >> 4,90,1713;REGS: c0007e59fb50 TRAP: 0700 Tainted: GW >> (3.5.0-rc4-mikey) >> 4,91,1716;MSR: 90021032 CR: 2282 XER: >> 0200 > > Hmm, I don't understand, which parent commit do you mean? You maybe > mean without 084681d? > > I think it's a race of the two CPUs printing continuation lines, and > the continuation buffer is still occupied with data from one CPU and > not available to the other one at the same time. > > What you see is likely not the direct output to the console (that > would work) but the replay of the stored buffer when the console is > registered. Because the cont buffer was still busy with one CPU, the > other thread needs to store the continuation line prints in individual > records, which leads to the (unwanted) printed newlines when > replaying. > > The data we store looks all fine, it just looks needlessly separated > when we replay fromt he buffer on a newly registered boot console. We > need to merge the lines in the output, so they *look* like they are > all in one line. I'll work on a fix for that now. It could be that the console semaphore is still help by the other CPU, for whatever reason, when your box runs into this situation. Mind pasting more context (/dev/kmsg) of the log when this happens, not only the one line that get split-up? Is this possibly during an oops or backtrace going on when you see this? Which code calls show_regs() here? Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Fri, Jul 6, 2012 at 5:47 AM, Michael Neuling wrote: >> 4,89,24561;NIP: c0048164 LR: c0048160 CTR: >> 4,90,24576;REGS: c0007e59fb50 TRAP: 0700 Tainted: GW >> (3.5.0-rc4-mikey) >> 4,91,24583;MSR: 90021032 >> 4,92,24586;< >> 4,93,24591;SF >> 4,94,24596;,HV >> 4,95,24601;,ME >> 4,96,24606;,IR >> 4,97,24611;,DR >> 4,98,24616;,RI >> 4,99,24619;> >> 4,100,24628; CR: 2842 XER: 2200 > > FWIW, compiling with the parent commit gives this: > > 4,89,1712;NIP: c0048164 LR: c0048160 CTR: > 4,90,1713;REGS: c0007e59fb50 TRAP: 0700 Tainted: GW > (3.5.0-rc4-mikey) > 4,91,1716;MSR: 90021032 CR: 2282 XER: > 0200 Hmm, I don't understand, which parent commit do you mean? You maybe mean without 084681d? I think it's a race of the two CPUs printing continuation lines, and the continuation buffer is still occupied with data from one CPU and not available to the other one at the same time. What you see is likely not the direct output to the console (that would work) but the replay of the stored buffer when the console is registered. Because the cont buffer was still busy with one CPU, the other thread needs to store the continuation line prints in individual records, which leads to the (unwanted) printed newlines when replaying. The data we store looks all fine, it just looks needlessly separated when we replay fromt he buffer on a newly registered boot console. We need to merge the lines in the output, so they *look* like they are all in one line. I'll work on a fix for that now. Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Fri, Jul 6, 2012 at 2:41 AM, Michael Neuling wrote: >> > Does this happen only very early during bootup, or also later when the >> > box fully initialized? > > I'm seeing during boot but not later (xmon (ppc kernel debugger) doesn't > see it if I do 'echo x > /proc/sysrq-trigger') . I wouldn't say > it's "very early boot". It's a secondary CPU coming up and the primary > is waiting for it. We've already configured the console when this > happens. Sounds like an early boot console. >> > The output of 'dmesg' later looks always correct, right? > > No, dmesg also has the extra new lines: eg > > <4>NIP: c004e234 LR: c004e230 CTR: > <4>REGS: c0007c3b7b50 TRAP: 0700 Tainted: GW > (3.5.0-rc4-mikey) > <4>MSR: 90021032 > <4>< > <4>SF > <4>,HV > <4>,ME > <4>,IR > <4>,DR > <4>,RI > <4>> > <4> CR: 2842 XER: 2200 Can you please paste the output of /dev/kmsg of this section? So we can see the timestamps and what really went into the record buffer. >> Could you possibly try this patch? > > Sorry, doesn't help. It also reprints the entire boot log to the > console once the console get inited. Which is the normal behaviour to do that, right? We should not have touched any of that logic. Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Thu, 2012-07-05 at 13:47 +0200, Kay Sievers wrote: > On Thu, Jul 5, 2012 at 12:20 PM, Michael Neuling wrote: > > > I can only make 2) happen on SMP. It's when the second CPU is coming up > > and it's printing something. The first CPU isn't printing anything at > > this stage (there is no garbled console here) so I don't think it's a > > race. I see it consistently in show_regs(). Every printk without a > > newline. ie I get this: > > --- > > NIP: c0048164 LR: c0048160 CTR: > > REGS: c0007e59fb50 TRAP: 0700 Tainted: GW > > (3.5.0-rc5-mikey) > > MSR: 90021032 > > < > > SF > > ,HV > > ,ME > > ,IR > > ,DR > > ,RI > >> > > CR: 2842 XER: 2200 > > SOFTE: 0 > > CFAR: c07402f8 > > TASK = c0007e56bb40[0] 'swapper/1' THREAD: c0007e59c000 > > CPU: 1 > > --- > > > > It's consistent for printks without newlines in show_regs(). MSR > > through to XER should all be on the same line. > > I see. Something to fix then, I'll have a look. > > Does this happen only very early during bootup, or also later when the > box fully initialized? > > The output of 'dmesg' later looks always correct, right? If it's an early printk issue like it looks like, where stuff got printed before we had any console registered, this might fix the issue you are seeing. Could you possibly try this patch? Thanks, Kay --- a/kernel/printk.c +++ b/kernel/printk.c @@ -1957,6 +1951,12 @@ skip: */ console_idx = log_next(console_idx); console_seq++; + /* +* We will get here again when we register a new +* CON_PRINTBUFFER console. Clear the flag so we +* will properly dump everything later. +*/ + msg->flags &= ~LOG_NOCONS; goto skip; } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Thu, Jul 5, 2012 at 12:20 PM, Michael Neuling wrote: > I can only make 2) happen on SMP. It's when the second CPU is coming up > and it's printing something. The first CPU isn't printing anything at > this stage (there is no garbled console here) so I don't think it's a > race. I see it consistently in show_regs(). Every printk without a > newline. ie I get this: > --- > NIP: c0048164 LR: c0048160 CTR: > REGS: c0007e59fb50 TRAP: 0700 Tainted: GW (3.5.0-rc5-mikey) > MSR: 90021032 > < > SF > ,HV > ,ME > ,IR > ,DR > ,RI >> > CR: 2842 XER: 2200 > SOFTE: 0 > CFAR: c07402f8 > TASK = c0007e56bb40[0] 'swapper/1' THREAD: c0007e59c000 > CPU: 1 > --- > > It's consistent for printks without newlines in show_regs(). MSR > through to XER should all be on the same line. I see. Something to fix then, I'll have a look. Does this happen only very early during bootup, or also later when the box fully initialized? The output of 'dmesg' later looks always correct, right? Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Thu, Jul 5, 2012 at 10:39 AM, Kay Sievers wrote: > On Thu, Jul 5, 2012 at 9:03 AM, Michael Neuling wrote: >>> On Mon, 2012-06-25 at 18:40 -0700, Linus Torvalds wrote: > >>> > I think it might be a great idea to buffer for logging in order to >>> > generate one individual buffer record there. >>> > >>> > But it needs to be printed as it is generated. >>> >>> That's a good idea. >>> >>> Something like this could work - only minimally tested at this moment. >> >> This breaks some powerpc configs and is in Linus' tree now as >> 084681d14e. >> >> When we have printks without a newline (like show_regs()), it >> sometimes: > > x86 has that a lot too. > >> 1) drops the console output for that line (dmesg is fine). Patch to fix >>this below. > > That doesn't look right. We should already have put that out to the > console, and we only want to store it away. Your patch, as expected, > duplicates all the continuation lines on the console here: > [0.674957] hpet0: at MMIO 0xfed0, IRQs 2, 8, 0 > [0.674957] hpet0: at MMIO 0xfed0, IRQs 2, 8, 0 > >> 2) adds a newline unnecessary to both console and dmesg. I have no fix >>for this currently. >> Reverting this patch fixes both problems. > > Not here. I can't reproduce any of this here, it all looks fine. > > Is that possibly some early printk() or other console trickery on ppc > that produces the issue? Or, are you really sure this isn't just a race with another printk and the content got merged with some other line of the output? The added newline you mention could suggest that. With the buffering for the console output removed, I see garbled console output here too (like we always had before the new kmsg) when continuation printks race against printks from any other process. That was all gone with the buffering (only two continuation prints would race, not any other), but we needed to skip buffering the console output for other, more important reasons. Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3] printk: Have printk() never buffer its data
On Thu, Jul 5, 2012 at 9:03 AM, Michael Neuling wrote: >> On Mon, 2012-06-25 at 18:40 -0700, Linus Torvalds wrote: >> > I think it might be a great idea to buffer for logging in order to >> > generate one individual buffer record there. >> > >> > But it needs to be printed as it is generated. >> >> That's a good idea. >> >> Something like this could work - only minimally tested at this moment. > > This breaks some powerpc configs and is in Linus' tree now as > 084681d14e. > > When we have printks without a newline (like show_regs()), it > sometimes: x86 has that a lot too. > 1) drops the console output for that line (dmesg is fine). Patch to fix >this below. That doesn't look right. We should already have put that out to the console, and we only want to store it away. Your patch, as expected, duplicates all the continuation lines on the console here: [0.674957] hpet0: at MMIO 0xfed0, IRQs 2, 8, 0 [0.674957] hpet0: at MMIO 0xfed0, IRQs 2, 8, 0 > 2) adds a newline unnecessary to both console and dmesg. I have no fix >for this currently. > Reverting this patch fixes both problems. Not here. I can't reproduce any of this here, it all looks fine. Is that possibly some early printk() or other console trickery on ppc that produces the issue? Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/8] v2 De-Couple sysfs memory directories from memory sections
On Wed, Sep 29, 2010 at 14:37, Greg KH wrote: > On Wed, Sep 29, 2010 at 10:32:34AM +0200, Avi Kivity wrote: >> On 09/29/2010 04:50 AM, Greg KH wrote: >>> > >>> > Because the old ABI creates 129,000+ entries inside >>> > /sys/devices/system/memory with their associated links from >>> > /sys/devices/system/node/node*/ back to those directory entries. >>> > >>> > Thankfully things like rpm, hald, and other miscellaneous commands scan >>> > that information. >>> >>> Really? Why? Why would rpm care about this? hald is dead now so we >>> don't need to worry about that anymore, >> >> That's not what compatiblity means. We can't just support >> latest-and-greatest userspace on latest-and-greatest kernels. > > Oh, I know that, that's not what I was getting at at all here, sorry if > it came across that way. > > I wanted to know so we could go fix programs that are mucking around in > these files, as odds are, the shouldn't be doing that in the first > place. > > Like rpm, why would it matter what the memory in the system looks like? HAL does many inefficient things, but I don't think it's using /sys/system/, besides that it may check the cpufreq govenors state there. Kay ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
BUS_ID_SIZE is going away
Hey, care to replace: bus_id[BUS_ID_SIZE]; in: include/linux/fsl_devices.h with a locally defined limit, or whatever fits your needs? Or let me know how you like it to be converted, and I can make a patch, and we can push it through the driver-core tree. The driver core and kobjects have no longer a limit on the device names. We want to remove BUS_ID_SIZE from the driver core header in the current 2.6.30 timeframe. Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] i2c: Add support for device alias names
On Thu, 2008-05-01 at 10:04 +0200, Jean Delvare wrote: > On Mon, 28 Apr 2008 18:16:17 +0200, Kay Sievers wrote: > > On Mon, 2008-04-28 at 17:40 +0200, Jean Delvare wrote: > > > Why would i2c device modaliases ever contain multiple strings? A device > > > can't have multiple names, can it? > > > > Like ACPI/PNP devices, which can have several compat id's, which means > > that a single device can have "multiple names": > > $ cat /sys/bus/pnp/devices/00:09/id > > IBM0057 > > PNP0f13 > > Ah, I didn't know about this. Now I'm curious how it can work. Does it > mean that several drivers attempt to bind to this device? They are usually all id's for the same type of device, and don't describe multiple functions at the same time. In most cases the vendor id's, like "IBM0057" here, do not match anything. > > > Can't we just stop handle_moddevtable() from adding a tailing "*" > > > automatically, and just let the device types which need it, add it on > > > their own? > > > > For a lot subsystems it's fine to have it appended, as there is a > > defined list of identifiers, which must appear in the same order, and > > new identifiers are appended to the end. So the "*" still matches > > modules with possibly extended modalias strings. > > I understand the logic, however I am skeptical how useful it is in > practice. If we add an identifier to the device aliases, then we also > update the corresponding modalias, so no in-tree driver can break. The > only case where it makes a difference, as far as I can see, is for > out-of-tree drivers. Am I correct? That sounds correct, yes. > On top of that, I doubt that we > actually add new identifiers that frequently, do we? Not that I know. > > We would also need to review all buses which export modalias, if they > > need the "*" or not, and add them by hand, if needed. > > > > I guess, it's easier to introduce an additional parameter to > > file2alias::do_table() and suppress the trailing "*" for i2c? > > That's one possibility, but I had a slightly different approach, which > is to just let the type-specific handlers add the trailing "*" by > themselves if they need it. This allows for optimization in a few > cases. > > Subject: modpost: i2c aliases need no trailing wildcard > > Not all device type aliases need a trailing wildcard, in particular > the i2c aliases don't. Don't add a wildcard by default in do_table(), > instead let each device type handler add it if needed. ... > The patch only changes the i2c aliases, all the rest is the same as > before (unless I messed up somewhere, that is.) Do you think this would > be acceptable for upstream? If you think it's better to add a parameter > to do_table() and let it add the "*" as it did so far, that's also fine > with me, I can update the patch to do that. Looks fine to me. If the content of: /lib/modules/$(uname -r)/modules.alias looks correct after the patch, this should go in, I think. Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] i2c: Add support for device alias names
On Mon, 2008-04-28 at 17:40 +0200, Jean Delvare wrote: > On Mon, 28 Apr 2008 17:07:25 +0200, Kay Sievers wrote: > > On Mon, 2008-04-28 at 11:39 +0200, Jean Delvare wrote: > > > One thing I am still not happy with is that the aliases created have a > > > trailing "*": > > > > > > $ /sbin/modinfo lm90 > > > filename: /lib/modules/2.6.25-git11/kernel/drivers/hwmon/lm90.ko > > > author: Jean Delvare <[EMAIL PROTECTED]> > > > description:LM90/ADM1032 driver > > > license:GPL > > > vermagic: 2.6.25-git11 mod_unload > > > depends:hwmon > > > alias: i2c:lm90* > > > alias: i2c:adm1032* > > > alias: i2c:lm99* > > > alias: i2c:lm86* > > > alias: i2c:max6657* > > > alias: i2c:adt7461* > > > alias: i2c:max6680* > > > $ > > > > > > This would cause trouble if one I2C chip name matches the beginning of > > > another I2C chip name and both chips are supported by different > > > drivers. This has yet to be seen, but still, I'd like to see this > > > problem fixed quickly. > > > > Right, the trailing "*" is not nice. > > > > We should terminate the string, so the trailing "*" will not match > > longer strings. The usual thing is to add a ":" to the end, which would > > then show up as > > alias: i2c:max6680:* > > > > See DMI and ACPI: > > alias dmi:*:svnFUJITSU:pnLifeBook*:pvr*:rvnFUJITSU:* apanel > > alias acpi*:ASIM:* atlas_btns > > I didn't know about these cases, thanks for the hint. > > > > > If i2c device modaliases could ever contain multiple strings, it should > > be: > > alias: i2c*:max6680:* > > to match the module, regardless of the order of the strings in the > > modalias: > > Why would i2c device modaliases ever contain multiple strings? A device > can't have multiple names, can it? Like ACPI/PNP devices, which can have several compat id's, which means that a single device can have "multiple names": $ cat /sys/bus/pnp/devices/00:09/id IBM0057 PNP0f13 > Adding a ":" at the end of the i2c device names solves the problem I > was mentioning, sure, but why don't we simply remove the trailing "*", > instead of trying to work around it? A trailing "*" simply makes no > sense for aliases which are simple device names. Sure, if there is only one single string, it's not useful. > This is not only i2c > devices, but also platform devices, acpi, dmi, pnp... ACPI, DMI, PNP (PNP does not do modalias) needs to be able to match only one string in a given list, so the trailing "*" is needed. > Looking at the > various device types handled by file2alias.c, it seems that most of > them don't need the trailing "*", and many of them have the problem I > was mentioning. > > Can't we just stop handle_moddevtable() from adding a tailing "*" > automatically, and just let the device types which need it, add it on > their own? For a lot subsystems it's fine to have it appended, as there is a defined list of identifiers, which must appear in the same order, and new identifiers are appended to the end. So the "*" still matches modules with possibly extended modalias strings. We would also need to review all buses which export modalias, if they need the "*" or not, and add them by hand, if needed. I guess, it's easier to introduce an additional parameter to file2alias::do_table() and suppress the trailing "*" for i2c? Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/2] i2c: Add support for device alias names
On Mon, 2008-04-28 at 11:39 +0200, Jean Delvare wrote: > Based on earlier work by Jon Smirl and Jochen Friedrich. > > This patch allows new-style i2c chip drivers to have alias names using > the official kernel aliasing system and MODULE_DEVICE_TABLE(). At this > point, the old i2c driver binding scheme (driver_name/type) is still > supported. > > Signed-off-by: Jean Delvare <[EMAIL PROTECTED]> > Cc: Jochen Friedrich <[EMAIL PROTECTED]> > Cc: Jon Smirl <[EMAIL PROTECTED]> > Cc: Kay Sievers <[EMAIL PROTECTED]> > --- > One thing I am still not happy with is that the aliases created have a > trailing "*": > > $ /sbin/modinfo lm90 > filename: /lib/modules/2.6.25-git11/kernel/drivers/hwmon/lm90.ko > author: Jean Delvare <[EMAIL PROTECTED]> > description:LM90/ADM1032 driver > license:GPL > vermagic: 2.6.25-git11 mod_unload > depends:hwmon > alias: i2c:lm90* > alias: i2c:adm1032* > alias: i2c:lm99* > alias: i2c:lm86* > alias: i2c:max6657* > alias: i2c:adt7461* > alias: i2c:max6680* > $ > > This would cause trouble if one I2C chip name matches the beginning of > another I2C chip name and both chips are supported by different > drivers. This has yet to be seen, but still, I'd like to see this > problem fixed quickly. Right, the trailing "*" is not nice. We should terminate the string, so the trailing "*" will not match longer strings. The usual thing is to add a ":" to the end, which would then show up as alias: i2c:max6680:* See DMI and ACPI: alias dmi:*:svnFUJITSU:pnLifeBook*:pvr*:rvnFUJITSU:* apanel alias acpi*:ASIM:* atlas_btns If i2c device modaliases could ever contain multiple strings, it should be: alias: i2c*:max6680:* to match the module, regardless of the order of the strings in the modalias: Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] update module-init-tools to support the i2c subsystem
On Jan 14, 2008 6:50 PM, Jean Delvare <[EMAIL PROTECTED]> wrote: > On Mon, 14 Jan 2008 18:08:16 +0100 (CET), Geert Uytterhoeven wrote: > > On Mon, 14 Jan 2008, Jean Delvare wrote: > > > I thought that the module aliases were generated by > > > scripts/mod/modpost? As a matter of fact, I did not apply Jon's patch > > > > Sorry, you're right. Too early in the morning :-) > > > > > to module-init-tools, and "modinfo" shows me module aliases properly > > > for i2c drivers that call MODULE_DEVICE_TABLE(): > > > > I've just looked it up again (I had to do a similar thing for Zorro bus > > support). Module-init-tools (depmod) also creates the modules.*map files, > > which are used to map from device IDs to module names. I think these are > > used > > by udev to load the appropriate module when a device with a specific device > > ID > > pops up in sysfs. > > Ah, right. I see it now, there's modules.isapnpmap, > modules.ieee1394map, modules.pcimap etc. but no modules.i2cmap. > However, there is modules.alias which contains the i2c aliases for all > device types (including one ieee1394 and many pci aliases) which seems > somewhat redundant with the modules.*map files. > > > > $ /sbin/modinfo lm90 > > > filename: /lib/modules/2.6.24-rc7-git4/kernel/drivers/hwmon/lm90.ko > > > author: Jean Delvare <[EMAIL PROTECTED]> > > > description:LM90/ADM1032 driver > > > license:GPL > > > vermagic: 2.6.24-rc7-git4 mod_unload > > > depends:hwmon > > > alias: i2c:Nlm90* > > > alias: i2c:Nadm1032* > > > alias: i2c:Nlm99* > > > alias: i2c:Nlm86* > > > alias: i2c:Nmax6657* > > > alias: i2c:Nadt7461* > > > alias: i2c:Nmax6680* > > > $ > > > > > > "modprobe i2c:Nadm1032" loads the lm90 driver as expected. > > > > Yes, it's also still not 100% clear to me when `i2c:Nadm1032' is used, and > > when > > modules.i2cmap would be used... > > I am under the impression that modules.*map are the old way to get > automatic driver loading and aliases are the new way to do the same. > But maybe that's just me. Right, nothing on recent systems is using the map files. This patch should not be needed. The plan is to deprecate the creation of these files in depmod. Thanks, Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Fix Firmware class name collision
On Fri, 2007-12-14 at 22:39 -0800, Greg KH wrote: > On Sat, Dec 15, 2007 at 05:14:29PM +1100, Benjamin Herrenschmidt wrote: > > > > On Fri, 2007-12-14 at 14:40 -0800, Greg KH wrote: > > > On Tue, Dec 04, 2007 at 07:28:00PM -0600, Timur Tabi wrote: > > > > Scott Wood wrote: > > > > > > > >> The physical address certainly is useful when you have more than one > > > >> device of the same name. > > > > > > > > What I meant was that the physical address isn't helpful by itself. > > > > > > > >> So then you'd get "firmware-ucc.e01024". What if there's another ucc > > > >> at > > > >> e0102480? For devices with longer names, you'd have even less > > > >> precision > > > >> in the address. > > > > > > > > Maybe we need to consider a more sophisticated algorithm, one that > > > > guarantees that the device name in its entirety is preserved? Either > > > > that, > > > > or replace the physical address with something shorter, like the offset > > > > to > > > > the root node only? That way, ucc.e0102400 because just ucc.2400. > > > > > > You should do something :) > > > > > > In the near future (2.6.26) there will not be a limit on the size of the > > > file name, so we should not have this problem anymore. > > > > Not even .25 ? damn ! Any way that fix can be fastracked ? This > > limitation has been a major PITA for some time now (this is just -one- > > example where it gets in the way). > > I'll let Kay answer that, last he said, it involves a _lot_ of changes > throughout the kernel :( The current patch gets rid of bus_id and uses the dynamic kobject_name directly all over the place. It's huge, because the current code expects a static array and uses strncpy/snprintf all over the place. I'm currently waiting for the new kobject api to finish, before I update the patch, not sure if we will make it in time for .25. Kay ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
