Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
Ingo Molnar <[EMAIL PROTECTED]> wrote: > so how about the following, different approach: anyone who has a tasklet > in any performance-sensitive codepath, please yell now. The cafe_ccic (OLPC) camera driver uses a tasklet to move frames out of the DMA buffers in the streaming I/O path. With this change in place, I'd worry that the possibility of dropping frames would increase, especially considering that (1) this is running on OLPC hardware, and (2) there is typically a streaming video application running in user space. Obviously some testing is called for here. I will make an attempt to do that testing, but the next few weeks involve some insane travel which will make that hard. Stay tuned. Thanks, jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
A couple of days ago I said: > The cafe_ccic (OLPC) camera driver uses a tasklet to move frames out of > the DMA buffers in the streaming I/O path > > Obviously some testing is called for here. I will make an attempt to do > that testing I've done that testing - I have an OLPC B3 unit running V2 of the tasklet->workqueue patch, and all seems well. 30 FPS to the display and no dropped frames. The tasklets/0 process is running 3-5% CPU, in case that's interesting. For whatever reason, I see about 3% *more* idle time when running just mplayer than I did without the patch. Consider my minor qualms withdrawn, there doesn't seem to be any trouble in this area. Thanks, jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH] doc: volatile considered evil
Randy Dunlap <[EMAIL PROTECTED]> wrote: > Here are some 'volatile' comments from Linus, extracted from > several emails in at least 2 threads. > > If this is close to useful, we can add it to Documentation/. I just took a shot at turning this into something more like a normal document: http://lwn.net/Articles/233479/ Comments welcome. If people think it suits the need I'll happily make a version which moves from the LWN style to Documentation/ and send it in. Thanks, jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] "volatile considered harmful" document
OK, here's an updated version of the volatile document - as a plain text file this time. It drops a new file in Documentation/, but might it be better as an addition to CodingStyle? Comments welcome, jon Tell kernel developers why they shouldn't use volatile. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff -ruNp linux-2.6/Documentation/volatile.txt volatile/Documentation/volatile.txt --- linux-2.6/Documentation/volatile.txt1969-12-31 17:00:00.0 -0700 +++ volatile/Documentation/volatile.txt 2007-05-09 14:56:40.0 -0600 @@ -0,0 +1,127 @@ +Why the "volatile" type class should not be used + + +The C Programming Language, Second Edition (copyright 1988, still known as +"the new C book") has the following to say about the volatile keyword: + + The purpose of volatile is to force an implementation to suppress + optimization that could otherwise occur. For example, for a + machine with memory-mapped input/output, a pointer to a device + register might be declared as a pointer to volatile, in + order to prevent the compiler from removing apparently redundant + references through the pointer. + +C programmers have often taken volatile to mean that the variable could be +changed outside of the current thread of execution; as a result, they are +sometimes tempted to use it in kernel code when shared data structures are +being used. Andrew Morton recently called out[1] use of volatile in a +submitted patch, saying: + + The volatiles are a worry - volatile is said to be + basically-always-wrong in-kernel, although we've never managed to + document why, and i386 cheerfully uses it in readb() and friends. + +In response, Randy Dunlap pulled together some email from Linus[2] on the +topic and suggested that we could maybe "document why." Here is the +result. + +The point that Linus often makes with regard to volatile is that +its purpose is to suppress optimization, which is almost never what one +really wants to do. In the kernel, one must protect accesses to data +against race conditions, which is very much a different task. + +Like volatile, the kernel primitives which make concurrent access to data +safe (spinlocks, mutexes, memory barriers, etc.) are designed to prevent +unwanted optimization. If they are being used properly, there will be no +need to use volatile as well. If volatile is still necessary, there is +almost certainly a bug in the code somewhere. In properly-written kernel +code, volatile can only serve to slow things down. + +Consider a typical block of kernel code: + +spin_lock(&the_lock); +do_something_on(&shared_data); +do_something_else_with(&shared_data); +spin_unlock(&the_lock); + +If all the code follows the locking rules, the value of shared_data cannot +change unexpectedly while the_lock is held. Any other code which might +want to play with that data will be waiting on the lock. The spinlock +primitives act as memory barriers - they are explicitly written to do so - +meaning that data accesses will not be optimized across them. So the +compiler might think it knows what will be in some_data, but the +spin_lock() call will force it to forget anything it knows. There will be +no optimization problems with accesses to that data. + +If shared_data were declared volatile, the locking would +still be necessary. But the compiler would also be prevented from +optimizing access to shared _within_ the critical section, +when we know that nobody else can be working with it. While the lock is +held, shared_data is not volatile. This is why Linus says: + + Also, more importantly, "volatile" is on the wrong _part_ of the + whole system. In C, it's "data" that is volatile, but that is + insane. Data isn't volatile - _accesses_ are volatile. So it may + make sense to say "make this particular _access_ be careful", but + not "make all accesses to this data use some random strategy". + +When dealing with shared data, proper locking makes volatile unnecessary - +and potentially harmful. + +The volatile storage class was originally meant for memory-mapped I/O +registers. Within the kernel, register accesses, too, should be protected +by locks, but one also does not want the compiler "optimizing" register +accesses within a critical section. But, within the kernel, I/O memory +accesses are always done through accessor functions; accessing I/O memory +directly through pointers is frowned upon and does not work on all +architectures. Those accessors are written to prevent unwanted +optimization, so, once again, volatile is unnecessary. + +Another situation where one might be tempted to use volatile is +when the processor is busy-waiting on the value of a variable. The right +way to perf
[PATCH] "volatile considered harmful", take 2
Who knew a documentation patch would get so many reviews? I like it... Anyway, here's a new version in which I attempt to respond to all the comments that came in. Thanks to everybody for looking it over. jon Steer developers away from the volatile type. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/volatile-considered-harmful.txt b/Documentation/volatile-considered-harmful.txt new file mode 100644 index 000..e67745f --- /dev/null +++ b/Documentation/volatile-considered-harmful.txt @@ -0,0 +1,118 @@ +Why the "volatile" type class should not be used + + +C programmers have often taken volatile to mean that the variable could be +changed outside of the current thread of execution; as a result, they are +sometimes tempted to use it in kernel code when shared data structures are +being used. In other words, they have been known to treat volatile types +as a sort of easy atomic variable, which they are not. The use of volatile in +kernel code is almost never correct; this document describes why. + +The key point to understand with regard to volatile is that its purpose is +to suppress optimization, which is almost never what one really wants to +do. In the kernel, one must protect shared data structures against +unwanted concurrent access, which is very much a different task. As it +happens, once the critical sections are properly implemented, the compiler +optimization issues which volatile was added to prevent will have been +taken care of in a more efficient way. + +Like volatile, the kernel primitives which make concurrent access to data +safe (spinlocks, mutexes, memory barriers, etc.) are designed to prevent +unwanted optimization. If they are being used properly, there will be no +need to use volatile as well. If volatile is still necessary, there is +almost certainly a bug in the code somewhere. In properly-written kernel +code, volatile can only serve to slow things down. + +Consider a typical block of kernel code: + +spin_lock(&the_lock); +do_something_on(&shared_data); +do_something_else_with(&shared_data); +spin_unlock(&the_lock); + +If all the code follows the locking rules, the value of shared_data cannot +change unexpectedly while the_lock is held. Any other code which might +want to play with that data will be waiting on the lock. The spinlock +primitives act as memory barriers - they are explicitly written to do so - +meaning that data accesses will not be optimized across them. So the +compiler might think it knows what will be in some_data, but the +spin_lock() call, since it acts as a memory barrier, will force it to +forget anything it knows. There will be no optimization problems with +accesses to that data. + +If shared_data were declared volatile, the locking would still be +necessary. But the compiler would also be prevented from optimizing access +to shared_data _within_ the critical section, when we know that nobody else +can be working with it. While the lock is held, shared_data is not +volatile. When dealing with shared data, proper locking makes volatile +unnecessary - and potentially harmful. + +The volatile storage class was originally meant for memory-mapped I/O +registers. Within the kernel, register accesses, too, should be protected +by locks, but one also does not want the compiler "optimizing" register +accesses within a critical section. But, within the kernel, I/O memory +accesses are always done through accessor functions; accessing I/O memory +directly through pointers is frowned upon and does not work on all +architectures. Those accessors are written to prevent unwanted +optimization, so, once again, volatile is unnecessary. + +Another situation where one might be tempted to use volatile is +when the processor is busy-waiting on the value of a variable. The right +way to perform a busy wait is: + +while (my_variable != what_i_want) +cpu_relax(); + +The cpu_relax() call can lower CPU power consumption or yield to a +hyperthreaded twin processor; it also happens to serve as a memory barrier, +so, once again, volatile is unnecessary. Of course, busy-waiting is +generally an anti-social act to begin with. + +There are still a few rare situations where volatile makes sense in the +kernel: + + - The above-mentioned accessor functions might use volatile on +architectures where direct I/O memory access does work. Essentially, +each accessor call becomes a little critical section on its own and +ensures that the access happens as expected by the programmer. + + - Inline assembly code which changes memory, but which has no other +visible side effects, risks being deleted by GCC. Adding the volatile +keyword to asm statements will prevent this removal. + + - The jiffies variable is special in that it can have a different value +every time it is referenced, but it ca
[PATCH] "volatile considered harmful", take 3
Here's another version of the volatile document. Once again, I've tried to address all of the comments. There haven't really been any recent comments addressing the correctness of the document; people have been more concerned with how it's expressed. I'm glad to see files in Documentation/ held to a high standard of writing, but, unless somebody has a factual issue this time around I would like to declare Mission Accomplished and move on. Thanks, jon --- Encourage developers to avoid the volatile type class in kernel code. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/volatile-considered-harmful.txt b/Documentation/volatile-considered-harmful.txt new file mode 100644 index 000..955c309 --- /dev/null +++ b/Documentation/volatile-considered-harmful.txt @@ -0,0 +1,119 @@ +Why the "volatile" type class should not be used + + +C programmers have often taken volatile to mean that the variable could be +changed outside of the current thread of execution; as a result, they are +sometimes tempted to use it in kernel code when shared data structures are +being used. In other words, they have been known to treat volatile types +as a sort of easy atomic variable, which they are not. The use of volatile in +kernel code is almost never correct; this document describes why. + +The key point to understand with regard to volatile is that its purpose is +to suppress optimization, which is almost never what one really wants to +do. In the kernel, one must protect shared data structures against +unwanted concurrent access, which is very much a different task. The +process of protecting against unwanted concurrency will also avoid almost +all optimization-related problems in a more efficient way. + +Like volatile, the kernel primitives which make concurrent access to data +safe (spinlocks, mutexes, memory barriers, etc.) are designed to prevent +unwanted optimization. If they are being used properly, there will be no +need to use volatile as well. If volatile is still necessary, there is +almost certainly a bug in the code somewhere. In properly-written kernel +code, volatile can only serve to slow things down. + +Consider a typical block of kernel code: + +spin_lock(&the_lock); +do_something_on(&shared_data); +do_something_else_with(&shared_data); +spin_unlock(&the_lock); + +If all the code follows the locking rules, the value of shared_data cannot +change unexpectedly while the_lock is held. Any other code which might +want to play with that data will be waiting on the lock. The spinlock +primitives act as memory barriers - they are explicitly written to do so - +meaning that data accesses will not be optimized across them. So the +compiler might think it knows what will be in shared_data, but the +spin_lock() call, since it acts as a memory barrier, will force it to +forget anything it knows. There will be no optimization problems with +accesses to that data. + +If shared_data were declared volatile, the locking would still be +necessary. But the compiler would also be prevented from optimizing access +to shared_data _within_ the critical section, when we know that nobody else +can be working with it. While the lock is held, shared_data is not +volatile. When dealing with shared data, proper locking makes volatile +unnecessary - and potentially harmful. + +The volatile storage class was originally meant for memory-mapped I/O +registers. Within the kernel, register accesses, too, should be protected +by locks, but one also does not want the compiler "optimizing" register +accesses within a critical section. But, within the kernel, I/O memory +accesses are always done through accessor functions; accessing I/O memory +directly through pointers is frowned upon and does not work on all +architectures. Those accessors are written to prevent unwanted +optimization, so, once again, volatile is unnecessary. + +Another situation where one might be tempted to use volatile is +when the processor is busy-waiting on the value of a variable. The right +way to perform a busy wait is: + +while (my_variable != what_i_want) +cpu_relax(); + +The cpu_relax() call can lower CPU power consumption or yield to a +hyperthreaded twin processor; it also happens to serve as a memory barrier, +so, once again, volatile is unnecessary. Of course, busy-waiting is +generally an anti-social act to begin with. + +There are still a few rare situations where volatile makes sense in the +kernel: + + - The above-mentioned accessor functions might use volatile on +architectures where direct I/O memory access does work. Essentially, +each accessor call becomes a little critical section on its own and +ensures that the access happens as expected by the programmer. + + - Inline assembly code which changes memory, but which has no other +vis
Re: [PATCH RFC] [1/9] Core module symbol namespaces code and intro.
Rusty said: > Well, introduce an EXPORT_SYMBOL_INTERNAL(). It's a lot less code. But > you'd > still need to show that people are having trouble knowing what APIs to use. Might the recent discussion on the exporting of sys_open() and sys_read() be an example here? There would appear to be a consensus that people should not have used those functions, but they are now proving difficult to unexport. Perhaps the best use of the namespace stuff might be for *future* exports which are needed to help make the mainline kernel fully modular, but which are not meant to be part of a more widely-used API? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] New kobject/kset/ktype documentation and example code
Greg KH <[EMAIL PROTECTED]> wrote: > Jonathan, I used your old lwn.net article about kobjects as the basis > for this document, I hope you don't mind Certainly I have no objections, I'm glad it was useful. A few little things... > It is rare (even unknown) for kernel code to create a standalone kobject; > with one major exception explained below. You don't keep this promise - bet you thought we wouldn't notice... Actually I guess you do, in the "creating simple kobjects" section. When you get to that point, you should mention that this is a situation where standalone kobjects make sense. Given that there are quite a few standalone kobjects created by this patch set (kernel_kobj, security_kobj, s390_kobj, etc.), the "(even unknown)" should probably come out. > So, for example, UIO code has a structure that defines the memory region > associated with a uio device: *The* UIO code, presumably. > the given type. So, for example, a pointer to a struct kobject embedded > within a struct cdev called "kp" could be converted to a pointer to the > containing structure with: That should be "struct uio_mem", I think. > one. Calling kobject_init() is not sufficient, however. Kobject users > must, at a minimum, set the name of the kobject; this is the name that will > be used in sysfs entries. Is setting the name mandatory now, or are there still places where kobjects (which do not appear in sysfs) do have - and do not need - a name? > Because kobjects are dynamic, they must not be declared statically or on > the stack, but instead, always from the heap. Future versions of the "always be allocated from the heap"? > "empty" release function, you will be mocked merciously by the kobject > maintainer if you attempt this. So just how should severely should we mock kobject maintainers who can't spell "mercilessly"? :) > - A kset can provide a set of default attributes that all kobjects that >belong to it automatically inherit and have created whenever a kobject >is registered belonging to the kset. Can we try that one again? - A kset can provide a set of default attributes for all kobjects which belong to it. > There is currently > no other way to add a kobject to a kset without directly messing with the > list pointers. Presumably the latter way is not recommended; I would either say so or not mention this possibility at all. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [PATCH] A clean approach to writeout throttling
Hey, Daniel,
I'm just getting around to looking at this. One thing jumped out at me:
> + if (bio->bi_throttle) {
> + struct request_queue *q = bio->bi_queue;
> + bio->bi_throttle = 0; /* or detect multiple endio and err? */
> + atomic_add(bio->bi_throttle, &q->available);
> + wake_up(&q->throttle_wait);
> + }
I'm feeling like I must be really dumb, but...how can that possibly
work? You're zeroing >bi_throttle before adding it back into
q->available, so the latter will never increase...
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Extend sys_clone and sys_unshare system calls API
Hi, Pavel, [Adding Ulrich] > I use the last bit in the clone_flags for CLONE_LONGARG. When set it > will denote that the child_tidptr is not a pointer to a tid storage, > but the pointer to the struct long_clone_struct which currently > looks like this: I'm probably just totally off the deep end, but something did occur to me: this looks an awful lot like a special version of the sys_indirect() idea. Unless it has been somehow decided that sys_indirect() is the wrong idea, might it not be better to look at making that interface solve the extended clone() problem as well? jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Extending syscalls (was: [PATCH 1/2] Extend sys_clone and sys_unshare system calls API)
Al Viro sez: > Nah, just put an XML parser into the kernel to have the form match the > contents... > > Al "perhaps we should newgroup alt.tasteless.api for all that stuff" Viro Heh, indeed. But we do seem to have a recurring problem of people wanting to extend sys_foo() beyond the confines of its original API. I've observed a few ways of doing that: - create sys_foo2() (or sys_foo64(), or sys_fooat(), or sys_pfoo(), or...) and add the new stuff there. - Put a version number into the API somewhere - wireless extensions, for example. - Set a flag saying "I've stashed some additional parameters somewhere else." That's sys_indirect() and the current proposal for extending clone(). - Just do it all with a kernel-based XML parser. I think we should call this approach sys_viro() in honor of its champion. - Do it all in sysfs The first approach has traditionally been the most popular. If we have a consensus that this is the way to extend system calls in the future, it would be nice to set that down somewhere. We could avoid a lot of API blind alleys that way. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Patch tags [was writeout stalls in current -git]
Adrian Bunk <[EMAIL PROTECTED]> wrote: > What's missing is a definition which of them are formal tags that must > be explicitely given (look at point 13 in SubmittingPatches). > > Signed-off-by: and Reviewed-by: are the formal tags someone must have > explicitely given and that correspond to some statement. > > OTOH, I can translate a "sounds fine" or "works for me" someone else > gave me into an Acked-by: resp. Tested-by: tag. The discussion of the Cc: tag says: This is the only tag which might be added without an explicit action by the person it names. I think that addresses your comment, no? Certainly I wouldn't feel that I could add any of the other tags to a patch I posted - that's the job of the person named there. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC/PATCH 2/8] revoke: inode revoke lock V7
This is a relatively minor detail in the rather bigger context of this
patch, but...
> @@ -642,6 +644,7 @@ struct inode {
> struct list_headinotify_watches; /* watches on this inode */
> struct mutexinotify_mutex; /* protects the watches list */
> #endif
> + wait_queue_head_t i_revoke_wait;
That seems like a relatively hefty addition to every inode in the system
when revoke - I think - will be a fairly rare operation. Would there be
any significant cost to using a single, global revoke-wait queue instead
of growing the inode structure?
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RFC: reviewer's statement of oversight
Last month, at the kernel summit, there was discussion of putting a Reviewed-by: tag onto patches to document the oversight they had received on their way into the mainline. That tag has made an occasional appearance since then, but there has not yet been a discussion of what it really means. So it has not yet brought a whole lot of value to the process. As I was trying to sleep last night, it occurred to me that what we might need is an equivalent of the DCO for the Reviewed-by tag. To that end, I dedicated a few minutes of my life to the following bit of text. It's really just meant to be a starting point for the discussion. Is the following something close to what we understand Reviewed-by to mean? jon Reviewer's statement of oversight v0.01 By offering my Reviewed-by: tag, I state that: (a) I have carried out a technical review of this patch to evaluate its appropriateness and readiness for inclusion into the mainline kernel. (b) Any problems, concerns, or questions relating to the patch have been communicated back to the submitter. I am satisfied with how the submitter has responded to my comments. (c) While there may (or may not) be things which could be improved with this submission, I believe that it is, at this time, (1) a worthwhile addition to the kernel, and (2) free of serious known issues which would argue against its inclusion. (d) While I have reviewed the patch and believe it to be sound, I can not (unless explicitly stated elsewhere) make any warranties or guarantees that it will achieve its stated purpose or function properly in any given situation. (e) I understand and agree that this project and the contribution are public and that a record of the contribution (including my Reviewed-by tag and any associated public communications) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC: reviewer's statement of oversight
Al Viro <[EMAIL PROTECTED]> wrote: > > A patch which is not "worthwhile" is also not "appropriate". Mere > > correctness in a mathematical sense is not enough as technical review > > criterion. > > Yes, but there's also such thing as "worthwhile removal". Good point. So the text should probably say "worthwhile change" rather than "worthwhile addition." I do believe that thinking about whether the change as a whole is a desirable thing is an important part of the review process. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC: reviewer's statement of oversight
Sam Ravnborg <[EMAIL PROTECTED]> wrote: > Or maybe we need something much less formal that explain the purpose of the > four tags we use: ...or maybe a combination? How does the following patch look as a way to describe how the tags are used and what Reviewed-by, in particular, means? Perhaps the DCO should move to this file as well? jon --- Add a document on patch tags. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 43e89b1..fa1518b 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -284,6 +284,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 000..fb5f8e1 --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,66 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) its progress. All of these +tags have the form: + + Something-done-by: Full name <[EMAIL PROTECTED]> + +These tags are: + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch is, to the best of his or her knowledge, legally able + to be merged into the mainline and distributed under the + terms of the GNU General Public License, version 2. See + the Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion. An Acked-by tag does not + imply any involvement in the development of the patch or + that a detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptible according + to the Reviewer's Statement as found at the bottom of this + file. A Reviewed-by tag is a statement of opinion that the + patch is an appropriate modification of the kernel without + any remaining serious technical issues. Any interested + reviewer (who has done the work) can offer a Reviewed-by + tag for a patch. + +Cc:The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. + + + + +Reviewer's statement of oversight, v0.02 + +By offering my Reviewed-by: tag, I state that: + + (a) I have carried out a technical review of this patch to evaluate its + appropriateness and readiness for inclusion into the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch have been + communicated back to the submitter. I am satisfied with how the + submitter has responded to my comments. + + (c) While there may (or may not) be things which could be improved with + this submission, I believe that it is, at this time, (1) a worthwhile + modification to the kernel, and (2) free of known issues which would + argue against its inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I can not + (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation. + + (e) I understand and agree that this project and the contribution are + public and that a record of the contribution (including my Reviewed-by + tag and any associated public communications) is maintained + indefinitely and may be redistributed consistent with this project or + the open source license(s) involved. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC: reviewer's statement of oversight
Neil Brown <[EMAIL PROTECTED]> wrote: > I find it is always good to know *why* we have the tags. That > information is a useful complement to what they mean, and can guide > people in adding them. Hmm...I was just going to go with the "because I told you so" approach that I use with my kids. It works so well with them after all. I agree with just about everything you've said, and am tweaking things accordingly. But... > > + (b) Any problems, concerns, or questions relating to the patch have been > > + communicated back to the submitter. I am satisfied with how the > > + submitter has responded to my comments. > > This seems more detailed that necessary. The process (communicated > back / responded) is not really relevant. Instead, it seems to me that the process is crucially important. Reviewed-by shouldn't be a rubber stamp that somebody applies to a patch; I think it should really imply that issues of interest have been communicated to the developers. If we are setting expectations for what Reviewed-by means, I would prefer to leave an explicit mention of communication in there. If I'm in the minority here, though, it can certainly come out. Thanks, jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Documentation/patch-tags V2
Here's a new version of the patch-tags document. I've incorporated comments from Randy Dunlap, Stefan Richter, and Neil Brown, though I have retained, for now, the more verbose process discussion that Neil didn't like. Comments? jon -- Document the tags used with kernel patches Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 43e89b1..fa1518b 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -284,6 +284,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 000..d955fa2 --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,81 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) its progress. All of these +tags have the form: + + Something-done-by: Full name <[EMAIL PROTECTED]> [optional random stuff] + +These tags are: + +From: The original author of the patch. This tag will ensure + that credit is properly given when somebody other than the + original author submits the patch. + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch is, to the best of his or her knowledge, legally able + to be merged into the mainline and distributed under the + terms of the GNU General Public License, version 2. See + the Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. This tag assures upstream maintainers that + the provenance of the patch is known and allows the origin + of the patch to be reviewed should copyright questions + arise. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion; it informs upstream + maintainers that a certain degree of consensus on the patch + as been achieved.. An Acked-by tag does not imply any + involvement in the development of the patch or that a + detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptable according + to the Reviewer's Statement as found at the bottom of this + file. A Reviewed-by tag is a statement of opinion that the + patch is an appropriate modification of the kernel without + any remaining serious technical issues. Any interested + reviewer (who has done the work) can offer a Reviewed-by + tag for a patch. This tag serves to give credit to + reviewers and to inform maintainers of the degree of review + which has been done on the patch. + +Cc:The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. This + tag documents that potentially interested parties have been + included in the discussion. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. This tag informs + maintainers that some testing has been performed and + ensures credit for the testers. + + + + +Reviewer's statement of oversight, v0.02 + +By offering my Reviewed-by: tag, I state that: + + (a) I have carried out a technical review of this patch to evaluate its + appropriateness and readiness for inclusion into the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch have been + communicated back to the submitter. I am satisfied with how the + submitter has responded to my comments. + + (c) While there may (or may not) be things which could be improved with + this submission, I believe that it is, at this time, (1) a worthwhile + modification to the kernel, and (2) free of known issues which would + argue against its inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I cannot + (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation. + + (e) I understand and agree that this project and the contribution are + public and that a record of the contribution (including my Revie
[PATCH] Documentation/patch-tags v3
Here's the current version of the patch-tags document; I've made changes in response to comments from a Randy Dunlap and Scott Preece. The only substantive change, though, is the removal of the privacy term, since it really does seem to be overkill. It can come back if it turns out to be truly needed. Andrew, you've been quiet on this. Does it correspond to your notion of what Reviewed-by should mean? jon diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 43e89b1..fa1518b 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -284,6 +284,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 000..6acde5e --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,76 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) their progress. All of +these tags have the form: + + Something-done-by: Full name <[EMAIL PROTECTED]> [optional random stuff] + +These tags are: + +From: The original author of the patch. This tag will ensure + that credit is properly given when somebody other than the + original author submits the patch. + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch is, to the best of his or her knowledge, legally able + to be merged into the mainline and distributed under the + terms of the GNU General Public License, version 2. See + the Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. This tag assures upstream maintainers that + the provenance of the patch is known and allows the origin + of the patch to be reviewed should copyright questions + arise. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion; it informs upstream + maintainers that a certain degree of consensus on the patch + as been achieved.. An Acked-by tag does not imply any + involvement in the development of the patch or that a + detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptable according + to the Reviewer's Statement as found at the bottom of this + file. A Reviewed-by tag is a statement of opinion that the + patch is an appropriate modification of the kernel without + any remaining serious technical issues. Any interested + reviewer (who has done the work) can offer a Reviewed-by + tag for a patch. This tag serves to give credit to + reviewers and to inform maintainers of the degree of review + which has been done on the patch. + +Cc:The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. This + tag documents that potentially interested parties have been + included in the discussion. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. This tag informs + maintainers that some testing has been performed, provides + a means to locate testers for future patches, and ensures + credit for the testers. + + + + +Reviewer's statement of oversight + +By offering my Reviewed-by: tag, I state that: + + (a) I have carried out a technical review of this patch to evaluate its + appropriateness and readiness for inclusion into the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch have been + communicated back to the submitter. I am satisfied with the + submitter's response to my comments. + + (c) While there may be things that could be improved with this submission, + I believe that it is, at this time, (1) a worthwhile modification to + the kernel, and (2) free of known issues which would argue against its + inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I do not + (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation. - To unsubscribe from this list: send the line "unsubscribe lin
Re: [PATCH] cafe_ccic: default to allocating DMA buffers at probe time
Andres Salomon <[EMAIL PROTECTED]> wrote: > This patch makes DMA buffer allocation happen during device probe by > default, and changes the parameter to 'alloc_bufs_at_read'. The > camera hardware is there, if the cafe_ccic driver is enabled/loaded it > should do its best to ensure that the camera is actually usable; > delaying DMA buffer allocation saves an insignicant amount of memory, > and causes the driver to be much less useful. The amount of memory isn't quite "insignificant" (three 640x480x2 buffers), but the OLPC people clearly need things to work this way. There are, as far as I know, no other systems out there using the CAFE controller. Making things work by default for the one user (with a fair number of deployed systems!) makes sense to me. So... Acked-by: Jonathan Corbet <[EMAIL PROTECTED]> That said, I do prefer the original name for the parameter. jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time
Andres Salomon <[EMAIL PROTECTED]> wrote: > That said, I'm not opposed to keeping the parameter name the same while > making the default 1; I just thought that the name 'alloc_bufs_at_read' was > clearer. Another option is to change it to 'no_alloc_bufs_at_load'. Jon, > any preference there? I don't think that negating it by adding no_ at the front helps much. In general, I prefer the name it has now, but it's not *that* big a deal. We've probably already expended more bandwidth than it's worth. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] module: implement module_inhibit_unload()
Hi, Tejun, I was just looking over these changes... > + /* Don't proceed till inhibition is lifted. */ > + add_wait_queue(&module_unload_wait, &wait); > + set_current_state(TASK_UNINTERRUPTIBLE); > + if (atomic_read(&module_unload_inhibit_cnt)) > + schedule(); > + __set_current_state(TASK_RUNNING); > + remove_wait_queue(&module_unload_wait, &wait); > + > + mutex_lock(&module_mutex); Maybe I'm missing something, but this looks racy to me. There's no check after schedule() to see if module_unload_inhibit_cnt is really zero, and nothing to keep somebody else from slipping in and raising it again afterward. Given your description of this tool as a "sledgehammer," might it not be easier to just take and hold module_mutex for the duration of the unload block? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/4] new timerfd API v2 - new timerfd API
One quick question: > Like the previous timerfd API implementation, read(2) and poll(2) are > supported > (with the same interface). Looking at that interface, it appears that a process doing a read() on a timerfd with no timer set will block for a very long time. It's an obvious "don't do that" situation, but perhaps we could help an occasional developer get a clue by returning something like -EINVAL when the timer has not been set? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Revised timerfd() interface
Sorry for the late commentary...as I looked this over, one thing popped into my mind > b) Make the 'clockid' immutable: it can only be set >if 'ufd' is -1 -- that is, on the timerfd() call that >first creates a timer. timerfd() is looking increasingly like a multiplexor system call in disguise. There is no form of invocation which uses all of the arguments. Are we sure we wouldn't rather have something like: timerfd_open(clock); timerfd_adjust(fd, new_time, old_time); ? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH/RFC] msleep() with hrtimers
The OLPC folks and I recently discovered something interesting: on a
HZ=100 system, a call to msleep(1) will delay for about 20ms. The
combination of jiffies timekeeping and rounding up means that the
minimum delay from msleep will be two jiffies, never less. That led to
multi-second delays in a driver which does a bunch of short msleep()
calls and, in response, a change to mdelay(), which will come back in
something closer to the requested time.
Here's another approach: a reimplementation of msleep() and
msleep_interruptible() using hrtimers. On a system without real
hrtimers this code will at least drop down to single-jiffy delays much
of the time (though not deterministically so). On my x86_64 system with
Thomas's hrtimer/dyntick patch applied, msleep(1) gives almost exactly
what was asked for.
jon
Use hrtimers for msleep() and msleep_interruptible()
Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]>
--- 2.6.22-vanilla/kernel/timer.c 2007-07-15 13:08:36.0 -0600
+++ 2.6.22-timer/kernel/timer.c 2007-07-15 16:13:34.0 -0600
@@ -1522,18 +1522,43 @@ unregister_time_interpolator(struct time
}
#endif /* CONFIG_TIME_INTERPOLATION */
+
+
+
+static void do_msleep(unsigned int msecs, struct hrtimer_sleeper *sleeper,
+ int sigs)
+{
+ enum hrtimer_mode mode = HRTIMER_MODE_REL;
+ int state = sigs ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
+
+ /*
+* This is really just a reworked and simplified version
+* of do_nanosleep().
+*/
+ hrtimer_init(&sleeper->timer, CLOCK_MONOTONIC, mode);
+ sleeper->timer.expires = ktime_set(0, msecs*NSEC_PER_MSEC);
+ hrtimer_init_sleeper(sleeper, current);
+
+ do {
+ set_current_state(state);
+ hrtimer_start(&sleeper->timer, sleeper->timer.expires, mode);
+ if (sleeper->task)
+ schedule();
+ hrtimer_cancel(&sleeper->timer);
+ mode = HRTIMER_MODE_ABS;
+ } while (sleeper->task && !(sigs && signal_pending(current)));
+}
+
/**
* msleep - sleep safely even with waitqueue interruptions
* @msecs: Time in milliseconds to sleep for
*/
void msleep(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+ struct hrtimer_sleeper sleeper;
- while (timeout)
- timeout = schedule_timeout_uninterruptible(timeout);
+ do_msleep(msecs, &sleeper, 0);
}
-
EXPORT_SYMBOL(msleep);
/**
@@ -1542,11 +1567,15 @@ EXPORT_SYMBOL(msleep);
*/
unsigned long msleep_interruptible(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+ struct hrtimer_sleeper sleeper;
+ ktime_t left;
- while (timeout && !signal_pending(current))
- timeout = schedule_timeout_interruptible(timeout);
- return jiffies_to_msecs(timeout);
-}
+ do_msleep(msecs, &sleeper, 1);
+ if (! sleeper.task)
+ return 0;
+ left = ktime_sub(sleeper.timer.expires,
+sleeper.timer.base->get_time());
+ return max(((long) ktime_to_ns(left))/NSEC_PER_MSEC, 1L);
+}
EXPORT_SYMBOL(msleep_interruptible);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC] msleep() with hrtimers
Hey, Roman, > One possible problem here is that setting up that timer can be > considerably more expensive, for a relative timer you have to read the > current time, which can be quite expensive (e.g. your machine now uses the > PIT timer, because TSC was deemed unstable). That's a possibility, I admit I haven't benchmarked it. I will say that I don't think it will be enough to matter - msleep() is not a hot-path sort of function. Once the system is up and running it almost never gets called at all - at least, on my setup. > One question here would be, is it really a problem to sleep a little more? "A little more" is a bit different than "twenty times as long as you asked for." That "little bit more" added up to a few seconds when programming a device which needs a brief delay after tweaking each of almost 200 registers. > BTW there is another thing to consider. If you already run with hrtimer/ > dyntick, there is not much reason to keep HZ at 100, so you could just > increase HZ to get the same effect. Except that then, with the current implementation, you're paying for the higher HZ whenever the CPU is busy. I bet that doesn't take long to overwhelm any added overhead in the hrtimer msleep(). In the end, I did this because I thought msleep() should do what it claims to do, because I thought that getting a known-to-expire timeout off the timer wheel made sense, and to make a tiny baby step in the direction of reducing the use of jiffies in the core code. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC] msleep() with hrtimers
Roman Zippel <[EMAIL PROTECTED]> wrote: > > That's a possibility, I admit I haven't benchmarked it. I will say that > > I don't think it will be enough to matter - msleep() is not a hot-path > > sort of function. Once the system is up and running it almost never > > gets called at all - at least, on my setup. > > That's a bit my problem - we have to consider other setups as well. > Is it worth converting all msleep users behind their back or should we > just a provide a separate function for those who care? Any additional overhead is clearly small - enough not to disrupt a *high resolution* timer, after all. And msleep() is used mostly during initialization time. My box had a few hundred calls, almost all during boot. Any cost will be bounded by the fact that, well, it sleeps for milliseconds on every call. I could run a million-msleep profile if you really want, but I just can't imagine it's going to have a measurable effect on any real-world situation. Is there a specific setup you're worried about? I'm not *that* attached to this patch; if it causes heartburn we can just forget it. But I had thought it might be useful... > Which driver is this? I'd like to look at this, in case there's some other > hidden problem. drivers/media/video/cafe_ccic.c, and cafe_smbus_write_data() in particular. The "hidden problem," though, is that the hardware has periods where reading the status registers will send it off into its room where it will hide under its bed and never come out. I have an alternative which avoids the sleep at the cost of a small, relatively harmless race; it may be my real solution in the end, we'll see. > > Except that then, with the current implementation, you're paying for the > > higher HZ whenever the CPU is busy. I bet that doesn't take long to > > overwhelm any added overhead in the hrtimer msleep(). > > Actually if that's the case I'd consider this a bug, where is that extra > cost coming from? My understanding is that the current dyntick code only turns off the tick during idle periods; while things are running it's business as usual. Perhaps I misunderstood? jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Documentation: improvement to volatile considered harmful (resubmit)
Heikki Orsila <[EMAIL PROTECTED]> wrote: > I'm resubmitting this as I didn't get any replies, this time CCeing > proper people, sorry.. > > Kernel locking/synchronization primitives are better than volatile types > from code readability point of view also. I think that just dilutes the real point. It's not a choice between locking and volatile - the locking must be there regardless. It's a correctness issue; if the result happens to be more readable too that's a bonus. If somebody wants to put this sentence in I won't object, but I don't think it really improves the document either. Thanks, jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add seq_file howto to Documentation/
> +[Another seq_file reference is "Driver porting: The seq_file interface" > +at <http://lwn.net/Articles/22355/>, which is part of the > +LWN.net series "Porting Drivers to 2.5" that is located at > +<http://lwn.net/Articles/driver-porting/>.] Funny thing, that...I had sent in a version of that document for consideration back in 2003. Guess that was still the Good Old Days when the patch management system was rather lossier, so I never heard back. Here it is again just in case anybody's interested. In general, I'm more than happy to have anything I put on LWN go into Documentation/ if there's interest - just say the word. jon To: linux-kernel@vger.kernel.org Subject: [PATCH] seq_file documentation cc: [EMAIL PROTECTED] From: Jonathan Corbet <[EMAIL PROTECTED]> Date: Thu, 13 Nov 2003 10:16:31 -0700 Sender: corbet I went ahead and packaged up my seq_file document as a basic text file, in the interests of getting something out there. jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] diff -urN test9-vanilla/Documentation/filesystems/seq_file.txt test9/Documentation/filesystems/seq_file.txt --- test9-vanilla/Documentation/filesystems/seq_file.txtWed Dec 31 17:00:00 1969 +++ test9/Documentation/filesystems/seq_file.txtFri Nov 14 01:03:58 2003 @@ -0,0 +1,268 @@ +The seq_file interface + + Copyright 2003 Jonathan Corbet <[EMAIL PROTECTED]> + This file is originally from the LWN.net Driver Porting series at + http://lwn.net/Articles/driver-porting/ + + +There are numerous ways for a device driver (or other kernel component) to +provide information to the user or system administrator. One very useful +technique is the creation of virtual files, in /proc or elsewhere. Virtual +files can provide human-readable output that is easy to get at without any +special utility programs; they can also make life easier for script +writers. It is not surprising that the use of virtual files has grown over +the years. + +Creating those files correctly has always been a bit of a challenge, +however. It is not that hard to make a /proc file which returns a +string. But life gets trickier if the output is long - anything greater +than an application is likely to read in a single operation. Handling +multiple reads (and seeks) requires careful attention to the reader's +position within the virtual file - that position is, likely as not, in the +middle of a line of output. The kernel is full of /proc file +implementations that get this wrong. + +The 2.6 kernel contains a set of functions (implemented by Alexander Viro) +which are designed to make it easy for virtual file creators to get it +right. This interface (called "seq_file") is not strictly a 2.6 feature - +it was also merged into 2.4.15. + +The seq_file interface is available via . There are +three aspects to seq_file: + + * An iterator interface which lets a virtual file implementation + step through the objects it is presenting. + + * Some utility functions for formatting objects for output without + needing to worry about things like output buffers. + + * A set of canned file_operations which implement most operations on + the virtual file. + +We'll look at the seq_file interface via an extremely simple example: a +loadable module which creates a file called /proc/sequence. The file, when +read, simply produces a set of increasing integer values, one per line. The +sequence will continue until the user loses patience and finds something +better to do. The file is seekable, in that one can do something like the +following: + +dd if=/proc/sequence of=out1 count=1 +dd if=/proc/sequence skip=1 out=out2 count=1 + +Then concatenate the output files out1 and out2 and get the right +result. Yes, it is a thoroughly useless module, but the point is to show +how the mechanism works without getting lost in other details. (Those +wanting to see the full source for this module can find it at +http://lwn.net/Articles/22359/). + + +The iterator interface + +Modules implementing a virtual file with seq_file must implement a simple +iterator object that allows stepping through the data of +interest. Iterators must be able to move to a specific position - like the +file they implement - but the interpretation of that position is up to the +iterator itself. A seq_file implementation that is formatting firewall +rules, for example, could interpret position N as the Nth rule in the +chain. Positioning can thus be done in whatever way makes the most sense +for the generator of the data, which need not be aware of how a position +translates to an offset in the virtual file. The one obvious exception is +that a position of zero should indicate the beginning of the file. + +The /proc/sequence iterator just uses the count of the next number it +will output as its position. + +Four functions must be implem
Re: [PATCH 1/7] lguest: documentation pt I: Preparation
> Items for consideration would be: > > - if this stuff is good, shouldn't other code be using it? If so, is > this new infrastructure in the correct place? > > - if, otoh, this infrastructure is _not_ suitable for other code, well, > what was wrong with it? > > - if the requirement is good, perhaps alternative implementations should > be explored (dunno what). Well, we could just rewrite the whole kernel in Web and judge all future patches on how literate they are... Seriously, though, even if the (small) infrastructure is not going to take over the driver tree anytime soon, it's hard to imagine why we would not want to incorporate this sort of documentation. It's good stuff, something which can really help people get a sense for how this whole virtualization thing works. And it makes the code fun to read. jon P.S. I am currently considering a no-limericks policy for the LWN quote of the week. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 04/22] CRED: Request a credential record for a kernel service
Hi, David, One little thing I noticed: > + * @cred_kernel_act_as: > + * Set the credentials for a kernel service to act as (subjective context). > + * @cred points to the credentials structure to be filled in. > + * @service names the service making the request. > + * @daemon: A userspace daemon to be used as a base for the context. > + * @dentry: A file or dir to be used as a base for the file creation > + * context. > + * Return 0 if successful. The comment describes a "dentry" argument, but the actual function does not have that argument. jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: A unresponsive file system can hang all I/O in the system on linux-2.6.23-rc6 (dirty_thresh problem?)
Andrew wrote: > It's unrelated to the actual value of dirty_thresh: if the machine fills up > with dirty (or unstable) NFS pages then eventually new writers will block > until that condition clears. > > 2.4 doesn't have this problem at low levels of dirty data because 2.4 > VFS/MM doesn't account for NFS pages at all. Is it really NFS-related? I was trying to back up my 2.6.23-rc8 system to an external USB drive the other day when something flaked and the drive fell off the bus. That, too, was sufficient to wedge the entire system, even though the only thing which needed the dead drive was one rsync process. It's kind of a bummer to have to hit the reset button after the failure of (what should be) a non-critical piece of hardware. Not that I have a fix to propose...:) jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] msleep() with hrtimers
Here's the second (and probably final) posting of the msleep() with
hrtimers patch. The problem being addressed here is that the current
msleep() will stop for a minimum of two jiffies, meaning that, on a
HZ=100 system, msleep(1) delays for for about 20ms. In a driver with
one such delay for each of 150 or so register setting operations, the
extra time adds up to a few seconds.
This patch addresses the situation by using hrtimers. On tickless
systems with working timers, msleep(1) now sleeps for 1ms, even with
HZ=100.
Most comments last time were favorable. The one dissenter was Roman,
who worries about the overhead of using hrtimers for this operation; my
understanding is that he would rather see a really_msleep() function for
those who actually want millisecond resolution. I'm not sure how to
characterize what the cost could be, but it can only be buried by the
fact that every call sleeps for some number of milliseconds. On my
system, the several hundred total msleep() calls can't cause any real
overhead, and almost all happen at initialization time.
I still think it would be useful for msleep() to do what it says it does
and not vastly oversleep with small arguments. A quick grep turns up
450 msleep(1) calls in the current mainline. Andrew, if you agree, can
you drop this into -mm? If not, I guess I'll let it go.
jon
---
Use hrtimers so that msleep() sleeps for the requested time
Current msleep() snoozes for at least two jiffies, causing msleep(1) to
sleep for at least 20ms on HZ=100 systems. Using hrtimers allows
msleep() to sleep for something much closer to the requested time.
Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]>
--- 2.6.23-rc1/kernel/timer.c.orig 2007-08-02 13:45:20.0 -0600
+++ 2.6.23-rc1/kernel/timer.c 2007-08-03 12:34:48.0 -0600
@@ -1349,18 +1349,43 @@ void __init init_timers(void)
open_softirq(TIMER_SOFTIRQ, run_timer_softirq, NULL);
}
+
+
+
+static void do_msleep(unsigned int msecs, struct hrtimer_sleeper *sleeper,
+ int sigs)
+{
+ enum hrtimer_mode mode = HRTIMER_MODE_REL;
+ int state = sigs ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
+
+ /*
+* This is really just a reworked and simplified version
+* of do_nanosleep().
+*/
+ hrtimer_init(&sleeper->timer, CLOCK_MONOTONIC, mode);
+ sleeper->timer.expires = ktime_set(0, msecs*NSEC_PER_MSEC);
+ hrtimer_init_sleeper(sleeper, current);
+
+ do {
+ set_current_state(state);
+ hrtimer_start(&sleeper->timer, sleeper->timer.expires, mode);
+ if (sleeper->task)
+ schedule();
+ hrtimer_cancel(&sleeper->timer);
+ mode = HRTIMER_MODE_ABS;
+ } while (sleeper->task && !(sigs && signal_pending(current)));
+}
+
/**
* msleep - sleep safely even with waitqueue interruptions
* @msecs: Time in milliseconds to sleep for
*/
void msleep(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+ struct hrtimer_sleeper sleeper;
- while (timeout)
- timeout = schedule_timeout_uninterruptible(timeout);
+ do_msleep(msecs, &sleeper, 0);
}
-
EXPORT_SYMBOL(msleep);
/**
@@ -1369,11 +1394,15 @@ EXPORT_SYMBOL(msleep);
*/
unsigned long msleep_interruptible(unsigned int msecs)
{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+ struct hrtimer_sleeper sleeper;
+ ktime_t left;
- while (timeout && !signal_pending(current))
- timeout = schedule_timeout_interruptible(timeout);
- return jiffies_to_msecs(timeout);
-}
+ do_msleep(msecs, &sleeper, 1);
+ if (!sleeper.task)
+ return 0;
+ left = ktime_sub(sleeper.timer.expires,
+sleeper.timer.base->get_time());
+ return max(((long) ktime_to_ns(left))/NSEC_PER_MSEC, 1L);
+}
EXPORT_SYMBOL(msleep_interruptible);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Avoid buffer overflows in get_user_pages()
Avoid buffer overflows in get_user_pages()
So I spent a while pounding my head against my monitor trying to figure
out the vmsplice() vulnerability - how could a failure to check for
*read* access turn into a root exploit? It turns out that it's a buffer
overflow problem which is made easy by the way get_user_pages() is
coded.
In particular, "len" is a signed int, and it is only checked at the
*end* of a do {} while() loop. So, if it is passed in as zero, the loop
will execute once and decrement len to -1. At that point, the loop will
proceed until the next invalid address is found; in the process, it will
likely overflow the pages array passed in to get_user_pages().
I think that, if get_user_pages() has been asked to grab zero pages,
that's what it should do. Thus this patch; it is, among other things,
enough to block the (already fixed) root exploit and any others which
might be lurking in similar code. I also think that the number of pages
should be unsigned, but changing the prototype of this function probably
requires some more careful review.
Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]>
diff --git a/mm/memory.c b/mm/memory.c
index e5628a5..7f50fd8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -989,6 +989,8 @@ int get_user_pages(struct task_struct *tsk, struct
mm_struct *mm,
int i;
unsigned int vm_flags;
+ if (len <= 0)
+ return 0;
/*
* Require read or write permissions.
* If 'force' is set, we only require the "MAY" flags.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/8][for -mm] mem_notify v6
> the Linux Today article is very nice description. (great works by Jake Edge) > http://www.linuxworld.com/news/2008/020508-kernel.html Just for future reference...the above-mentioned article is from LWN, syndicated onto LinuxWorld. It has, so far as I know, never been near Linux Today. Glad you liked it, though :) Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Avoid buffer overflows in get_user_pages()
Oliver Pinter <[EMAIL PROTECTED]> wrote: > for stable (.22 .23 .24) ? > > git id in mainline: 900cf086fd2fbad07f72f4575449e0d0958f860f I sent it to the stable folks a couple days ago. Thanks, jon Jonathan Corbet / LWN.net / [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] videobuf2-dma-streaming: new videobuf2 memory allocator
On Thu, 13 Sep 2012 17:46:32 +0200 Federico Vaga wrote: > > A few words explaining why this memory handling module is required or > > beneficial will definitely improve the commit :) > > ok, I will write some lines In general, all of these patches need *much* better changelogs (i.e. they need changelogs). What are you doing, and *why* are you doing it? The future will want to know. I'm going to try to look at the actual code, but time isn't something I have a lot of, still... Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] videobuf2-dma-streaming: new videobuf2 memory allocator
On Thu, 13 Sep 2012 17:42:33 +0200 Federico Vaga wrote: > > The header and esp. the source could really do with more > > documentation. It is not at all clear from the code what the > > dma-streaming allocator does and how it differs from other > > allocators. > > The other allocators are not documented and to understand them I read > the code. All the memory allocators reflect a kind of DMA interface: SG > for scatter/gather, contig for choerent and (now) streaming for > streaming. So, I'm not sure to understand what do you think is the > correct way to document a memory allocator; I can write one or two lines > of comment to summarize each function. what do you think? Well, there is some documentation here: https://lwn.net/Articles/447435/ This reminds me that I've always meant to turn it into something to put into Documentation/. I'll try to get to that soon. In general, the fact that a subsystem is insufficiently documented is not a good reason to add more poorly-documented code. We are, after all, trying to make the kernel better over time... Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Davicom DM9000C driver
On Wed, 19 Sep 2012 13:58:08 +0800
Allen Huang (黃偉格) wrote:
> I'm Allen Huang from Davicom. We are hereby opensourcing the linux
> driver for our DM9000C.
That is great, but please read the development process documentation on
how best to submit something like this. We would much rather see a patch
(inline, not as an attachment) to facilitate the review.
I'm going to take a quick look, but this is *not* a thorough review.
> /*
> * Davicom DM9000 Fast Ethernet driver for Linux.
> *Copyright (C) 1997 Sten Wang
> *
> *This program is free software; you can redistribute it and/or
> *modify it under the terms of the GNU General Public License
> *as published by the Free Software Foundation; either version 2
> *of the License, or (at your option) any later version.
> *
> *This program is distributed in the hope that it will be useful,
> *but WITHOUT ANY WARRANTY; without even the implied warranty of
> *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> *GNU General Public License for more details.
> *
> * (C) Copyright 1997-1998 DAVICOM Semiconductor,Inc. All Rights Reserved.
This line contradicts what comes above; GPL is not "all rights reserved."
> * Additional updates, Copyright:
> *Ben Dooks
> *Sascha Hauer
> *
> * 2010.07.20 V_R1 1.Write PHY Reg27 = 0xE100
> * 2.Just enable
> PHY once after GPIO setting in dm9000_init_dm9000()
The 80-column limit isn't as set in stone as it once was, but this still
pushes the limits rather farther than needed. In general, though,
changelog information belongs in the VCS, not in the header comments.
> /* DM9000 register address locking.
> *
> * The DM9000 uses an address register to control where data written
> * to the data register goes. This means that the address register
> * must be preserved over interrupts or similar calls.
> *
> * During interrupt and other critical calls, a spinlock is used to
> * protect the system, but the calls themselves save the address
> * in the address register in case they are interrupting another
> * access to the device.
> *
> * For general accesses a lock is provided so that calls which are
> * allowed to sleep are serialised so that the address register does
> * not need to be saved. This lock also serves to serialise access
> * to the EEPROM and PHY access registers which are shared between
> * these two devices.
> */
This seems like a bit of a strange locking scheme. Are the accesses to
this register pair so slow that you can't just protect the set with a
spinlock and be done with it?
> /* Structure/enum declaration --- */
> typedef struct board_info {
typedef is strongly discouraged in kernel code; just use "struct
board_info" in your code. Though a more descriptive name wouldn't hurt.
> void __iomem*io_addr; /* Register I/O base address */
> void __iomem*io_data; /* Data I/O address */
> u16 irq; /* IRQ */
>
> u16 tx_pkt_cnt;
> u16 queue_pkt_len;
> u16 queue_start_addr;
> u16 dbug_cnt;
> u8 io_mode;/* 0:word, 2:byte */
> u8 phy_addr;
> u8 imr_all;
>
> unsigned intflags;
> unsigned intin_suspend :1;
> int debug_level;
This structure will have a lot of holes; maybe that's not a problem.
[...]
>
> static void
> dm9000_phy_write(struct net_device *dev,
>int phyaddr_unused, int reg, int value);
Why not just define this function here and avoid the forward declaration?
> /* debug code */
>
> #define dm9000_dbg(db, lev, msg...) do { \
> if ((lev) < CONFIG_DM9000_DEBUGLEVEL && \
> (lev) < db->debug_level) { \
> dev_dbg(db->dev, msg); \
> } \
> } while (0)
We have nice dynamic debugging facilities that make this kind of stuff
unnecessary. See https://lwn.net/Articles/434833/
> /* DM9000 network board routine */
>
> static void
> dm9000_reset(board_info_t * db)
> {
> dev_dbg(db->dev, "resetting device\n");
>
> /* RESET device */
> writeb(DM9000_NCR, db->io_addr);
> udelay(200);
> writeb(NCR_RST, db->io_data);
> udelay(200);
> }
>
> /*
> * Read a byte from I/O port
> */
> static u8
> ior(board_info_t * db, int reg)
> {
> writeb(reg, db->io_addr);
> return readb(db->io_data);
> }
>
> /*
> * Write a byte to I/O port
> */
>
> static void
> iow(board_info_t * db, int reg, int value)
> {
> writeb(reg, db->io_addr);
> writeb(value, db->io_data);
> }
So the locking you were talking about is clearly happening at a higher
level than here. It
Re: Davicom DM9000C driver
On Thu, 20 Sep 2012 09:08:00 + Arnd Bergmann wrote: > More importantly, the patch should be against the version that is > already present in the kernel as drivers/net/ethernet/davicom/dm9000.c, > which appears to be mostly the same as the version that is submitted > here. Sigh. Leave it to me to miss the most important part of all. Ignore me, please, and listen to Arnd - good advice in general. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Documentation/SubmittingPatches: implicit permission for public reports
On Wed, 24 Oct 2012 10:22:02 -0400 Ed Cashin wrote: > +contribution. Please note that this tag should not be added without > +the reporter's permission unless the problem was reported in a public > +forum. That said, if we diligently credit our bug reporters, they I know what you're getting at, but this reads as if one should add the tag even if it's contrary to the reporter's wishes. How about something like: If the bug report was via a private channel, please obtain the reporter's permission before adding this tag - or, even better, ask that the report be posted publicly. ? jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/3] mm: Add VM pressure notifications
On Wed, 7 Nov 2012 03:01:28 -0800
Anton Vorontsov wrote:
> This patch introduces vmpressure_fd() system call. The system call creates
> a new file descriptor that can be used to monitor Linux' virtual memory
> management pressure.
I noticed a couple of quick things as I was looking this over...
> +static ssize_t vmpressure_read(struct file *file, char __user *buf,
> +size_t count, loff_t *ppos)
> +{
> + struct vmpressure_watch *watch = file->private_data;
> + struct vmpressure_event event;
> + int ret;
> +
> + if (count < sizeof(event))
> + return -EINVAL;
> +
> + ret = wait_event_interruptible(watch->waitq,
> +atomic_read(&watch->pending));
Would it make sense to support non-blocking reads? Perhaps a process would
like to simply know that current pressure level?
> +SYSCALL_DEFINE1(vmpressure_fd, struct vmpressure_config __user *, config)
> +{
> + struct vmpressure_watch *watch;
> + struct file *file;
> + int ret;
> + int fd;
> +
> + watch = kzalloc(sizeof(*watch), GFP_KERNEL);
> + if (!watch)
> + return -ENOMEM;
> +
> + ret = copy_from_user(&watch->config, config, sizeof(*config));
> + if (ret)
> + goto err_free;
This is wrong - you'll return the number of uncopied bytes to user space.
You'll need a "ret = -EFAULT;" in there somewhere.
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Question about using new request_threaded_irq
On Mon, 17 Dec 2012 16:11:22 +0100 Marcos Lois Bermúdez wrote: > For my understand if i call for example: > > request_threaded_irq(irqmum, NULL, irq_handle, IRQF_TRIGGER_FALLING, > DEVICE_NAME, priv); > > This seem to make a old Hard IRQ handler, and inside of this handler > sleep APIs can't be used, but i see some SPI drivers that seem to > register a IRQ of this form and make API calls that can sleep in the > handler. Not quite. The prototype for request_threaded_irq() is: int request_threaded_irq(unsigned int irq, irq_handler_t handler, irq_handler_t thread_fn, unsigned long irqflags, const char *devname, void *dev_id) Note the presents of *two* handlers, called "handler" and "thread_fn". The first, "handler", is called in interrupt context; it's job is usually to quiet the device and return; it cannot sleep. If it's return value is IRQ_WAKE_THREAD, the thread_fn() will be called in process context; it *can* sleep. In the example you cite, there is no immediate handler, only the thread_fn(); the call to a blocking function from within the thread_fn() is correct. Hope that helps, jon Jonathan Corbet / LWN.net / cor...@lwn.net -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Question about using new request_threaded_irq
On Mon, 17 Dec 2012 17:06:43 +0100 Marcos Lois Bermúdez wrote: > I'm a bit confusing because i see a outdated page that talks about this > new IRQ API, but now i see that it's very outdated: > > http://lwn.net/Articles/302043/ I normally encourage people to rely on LWN for everything, of course - even the articles that Jake writes :) But that *was* four years ago; a lot of things change in the kernel in that much time. Out of curiosity, I looked back at the old thread and the article did, indeed, accurately match the API proposed at that time. The order of those arguments got switched at some later point. The moral of the story: when in doubt, check the source. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/4] videobuf2-dma-streaming: new videobuf2 memory allocator
On Mon, 7 Jan 2013 00:09:47 +0100 Alessandro Rubini wrote: > I don't expect you'll see serious performance differences on the PC. I > think ARM users will have better benefits, due to the different cache > architecture. You told me Jon measured meaningful figures on a Marvel > CPU. It made the difference between 10 frames per second with the CPU running flat out and 30fps mostly idle. I think that probably counts as meaningful, yeah...:) jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/4] videobuf2-dma-streaming: new videobuf2 memory allocator
On Tue, 08 Jan 2013 07:50:41 +0100 Marek Szyprowski wrote: > > Couldn't this performance difference be due to the usage of GFP_DMA inside > > the VB2 code, like Federico's new patch series is proposing? > > > > If not, why are there a so large performance penalty? > > Nope, this was caused rather by a very poor CPU access to non-cached (aka > 'coherent') memory and the way the video data has been accessed/read > with CPU. Exactly. Uncached memory *hurts*, especially if you're having to touch it all with the CPU. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2nd attempt: help with dma_alloc_coherent() + dma_free_coherent()
On Tue, 8 Jan 2013 23:51:59 +0800
Jeff Chua wrote:
> I'm trying to understand how this oops in the diva driver and it's just a
> simple dma_alloc_coherent() followed by dma_free_coherent(), but it oops.
> Why?
Hmm...from a quick look...
> static u32 *clock_data_addr;
> static dma_addr_t clock_data_bus_addr;
>
> if((clock_data_addr = dma_alloc_coherent(NULL, PAGE_SIZE,
> &clock_data_bus_addr, GFP_KERNEL)) != 0) {
> printk(KERN_INFO "dma_alloc_coherent ok\n");
> memset (clock_data_addr, 0x00, PAGE_SIZE);
> } else
> printk(KERN_INFO "dma_alloc_coherent bad!!!\n");
> if(clock_data_addr) {
> printk(KERN_INFO "dma_free_coherent!!!\n");
> dma_free_coherent(NULL, PAGE_SIZE, clock_data_addr,
> clock_data_bus_addr);
> clock_data_addr = NULL;
> }
Perhaps passing NULL as your device structure pointer might just have
something to do with a crash due to a null pointer dereference? If you're
doing DMA, you should have a device. Try passing it and you might just
find that things work better.
(Incidentally, this:
> IP: [] iommu_no_mapping+0xc/0xee
is a fairly good clue as well - it's trying to dereference the device
structure pointer).
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH -v2 0/4] Persistent events
On Thu, 16 Aug 2012 19:45:19 +0200 Borislav Petkov wrote: > off and on I get some free time to work on that, here's the latest > incarnation. It contains review feedback from the earlier round. Can I add one small request for the next round? I've looked through the patches and the code a bit, and I still have absolutely no clue of what a "persistent event" is or why I might want one. Now, admittedly, I'm slower than most, but it still might not hurt to add some overall description of what this is for...? Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Documentation/patch-tags, one more time
Somebody recently asked me about this patch, so I dug it up for one last try. I do believe there is value in describing patch tags, and, certainly, nobody has objected to the idea. Comments from several reviewers were addressed before the previous posting. jon -- Add a document describing the various tags attached to patches. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> --- Documentation/00-INDEX |2 + Documentation/patch-tags | 76 ++ 2 files changed, 78 insertions(+), 0 deletions(-) create mode 100644 Documentation/patch-tags diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 6e9c405..a900a6d 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -289,6 +289,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 000..c2fb56c --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,76 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) their progress. All of +these tags have the form: + + Something-done-by: Full name <[EMAIL PROTECTED]> [optional random stuff] + +These tags are: + +From: The original author of the patch. This tag will ensure + that credit is properly given when somebody other than the + original author submits the patch. + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch, to the best of his or her knowledge, can legally be + merged into the mainline and distributed under the terms of + the GNU General Public License, version 2. See the + Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. This tag assures upstream maintainers that + the provenance of the patch is known and allows the origin + of the patch to be reviewed should copyright questions + arise. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion; it informs upstream + maintainers that a certain degree of consensus on the patch + as been achieved.. An Acked-by tag does not imply any + involvement in the development of the patch or that a + detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptable according + to the Reviewer's Statement as found at the bottom of this + file. A Reviewed-by tag is a statement of opinion that the + patch is an appropriate modification of the kernel without + any remaining serious technical issues. Any interested + reviewer (who has done the work) can offer a Reviewed-by + tag for a patch. This tag serves to give credit to + reviewers and to inform maintainers of the degree of review + which has been done on the patch. + +Cc:The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. This + tag documents that potentially interested parties have been + included in the discussion. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. This tag informs + maintainers that some testing has been performed, provides + a means to locate testers for future patches, and ensures + credit for the testers. + + + + +Reviewer's statement of oversight + +By offering my Reviewed-by: tag, I state that: + + (a) I have carried out a technical review of this patch to evaluate its + appropriateness and readiness for inclusion into the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch have been + communicated back to the submitter. I am satisfied with the + submitter's response to my comments. + + (c) While there may be things that could be improved with this submission, + I believe that it is, at this time, (1) a worthwhile modification to + the kernel, and (2) free of known issues which would argue against its + inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I do not + (unless expli
[PATCH] Documenting patch tags yet one more time
OK, Linus questioned the From: tag, so I've just taken that out for now. Paul Jackson asked: > Question -- should this documentation of patch-tags be in its own file, > or added to Documentation/SubmittingPatches. Clearly I had once thought the former, but, on review, I've changed my mind. So here's a version which merges the information into SubmittingPatches instead. Thanks, jon -- Add documentation for more patch tags Add documentation of the Cc:, Tested-by:, and Reviewed-by: tags to SubmittingPatches, with an emphasis on trying to nail down what Reviewed-by: really means. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 08a1ed1..cc00c8e 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -328,7 +328,7 @@ now, but you can do this to mark internal company procedures or just point out some special detail about the sign-off. -13) When to use Acked-by: +13) When to use Acked-by: and Cc: The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path. @@ -349,11 +349,59 @@ Acked-by: does not necessarily indicate acknowledgement of the entire patch. For example, if a patch affects multiple subsystems and has an Acked-by: from one subsystem maintainer then this usually indicates acknowledgement of just the part which affects that maintainer's code. Judgement should be used here. - When in doubt people should refer to the original discussion in the mailing +When in doubt people should refer to the original discussion in the mailing list archives. +If a person has had the opportunity to comment on a patch, but has not +provided such comments, you may optionally add a "Cc:" tag to the patch. +This is the only tag which might be added without an explicit action by the +person it names. This tag documents that potentially interested parties +have been included in the discussion -14) The canonical patch format + +14) Using Test-by: and Reviewed-by: + +A Tested-by: tag indicates that the patch has been successfully tested (in +some environment) by the person named. This tag informs maintainers that +some testing has been performed, provides a means to locate testers for +future patches, and ensures credit for the testers. + +Reviewed-by:, instead, indicates that the patch has been reviewed and found +acceptable according to the Reviewer's Statement: + + Reviewer's statement of oversight + + By offering my Reviewed-by: tag, I state that: + +(a) I have carried out a technical review of this patch to +evaluate its appropriateness and readiness for inclusion into +the mainline kernel. + +(b) Any problems, concerns, or questions relating to the patch +have been communicated back to the submitter. I am satisfied +with the submitter's response to my comments. + +(c) While there may be things that could be improved with this +submission, I believe that it is, at this time, (1) a +worthwhile modification to the kernel, and (2) free of known +issues which would argue against its inclusion. + +(d) While I have reviewed the patch and believe it to be sound, I +do not (unless explicitly stated elsewhere) make any +warranties or guarantees that it will achieve its stated +purpose or function properly in any given situation. + +A Reviewed-by tag is a statement of opinion that the patch is an +appropriate modification of the kernel without any remaining serious +technical issues. Any interested reviewer (who has done the work) can +offer a Reviewed-by tag for a patch. This tag serves to give credit to +reviewers and to inform maintainers of the degree of review which has been +done on the patch. Reviewed-by: tags, when supplied by reviewers known to +understand the subject area and to perform thorough reviews, will normally +increase the liklihood of your patch getting into the kernel. + + +15) The canonical patch format The canonical patch subject line is: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Update VIP to videobuf2 and control framework
On Wed, 1 Aug 2012 08:41:56 +0200 Hans Verkuil wrote: > > The second patch adds a new memory allocator for the videobuf2. I name it > > videobuf2-dma-streaming but I think "streaming" is not the best choice, so > > suggestions are welcome. My inspiration for this buffer come from > > videobuf-dma-contig (cached) version. After I made this buffer I found the > > videobuf2-dma-nc made by Jonathan Corbet and I improve the allocator with > > some suggestions (http://patchwork.linuxtv.org/patch/7441/). The VIP doesn't > > work with videobu2-dma-contig and I think this solution is easier the sg. > > I leave this to the vb2 experts. It's not obvious to me why we would need > a fourth memory allocator. I first wrote my version after observing that performance dropped by a factor of three on the OLPC XO 1.75 when using contiguous, coherent memory. If the architecture needs to turn off caching, things really slow down, to the point that it's unusable. There's no real reason why a V4L2 device shouldn't be able to use streaming mappings in this situation; it performs better and doesn't eat into the limited amounts of coherent DMA space available on some systems. I never got my version into a mergeable state only because I finally figured out how to bludgeon the hardware into doing s/g DMA and didn't need it anymore. But I think it's a worthwhile functionality to have, and, with CMA, it could be the preferred mode for a number of devices. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Patch tags [was writeout stalls in current -git]
Andrew wrote: > > Reviewed-by: Fengguang Wu <[EMAIL PROTECTED]> > > I would prefer Tested-by: :( This seems like as good an opportunity as any to toss my patch tags document out there one more time. I still think it's a good idea to codify some sort of consensus on what these tags mean... jon diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 299615d..1948a93 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -286,6 +286,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 000..6acde5e --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,76 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) their progress. All of +these tags have the form: + + Something-done-by: Full name <[EMAIL PROTECTED]> [optional random stuff] + +These tags are: + +From: The original author of the patch. This tag will ensure + that credit is properly given when somebody other than the + original author submits the patch. + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch is, to the best of his or her knowledge, legally able + to be merged into the mainline and distributed under the + terms of the GNU General Public License, version 2. See + the Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. This tag assures upstream maintainers that + the provenance of the patch is known and allows the origin + of the patch to be reviewed should copyright questions + arise. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion; it informs upstream + maintainers that a certain degree of consensus on the patch + as been achieved.. An Acked-by tag does not imply any + involvement in the development of the patch or that a + detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptable according + to the Reviewer's Statement as found at the bottom of this + file. A Reviewed-by tag is a statement of opinion that the + patch is an appropriate modification of the kernel without + any remaining serious technical issues. Any interested + reviewer (who has done the work) can offer a Reviewed-by + tag for a patch. This tag serves to give credit to + reviewers and to inform maintainers of the degree of review + which has been done on the patch. + +Cc:The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. This + tag documents that potentially interested parties have been + included in the discussion. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. This tag informs + maintainers that some testing has been performed, provides + a means to locate testers for future patches, and ensures + credit for the testers. + + + + +Reviewer's statement of oversight, v0.02 + +By offering my Reviewed-by: tag, I state that: + + (a) I have carried out a technical review of this patch to evaluate its + appropriateness and readiness for inclusion into the mainline kernel. + + (b) Any problems, concerns, or questions relating to the patch have been + communicated back to the submitter. I am satisfied with the + submitter's response to my comments. + + (c) While there may be things that could be improved with this submission, + I believe that it is, at this time, (1) a worthwhile modification to + the kernel, and (2) free of known issues which would argue against its + inclusion. + + (d) While I have reviewed the patch and believe it to be sound, I do not + (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majo
Re: [RFC] mmiotrace full patch, preview 1
Hey, Pekka,
A couple of little things I noticed...
> +static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs)
> +{
> + int ret = 0;
> + struct kmmio_probe *probe;
> + struct kmmio_fault_page *faultpage;
> + struct kmmio_context *ctx = &get_cpu_var(kmmio_ctx);
> +
> + if (!ctx->active)
> + goto out;
Should that text read something like:
if (condition != DIE_TRAP || !ctx->active)
Presumably you won't be active if something else is going wrong, but one
never knows.
> +int register_kmmio_probe(struct kmmio_probe *p)
> +{
> + int ret = 0;
> + unsigned long size = 0;
> +
> + spin_lock_irq(&kmmio_lock);
> + kmmio_count++;
> + if (get_kmmio_probe(p->addr)) {
> + ret = -EEXIST;
> + goto out;
> + }
That only checks the first page; if the probed region partially overlaps
another one found later in memory, the registration will succeed.
Maybe you want to decrement kmmio_count if you decide to return -EEXIST
(or hold off on the increment until after the test)?
In general, I worry about what happens if an interrupt handler generates
traced MMIO traffic while a fault handler is active. It looks a lot
like the "all hell breaks loose" scenario mentioned in the comments. Is
there some way of preventing that which I missed? Otherwise, maybe,
should the ioremap() wrappers take an additional argument, being an IRQ
to disable while trace handlers are active?
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
The zen of kernel virtual addresses
Howdy, I'm trying to get a better handle on how the kernel handles addresses. Could anybody who is so inclined have a look at the following and tell me (gently) where I've gone wrong? There are several different ways of specifying an address in the kernel, and the code is not always all that explicit on which it's using in any given place. The terminology doesn't even seem to be there to make the distinction, so I've made up some of my own. Here's the various ways I've been able to find to refer to a page: struct page The cookie used, increasingly, to represent any page in the system. Also where much of the page's housekeeping information lives. page frame number Essentially an index into mem_map - what you would once get from MAP_NR. There is very little use of page frame numbers in the 2.4 kernel - struct page is used instead. physical address An address as known by the low-level hardware. In the modern world, these can be 64-bit quantities, even on 32-bit systems. These are the addresses used by /dev/mem - which appears to work only for low memory. direct-mapped kernel virtual address An address as seen by the kernel - on many architectures it's just the physical address with an offset applied. Much of the kernel treats these as if they were physical addresses, but they are virtual and do not work for highmem pages. Back in my VMS days we would have called these "logical" addresses. There appears, however, to be a form of Godwin's law that applies to this list: if somebody mentions VMS the conversation stops. So I won't do that. pure kernel virtual address An address which can be dereferenced by the kernel, but which does not directly map to a physical address. Examples include addresses returned by vmalloc() and kmap(). The union of the direct-mapped and pure kernel virtual addresses is exported by /dev/kmem. user virtual address A basic virtual address used in user space. There are ways of moving between various types of addresses: virt_to_page() Maps a "logical" kernel address to a struct page. Does not work for pure kernel virtual or user virtual addresses. __pa() Turns a logical kernel address into a physical address. Also known as virt_to_phys(). __va() Turns a (lowmem) physical address into a logical address. Also known as phys_to_virt(). page_address() Maps a struct page to a logical (for lowmem) or kernel virtual (for highmem which has been mapped) address. The comment on the i386 version claims it returns the "permanent" address, but that is not true for highmem pages which have been kmap'd. pte_page() Turns a page table entry into a struct page. There does not appear to be a function to go from a user or kernel virtual address to a physical address or struct page - one must manually walk through the levels of page tables to get there via the pte. Any corrections / suggestions would be much appreciated. jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Current doc for writing device drivers?
> The Rubini book is being updated for 2.2 and 2.4, but I dunno when it > will go to press. We're working on it - honest! The book will go out for technical review before too long; I believe the current target date to have it on the shelves is April. We'd hoped for sooner, but, given how 2.4 development has gone, I think the timing is about right. After all, we wouldn't want to present the wrong prototype for kmap() or miss out on the new inter-module communication API...:) jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [BUG] Hard lockup using emu10k1-based sound card
Just as another data point, I, too, had trouble with lockups with the emu10k1 (with the 2.4.0-test driver and ALSA both). I noticed that it was sharing an interrupt with ACPI. As soon as I rebuilt the kernel with the ACPI Interpreter option turned off, the problem went away. It's not the first time I've gotten burned with the "turn on some option because I might want to mess with it someday" approach to kernel configuration... jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [ANNOUNCE] Kernel Janitor's TODO list (really sleep_on)
> Anything which uses sleep_on() has a 90% chance of being broken. Fix > them all, because we want to remove sleep_on() and friends in 2.5. This reminds me of a question I've been meaning to ask... Suppose you were working on the new edition of the device drivers book, which was just in the process of going out for tech review. You would, of course, have put in a lot of words about the sorts of race conditions that can come about when sleep_on() is used. Is that enough? Or would you omit coverage of those functions in favor of "doing it right" from the beginning? Obviously, I'm thinking about ripping out much of the sleep_on() discussion as a last-minute change. I would be most curious to hear whether people think that would be the right thing to do. Thanks, jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
On running user mode helpers
I see we have a new "call_usermodehelper" routine now. It looks like the end result is similer to exec_usermodehelper, except that you no longer need to mess with creating kernel threads yourself. Is call_usermodehelper now the officially blessed way for kernel code to run something in user space? Perhaps exec_usermodehelper should become private to kmod.c? I also see that call_usermodehelper will call do_exit() if the exec fails, while the path taken in request_module does not do that. Can both be right? Thanks, jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: When the FUD is all around (sniff).
The Repubblica article was bad enough, but if you want serious kernel FUD you should see this bit of delight on AsiaBizTech: http://www.nikkeibp.asiabiztech.com/wcs/leaf?CID=onair/asabt/fw/133671 For example: Also, the casual attitude of Torvald [sic], which doesn't meet the needs of the market and minds of investors, is one of the reasons that investors have rapidly lost interest in Linux distributors and Linux-related businesses. Cool. Linus caused the end of the stock bubble... jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Linux kernel programming for beginners
> Does anybody know any nice resource for beginners to try to write
> some device drivers/other interesting stuff ?
Well... if you can wait just a little longer, O'Reilly tells me that the
second edition of Linux Device Drivers should hit the shelves on June 28.
We're still working on the right license for the online release - if people
have suggestions, I would be glad to receive them privately.
jon (who's glad we didn't tell people how to request major device
numbers...)
Jonathan Corbet
Executive editor, LWN.net
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Making a module 2.4 compatible
> It would be nice to have it working under 2.4, so is there someplace > that outlines some of the major things that would have changed so I can > update the module accordingly? Within a week or so, "Linux Device Drivers" second edition should hit the shelves. It will also hit the net, but that's going to take a little longer. If it doesn't answer all your questions, then we failed to achieve what we set out to do. jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6 partition support driver methods
> Do I need to handle any thing in the request function to handle > read/writes to the device partitions? It looks like you've done most of what you need; in 2.6, block drivers need not worry about the details of partitioning. Lots of details in the block drivers chapter of LDD3 if you need them: http://lwn.net/Kernel/LDD3/ jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6 partition support driver methods
Mukund JB. <[EMAIL PROTECTED]> wrote: > BUT, when a card is inserted in the socket 3, I am NOT able to mount and > it says. > Mount: /dev/tfa12 is not a valid block device > > Can you convey me where exactly I am missing or why is it failing? First step would be to look in /proc/partitions (and the system logfile) to get an idea of what the kernel thinks is there. When you call add_disk(), the kernel should emit some messages noting the partitions that it sees. My guess is a mismatch of minor numbers between your device nodes and what the kernel sees (it's hard for me to make complete sense out of your minor number logic), but I could be wrong. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Linux Device Drivers, Third Edition online
The third edition of Linux Device drivers (by me, Alessandro Rubini, and Greg Kroah-Hartman) is now available online: http://lwn.net/Kernel/LDD3/ For the moment, it is only available in PDF format. The HTML and DocBook versions will be posted once we have them in shape. This edition is current through the 2.6.10 kernel; I'm keeping a list of subsequent API changes at: http://lwn.net/Articles/2.6-kernel-api/ The book is freely redistributable under the (GPLish) Creative Commons Attribution-ShareAlike 2.0 license. Don't let that stop you from buying copies for all of your friends, though :) jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] rwsem removal from kobj_map
[Since this is something I once looked at too...] > struct kobj_map *kobj_map_init(kobj_probe_t *base_probe, > - struct subsystem *s) > + struct subsystem *s, struct semaphore *sem) The only reason kobj_map_init() needed the subsystem argument was for the semaphore. Since you're passing that separately now, I would think that the subsystem argument could simply go away altogether. Once that's done, you should be able to delete cdev_subsys as well; when I cleaned that stuff up, I only left it there for kobj_map(). jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Device Driver Question.
> I have a trivial question, is it possible to Open a Device Driver (A) > within and other Device Driver (B) and Handle the driver-A similar to how > an application uses the Driver. It is certainly possible to call the same operations, yes, if done with proper care. One example you could look at would be the Raw I/O driver, .../drivers/char/raw.c, though that requires an understanding of the kiovec structure as well. jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Fix up PCI power management doc
I noticed that the PCI power management document file had fallen a little behind the times, so I fixed it up. jon Update the documentation of PCI power management functions. Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> diff --git a/Documentation/power/pci.txt b/Documentation/power/pci.txt index c750f9f..b6a3cbf 100644 --- a/Documentation/power/pci.txt +++ b/Documentation/power/pci.txt @@ -102,31 +102,28 @@ pci_save_state -- Usage: - pci_save_state(dev, buffer); + pci_save_state(struct pci_dev *dev); Description: - Save first 64 bytes of PCI config space. Buffer must be allocated by - caller. + Save first 64 bytes of PCI config space, along with any additional + PCI-Express or PCI-X information. pci_restore_state - Usage: - pci_restore_state(dev, buffer); + pci_restore_state(struct pci_dev *dev); Description: - Restore previously saved config space. (First 64 bytes only); - - If buffer is NULL, then restore what information we know about the - device from bootup: BARs and interrupt line. + Restore previously saved config space. pci_set_power_state --- Usage: - pci_set_power_state(dev, state); + pci_set_power_state(struct pci_dev *dev, pci_power_t state); Description: Transition device to low power state using PCI PM Capabilities @@ -142,7 +139,7 @@ pci_enable_wake --- Usage: - pci_enable_wake(dev, state, enable); + pci_enable_wake(struct pci_dev *dev, pci_power_t state, int enable); Description: Enable device to generate PME# during low power state using PCI PM - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fix adm9240 oops
The adm9240 driver, in adm9240_detect(), allocates a structure. The error path attempts to kfree() a subfield of that structure, resulting in an oops (or slab corruption) if the hardware is not present. This one seems worth fixing for 2.6.13. jon Signed-off-by: Jonathan Corbet <[EMAIL PROTECTED]> --- 2.6.13-rc7/drivers/hwmon/adm9240.c.orig 2005-08-25 14:30:04.0 -0600 +++ 2.6.13-rc7/drivers/hwmon/adm9240.c 2005-08-25 14:30:26.0 -0600 @@ -616,7 +616,7 @@ static int adm9240_detect(struct i2c_ada return 0; exit_free: - kfree(new_client); + kfree(data); exit: return err; } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/hwmon/*: kfree() correct pointers
> Already fixed in Greg's i2c tree and -mm for quite some time now... So it is. The comment says, however, that "the existing code works somewhat by accident." In the case of the 9240 driver, however, the existing code demonstrably does not work - it oopsed on me. The patch in Greg's tree looks fine (it's a straightforward fix, after all); I'd recommend that it be merged before 2.6.13. jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.13 new option timer frequency
> I built and installed 2.6.13 today, and oldconfig revealed the new option for > timer frequency. > > I searched the LKML on this, but all I found is the technical stuff - not > really any layman solutions. I wrote a bit about the timer frequency option a few weeks ago: http://lwn.net/Articles/145973/ jon Jonathan Corbet Executive editor, LWN.net [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Make OLPC camera driver depend on x86.
Dave Jones <[EMAIL PROTECTED]> wrote: > - depends on I2C && VIDEO_V4L2 > + depends on I2C && VIDEO_V4L2 && X86_32 Any particular reason why? I wouldn't be surprised to see Cafe used with other processors in the future. And I happen to know the driver works on x86-64 systems...or at least it did a few iterations back. In any case, x86 for now is fine, I guess; we can always change it later if need be. jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: GPL only modules [was Re: [GIT PATCH] more Driver core patches for 2.6.19]
Greg's patch: > + printk(KERN_WARNING "%s: This module will not be able " > + "to be loaded after January 1, 2008 due to its " > + "license.\n", mod->name); If you're going to go ahead with this, shouldn't the message say that the module will not be loadable into *kernels released* after January 1, 2008? I bet a lot of people would read the above to say that their system will just drop dead of a New Year's hangover, and they'll freak. I wouldn't want to be the one getting all the email at that point... jon - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 07/16] slab: overloading the RCU head over the LRU for RCU free
On Thu, 22 Aug 2013 17:44:16 +0900
Joonsoo Kim wrote:
> With build-time size checking, we can overload the RCU head over the LRU
> of struct page to free pages of a slab in rcu context. This really help to
> implement to overload the struct slab over the struct page and this
> eventually reduce memory usage and cache footprint of the SLAB.
So I'm taking a look at this, trying to figure out what's actually in
struct page while this stuff is going on without my head exploding. A
couple of questions come to mind.
> static void kmem_rcu_free(struct rcu_head *head)
> {
> - struct slab_rcu *slab_rcu = (struct slab_rcu *)head;
> - struct kmem_cache *cachep = slab_rcu->page->slab_cache;
> + struct kmem_cache *cachep;
> + struct page *page;
>
> - kmem_freepages(cachep, slab_rcu->page);
> + page = container_of((struct list_head *)head, struct page, lru);
> + cachep = page->slab_cache;
> +
> + kmem_freepages(cachep, page);
> }
Is there a reason why you don't add the rcu_head structure as another field
in that union alongside lru rather than playing casting games here? This
stuff is hard enough to follow as it is without adding that into the mix.
The other question I had is: this field also overlays slab_page. I guess
that, by the time RCU comes into play, there will be no further use of
slab_page? It might be nice to document that somewhere if it's the case.
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH for-next 3/4] epoll: struct epoll support
So I was just looking things over quickly, and something jumped out at
me. In ep_control():
> + } else if (!(*io) && epi) {
> + /* delete this eventpoll entry */
> + if (is_file_epoll(target)) {
> + tep = target->private_data;
> + mutex_lock_nested(&tep->mtx, 1);
> + }
> + if (is_file_epoll(target))
> + mutex_lock_nested(&tep->mtx, 1);
How could that possibly work? I can't imagine tep->mtx is going to
react well to being locked a second time...
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Enable A20 gate
On Tue, 7 May 2013 16:54:23 +0500 Syed Salman Mansoor wrote: > I am a newbie and trying to learn so that I can contribute whatever I can. If you have not already done so, please have a look at: Documentation/HOWTO Documentation/development-process both of which can be found in your source tree. They will help you to learn how to work with the kernel development community. Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Claws (was [BUG] WARN_ON(!context) in drivers/pci/hotplug/acpiphp_glue.c)
On Thu, 10 Oct 2013 14:37:15 -0700 Linus Torvalds wrote: > Steven, I'd suggest just jettisoning that mailer. FWIW, I've used claws for years and never run into an issue like that. This email comes from claws; the quoting looks right in the compose window now, I assume it will go out in good form. Otherwise I'll have just made a fool of myself in front of everybody, but, then, that's what I do for a living, so that's OK too. The problem, from what Steve said before, had to do with the address book. I assume there's a real bug there...I bet we could get it fixed. They've taken patches from me without grief, and are generally responsive to this sort of thing. I'll forward things over to the claws list and see what they say. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [BUG] WARN_ON(!context) in drivers/pci/hotplug/acpiphp_glue.c
On Thu, 10 Oct 2013 18:00:48 -0700 Linus Torvalds wrote: > File a bug on Claws, and if the developers brush it off as your own > problem, just stop using the PoS. They acknowledge the bug - it was already known to them, actually. With luck we'll see a fix soon. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/6] rcu: Create rcu_sync infrastructure
OK, so this is a real nit, but...in the changelog:
> static inline void rcu_sync_enter(struct rcu_sync_struct *xxx)
> {
> atomic_inc(&xxx->counter);
> synchronize_sched();
> }
>
> static inline void rcu_sync_enter(struct rcu_sync_struct *xxx)
> {
> synchronize_sched();
> atomic_dec(&xxx->counter);
> }
That second rcu_sync_enter should really be rcu_sync_exit.
Is the "xxx" there to test spamassassin configurations? :)
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -next] media/platform/marvell-ccic: fix cafe_ccic build error
On Thu, 31 Oct 2013 11:07:28 -0700 Randy Dunlap wrote: > From: Randy Dunlap > > The cafe_ccic driver (the mcam-core.c part of it) uses dma_sg > interfaces so it needs to select VIDEOBUF2_DMA_SG to prevent > build errors. Geert sent a patch too a little while back. BUT, this shouldn't be happening. Could you send a .config file that evokes this failure? All of the s/g stuff is ifdeffed out if VIDEOBUF2_DMA_SG isn't enabled...or it used to be... I'd rather figure out what's going on and not drag the s/g stuff into OLPC XO builds, where memory is tight and there's no use for it. (Sorry, I'm *way* behind on everything...) Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC] wait_for_completion_timeout() considered harmful.
On Tue, 19 Nov 2013 00:42:09 +0100 Peter Zijlstra wrote: > I briefly talked to Thomas about this earlier today and we need to fix > this at a lower level -- the quick 'n dirty solution is to add 1 jiffy > down in the timer-wheel when we enqueue these things. That can lead to situations like the one I encountered years ago where msleep(1) would snooze for 20ms. I didn't get much love for my idea of switching msleep() to hrtimers back then, but I still think it might be be better to provide the resolution that the interface appears to promise. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] net: rename low latency sockets functions to busy poll
On Mon, 08 Jul 2013 16:20:34 +0300 Eliezer Tamir wrote: > Rename POLL_LL to POLL_BUSY_LOOP. So pardon me if I speak out of turn, but it occurs to me to wonder...should the SO_LL socket option be renamed in a similar fashion before this interface escapes into the wild? Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Documentation: update references to v2.6.x in development-process
On Mon, 15 Jul 2013 19:34:44 -0400 Paul Gortmaker wrote: > The last mainline release of a v2.6.x kernel was back in May 2011. > Here we update references to be 3.x based, which also means updating > some dates and statistics. Ccing the author of the document never hurts :) I actually went through this exercise a while back, but somehow never got around to sending the changes out into the world. Easily distracted, I guess. Anyway, you can put my Acked-by on your changes if you like. > On a similar note, I was thinking about the recent thread on linux-next > where we were indicating that people shouldn't rebase linux-next content > on a whim, and that new devel (vs. bugfix) content shouldn't appear in > the linux-next content during the merge window. There is no question > that the linux-next process is integral to the main flow of patches to > mainline, so I think Documentation/development-process/2.Process (the > same file) should also capture those points in the linux-next section. > Do you have some pre-canned text we can insert there, or should I draft > something up for you to review? Seems useful, I could also try to help with this if you run out of steam. I'd be more inclined to put it into section 7, though, since it's the sort of thing early-stage developers don't normally need to worry about. Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: old trees
On Tue, 17 Sep 2013 17:01:24 +1000 Stephen Rothwell wrote: > jc_docs 2 years, 6 months ago I was afraid it would be the oldest tree on the list, but it's not even close...:) I've been meaning to do some stuff there for a bit; the problem is that life keeps getting in the way. If it's not a hassle for you, I'd just as soon keep it around, things have *got* to clear out one of these days... Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: Add Xilinx AXI Central Direct Memory Access Engine driver support
On Mon, 7 Apr 2014 20:22:54 +0530
Srikanth Thokala wrote:
> Kindly review this driver and please let me know if you have any comments.
Here's some comments from a quick look at the patch; they do not qualify as
a proper review by any means.
> +/**
> + * struct xilinx_cdma_chan - Driver specific cdma channel structure
> + * @xdev: Driver specific device structure
> + * @lock: Descriptor operation lock
> + * @done_list: Complete descriptors
> + * @pending_list: Descriptors waiting
> + * @active_desc: Active descriptor
> + * @allocated_desc: Allocated descriptor
> + * @common: DMA common channel
> + * @desc_pool: Descriptors pool
> + * @dev: The dma device
> + * @irq: Channel IRQ
> + * @has_sg: Support scatter transfers
> + * @err: Channel has errors
> + * @tasklet: Cleanup work after irq
> + */
> +struct xilinx_cdma_chan {
> + struct xilinx_cdma_device *xdev;
> + spinlock_t lock;
> + struct list_head done_list;
> + struct list_head pending_list;
> + struct xilinx_cdma_tx_descriptor *active_desc;
> + struct xilinx_cdma_tx_descriptor *allocated_desc;
> + struct dma_chan common;
> + struct dma_pool *desc_pool;
> + struct device *dev;
> + int irq;
> + bool has_sg;
> + int err;
> + struct tasklet_struct tasklet;
> +};
Have you thought about using a threaded IRQ handler instead of a tasklet?
The tasklet interface has its pitfalls and some reviewers frown on the
addition of more users.
[...]
> +/**
> + * xilinx_cdma_tx_descriptor - Allocate transaction descriptor
> + * @chan: Driver specific cdma channel
> + *
> + * Return: The allocated descriptor on success and NULL on failure.
> + */
> +static struct xilinx_cdma_tx_descriptor *
> +xilinx_cdma_alloc_tx_descriptor(struct xilinx_cdma_chan *chan)
> +{
> + struct xilinx_cdma_tx_descriptor *desc;
> + unsigned long flags;
> +
> + if (chan->allocated_desc)
> + return chan->allocated_desc;
This looks racy. What happens if two threads hit here at once, or, in
general, some other thread does something with chan->allocated_desc?
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return NULL;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> + chan->allocated_desc = desc;
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + INIT_LIST_HEAD(&desc->segments);
Using the lock to protect a single assignment doesn't really buy you much;
it's what is going on outside of the locked region that is going to bite
you.
Also, as soon as you do that assignment, desc is visible to the rest of the
world. Somebody else could try to use it before you get around to that
INIT_LIST_HEAD() call, with unpleasant results. You really need a lock
around the whole test/allocate/initialize operation.
> + return desc;
> +}
> +
> +/**
> + * xilinx_cdma_free_tx_descriptor - Free transaction descriptor
> + * @chan: Driver specific cdma channel
> + * @desc: cdma transaction descriptor
> + */
> +static void
> +xilinx_cdma_free_tx_descriptor(struct xilinx_cdma_chan *chan,
> + struct xilinx_cdma_tx_descriptor *desc)
> +{
> + struct xilinx_cdma_tx_segment *segment, *next;
> +
> + if (!desc)
> + return;
> +
> + list_for_each_entry_safe(segment, next, &desc->segments, node) {
> + list_del(&segment->node);
> + xilinx_cdma_free_tx_segment(chan, segment);
> + }
> +
> + kfree(desc);
> +}
What are the locking requirements for this function? It looks from a
casual reading like some callers hold the spinlock while others do not. It
would be good to sort out (and document!) the requirement here.
> +/**
> + * xilinx_cdma_free_chan_resources - Free channel resources
> + * @dchan: DMA channel
> + */
> +static void xilinx_cdma_free_chan_resources(struct dma_chan *dchan)
> +{
> + struct xilinx_cdma_chan *chan = to_xilinx_chan(dchan);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> + xilinx_cdma_free_desc_list(chan, &chan->done_list);
> + xilinx_cdma_free_desc_list(chan, &chan->pending_list);
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + dma_pool_destroy(chan->desc_pool);
> + chan->desc_pool = NULL;
Why is this part done outside the lock?
> + */
> +static void xilinx_cdma_chan_desc_cleanup(struct xilinx_cdma_chan *chan)
> +{
> + struct xilinx_cdma_tx_descriptor *desc, *next;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> +
> + list_for_each_entry_safe(desc, next, &chan->done_list, node) {
> + dma_async_tx_callback callback;
> + void *callback_param;
> +
> + /* Remove from the list of running transactions */
> + list_del(&desc->node);
> +
> + /* Run the link descriptor callback function */
> + callback = desc->asyn
Re: [Ksummit-2013-discuss] [ATTEND] DT, maintainership, development process
On Mon, 29 Jul 2013 15:10:33 +0200 "Rafael J. Wysocki" wrote: > That actually is simple enough. > > Check out the Linus' master branch and do > > $ git log --ancestry-path --merges .. So I picked a recent, single-signoff patch mostly at random: 8ade62857ef77bdf639185410fbcd811aa700cb2 That "git log" command gives me 156 intervening commits involving at least a dozen networking trees, along with virtio, parisc, blackfin, x86, ... Even if one stops looking at the first merge performed by Linus, that's 47 to look at. Did that patch really pass through all those peoples' hands? Plus, of course, this assumes there's no fast-forward merges in the mix. >From your other message: > And what about trusting maintainers? If Linus trusts them enough to pull from > them, why can't everybody else trust them enough to assume that they don't do > bad things on purpose? I hope you're not reading such thoughts into what *I* wrote. But most anybody who works on code occasionally does bad things by mistake, that's why we have a review process. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] trivial: adjust code alignment
On Mon, 5 Aug 2013 18:19:18 +0200 (CEST) Julia Lawall wrote: > Oops, thanks for spotting that. I'm not sure whether it is safe to abort > these calls as soon as the first one fails, but perhaps I could introduce > some more variables, and test them all afterwards. Yes, it would be safe. But it's hard to imagine a scenario where any of those particular calls would fail that doesn't involve smoke. The code is evidence of ancient laziness on my part. I'll add fixing it up to my list of things to do. Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC v01 1/3] PowerCap: Documentation
On Fri, 2 Aug 2013 11:08:50 -0700 Srinivas Pandruvada wrote: > +power_uw (rw): Current power counter in micro-watts. Write to this counter > +resets the counter to zero. If the counter can not be reset, then this > attribute > +is read-only. Sorry if I'm slow, but... power is an instantaneous quantity, so why would you use a counter for it? And why would you want to reset it to zero? Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ANNOUNCE] ktap 0.1 released
On Tue, 21 May 2013 11:56:14 +0800 "zhangwei(Jovi)" wrote: > we welcome bug reports and fixes for the issues. I'm messing with it...first impression: unable create tracepoint event sys_enter_mmap on cpu 4, err: -19 unable create tracepoint event sys_enter_mmap on cpu 5, err: -19 unable create tracepoint event sys_enter_mmap on cpu 6, err: -19 unable create tracepoint event sys_enter_mmap on cpu 7, err: -19 [...] Unsurprising, this is a four-core system. The code reads: > for_each_possible_cpu(cpu) > enable_tracepoint_on_cpu(cpu, &attr, call, arg, type); Maybe that needs to be for_each_online_cpu() instead? jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6 v14] gpio: Add userland device interface to block GPIO
On Tue, 22 Jan 2013 13:06:41 +0100
Roland Stigge wrote:
> This patch adds a character device interface to the block GPIO system.
So I was looking at this, and a couple of things caught my eye...
> +static int gpio_block_fop_open(struct inode *in, struct file *f)
> +{
> + int i;
> + struct gpio_block *block = gpio_block_find_by_minor(MINOR(in->i_rdev));
> + int status;
> + int irq;
> +
> + if (!block)
> + return -ENOENT;
> +
> + block->irq_controlled = false;
> + block->got_int = false;
> + spin_lock_init(&block->lock);
So... there is no protection I can find against multiple opens here.
Meaning that the second process to open the device will reinitialize the
lock (and other variables), regardless of their current state.
More to the point, though, I'm not at all clear on what this lock protects?
It seems to be restricted to the got_int flag, which could be manipulated -
without locks - with bitops? Or am I missing something?
> + init_waitqueue_head(&block->wait_queue);
> + f->private_data = block;
> +
> + for (i = 0; i < block->ngpio; i++) {
> + status = gpio_request(block->gpio[i], block->name);
Hmm... the documentation for the API says that gpio_request() has to be
called separately. But now you're doing it here? That's probably OK, but
calling gpio_free() at close time could lead to interesting results if the
code that set up the block expects them to still be allocated. It seems
like the API should be consistent with regard to this - either call
gpio_request() when the block is created, or always require the caller to
do it.
A quick look shows that the sysfs interface does the same thing? So now
you're already double-requesting and freeing the gpios?
> + if (status)
> + goto err1;
> +
> + irq = gpio_to_irq(block->gpio[i]);
> + if (irq >= 0 &&
> + !test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags) &&
> + !gpio_block_is_irq_duplicate(block, i)) {
> + status = request_irq(irq, gpio_block_irq_handler,
> + IRQF_SHARED,
> + block->name, block);
> + if (status)
> + goto err2;
> +
> + block->irq_controlled = true;
> + }
> + }
This forces the block to work in the IRQ-driven mode if a line is capable
of it, regardless of whether the creator (or the process calling open())
wants that. It seems like that should be controllable separately?
> +
> + return 0;
> +
> +err1:
> + while (i > 0) {
> + i--;
> +
> + irq = gpio_to_irq(block->gpio[i]);
> + if (irq >= 0 &&
> + !test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags) &&
> + !gpio_block_is_irq_duplicate(block, i))
> + free_irq(irq, block);
> +err2:
> + gpio_free(block->gpio[i]);
Um...wait...you're jumping into the middle of the while loop? I guess that
will work, but ... hmm...
> + }
> + return status;
> +}
> +
> +static int gpio_block_fop_release(struct inode *in, struct file *f)
> +{
> + int i;
> + struct gpio_block *block = (struct gpio_block *)f->private_data;
Is there anything that will have prevented a call to gpio_block_free()
while the device is open? This seems like a concern for all of the fops
here.
> + for (i = 0; i < block->ngpio; i++) {
> + int irq = gpio_to_irq(block->gpio[i]);
> +
> + if (irq >= 0 &&
> + !test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags) &&
> + !gpio_block_is_irq_duplicate(block, i))
> + free_irq(irq, block);
> +
> + gpio_free(block->gpio[i]);
> + }
> +
> + return 0;
> +}
> +
> +static int got_int(struct gpio_block *block)
> +{
> + unsigned long flags;
> + int result;
> +
> + spin_lock_irqsave(&block->lock, flags);
> + result = block->got_int;
> + spin_unlock_irqrestore(&block->lock, flags);
The lock doesn't really buy you much here. Might you have wanted to reset
block->got_int here too?
> +
> + return result;
> +}
> +
> +static ssize_t gpio_block_fop_read(struct file *f, char __user *buf, size_t
> n,
> +loff_t *offset)
> +{
> + struct gpio_block *block = (struct gpio_block *)f->private_data;
> + int err;
> + unsigned long flags;
> +
> + if (block->irq_controlled) {
> + if (!(f->f_flags & O_NONBLOCK))
> + wait_event_interruptible(block->wait_queue,
> + got_int(block));
> + spin_lock_irqsave(&block->lock, flags);
> + block->got_int = 0;
> + spin_unlock_irqrestore(&block->lock, flags);
> + }
If two processes are waiting on the device, they might bot
Re: [PATCH 1/1] ethernet driver: dm9000: davicom: Upgrade the driver to suit for all DM9000 series chips
On Mon, 25 Mar 2013 16:04:42 +0800
Joseph CHANG wrote:
> From: Joseph CHANG
>
> Some necessary modification for DM9000 series chips to be better in operation!
Interesting, this had been sent to me privately at about the same time.
For the record, here's what I sent back:
From: Jonathan Corbet
To: Allen Huang (黃偉格)
Subject: Re: Davicom Linux driver
Date: Mon, 25 Mar 2013 09:55:48 -0600
Organization: LWN.net
On Mon, 25 Mar 2013 18:31:35 +0800
Allen Huang (黃偉格) wrote:
> Please see our Linux Patch in the attachment and we would appreciate any
> comments that you have on our driver and/or whether it is ready to go into
> the kernel. Thanks.
I'll take a quick look at it. But the only true way to know whether it's
ready, of course, is to post it to the public lists.
> Some necessary modification for DM9000 series chips to be better in
> operation!
The patch will be better received if you say what "better in operation"
actually means. Are you supporting new hardware, adding new features,
fixing bugs? Hopefully not more than one of those in a single patch.
> Had tested to all of DM9000 series chips, include DM9000E, DM9000A, DM9000B,
> and DM9000C in X86 and ARM embedded Linux system these years.
>
> Signed-off-by: Joseph CHANG
> ---
> drivers/net/ethernet/davicom/dm9000.c | 28 +++-
> drivers/net/ethernet/davicom/dm9000.h |4 +++-
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/davicom/dm9000.c
> b/drivers/net/ethernet/davicom/dm9000.c
> index 8cdf025..54bdc92 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -47,7 +47,8 @@
> #define DM9000_PHY 0x40/* PHY address 0x01 */
>
> #define CARDNAME "dm9000"
> -#define DRV_VERSION "1.31"
> +/* [KERN-ADD-1](upgrade) */
I would lose the [KERN-ADD*] comments, those will certainly draw some
grumbling when the patch is posted publicly. They don't add any value; the
repository will show which changes were made when.
> +#define DRV_VERSION "1.39"
>
> /*
> * Transmit timeout, default 5 seconds.
> @@ -671,10 +672,16 @@ dm9000_poll_work(struct work_struct *w)
> if (netif_msg_link(db))
> dm9000_show_carrier(db, new_carrier, nsr);
>
> - if (!new_carrier)
> + /* [KERN-ADD-2] */
> + if (!new_carrier) {
> netif_carrier_off(ndev);
> - else
> + netdev_info(ndev, "[%s Ethernet Driver, V%s]:
> Link-Down!!\n",
> + CARDNAME, DRV_VERSION);
> + } else {
> netif_carrier_on(ndev);
> + netdev_info(ndev, "[%s Ethernet Driver, V%s]:
> Link-Up!!\n",
> + CARDNAME, DRV_VERSION);
> + }
So "better in operation" means more log messages? This, too, will likely
draw complaints. That could be a lot of log messages! The additional
information is also probably unnecessary, netdev_info() should print the
relevant device information.
> }
> } else
> mii_check_media(&db->mii, netif_msg_link(db), 0);
> @@ -794,6 +801,9 @@ dm9000_init_dm9000(struct net_device *dev)
> (dev->features & NETIF_F_RXCSUM) ? RCSR_CSUM : 0);
>
> iow(db, DM9000_GPCR, GPCR_GEP_CNTL);/* Let GPIO0 output */
> +/* [KERN-ADD-3] */
> + dm9000_phy_write(dev, 0, 0, 0x8000); /* reset PHY */
> + mdelay(2);
Magic constants aren't good, you can't define a symbol for that?
dm900_poll_work() is a workqueue function, right? So it can sleep. So you
should almost certainly use msleep() rather than mdelay().
> ncr = (db->flags & DM9000_PLATF_EXT_PHY) ? NCR_EXT_PHY : 0;
>
> @@ -830,6 +840,8 @@ dm9000_init_dm9000(struct net_device *dev)
> db->tx_pkt_cnt = 0;
> db->queue_pkt_len = 0;
> dev->trans_start = jiffies;
> +/* [KERN-ADD-4] */
> + dm9000_phy_write(dev, 0, 27, 0xE100);
Again, can we avoid the magic constants?
> }
>
> /* Our watchdog timed out. Called by the networking layer */
> @@ -1181,7 +1193,8 @@ dm9000_open(struct net_device *dev)
>
> /* GPIO0 on pre-activate PHY, Reg 1F is not set by reset */
> iow(db, DM9000_GPR, 0); /* REG_1F bit0 activate phyxcer */
> - mdelay(1); /* delay needs by DM9000B */
> + /* [KERN-ADD-5](Enable PHY) */
> + mdelay(2); /* delay needs by DM9000B */
Re: dummy irq trace 'Flags mismatch'
On Tue, 30 Apr 2013 06:59:22 +0200 (CEST)
Jiri Kosina wrote:
> Or have it depend on CONFIG_EXPERT would probably make most sense ... ?
We could also just have it tell you when you screwed up? Something like
the following (compile tested only)?
jon
---
dummy-irq: require the user to specify an IRQ number
Reported-by: Dave Jones
Signed-off-by: Jonathan Corbet
diff --git a/drivers/misc/dummy-irq.c b/drivers/misc/dummy-irq.c
index 7014167..c37eeed 100644
--- a/drivers/misc/dummy-irq.c
+++ b/drivers/misc/dummy-irq.c
@@ -19,7 +19,7 @@
#include
#include
-static int irq;
+static int irq = -1;
static irqreturn_t dummy_interrupt(int irq, void *dev_id)
{
@@ -36,6 +36,10 @@ static irqreturn_t dummy_interrupt(int irq, void *dev_id)
static int __init dummy_irq_init(void)
{
+ if (irq < 0) {
+ printk(KERN_ERR "dummy-irq: no IRQ given. Use irq=N\n");
+ return -EIO;
+ }
if (request_irq(irq, &dummy_interrupt, IRQF_SHARED, "dummy_irq", &irq))
{
printk(KERN_ERR "dummy-irq: cannot register IRQ %d\n", irq);
return -EIO;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH tip 5/5] tracing filter examples in BPF
On Mon, 2 Dec 2013 20:28:50 -0800 Alexei Starovoitov wrote: > GCC-BPF backend is available on github > (since gcc plugin infrastructure doesn't allow for out-of-tree backends) Do you have a pointer to where this backend can be found? I've done a bit of digging around but seem to be unable to find it... Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
On Wed, 4 Dec 2013 14:46:59 -0800
"Paul E. McKenney" wrote:
> From: "Paul E. McKenney"
>
> The situations in which ACCESS_ONCE() is required are not well documented,
> so this commit adds some verbiage to memory-barriers.txt.
[...]
> + But please note that the compiler is also closely watching what you
> + do with the value after the ACCESS_ONCE(). For example, suppose you
> + do the following and MAX is a preprocessor macro with the value 1:
> +
> + for ((tmp = ACCESS_ONCE(a)) % MAX)
> + do_something_with(tmp);
That sure looks like it was meant to be "while" instead of "for"?
[...]
> + (*) The compiler is within its rights to reorder memory accesses unless
> + you tell it not to. For example, consider the following interaction
> + between process-level code and an interrupt handler:
> +
> + void process_level(void)
> + {
> + msg = get_message();
> + flag = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (flag)
> + process_message(msg);
> + }
> +
> + There is nothing to prevent the the compiler from transforming
> + process_level() to the following, in fact, this might well be a
> + win for single-threaded code:
> +
> + void process_level(void)
> + {
> + flag = true;
> + msg = get_message();
> + }
> +
> + If the interrupt occurs between these two statement, then
> + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> + to prevent this as follows:
> +
> + void process_level(void)
> + {
> + ACCESS_ONCE(msg) = get_message();
> + ACCESS_ONCE(flag) = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (ACCESS_ONCE(flag))
> + process_message(ACCESS_ONCE(msg));
> + }
Looking at this, I find myself wondering why you couldn't just put a
barrier() between the two statements in process_level()? ACCESS_ONCE()
seems like a heavy hammer to just avoid reordering of two assignments.
What am I missing, and what could be added here to keep the other folks as
dense as me from missing the same thing?
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions
On Fri, 25 Jul 2014 12:38:08 -0500
Abhi Das wrote:
> This patch adds support in GFS2 for the xgetdents syscall by
> implementing the xreaddir file operation.
So I was trying to make sense of this, and ran into one little thing that
jumped out at me:
> +static int gfs2_xrdir_to_user_vars(struct gfs2_xrdir_ctx *xc,
> +struct gfs2_xdirent *x,
> +struct gfs2_xdirent *x_vb_p,
> +struct linux_xdirent __user *lxd,
> +size_t count, size_t *bytes)
Now, I'll readily admit that I could be overly confused by this function.
When the variables are named "x", "xx", "xc", "x_vb_p", "xblob", and "lxd",
it all starts to run together. But still...
> + if ((xc->xc_xattr_mask & XSTAT_XATTR_ALL) &&
> + lxd->xd_blob.xb_xattr_count) {
How can that be right? lxd is __user, it doesn't seem right to be
dereferencing it directly...?
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions
On Tue, 29 Jul 2014 18:25:57 -0400 (EDT)
Abhijith Das wrote:
> > > + if ((xc->xc_xattr_mask & XSTAT_XATTR_ALL) &&
> > > + lxd->xd_blob.xb_xattr_count) {
> >
> > How can that be right? lxd is __user, it doesn't seem right to be
> > dereferencing it directly...?
>
> Wouldn't the call to access_ok() at the start of the syscall take care of
> this? All the
> __user pointers point to areas within the user supplied buffer buf and
> overflow past the
> end of the buffer for the last lxd is checked for.
No, dereferencing user-space pointers in the kernel is never OK. What
if user space remapped that page after the access_ok() call? You need
to use copy_*_user() to get at user-space structures from the kernel.
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 net-next 0/2] split BPF out of core networking
On Mon, 2 Jun 2014 00:01:44 -0700 Alexei Starovoitov wrote: > This patch set splits BPF out of core networking into generic component Quick, probably dumb question: if you're going to split it out, why not split it out entirely, into kernel/ or (perhaps better) lib/? The whole point seems to be that BPF is outgrowing its networking home, so it seems like it might be better to make it truly generic. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 net-next] ipv4: add kernel parameter tcpmhash_entries
On Tue, 4 Nov 2014 20:23:08 +0100 Fabian Frederick wrote: > + Set tcp_metrics_hash slot number. So I'm not entirely clear on what that means. Did you mean "Set the number of tcp_metrics_hash slots"? If so, that wording should be used. Even in that case, though, what does this parameter actually *mean*? If we're going to document the parameter (and I think we should), it would be good to say why a user might want to change it and what effects they would expect to see. Care to redo with that in mind? Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH linux-next] Documentation: Build mic/mpssd only for x86_64
On Thu, 4 Dec 2014 13:27:29 -0800 Ashutosh Dixit wrote: > mic/mpssd along with MIC drivers are currently only usable on > x86_64. So build mic/mpssd only for x86_64 to avoid build breaks on > big-endian systems. I can certainly apply this. But it seems to me that this kind of code doesn't belong in the documentation directory. How about a patch to move it to tools/ ? jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kobject: grammar fix
On Sat, 6 Dec 2014 16:38:57 -0500 John de la Garza wrote: > -When the kobject is removed from the kernel (details on how to do that is > +When the kobject is removed from the kernel (details on how to do that are Makes sense, applied to the docs tree. Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] Update of misc 00-INDEXen in Documentation/
On Fri, 26 Dec 2014 09:26:21 +0100 Henrik Austad wrote: > Last time I went through some of the files, Andrew Morton questioned the > usefullness of the index-files [1]. Rob pointed out that he used to use > them for auto-generating some HTML-docs. OTOH, Paul G. noted that he > would not object to their blanket-removal. I'm not sure how useful they are either, but, as long as we have them, they might as well be current. So I've applied these, thanks. jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
