Re: [PATCH] Acorn SCSI loading
Torben Mathiasen writes: Just noticed the SCSI drivers for the ACORN bus weren't updated to the new initialization. AFAIK these drivers couldn't have been functional without this patch. Please don't apply these! I've already got patches in the ARM tree, and the -ac tree to fix this. The problem is to do the necessary stuff to get them into Linus tree. (They've existed since the SCSI changes came about). _ |_| - ---+---+- | |Russell King [EMAIL PROTECTED] --- --- | | | |http://www.arm.linux.org.uk// / | | +-+-+ --- -+- / | THE developer of ARM Linux |+| /|\ / | | | --- | +-+-+ - /\\\ | - 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: Inconsistent #ifdef __KERNEL__ on different architectures
On Sun, May 27, 2001 at 11:07:38PM +0200, Adrian Bunk wrote: I do also explicitely send this mail to the people that seem to be responsible for the pieces of code I touch. I'm not sure whether the compete removal of the #ifdef __KERNEL__'s is too rude but there are already other architectures that don't have it in their architecture specific versions of these files. You cannot use the kernel atomic/interrupt functions from userspace on ARM. You cannot disable interrupts in userspace, and therefore the kernel atomic functions do not work as you expect them to. If it is to do with code to be included into the kernel, then why aren't you defining __KERNEL__ ? Therefore this change (as far as ARM goes) makes zero sense. -- Russell King ([EMAIL PROTECTED])The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html - 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.4.2,3 nbd problem, works OK in 2.4.2-ac20,28
On Tue, Apr 03, 2001 at 04:16:04PM +0400, Vladimir Serov wrote: Unfortunately the details of handling these requests aren't clear for me and it's not simple to use Alan Cox patches on ARM cause there not supported by Russell King and other people in ARM community (I mean no patches again -acxx kernels) and i'm already overloaded by various beta and alpha software. I'll look into the possibility of rooting out the fix in the -ac tree (if any) tomorrow and dropping it into the next ARM tree. _ |_| - ---+---+- | |Russell King [EMAIL PROTECTED] --- --- | | | |http://www.arm.linux.org.uk// / | | +-+-+ --- -+- / | THE developer of ARM Linux |+| /|\ / | | | --- | +-+-+ - /\\\ | - 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]: Atmel Serial Console interrupt handler splitup
On Mon, Dec 17, 2007 at 09:56:30PM +0100, Remy Bohmer wrote: +#define lread(port) __raw_readl(port) +#define lwrite(v, port) __raw_writel(v, port) Why is this necessary, and what does 'l' stand for? There is a huge list of macros below these definitions. By doing it this way, the macros still fit on 80 characters wide, while without them, I had split up the macros over several lines, which does not make it more readable. That's all. 'l' refers at the last letter of __raw_readl, and writel. Nothing special. So why not keep to the Linux convention? How about at_readl() and at_writel() ? /* + * receive interrupt handler. + */ +static inline void +atmel_handle_receive(struct uart_port *port, unsigned int pending) Please drop inline here. The compiler will do it automatically if it has only one caller, and if it at some point gets several callers, we might not want to inline it after all. Funny, This was the first thing that Andrew started complaining about. He suggested to put an inline there which I had not. I already mentioned that this was against the CodingStyle, but I also mentioned that I did not wanted to start a fight about this :-) So, to prevent a discussion, I added the inline... There's two schools of thought - those who want to add 'inline' keywords all over the place and those who don't. It's quite correct that if a static function will be inlined by the compiler as it sees fit. It _can_ be that the compiler will chose not to inline it and that may result in better register allocation in the caller, resulting in overall faster code. + while (!(UART_GET_CSR(port) ATMEL_US_TXEMPTY)) + barrier(); Should probably use cpu_relax(), but it's probably out of scope for a codingstyle cleanup patch (and I don't think it matters on AVR32 or ARM.) Agree. Even though it doesn't matter at the moment, I rather like to think a bit about the future. If the kernel has a simple and cheap mechanism there's no reason to avoid using it. /* - * First, save IMR and then disable interrupts + * First, save IMR and then disable interrupts */ imr = UART_GET_IMR(port); /* get interrupt mask */ UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY); @@ -790,30 +828,32 @@ static void atmel_console_write(struct c uart_console_write(port, s, count, atmel_console_putchar); /* - * Finally, wait for transmitter to become empty - * and restore IMR + * Finally, wait for transmitter to become empty + * and restore IMR */ Looks like you're replacing TABs with spaces. Why? I think someone's mailer might be messing with the patches. The above removed and added lines appear to be identical. -- 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: syscall in a module
On Thu, Dec 20, 2007 at 10:03:53AM +0100, Pierre Savary wrote: I need some help about syscalls and modules. In fact I want to adapt the g_file_storage module to my own application. I need to use sys_umount and sys_mount but when I compile I obtain this message : [...] Kernel: arch/arm/boot/zImage is ready Building modules, stage 2. MODPOST 4 modules ERROR: sys_umount [drivers/usb/gadget/g_file_storage.ko] undefined! make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2 [...] So, how can I use sys_umount and sys_mount in this module? You don't. Syscalls are NOT for modules because they modify the filesystem, which depends on the user context. For example, processes running chroot have a different view of the filesystem from those which aren't - and if you happen to be executing with the context of a chrooted user process when you mount, and a non-chrooted process when you umount. I suggest you describe what you're trying to do in greater detail, and/or ask on the main kernel list - but note that if you ask the same question without explaining why you need it _and_ asking for alternative ideas, you'll get the same kind of response there. -- 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: syscall in a module
On Thu, Dec 20, 2007 at 10:26:08AM +0100, Pierre Savary wrote: In fact I have a part (/dev/mmcblk0p5) that I want share with USB gadget (g_file_storage). But if USB is not connect to a PC, I want that this part was mounted on my board. An I want unmount this part when the board is plugged on a PC. So I need to mount and unmount partition in this module. Arrange for the USB subsystem to send a uevent, which can then be picked up by userspace to issue the correct command to mount or unmount the partition. -- 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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Wed, Nov 28, 2007 at 03:19:14PM -0800, Daniel Walker wrote: On Wed, 2007-11-28 at 23:03 +, Russell King - ARM Linux wrote: On Wed, Nov 28, 2007 at 04:13:07PM -0500, Steven Rostedt wrote: On Wed, 28 Nov 2007, Daniel Walker wrote: Ignoring the ARM side of things for a sec, handle_simple_irq() will mask() the interrupt in the special case that an interrupt is already in the processes of being handled.. handle_simple_irq() also unmasks when it finishes handling an interrupt (something real time adds for some reason) .. In terms of threading the irq everything is the same except there is no unmask() call when the thread finishes .. OK, to be honest, I never fully understood the concept of this simple_irq. I figured it was because of the ARM architecture. If you read what I said compared with what Daniel said, you'll see that adding the mask/unmask is _pointless_ because for the case where the simple handler should be used, there is _no_ hardware masking available except via the parent interrupt signal. So actually Daniel's argument misses the basic point - that using handle_simple_irq for non-simple IRQs is just WRONG. Well we've got at least two ARM boards which need the additional unmask to work correctly with interrupt threading .. So there must be at least two ARM boards using handle_simple_irq incorrectly .. It sounds like you would prefer we send patches to move those handle_simple_irq users into another method ? If you read my explaination of the purpose of handle_simple_irq() you'll see that for the _correct_ case where this handler should be used, the mask and unmask can only be empty. To repeat for the third time - it's for the case where the hardware supplies NO way to mask the interrupt. So adding calls to the mask/unmask methods would be pointless for the correct use case. If you do have the ability to mask, then you should be using the level or edge handlers. - 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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Thu, Nov 29, 2007 at 11:14:30AM +0100, Remy Bohmer wrote: I do not think Russell is right here with assuming that the wrong interrupt handler type is used. Looking at the behaviour of the mainline kernel (non-RT), the implementation is quite different: On mainline the handle_simple_irq() in chip.c is not re-entrant, the masking is **only** done in case of errors, and therefor never unmasked again, of course. The issue comes down to a very simple question: Are you using handle_simple_irq() with an interrupt which can be masked? If the answer to that question is 'yes' then you're using the wrong handler. If 'no' then it's the right handler and the mask/unmask methods associated with the interrupt will be no-ops. Therefore, if you have mask/unmask methods for a simple IRQ which actually do something, clearly the first answer is the one which applies. You're using the wrong handler. Use the level or edge handlers instead. - 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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Thu, Nov 29, 2007 at 12:27:30PM +0100, Remy Bohmer wrote:
Hello Russell,
If 'no' then it's the right handler and the mask/unmask methods associated
with the interrupt will be no-ops.
I completely understand what you keep on saying, but that would imply
that the following piece of code in chip.c is completely bogus anyway!
(snip from mainline 2.6.23)
It _is_ bogus.
handle_simple_irq(unsigned int irq, struct irq_desc *desc)
...
if (unlikely(!action || (desc-status IRQ_DISABLED))) {
if (desc-chip-mask)
desc-chip-mask(irq);
...
}
Why trying to mask the interrupt if that is illegal use of the interrupt
type?
No idea, but that code is wrong. Take a look at the original code
which patch 3692/1 removed. You'll notice that it never touches
the mask/unmask handlers.
This piece of code assumes that there CAN be a handler for masking
interrupts for the simple_irq type. This is in contradiction what you
are trying to explain.
Shrug. I'm explaining how _I_ designed it, and the purpose of it being
there.
If people insist on adding the mask/unmask crap to it, the function
might as well be deleted and be an alias for handle_level_IRQ. Because
that's _precisely_ what you lot are turning it into.
Ah, and looking at the changes to the file, the addition of the mask
and unmask was done by someone who didn't understand what this was
trying to do. So that change should be backed out.
-
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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Thu, Nov 29, 2007 at 03:18:04PM +0100, Remy Bohmer wrote: Hello Russell, If people insist on adding the mask/unmask crap to it, the function might as well be deleted and be an alias for handle_level_IRQ. Because that's _precisely_ what you lot are turning it into. First, I want to make clear that I am just debugging a problem on RT that does not exist on mainline, and I am trying to find a way to get it solved nicely _on RT_, and preferable in a way that it works for everybody with the least chance for regression. While I realise that, I'm telling you that the _problem_ is being caused by the wrong handler being used. I already mentioned that RT is doing masking in this code during normal use, where the mainline kernel does not do this, **except** in an error situation. AT91 has edge based GPIO interrupts which need special handling so edges aren't missed. That's what the edge handler is there for - to ensure that edges aren't missed while the interrupt is soft-disabled. SA1100 and PXA have exactly the same setup. They use the correct handler. Why is AT91 special? So, as far as I can tell , the type really used on at91 for the GPIO stuff _is_ a simple_irq as you describe, but one that can be masked/unmasked **in case of errors**. It should never be masked during normal use. So, I propose option 1 to solve it on RT, and thus to trigger Steven to NACK my first patch. I will try and see if I can make it work _without_ masking on RT (except in case of errors, just as in mainline). ...and probably add some clear description about the purpose of simple_irq, especially related to masking... Do you agree on this Russell? Clearly no, I disagree. Ah, and looking at the changes to the file, the addition of the mask and unmask was done by someone who didn't understand what this was trying to do. So that change should be backed out. Maybe he was trying to mask the irq during an error situation? Who knows. The patch says nothing about that change. The change was never copied to me, so I've never reviewed it (and had it been, I would've indicated that the change was wrong.) - 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: [UPDATED PATCH] Support for Toshiba TMIO multifunction devices
On Thu, Nov 22, 2007 at 12:34:09AM +, ian wrote:
+void mfd_free_devices(struct platform_device *devices, int nr_devs)
+{
+ struct platform_device *dev = devices;
+ int i;
+
+ for (i = 0; i nr_devs; i++) {
+ struct resource *res = dev-resource;
+ platform_device_unregister(dev++);
+ kfree(res);
+ }
+ kfree(devices);
+}
+EXPORT_SYMBOL_GPL(mfd_free_devices);
Unfortunately, this is broken as designed (in fact this whole file is.)
I'm not sure why people just don't get it. sysfs. devices. device tree.
It has object lifetime rules. You can _not_ go around unregistering things
and then immediately freeing them - something else might _still_ be using
stuff even after the call to unregister returns.
It's a potential OOPS just waiting to happen.
That's why we have a proper management API for platform devices. Please
use it, I didn't add the code for just for fun.
See platform_device_alloc() + platform_device_add_resources() +
platform_device_add_data() + platform_device_add() to create, and
platform_device_unregister() to destroy.
(Not looked at the rest because you really really need to get this
right first.)
-
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: [UPDATED PATCH] Support for Toshiba TMIO multifunction devices
On Thu, Nov 22, 2007 at 10:52:46AM +, ian wrote:
On Thu, 2007-11-22 at 00:52 +, Russell King - ARM Linux wrote:
On Thu, Nov 22, 2007 at 12:34:09AM +, ian wrote:
Unfortunately, this is broken as designed (in fact this whole file is.)
Fix attached below.
Thanks.
+ /* Allocate space for the subdevice resources temporarily so
+that we can modify them */
+
+res = kzalloc(blk-num_resources * sizeof (struct
resource), GFP_KERNEL);
Looks like your mailer wrapped the patches.
+if (!res)
+goto fail;
Plus weird indentation.
+ platform_device_add_resources(sdev, res, blk-num_resources);
+ kfree(res);
+
+ if (platform_device_add(sdev)) {
+ goto fail;
+ }
+
+ printk(KERN_INFO MFD: registering %s\n, blk-name);
+ }
+ return devices;
+
+fail:
+ mfd_free_devices(devices, i + 1);
If 'sdev' failed to register, you don't want to call platform_device_del()
on it (via platform_device_unregister()) since it's not been registered
into the device tree.
The removal procedure for platform devices is:
if platform_device_alloc() fails, return -ENOMEM.
if platform_device_add() fails, call platform_device_put() on it.
if platform_device_add() suceeds, call either
platform_device_del() + platform_device_put() _or_
platform_device_unregister()
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 6187a85..5e8360a 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -57,6 +57,9 @@ struct resource_list {
#define IORESOURCE_IRQ_LOWLEVEL (13)
#define IORESOURCE_IRQ_SHAREABLE (14)
+/* MFD device IRQ specific bits (IORESOURCE_BITS) */
+#define IORESOURCE_IRQ_MFD_SUBDEVICE(15)
+
Something others (== mainline) needs to see first.
diff --git a/drivers/mfd/t7l66xb.c b/drivers/mfd/t7l66xb.c
new file mode 100644
index 000..e3057f6
--- /dev/null
+++ b/drivers/mfd/t7l66xb.c
@@ -0,0 +1,286 @@
+/*
+ * drivers/mfd/t7l66xb.c
+ *
+ * Toshiba T7L66XB support
+ * Copyright (c) 2005 Ian Molton
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This is based on my and Dirk Opfers work on the tc6393xb SoC.
+ *
+ */
+
+#include linux/module.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/delay.h
Do you need linux/delay.h ?
+#include linux/errno.h
+#include linux/ioport.h
+#include linux/irq.h
Do you need linux/irq.h ?
+#include linux/device.h
+#include linux/slab.h
+#include linux/spinlock.h
+
+#include asm/hardware.h
+#include asm/mach-types.h
Doesn't use machine types in this file.
+#include asm/io.h
+#include asm/arch/pxa-regs.h
+
+#include linux/t7l66xb.h
+#include linux/tmio_mmc.h
+#include mfd-core.h
+
+#define platform_get_platdata(_dev) ((_dev)-dev.platform_data)
Should this be something generic? Eg in platform_device.h ?
+ while ( (req = (readb(data-mapbase + T7L66XB_SYS_ISR)
+ ~(readb(data-mapbase + T7L66XB_SYS_IMR ) {
Do the additional parens buy anything here (other than more line noise) ?
+ for (i = 0; i = 7; i++) {
+ int dev_irq = data-irq_base + i;
+ struct irq_desc *d = NULL;
+ if ((req (1i))) {
Ditto.
+ set_irq_data (tchip-irq_nr, tchip);
+set_irq_chained_handler (tchip-irq_nr, t7l66xb_irq_handler);
Weird indentation.
+ set_irq_type (tchip-irq_nr, IRQT_FALLING);
+}
+
+static void t7l66xb_hwinit(struct platform_device *dev)
+{
+ struct t7l66xb_platform_data *pdata = platform_get_platdata(dev);
+
+ if (pdata pdata-hw_init)
+ pdata-hw_init();
+}
+
+
+#ifdef CONFIG_PM
+
+static int t7l66xb_suspend(struct platform_device *dev, pm_message_t
state)
+{
+ struct t7l66xb_platform_data *pdata = platform_get_platdata(dev);
+
+
+ if (pdata pdata-suspend)
+ pdata-suspend();
+
+ return 0;
+}
+
+static int t7l66xb_resume(struct platform_device *dev)
+{
+ struct t7l66xb_platform_data *pdata = platform_get_platdata(dev);
+
+ if (pdata pdata-resume)
+ pdata-resume();
+
+ t7l66xb_hwinit(dev);
+
+ return 0;
+}
+#endif
+
+static int t7l66xb_probe(struct platform_device *dev)
+{
+ struct t7l66xb_platform_data *pdata = dev-dev.platform_data;
+ unsigned long pbase = (unsigned long)dev-resource[0].start;
+ unsigned long plen = dev-resource[0].end - dev-resource[0].start;
+ int err = -ENOMEM;
+ struct t7l66xb_data *data;
+
+ data = kmalloc (sizeof (struct t7l66xb_data), GFP_KERNEL);
+ if (!data)
+ goto out
Re: [PATCH PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Wed, Nov 28, 2007 at 03:38:11PM +0100, Remy Bohmer wrote: Hello Daniel, * Note: The caller is expected to handle the ack, clear, mask and * unmask issues if necessary. So we shouldn't need any flow control unless there is some other factors.. This comment can be misinterpreted, I think. Who is assumed to be the caller in this context? The 2 other routines in the driver that actually do the unmasking stuff besides only calling this routine? Is it allowed to call it directly or should it always be done through a wrapper that does all these special things? The whole point of this simple handler is to accomodate interrupts such as those found on the Neponset board. There, you have a status register in a CPLD but no enable/disable registers. The status register tells you whether the SA, ethernet or 'USAR' chip asserted its interrupt. However, as there is no way to disable the sources, this situation has to be handled carefully - the function decoding the interrupt source needs to mask and unmask the _parent_ interrupt itself, and it's exactly that which the comment is directed towards. See neponset_irq_handler(). The simple IRQ handler is not meant for anything other than that really simple implementation. If people have been using it with interrupts which can be individually masked and unmasked, that's where the bug is. They should be using one of the other handlers. - 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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Wed, Nov 28, 2007 at 02:04:41PM -0500, Steven Rostedt wrote: Thanks for the reply and this nice explanation. I'm taking this as a NACK. Daniel or Remy, could you find the offending users and make send patches to fix them. Note that I'm not acking nor nacking the patch; I'm not involved with the RT stuff and I've never looked at the code, so I don't know what the implications of the patch itself are. I've merely explained the point of the simple irq handler. Maybe the simple irq handler should never have been something that got sucked into the generic IRQ stuff, but kept as something specific to Neponset. - 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 PREEMPT_RT]: On AT91 ARM: GPIO Interrupt handling can/will stall forever
On Wed, Nov 28, 2007 at 04:13:07PM -0500, Steven Rostedt wrote: On Wed, 28 Nov 2007, Daniel Walker wrote: Ignoring the ARM side of things for a sec, handle_simple_irq() will mask() the interrupt in the special case that an interrupt is already in the processes of being handled.. handle_simple_irq() also unmasks when it finishes handling an interrupt (something real time adds for some reason) .. In terms of threading the irq everything is the same except there is no unmask() call when the thread finishes .. OK, to be honest, I never fully understood the concept of this simple_irq. I figured it was because of the ARM architecture. If you read what I said compared with what Daniel said, you'll see that adding the mask/unmask is _pointless_ because for the case where the simple handler should be used, there is _no_ hardware masking available except via the parent interrupt signal. So actually Daniel's argument misses the basic point - that using handle_simple_irq for non-simple IRQs is just WRONG. - 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][AT91] Fix compile error for at91rm9200 in latest git
On Tue, Dec 04, 2007 at 09:14:29AM +0100, Jan Altenberg wrote: Hi all, Your patch looks correct, and seems to be the only obvious chunk that's missing. So, I'll ack it FWIW ... usual policy for these patches is to go through Russell. You can add my Ack for what it's worth. OK, CC'ed Russell and added your Acked-by. Signed-off-by: Jan Altenberg [EMAIL PROTECTED] Acked-by: Andrew Victor [EMAIL PROTECTED] - patch system please. -- 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: READ THIS NOW *BEFORE* RESPONDING TO THIS THREAD -- [PATCH 0/2] gpiolib: add support for PCA9539
On Fri, Dec 14, 2007 at 05:16:46PM +0100, Jean Delvare wrote: Hi Eric, On Mon, 10 Dec 2007 17:37:05 +0800, eric miao wrote: Support for PCA9539 as a GPIO chip is separated into two patches: 0001 - gpiolib: basic support for 16-bit PCA9539 GPIO expander 0002 - gpiolib: add Generic IRQ support for 16-bit PCA9539 GPIO expander the 2nd one uses workqueue for IRQ handling due to the interrupt mode nature of the i2c-core, thus making it a separate patch. Note that the situation could change according to the discussion in this thread: http://lists.lm-sensors.org/pipermail/i2c/2007-December/002378.html I'm afraid to have to say this, but your message (with its broken CC header) has caused several people to be unsubscribed from this mailing list. The reason seems to be that the list expander rejected your message, which causes bounces to be sent back to mailman for a _ton_ of people. Strangely enough, I run the exact same check (headers_syntax) on all messages the list server receives, and when I test it with your message, it correctly rejects it. Yet somehow it didn't earlier. No idea. In any case, EVERYONE, please drop the invalid David, in the CC list. -- 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] Tosa keyboard support
On Tue, Dec 11, 2007 at 06:38:51PM +0300, Dmitry Baryshkov wrote: Sorry, posted wrong version of patch. Here is correct version: Support keyboard on tosa (Sharp Zaurus SL-6000x). Largely based on patches by Dirk Opfer. Looks fine to me, but Dmitry Torokhov needs to look at it; he maintains the input subsystem. Note that the current input subsystem mailing list is not the one you have in the CC line - please always check MAINTAINERS for the correct addresses. -- 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: syscall in a module
On Thu, Dec 20, 2007 at 11:50:59AM +0100, Pierre Savary wrote: Could you give me some sample, because I have no idea to how to send a uevent in userspace ... TBH I don't know - only that my suggestion would be the most logical way to go about it. Maybe looking in MAINTAINERS and asking in a more appropriate place would be more fruitful? DRIVER CORE, KOBJECTS, AND SYSFS P: Greg Kroah-Hartman M: [EMAIL PROTECTED] L: linux-kernel@vger.kernel.org T: quilt kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/ S: Supported -- 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.24-rc5 sysfs pci bridge duplicate symlink
On Fri, Dec 28, 2007 at 03:03:22PM -0800, Andrew Morton wrote:
On Fri, 28 Dec 2007 13:11:37 + Bahadir Balban [EMAIL PROTECTED] wrote:
Hi,
On ARM with PCI, I get this error since -rc2 (didn't try rc1):
sysfs: duplicate filename 'bridge' can not be created
WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
[c00287fc] (dump_stack+0x0/0x14) from [c00cbc64]
(sysfs_add_one+0x50/0xf4)
[c00cbc14] (sysfs_add_one+0x0/0xf4) from [c00ccd30]
(sysfs_create_link+0xec/0x184)
r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
[c00ccc44] (sysfs_create_link+0x0/0x184) from [c0160b44]
(pci_bus_add_devices+0xf0/0x144)
r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
[c0160a54] (pci_bus_add_devices+0x0/0x144) from [c0160b30]
(pci_bus_add_devices+0xdc/0x144)
r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
[c0160a54] (pci_bus_add_devices+0x0/0x144) from [c0160b30]
(pci_bus_add_devices+0xdc/0x144)
r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
[c0160a54] (pci_bus_add_devices+0x0/0x144) from [c000a934]
(pci_common_init+0x148/0x184)
r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0
Any idea why?
(suitable cc added)
Looks to me as if the code added to pci_bus_add_devices() is off it's
rocker.
In brief, pci_bus_add_devices() is supposed to be callable given *any*
state of the bus, and it will add any new devices found in the bus tree
from the bus that it's called for downwards. (That's how I designed
that bit of PCI code - so it can cope with hotpluggable PCI bridges.)
It still does this. However, some bright spark decided to add sysfs
links to it - and break it. Now, whenever we descend into a subordinate
bus (and return to a higher level) we will try to create a sysfs symlink
even if the bus is one that we already registered.
So... that sysfs_create_link() call in there should not be unconditional.
It should _only_ ever do that call if list_empty(dev-subordinate-node)
was true. IOW, we only create symlinks for subordinate bus that we don't
already know about.
It looks like it was introduced by Greg's PCI: fix __must_check warnings
but why a patch advertised _solely_ as a fix is adding additional sysfs
attributes I've no idea.
Something like the following should fix it. Bahadir - please test.
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9e5ea07..132a91f 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -131,18 +131,21 @@ void pci_bus_add_devices(struct pci_bus *bus)
* it and then scan for unattached PCI devices.
*/
if (dev-subordinate) {
- if (list_empty(dev-subordinate-node)) {
+ int new_bus = list_empty(dev-subordinate-node);
+ if (new_bus) {
down_write(pci_bus_sem);
list_add_tail(dev-subordinate-node,
dev-bus-children);
up_write(pci_bus_sem);
}
pci_bus_add_devices(dev-subordinate);
- retval =
sysfs_create_link(dev-subordinate-class_dev.kobj,
- dev-dev.kobj, bridge);
- if (retval)
- dev_err(dev-dev, Error creating sysfs
- bridge symlink, continuing...\n);
+ if (new_bus) {
+ retval =
sysfs_create_link(dev-subordinate-class_dev.kobj,
+ dev-dev.kobj,
bridge);
+ if (retval)
+ dev_err(dev-dev, Error creating
sysfs
+ bridge symlink,
continuing...\n);
+ }
}
}
}
--
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] arch/arm: allow a scno of -1 to not cause a SIGILL
On Thu, Nov 01, 2012 at 12:46:38PM -0700, Kees Cook wrote:
From: Will Drewry w...@chromium.org
On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.
ARM is different. This adds a lightweight check to arm_syscall()
to make sure that ARM behaves the same way.
Signed-off-by: Will Drewry w...@chromium.org
Signed-off-by: Kees Cook keesc...@chromium.org
---
arch/arm/kernel/traps.c |4
1 file changed, 4 insertions(+)
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index b0179b8..f303ea6 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -540,6 +540,10 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
struct thread_info *thread = current_thread_info();
siginfo_t info;
+ /* Emulate/fallthrough. */
+ if (no == -1)
+ return regs-ARM_r0;
+
This won't work properly with OABI. The problem is that OABI has an
offset on its syscall numbers which is removed/added at appropriate
times, and this is one of the places where it's put back. So you end
up with -1 XOR 0x90 here, not -1.
It'd probably be better to do this check in the asm code here, which
prevents that yuckyness from affecting this.
__sys_trace:
mov r1, scno
add r0, sp, #S_OFF
bl syscall_trace_enter
adr lr, BSYM(__sys_trace_return)@ return address
mov scno, r0@ syscall number (possibly new)
add r1, sp, #S_R0 + S_OFF @ pointer to regs
cmp scno, #NR_syscalls @ check upper syscall limit
ldmccia r1, {r0 - r6} @ have to reload r0 - r6
stmccia sp, {r4, r5}@ and update the stack args
ldrcc pc, [tbl, scno, lsl #2] @ call sys_* routine
+ cmp scno, #-1
bne 2b
+ b ret_slow_syscall
--
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/4] arch/arm: move secure_computing into trace
On Thu, Nov 01, 2012 at 12:46:37PM -0700, Kees Cook wrote:
#ifdef CONFIG_SECCOMP
- tst r10, #_TIF_SECCOMP
- beq 1f
- mov r0, scno
- bl __secure_computing
- add r0, sp, #S_R0 + S_OFF @ pointer to regs
- ldmia r0, {r0 - r3} @ have to reload r0 - r3
-1:
+ tst r10, #_TIF_SECCOMP @ is seccomp enabled?
+ bne __sys_trace
#endif
tst r10, #_TIF_SYSCALL_WORK @ are we tracing syscalls?
It's pointless having:
tst r10, #_TIF_SECCOMP
bne __sys_trace
tst r10, #_TIF_SYSCALL_WORK
bne __sys_trace
Instead, make TIF_SECCOMP be bit 11, combine it into _TIF_SYSCALL_WORK, and
eliminate all of that CONFIG_SECCOMP block.
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 739db3a..6b0e14b 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,13 +916,15 @@ enum ptrace_syscall_dir {
PTRACE_SYSCALL_EXIT,
};
-static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
- enum ptrace_syscall_dir dir)
+asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
{
unsigned long ip;
current_thread_info()-syscall = scno;
+ if (secure_computing(scno) == -1)
+ return -1;
+
if (!test_thread_flag(TIF_SYSCALL_TRACE))
return scno;
I'm not sure this change is correct (combined with your hunk below).
What if we have auditing enabled but trace disabled? How do we reach
audit_syscall_entry()? Or the tracehook stuff?
This patch looks wrong in too many ways.
@@ -931,20 +933,13 @@ static int ptrace_syscall_trace(struct pt_regs *regs,
int scno,
* IP = 0 - entry, =1 - exit
*/
ip = regs-ARM_ip;
- regs-ARM_ip = dir;
-
- if (dir == PTRACE_SYSCALL_EXIT)
- tracehook_report_syscall_exit(regs, 0);
- else if (tracehook_report_syscall_entry(regs))
+ regs-ARM_ip = PTRACE_SYSCALL_ENTER;
+ if (tracehook_report_syscall_entry(regs))
current_thread_info()-syscall = -1;
-
regs-ARM_ip = ip;
- return current_thread_info()-syscall;
-}
-asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
-{
- scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+ scno = current_thread_info()-syscall;
+
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
trace_sys_enter(regs, scno);
audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs-ARM_r0, regs-ARM_r1,
@@ -954,7 +949,23 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs,
int scno)
asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
{
- scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+ unsigned long ip;
+
+ current_thread_info()-syscall = scno;
+
+ if (!test_thread_flag(TIF_SYSCALL_TRACE))
+ return scno;
+
+ /*
+ * IP is used to denote syscall entry/exit:
+ * IP = 0 - entry, =1 - exit
+ */
+ ip = regs-ARM_ip;
+ regs-ARM_ip = PTRACE_SYSCALL_EXIT;
+ tracehook_report_syscall_exit(regs, 0);
+ regs-ARM_ip = ip;
+
+ scno = current_thread_info()-syscall;
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
trace_sys_exit(regs, scno);
audit_syscall_exit(regs);
--
1.7.9.5
--
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] ARM: plat-versatile: move FPGA irq driver to drivers/irqchip
On Thu, Nov 01, 2012 at 11:20:10PM +0100, Thomas Petazzoni wrote: Linus, On Thu, 1 Nov 2012 22:28:49 +0100, Linus Walleij wrote: +void fpga_handle_irq(struct pt_regs *regs); This function does not need to be exposed in a public header: as proposed for the bcm2835 and armada-370-xp IRQ controller drivers, the driver should directly do handle_arch_irq = fpga_handle_irq, and therefore there is no need for the machine desc structure to reference fpga_handle_irq anymore. Err no, then you don't understand what's going on here. This may or may not be a top-level IRQ handler. Some ARM platforms have three of these cascaded, others have one of these cascaded off a VIC or GIC. To override the top level IRQ handler unconditionally is going to break platforms. -- 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] ARM: gic: fix irq affinity mismatch.
On Mon, Sep 24, 2012 at 12:06:19PM +0900, Chanho Min wrote: Even if we set affinity to multiple cpus via procfs, gic_set_affinity allows to set one target only. But, '/proc/irq/xxx/smp_affinity' shows as we are successful in the setting of multiple affinity. Thus irq_data- affinity mismatches with the GICD_ITARGETSRs. fix it as bellows. - Set the default affinity to the boot cpu in gic_dist_init. thereafter, irq_data-affinity will be set to default 1-bit affinity. - Even if we write multiple bits to smp_affinity, both irq_data and gic register will be set one cpu. procfs will show 1 bit affinity as well. This has come up before. There is no requirement to make the affinity mask reflect back to userspace what the hardware is doing. So, NAK. -- 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 RESEND 09/17] ARM: LPAE: use phys_addr_t for initrd location and size
On Fri, Sep 21, 2012 at 11:56:07AM -0400, Cyril Chemparathy wrote: From: Vitaly Andrianov vita...@ti.com This patch fixes the initrd setup code to use phys_addr_t instead of assuming 32-bit addressing. Without this we cannot boot on systems where initrd is located above the 4G physical address limit. The description needs updating. You're no longer using phys_addr_t for the initrd size. -- 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 RESEND 06/17] ARM: LPAE: use signed arithmetic for mask definitions
On Fri, Sep 21, 2012 at 11:56:04AM -0400, Cyril Chemparathy wrote: This patch applies to PAGE_MASK, PMD_MASK, and PGDIR_MASK, where forcing unsigned long math truncates the mask at the 32-bits. This clearly does bad things on PAE systems. This patch fixes this problem by defining these masks as signed quantities. We then rely on sign extension to do the right thing. Signed-off-by: Cyril Chemparathy cy...@ti.com Signed-off-by: Vitaly Andrianov vita...@ti.com Reviewed-by: Nicolas Pitre n...@linaro.org --- arch/arm/include/asm/page.h |2 +- arch/arm/include/asm/pgtable-3level.h |6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h index ecf9019..1e0fe08 100644 --- a/arch/arm/include/asm/page.h +++ b/arch/arm/include/asm/page.h @@ -13,7 +13,7 @@ /* PAGE_SHIFT determines the page size */ #define PAGE_SHIFT 12 #define PAGE_SIZE(_AC(1,UL) PAGE_SHIFT) -#define PAGE_MASK(~(PAGE_SIZE-1)) +#define PAGE_MASK(~((1 PAGE_SHIFT) - 1)) It's strange. Every other platform in the kernel, including those with PAE, manage to get away with defining PAGE_MASK as the original above (see asm-generic/pgtable.h) Why is ARM any different? Note that PAGE_MASK gets used at the moment on ARM for both virtual and physical addresses. x86 has PHYSICAL_PAGE_MASK for masking physical addresses. Maybe we should adopt the same approach? Whatever, I feel that we should not deviate from the established definitions across every other architecture. #ifndef __ASSEMBLY__ diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h index b249035..ae39d11 100644 --- a/arch/arm/include/asm/pgtable-3level.h +++ b/arch/arm/include/asm/pgtable-3level.h @@ -48,16 +48,16 @@ #define PMD_SHIFT21 #define PMD_SIZE (1UL PMD_SHIFT) -#define PMD_MASK (~(PMD_SIZE-1)) +#define PMD_MASK (~((1 PMD_SHIFT) - 1)) #define PGDIR_SIZE (1UL PGDIR_SHIFT) -#define PGDIR_MASK (~(PGDIR_SIZE-1)) +#define PGDIR_MASK (~((1 PGDIR_SHIFT) - 1)) /* * section address mask and size definitions. */ #define SECTION_SHIFT21 #define SECTION_SIZE (1UL SECTION_SHIFT) -#define SECTION_MASK (~(SECTION_SIZE-1)) +#define SECTION_MASK (~((1 SECTION_SHIFT) - 1)) These masks are applied to a _virtual_ kernel address, not the physical addresses, Even with LPAE, the virtual address space is still 32-bit. So this is definitely wrong. -- 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/3] Introduce devm_clk_register()
On Mon, Sep 24, 2012 at 01:20:42PM -0700, Stephen Boyd wrote:
On 09/22/12 03:06, Russell King - ARM Linux wrote:
On Tue, Sep 18, 2012 at 11:05:27PM -0700, Stephen Boyd wrote:
The first patch in this series fixes error checking in the wm831x clock
driver and is here to prevent context conflicts in the third patch.
I split it out in case it needed to merge sooner rather than later.
The goal of this series is to add devm_clk_register() so I can use it in
some MSM clock code I'm sending out in the near future. The second
patch adds the API and the third patch moves over an existing user of
clk_unregister() to the devm API.
Can we guarantee that the clocks are unused when the module is removed?
If we can't make that guarantee, then devm_* should not be used here,
and instead there should be refcounting done in the clocks (that's what
the __clk_get() and __clk_put() hooks are there for.)
We could guarantee that when clk_unregister() is actually implemented.
__clk_get() would need to forward a call to the module providing the
clock via try_module_get(). Similarly we would call module_put() in
__clk_put(). That would prevent unbinding the driver from the device via
module removal.
Strangely enough... mach-integrator's clkdev.h... precisely because it
has a module which may provide a clock.
static inline int __clk_get(struct clk *clk)
{
return try_module_get(clk-owner);
}
static inline void __clk_put(struct clk *clk)
{
module_put(clk-owner);
}
--
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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 01:52:03PM +0530, Poddar, Sourav wrote: Hi Greg, Ping on this? On Tue, Sep 18, 2012 at 6:10 PM, Sourav Poddar sourav.pod...@ti.com wrote: Greg's tty-next is not booting on 2420 based N800. The failure is observed at serial init itself. The reason might be that n800 tries to resume even though it is not suspended before. How is this happening? I think that needs proper investigation - or if it's had more investigation, then the results needs to be included in the commit description so that everyone can understand the issue here. We should not be resuming a device which hasn't been suspended. Maybe the runtime PM enable sequence is wrong, and that's what should be fixed instead? This sequence in the probe() function: pm_runtime_irq_safe(pdev-dev); pm_runtime_enable(pdev-dev); pm_runtime_get_sync(pdev-dev); would enable runtime PM while the s/w state indicates that it's disabled, and then that pm_runtime_get_sync() will want to resume the device. See the section 5. Runtime PM Initialization, Device Probing and Removal in Documentation/power/runtime_pm.txt, specifically the second paragraph of that section. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 11:31:20AM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 09:30:29AM +0100, Russell King - ARM Linux wrote: How is this happening? I think that needs proper investigation - or if it's had more investigation, then the results needs to be included in the commit description so that everyone can understand the issue here. We should not be resuming a device which hasn't been suspended. Maybe the runtime PM enable sequence is wrong, and that's what should be fixed instead? This sequence in the probe() function: pm_runtime_irq_safe(pdev-dev); pm_runtime_enable(pdev-dev); pm_runtime_get_sync(pdev-dev); would enable runtime PM while the s/w state indicates that it's disabled, and then that pm_runtime_get_sync() will want to resume the device. See the section 5. Runtime PM Initialization, Device Probing and Removal in Documentation/power/runtime_pm.txt, specifically the second paragraph of that section. that was tested. It worked in pandaboard but didn't work on beagleboard XM. Sourav tried to start a discussion about that, but it simply died... In any case, pm_runtime_get_sync() in probe will always call runtime_resume callback, right ? Well, if the runtime PM state says it's suspended, and then you enable runtime PM, the first call to pm_runtime_get_sync() will trigger a resume attempt. The patch description is complaining about resume events without there being a preceding suspend event. This could well be why. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 12:11:14PM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 10:12:28AM +0100, Russell King - ARM Linux wrote: On Tue, Sep 25, 2012 at 11:31:20AM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 09:30:29AM +0100, Russell King - ARM Linux wrote: How is this happening? I think that needs proper investigation - or if it's had more investigation, then the results needs to be included in the commit description so that everyone can understand the issue here. We should not be resuming a device which hasn't been suspended. Maybe the runtime PM enable sequence is wrong, and that's what should be fixed instead? This sequence in the probe() function: pm_runtime_irq_safe(pdev-dev); pm_runtime_enable(pdev-dev); pm_runtime_get_sync(pdev-dev); would enable runtime PM while the s/w state indicates that it's disabled, and then that pm_runtime_get_sync() will want to resume the device. See the section 5. Runtime PM Initialization, Device Probing and Removal in Documentation/power/runtime_pm.txt, specifically the second paragraph of that section. that was tested. It worked in pandaboard but didn't work on beagleboard XM. Sourav tried to start a discussion about that, but it simply died... In any case, pm_runtime_get_sync() in probe will always call runtime_resume callback, right ? Well, if the runtime PM state says it's suspended, and then you enable runtime PM, the first call to pm_runtime_get_sync() will trigger a resume attempt. The patch description is complaining about resume events without there being a preceding suspend event. This could well be why. that's most likely, of course. But should we cause a regression to beagleboard XM because of that ? What would cause a regression on beagleboard XM? I have not suggested any change other than more investigation of the issue and a fuller patch description - yet you're screaming (idiotically IMHO) that mere investigation would break beagleboard. Well, if it's _that_ fragile, that mere investigation of this issue by someone elsewhere on the planet would break your beagleboard, maybe it deserves to be broken! -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 12:48:16PM +0300, Felipe Balbi wrote: Hi, On Tue, Sep 25, 2012 at 10:21:18AM +0100, Russell King - ARM Linux wrote: On Tue, Sep 25, 2012 at 12:11:14PM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 10:12:28AM +0100, Russell King - ARM Linux wrote: On Tue, Sep 25, 2012 at 11:31:20AM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 09:30:29AM +0100, Russell King - ARM Linux wrote: How is this happening? I think that needs proper investigation - or if it's had more investigation, then the results needs to be included in the commit description so that everyone can understand the issue here. We should not be resuming a device which hasn't been suspended. Maybe the runtime PM enable sequence is wrong, and that's what should be fixed instead? This sequence in the probe() function: pm_runtime_irq_safe(pdev-dev); pm_runtime_enable(pdev-dev); pm_runtime_get_sync(pdev-dev); would enable runtime PM while the s/w state indicates that it's disabled, and then that pm_runtime_get_sync() will want to resume the device. See the section 5. Runtime PM Initialization, Device Probing and Removal in Documentation/power/runtime_pm.txt, specifically the second paragraph of that section. that was tested. It worked in pandaboard but didn't work on beagleboard XM. Sourav tried to start a discussion about that, but it simply died... In any case, pm_runtime_get_sync() in probe will always call runtime_resume callback, right ? Well, if the runtime PM state says it's suspended, and then you enable runtime PM, the first call to pm_runtime_get_sync() will trigger a resume attempt. The patch description is complaining about resume events without there being a preceding suspend event. This could well be why. that's most likely, of course. But should we cause a regression to beagleboard XM because of that ? What would cause a regression on beagleboard XM? I have not suggested any change other than more investigation of the issue and a fuller patch description - yet you're screaming (idiotically IMHO) that mere investigation would break beagleboard. Well, if it's _that_ fragile, that mere investigation of this issue by someone elsewhere on the planet would break your beagleboard, maybe it deserves to be broken! why are you always so over the top like that ? This is just counter-productive to say the least. Because you are accusing me of potentially breaking your beagleboard for merely suggesting further investigation and a better commit message. You are the one going over the top, not me. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
Right, let's get this thread back onto a constructive footing and try to understand the problems here. On Tue, Sep 25, 2012 at 03:26:06PM +0530, Poddar, Sourav wrote: The issue was observed at serial init itself in the N800 board and the log does not show up much. http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt Right, so output stops when ttyO2 is initialized. What we thought the problem might be with n800 is that it tries to resume when it didn't suspend before. There are two ways through which we thought of handling this issue: a) set device as active before enabling pm (which will prevent pm_runtime_set_active(dev); pm_runtime_enable(dev); OR b) adding a suspended flag to struct omap_uart_port which gets set on suspend and cleared on resume. Then on resume you can check: if (!up-suspended) return 0; But using pm_runtime_set_active approach breaks things even on beagle board xm, though it works fine on Panda. Right, so now the question becomes - what is different on the Beagle Board that prevents solution (a) from working - or put it another way, have we uncovered _another_ bug. For N800, for ttyO0 and ttyO1 which aren't in use, it may be correct. We don't know for certain. For ttyO2, the port is clearly already in use as it's being used for console output. So that means it's _not_ in suspended state, and therefore the initial runtime PM state is wrong. For Beagleboard - who knows, we have no information on that. All we know is that your (a) sequence causes a regression. Anyway, let's analyse the code paths. What is pm_runtime_get_sync() doing? Well, it calls __pm_runtime_resume(, RPM_GET_PUT). That alters the parent device's state (which doesn't matter for this, as the platform device is a child of the main platform device, which has no runtime PM.) It then calls the resume callback (from the pm domain/type/class/bus/ driver) and then does an idle check. OMAP devices have a pm domain, so this is the candidate for the callback at this level, which gets us into _od_runtime_resume(). This calls omap_device_enable() before then moving on to the driver's runtime resume method, and at that point the runtime PM resume is complete. So, with your (a) solution, what's different is that we omit calling omap_device_enable(). Therefore, my hunch is the reason that Beagleboard doesn't work is because it doesn't like missing that call. I bet if you do this: omap_device_enable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); that this will solve your problem, and Beagleboard will continue working. What we have is a mismatch in both your case, and the Beagleboard case, between the real state of the hardware, the runtime PM state, and the OMAP hwmod state, and the above should bring all those states entirely into line with each other for _every_ case. It doesn't need any hacks to prevent resume callbacks without prior suspends (which, incidentally, the runtime PM core already guarantees provided you get the initial state correct.) -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 01:37:03PM +0300, Felipe Balbi wrote: On Tue, Sep 25, 2012 at 11:29:58AM +0100, Russell King - ARM Linux wrote: Because you are accusing me of potentially breaking your beagleboard for merely suggesting further investigation and a better commit message. Where did I accuse you of anyting ? I just mentioned we experienced a regression with beagleboard XM when using pm_runtime_set_active(). I quote: : But should we cause a regression to beagleboard XM because of that ? I say again: I was _only_ suggesting further investigation, yet you were mouthing off about causing a regression to beagleboard because of that, effectively saying that no, we should not do any further investigation and this is the only fix. To add extra info, here you go: Finally, something constructive. We pinged Paul and asked if he had seen that before, he had no pointers... Because Documentation/power/runtime_pm.txt was using a mystruct-is_suspended flag, we just decided to follow the same design since no-one was able to suggest why pm_runtime_set_active() was breaking beagleXM nor how it was supposed to actually work. Reading the code: pm_runtime_set_active() would tell pm_runtime core the device is actually active by setting runtime_status to RPM_ACTIVE, thus the following pm_runtime_get_sync() wouldn't actually call runtime_resume() callback, but it would increment usage_counter. I can't see why this would fail on beagleXM, but it does and we'd like to hear in which situations this could fail... Well, I've just spent five minutes analysing the code paths - which I hadn't looked at before - and I've pointed out what's probably causing the problem for Beagle. I think you owe me an appology over your aggressive attitude towards my suggestions that there needed to be some further investigation. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 12:11:14PM +0300, Felipe Balbi wrote: that's most likely, of course. But should we cause a regression to beagleboard XM because of that ? Also, if you look into chapter 9 of the runtime_pm documentation, starting on line 822 you'll see documentation suggests the use of mystruct-is_suspended flag. BTW, I'll point out a fatal flaw in your justification above. If you read the entire example, you'll see that the is_suspended flag is _not_ used to prevent resumes without suspends, but is used as a flag to control whether the driver processes requests or not. That's entirely functionally different from using a is_suspended flag in the way you mention above. Section 5 is quite clear about the requirements at initialization time for runtime PM, and nothing in section 9 contradicts that, and the is_suspended flag in that example has nothing to do with any of this. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Tue, Sep 25, 2012 at 02:12:43PM +0300, Felipe Balbi wrote: I don't see any aggressive attitude towards what you suggested, actually. Mailing list archives are available to check, but the one cursing around was always yourself and THAT deserves an apology. Total rubbish. No apology, because I wasn't cursing you. Whatever, I'm not going to re-explain the email thread. What I said was that your raising of beagleboard breakage (twice) was idiotic to a request for investigation. That's a statement I _still_ fully agree with, and I'll say it again if I have to. Trying to shut down investigation because investigation may break something _is_ idiotic - especially if the investigation reveals potential reasons why that breakage would occur. Further investigation is precisely how we arrive at better solutions. -- 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] Deferred probing in driver model is racy, resulting in lost probes
On Wed, Sep 26, 2012 at 01:08:33PM -0700, Greg Kroah-Hartman wrote:
On Sun, Sep 16, 2012 at 09:24:43PM +0800, Ming Lei wrote:
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 181ed26..17d7437 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -714,12 +714,12 @@ int bus_add_driver(struct device_driver *drv)
if (error)
goto out_unregister;
+ klist_add_tail(priv-knode_bus, bus-p-klist_drivers);
if (drv-bus-p-drivers_autoprobe) {
error = driver_attach(drv);
if (error)
goto out_unregister;
}
- klist_add_tail(priv-knode_bus, bus-p-klist_drivers);
module_add_driver(drv-owner, drv);
error = driver_create_file(drv, driver_attr_uevent);
Did the above patch ever prove to solve the issue or not?
To be honest, I've not bothered to test the above patch, and now when I
look at it, I notice it's broken - in that on error it will corrupt the
driver list. Take a look at the error path.
priv is drv-p. We add priv-knode_bus to the driver list. If
driver_attach() returns an error, then we go to out_unregister, which
does:
out_unregister:
kobject_put(priv-kobj);
kfree(drv-p);
drv-p = NULL;
thereby freeing the node we just added to the driver list without first
removing it.
I suspect it will fix the problem, but let's get the patch to be correct
before it gets tested...
--
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] pinctrl/nomadik: allocate IRQ descriptors dynamically
On Wed, Sep 26, 2012 at 07:18:07PM +0200, Linus Walleij wrote:
+ irq_start = NOMADIK_GPIO_TO_IRQ(pdata-first_gpio);
+ irq_base = irq_alloc_descs(irq_start, 0, NMK_GPIO_PER_CHIP,
+numa_node_id());
+ if (IS_ERR_VALUE(irq_base)) {
commit 07ab67c8d0d7c1021343b7d5c045033d6bf7be69
Author: Linus Torvalds torva...@ppc970.osdl.org
Date: Thu May 19 22:43:37 2005 -0700
Fix get_unmapped_area sanity tests
As noted by Chris Wright, we need to do the full range of tests regardless
of whether MAP_FIXED is set or not, so re-organize get_unmapped_area()
slightly to do the sanity checks unconditionally.
diff --git a/include/linux/err.h b/include/linux/err.h
index 17c55df..ff71d2a 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -13,6 +13,8 @@
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
+#define IS_ERR_VALUE(x) unlikely((x) (unsigned long)-1000L)
This is because get_unmapped_area() can return negative numbers which are
not error codes.
My position on this is that IS_ERR_VALUE() should only be used for checking
the return value of a function where some negative numbers are not error
codes, and everywhere else should use ret 0.
Just because we have a funky macro is no reason to blindly (ab)use it.
--
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] Deferred probing in driver model is racy, resulting in lost probes
On Thu, Sep 27, 2012 at 07:47:46AM +0800, Ming Lei wrote:
On Thu, Sep 27, 2012 at 4:23 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
To be honest, I've not bothered to test the above patch, and now when I
look at it, I notice it's broken - in that on error it will corrupt the
driver list. Take a look at the error path.
priv is drv-p. We add priv-knode_bus to the driver list. If
driver_attach() returns an error, then we go to out_unregister, which
In fact, driver_attach() always returns zero, so it does __not__ affect
your test.
It may not affect my test, but the fact is, that patch lays down the
foundations for bugs to be introduced. Now, while I can test it (and
have done) I don't think it's suitable for mainline because of _that_.
If driver_attach() always returns zero, there's no point in it returning
a value - it might as well return void and stop leading people to
believe that it might return an error. So I suggest this alternative
patch instead, which gets rid of what is effectively dead code, and
probably a few warnings about not checking the return value from a
__must_check function (see usb-serial.c).
With this patch, no one is lead into a false sense of security that,
after your patch, if driver_attach() ever returns an error, proper
cleanup will happen - it's blatently obvious to anyone modifying it
that if they do want it to return an error, they have to audit all the
users of it, which IMHO is a good thing.
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 2bcef65..39b3ef4 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -714,12 +714,10 @@ int bus_add_driver(struct device_driver *drv)
if (error)
goto out_unregister;
- if (drv-bus-p-drivers_autoprobe) {
- error = driver_attach(drv);
- if (error)
- goto out_unregister;
- }
klist_add_tail(priv-knode_bus, bus-p-klist_drivers);
+ if (drv-bus-p-drivers_autoprobe)
+ driver_attach(drv);
+
module_add_driver(drv-owner, drv);
error = driver_create_file(drv, driver_attr_uevent);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4b01ab3..2999df5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -456,7 +456,7 @@ static int __driver_attach(struct device *dev, void *data)
* returns 0 and the @dev-driver is set, we've found a
* compatible pair.
*/
-int driver_attach(struct device_driver *drv)
+void driver_attach(struct device_driver *drv)
{
return bus_for_each_dev(drv-bus, NULL, drv, __driver_attach);
}
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 444f8b6..4c16681 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -785,10 +785,12 @@ int __init agp_amd64_init(void)
/* Look for any AGP bridge */
agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table;
- err = driver_attach(agp_amd64_pci_driver.driver);
- if (err == 0 agp_bridges_found == 0) {
+ driver_attach(agp_amd64_pci_driver.driver);
+ if (agp_bridges_found == 0) {
pci_unregister_driver(agp_amd64_pci_driver);
err = -ENODEV;
+ } else {
+ err = 0;
}
}
return err;
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bb0608c..d2a8de5 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1722,9 +1722,9 @@ static ssize_t store_new_id(struct device_driver *drv,
const char *buf,
list_add_tail(dynid-list, hdrv-dyn_list);
spin_unlock(hdrv-dyn_lock);
- ret = driver_attach(hdrv-driver);
+ driver_attach(hdrv-driver);
- return ret ? : count;
+ return count;
}
static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
diff --git a/drivers/input/gameport/gameport.c
b/drivers/input/gameport/gameport.c
index da739d9..84f9878 100644
--- a/drivers/input/gameport/gameport.c
+++ b/drivers/input/gameport/gameport.c
@@ -670,12 +670,7 @@ static int gameport_driver_remove(struct device *dev)
static void gameport_attach_driver(struct gameport_driver *drv)
{
- int error;
-
- error = driver_attach(drv-driver);
- if (error)
- pr_err(driver_attach() failed for %s, error: %d\n,
- drv-driver.name, error);
+ driver_attach(drv-driver);
}
int __gameport_register_driver(struct gameport_driver *drv, struct module
*owner,
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index d0f7533..83f4eeb 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -802,12 +802,7 @@ static void serio_shutdown(struct device *dev)
static void serio_attach_driver(struct serio_driver *drv)
{
- int error;
-
- error = driver_attach(drv-driver);
- if (error
Re: [BUG] Deferred probing in driver model is racy, resulting in lost probes
On Thu, Sep 27, 2012 at 02:58:09PM +0100, Russell King - ARM Linux wrote:
On Thu, Sep 27, 2012 at 07:47:46AM +0800, Ming Lei wrote:
On Thu, Sep 27, 2012 at 4:23 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
To be honest, I've not bothered to test the above patch, and now when I
look at it, I notice it's broken - in that on error it will corrupt the
driver list. Take a look at the error path.
priv is drv-p. We add priv-knode_bus to the driver list. If
driver_attach() returns an error, then we go to out_unregister, which
In fact, driver_attach() always returns zero, so it does __not__ affect
your test.
It may not affect my test, but the fact is, that patch lays down the
foundations for bugs to be introduced. Now, while I can test it (and
have done) I don't think it's suitable for mainline because of _that_.
If driver_attach() always returns zero, there's no point in it returning
a value - it might as well return void and stop leading people to
believe that it might return an error. So I suggest this alternative
patch instead, which gets rid of what is effectively dead code, and
probably a few warnings about not checking the return value from a
__must_check function (see usb-serial.c).
With this patch, no one is lead into a false sense of security that,
after your patch, if driver_attach() ever returns an error, proper
cleanup will happen - it's blatently obvious to anyone modifying it
that if they do want it to return an error, they have to audit all the
users of it, which IMHO is a good thing.
Sorry, old version of that patch. Here's the right one.
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 2bcef65..39b3ef4 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -714,12 +714,10 @@ int bus_add_driver(struct device_driver *drv)
if (error)
goto out_unregister;
- if (drv-bus-p-drivers_autoprobe) {
- error = driver_attach(drv);
- if (error)
- goto out_unregister;
- }
klist_add_tail(priv-knode_bus, bus-p-klist_drivers);
+ if (drv-bus-p-drivers_autoprobe)
+ driver_attach(drv);
+
module_add_driver(drv-owner, drv);
error = driver_create_file(drv, driver_attr_uevent);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4b01ab3..77e2412 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -456,9 +456,9 @@ static int __driver_attach(struct device *dev, void *data)
* returns 0 and the @dev-driver is set, we've found a
* compatible pair.
*/
-int driver_attach(struct device_driver *drv)
+void driver_attach(struct device_driver *drv)
{
- return bus_for_each_dev(drv-bus, NULL, drv, __driver_attach);
+ bus_for_each_dev(drv-bus, NULL, drv, __driver_attach);
}
EXPORT_SYMBOL_GPL(driver_attach);
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 444f8b6..4c16681 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -785,10 +785,12 @@ int __init agp_amd64_init(void)
/* Look for any AGP bridge */
agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table;
- err = driver_attach(agp_amd64_pci_driver.driver);
- if (err == 0 agp_bridges_found == 0) {
+ driver_attach(agp_amd64_pci_driver.driver);
+ if (agp_bridges_found == 0) {
pci_unregister_driver(agp_amd64_pci_driver);
err = -ENODEV;
+ } else {
+ err = 0;
}
}
return err;
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bb0608c..d2a8de5 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1722,9 +1722,9 @@ static ssize_t store_new_id(struct device_driver *drv,
const char *buf,
list_add_tail(dynid-list, hdrv-dyn_list);
spin_unlock(hdrv-dyn_lock);
- ret = driver_attach(hdrv-driver);
+ driver_attach(hdrv-driver);
- return ret ? : count;
+ return count;
}
static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
diff --git a/drivers/input/gameport/gameport.c
b/drivers/input/gameport/gameport.c
index da739d9..84f9878 100644
--- a/drivers/input/gameport/gameport.c
+++ b/drivers/input/gameport/gameport.c
@@ -670,12 +670,7 @@ static int gameport_driver_remove(struct device *dev)
static void gameport_attach_driver(struct gameport_driver *drv)
{
- int error;
-
- error = driver_attach(drv-driver);
- if (error)
- pr_err(driver_attach() failed for %s, error: %d\n,
- drv-driver.name, error);
+ driver_attach(drv-driver);
}
int __gameport_register_driver(struct gameport_driver *drv, struct module
*owner,
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index d0f7533..83f4eeb 100644
--- a/drivers/input/serio/serio.c
Re: [PATCH v2 1/2] usb: phy: add a new driver for usb3 phy
On Thu, Sep 27, 2012 at 07:34:07PM +0530, Kishon Vijay Abraham I wrote:
+static int omap5_usb_phy_power(struct omap_usb *phy, bool on)
+{
+ u32 val;
+ unsigned long rate;
+ struct clk *sys_clk;
+
+ sys_clk = clk_get(NULL, sys_clkin);
+ if (IS_ERR(sys_clk)) {
+ pr_err(%s: unable to get sys_clkin\n, __func__);
+ return -EINVAL;
+ }
+
+ rate = clk_get_rate(sys_clk);
+ rate = rate/100;
+ clk_put(sys_clk);
Actually, you're supposed to hold on to the struct clk all the time your
driver is making use of that - you're not supposed to drop it.
That has several advantages: if clk_get() fails, then you're failing
earlier on (when the driver is being probed) and when some event occurs.
--
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/4] Clean up the long arch list for the UID16 config option
On Wed, Oct 03, 2012 at 03:15:05PM -0700, Andrew Morton wrote: Guys, don't do this. Either put the new item into a randomly-chosen position or, probably better, alphanumerically sort the list. I've been telling people to alphanumerically sort Kconfig select options for some time, but as normal, no one's listening. -- 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/4] Clean up the long arch list for the UID16 config option
On Thu, Oct 04, 2012 at 09:45:35PM +0200, Geert Uytterhoeven wrote: On Thu, Oct 4, 2012 at 9:41 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Oct 03, 2012 at 03:15:05PM -0700, Andrew Morton wrote: Guys, don't do this. Either put the new item into a randomly-chosen position or, probably better, alphanumerically sort the list. I've been telling people to alphanumerically sort Kconfig select options for some time, but as normal, no one's listening. Anyone who cares to sort the existing ones at the end of the merge window, and teaches checkpatch to enforce it for new ones? Well, I'll certainly do that for ARM... but how long it'll stay like that will be very hit and miss... -- 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: DEBUG_STACKOVERFLOW issue on ARM
On Wed, Oct 03, 2012 at 05:18:56PM +0530, trisha yad wrote: I wish to know how can I support this function in 3.0 ? If your description is correct, then all you need to do is to ensure that you route interrupts to other CPUs. You can do that by running the userspace irqbalance daemon. -- 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] [ARM] Use AT() in the linker script to create correct program headers
On Mon, Oct 01, 2012 at 10:06:39AM -0600, Jason Gunthorpe wrote: On Mon, Oct 01, 2012 at 04:39:53PM +0100, Dave Martin wrote: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x008000 0xc0008000 0x8000 0x372244 0x3a4310 RWE 0x8000 Not related directly to your patch, but I wonder why we don't we see separate r-x and rw- segments? I think this is because the sections are not aligned when the protections change, and the sections are not sorted by protection type. They aren't sorted by protection type, they're ordered according to what's required for the kernel - which is to have the init sections together as one complete block so that it can be freed, and to place all of the kernel text as close to the beginning of the image as possible. -- 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: [GIT PULL] Disintegrate UAPI for arm
On Thu, Oct 04, 2012 at 08:51:01PM +0100, David Howells wrote: Can you merge the following branch into the arm tree please. This is to complete part of the UAPI disintegration for which the preparatory patches were pulled recently. I'll merge this only after I've sorted through the issues arising in my current tree post merge window opening, so that I can at least get what was queued up _before_ the merge window into mainline. Thanks. -- 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] ARM: OMAP: Fix dependency for OMAP_DEBUG_LEDS
On Sun, Oct 07, 2012 at 04:00:37PM +0800, Axel Lin wrote: config OMAP_DEBUG_LEDS - def_bool y if NEW_LEDS + default y if LEDS_CLASS depends on OMAP_DEBUG_DEVICES This change is wrong. You're making this config entry untyped. -- 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] ARM: quiet down the non make -s output
On Mon, Oct 29, 2012 at 09:34:25AM -0600, Josh Cartwright wrote: Commit edc88ceb0c7d285b9f58bc29a638cd8163b59989 silenced the make -s build, but inadvertently made louder the non-silent build. Fix by prepending '@' to each of the added $(kecho) statements. Please put this in the patch system, thanks. -- 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/6 v2] arm: use devicetree to get smp_twd clock
On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
-static struct clk *twd_get_clock(void)
+static struct clk *twd_get_clock(struct device_node *np)
{
- struct clk *clk;
+ struct clk *clk = NULL;
int err;
- clk = clk_get_sys(smp_twd, NULL);
+ if (np)
+ clk = of_clk_get(np, 0);
+ if (!clk)
What does a NULL return from of_clk_get() mean? Where is this defined?
@@ -349,6 +348,10 @@ int __init twd_local_timer_register(struct
twd_local_timer *tlt)
if (!twd_base)
return -ENOMEM;
+ twd_clk = twd_get_clock(NULL);
+
+ twd_clk = twd_get_clock(NULL);
+
Why twice?
--
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 8/9] ARM: support uprobe handling
On Sun, Oct 14, 2012 at 09:23:12PM +0200, Rabin Vincent wrote: Extend the kprobes code to handle user-space probes. Much of the code can be reused so currently the ARM uprobes code reuses the kprobes structures. The decode tables are reused, with the modification that for those instruction that require custom decoding for uprobes, a new element is added in the table to specify a custom decoder function. How are accesses to userspace handled by the kprobes code? Please point me to where these accesses are performed. -- 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/6 v2] arm: use devicetree to get smp_twd clock
On Mon, Nov 05, 2012 at 04:28:05PM -0600, Mark Langsdorf wrote:
On 11/04/2012 04:08 AM, Russell King - ARM Linux wrote:
On Fri, Nov 02, 2012 at 01:51:44PM -0500, Mark Langsdorf wrote:
-static struct clk *twd_get_clock(void)
+static struct clk *twd_get_clock(struct device_node *np)
{
- struct clk *clk;
+ struct clk *clk = NULL;
int err;
- clk = clk_get_sys(smp_twd, NULL);
+ if (np)
+ clk = of_clk_get(np, 0);
+ if (!clk)
What does a NULL return from of_clk_get() mean? Where is this defined?
Well, it's a valid path if (np) is NULL. I'll add an IS_ERR(clk) and
resubmit.
Hang on - what logic are you trying to achieve here? Wouldn't:
if (np)
clk = of_clk_get(np, 0);
else
clk = clk_get_sys(smp_twd, NULL);
be sufficient? If we have DT, why would we ever want to fall back to
smp_twd ?
--
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] ARM: plat-versatile: move FPGA irq driver to drivers/irqchip
On Mon, Nov 05, 2012 at 10:42:26PM +, Arnd Bergmann wrote:
On Monday 05 November 2012, Rob Herring wrote:
But this should work:
if (!handle_arch_irq)
handle_arch_irq = fpga_handle_irq;
As long as the primary controller is always initialized first, this will
work. This is guaranteed by DT of_irq_init, and you will probably have
other problems if that wasn't the case for non-DT.
How about adding a top-level function in arch/arm that does the assignment
and hides the handle_arch_irq variable:
void set_handle_irq(void (*handle_irq)(struct pt_regs *))
{
if (WARN_ON(handle_arch_irq))
return;
handle_arch_irq = handle_irq;
}
EXPORT_SYMBOL_GPL(set_handle_irq);
Hmm, maybe putting the top-level handler into a loadable module is a bit
far-fetched, but one can hope ;-)
Definitely no point in exporting this (never export a symbol unless you
really want to use it from a module) - if you don't already have something
in handle_arch_irq, you're not going to get anywhere near the module
loader.
--
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] clk: Make the generic clock API available by default
On Mon, Oct 22, 2012 at 02:02:50PM +0100, Mark Brown wrote: @@ -327,6 +328,7 @@ config ARCH_AT91 select ARCH_REQUIRE_GPIOLIB select CLKDEV_LOOKUP select HAVE_CLK + select HAVE_CUSTOM_CLK This is silly. If you select HAVE_CUSTOM_CLK then isn't it true that HAVE_CLK should also be selected? If so, why not have HAVE_CUSTOM_CLK do that selection and remove it from all these entries? -- 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] clk: Make the generic clock API available by default
On Mon, Oct 22, 2012 at 02:27:11PM +0100, Mark Brown wrote: On Mon, Oct 22, 2012 at 02:05:57PM +0100, Russell King - ARM Linux wrote: On Mon, Oct 22, 2012 at 02:02:50PM +0100, Mark Brown wrote: @@ -327,6 +328,7 @@ config ARCH_AT91 select ARCH_REQUIRE_GPIOLIB select CLKDEV_LOOKUP select HAVE_CLK + select HAVE_CUSTOM_CLK This is silly. If you select HAVE_CUSTOM_CLK then isn't it true that HAVE_CLK should also be selected? If so, why not have HAVE_CUSTOM_CLK do that selection and remove it from all these entries? If we're worrying about that there's the larger point that the effect of this patch is to make HAVE_CLK meaningless as there will be no platform for which it's not true. I was just leaving HAVE_CLK alone for now ready to circle around on it if we ever manage to get the enabling bit sorted. Are you sure that all architectures are fine with having that permanently enabled? What about nommu architectures? -- 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/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote: On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote: Further to the discussion, my preference is still for of_clk_get() (although I've changed the patch anyway as you saw because it makes no difference in this case) :) clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to allow platforms to convert to DT without having to update all their drivers first. It only allows the first (default) clock, as your pointed out. Getting a 2nd... clock relies on an optional property in DT (which again, seems like it is there to support 'old' drivers) which allows you to request clocks by name. of_clk_get() on the other hand seems like a properly native DT function. You don't need to know anything about the clock, as long as the correct clock is specified in the correct order as documented by the binding. Relying on 'pre-OF' code for a OF-only driver also seems counter-intuitive. I do agree with those arguments. What I was saying is that for drivers which aren't DT only, of_clk_get() is not an option and that maybe others would be encouraged by the example to not use the generic APIs even if their driver could be used in non-DT setups. But maybe I'm worrying needlessly. That said, maybe somebody with a broader view of things like Arnd (Cc'ed) could share his thoughts. As I have already said, the way the DT bindings were done for the clk stuff was wrong. A little thought put into it would've come up with a much better solution which wouldn't have needed of_clk_get() at all. How? The arguments for clk_get() are: 1. the struct device, which you can get the OF-node from. 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only one.) So, we have something that defines a hardware clock input name, which can be used to generate a property name for OF. So, what _could_ have been done is this: clock-input-name = provider-node clk-output-index; where the property name is generated by: snprintf(prop, sizeof(prop), clk-%s, name ? name : default); So I continue to assert that our current design is wrong - and it will cause driver authors to pointlessly have to make a choice at every stage between DT and non-DT based systems. -- 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] mmc: sdhci: Defer probe if regulator_get fails
On Tue, Oct 23, 2012 at 03:21:58PM +0200, Philip Rakity wrote:
On 23 Oct 2012, at 09:19, Pavan Kunapuli pkunap...@nvidia.com wrote:
- /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
+ /*
+* If vqmmc regulator and no 1.8V signalling, then there's no UHS.
+* vqmmc regulator should be present. If it's not present,
+* assume the regulator driver registration is not yet done and
+* defer the probe.
+*/
host-vqmmc = regulator_get(mmc_dev(mmc), vqmmc);
if (IS_ERR(host-vqmmc)) {
- pr_info(%s: no vqmmc regulator found\n, mmc_hostname(mmc));
+ pr_err(%s: no vqmmc regulator found\n, mmc_hostname(mmc));
host-vqmmc = NULL;
+ return -EPROBE_DEFER;
Some systems exist where vmmc regulator exists and vqmmc regulator does not.
The vmmc regular is fixed at 3.3v.
Deferring the probe will cause issues.
That's one of the issues of deferred probing: you don't know if one of the
_get() functions failed because it just isn't present, or whether it's
failed because it hasn't been registered yet.
The two conditions are indistinguishable from the driver point of view.
The solution to it is to ensure that those drivers where hardware resource
X is optional provide a dummy resource to the driver when the hardware
resource is not available. That means, as far as the driver is concerned,
that it will always expect to get a full set of resources whether or not
the hardware has them.
If drivers care about such stuff, then we should have xxx_is_dummy()
functions (eg, clk_is_dummy(clk)) which return TRUE when the hardware
resource is not available. (which we could use NULL to indicate and
would be in keeping with the specified IS_ERR() usage of these APIs.)
--
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 007/193] arch/arm: remove CONFIG_EXPERIMENTAL
On Tue, Oct 23, 2012 at 01:01:20PM -0700, Kees Cook wrote: This config item has not carried much meaning for a while now and is almost always enabled by default. As agreed during the Linux kernel summit, remove it. I have a patch queued up for -rc which removes a number of these, but specifically leaves some which do cause people problems. So I'd appreciate it if we kept some of these (EXPERIMENTAL) tags even if we do drop the dependency. -- 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][GPIO] Add IRQ edge setter to gpiolib
On Tue, Oct 09, 2012 at 02:00:34PM +0200, Linus Walleij wrote: On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC drasko.drasko...@gmail.com wrote: [Me] If I understand correctly the below more or less exports struct irq_chip to userspace, trying to hide it by instead exposing a property of the containing struct gpio_chip and it worries me. No, it should not. You are exporting all of the defines from irq.h, IRQ_TYPE_NONE, IRQ_TYPE_EDGE_FALLING, etc to userspace. These are defined in linux/irq.h and that file has this comment on top: /* * Please do not include this file in generic code. There is currently * no requirement for any architecture to implement anything held * within this file. * * Thanks. --rmk */ And that comment is even only about generic *KERNEL* code, userspace is way, way more than that. It is always worth remembering this simple fact: If you export something from the kernel to userspace, it becomes part of the kernel's userspace API. Userspace APIs are more stable than the kernel, some suggest that the userspace API should be maintained for 10 years. Are you willing to set in stone for years part of the kernel internal structures without putting a lot of thought into how you export the data? If you haven't spent a significant amount of time thinking about how you're going to export something - and thinking about whether it should even be exported, then you haven't done enough to outweigh the pain of having it exported. -- 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 5/9] ARM: Xen: fix initial build problems:
On Tue, Oct 09, 2012 at 05:22:59PM +0200, Arnd Bergmann wrote: * The XEN_BALLOON code requires the balloon infrastructure that is not getting built on ARM. * The tmem hypercall is not available on ARM * ARMv6 does not support cmpxchg on 16-bit words that are used in the in the what? -- 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 4/9] ARM: export default read_current_timer
On Tue, Oct 09, 2012 at 04:40:54PM +0100, Jonathan Austin wrote: Hi Arnd, On 09/10/12 16:22, Arnd Bergmann wrote: diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c index 9d0a300..0dc5385 100644 --- a/arch/arm/lib/delay.c +++ b/arch/arm/lib/delay.c @@ -45,6 +45,7 @@ int read_current_timer(unsigned long *timer_val) *timer_val = delay_timer-read_current_timer(); return 0; } +EXPORT_SYMBOL_GPL(read_current_timer); Perhaps this fits better in armksyms.c? That way it lives with arm_delay_ops and friends. It's always much better to put things next to where they're defined rather than spreading them around. armksyms.c is a reminant of the 1.x days of doing things... but still remains to allow what are mostly assembly symbols to be exported. -- 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: [GIT PULL 0/9] ARM architecture fixes for 3.7
On Tue, Oct 09, 2012 at 05:22:54PM +0200, Arnd Bergmann wrote: Here are some patches that belong into your domain, I hope you can just send the lot to Linus the next time you send other patches. These bug fixes all address problems found with automated build testing. Some of them have been around for a long time, other bugs are regressions since the merge window. Apart from the sliced off comment in one commit (which XEN people need to confirm they're happy with the final version), I think these are otherwise fine... I'll hold off pulling them until that can be fixed. Thanks. -- 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: DEBUG_STACKOVERFLOW issue on ARM
On Thu, Oct 11, 2012 at 01:09:46PM +0530, vaibhav shinde wrote: Hi Russel, On Fri, Oct 5, 2012 at 2:01 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Oct 03, 2012 at 05:18:56PM +0530, trisha yad wrote: I wish to know how can I support this function in 3.0 ? If your description is correct, then all you need to do is to ensure that you route interrupts to other CPUs. You can do that by running the userspace irqbalance daemon. I also tried with the irqbalance daemon, on ARM quad core architecture, and as per my understanding, the daemon sets the /proc/irq/irq.no./smp_affinity file according to the irq frequency on a particular processor. However, the daemon doesn't work as expected, I also tried accessing the proc file mentioned above to set the cpu mask for particular irq, but this just block the processing of the irq, as I see the count of that irq doesnt increase in /proc/interrupts. My conclusion is that the irqbalancing is not supported on ARM arch or there is something more required for this to work. Kindly provide some pointers over this. Sounds like something has been broken, no idea what. It needs investigation. -- 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 5/8] clk: don't mark clkdev_add_table as init
On Tue, Oct 09, 2012 at 10:13:55PM +0200, Arnd Bergmann wrote: s3c2440_clk_add is a subsys_interface method and calls clkdev_add_table, which means we might be calling it after the __init section is discarded. Without this patch, building mini2440_defconfig results in: WARNING: vmlinux.o(.text+0x9848): Section mismatch in reference from the function s3c2440_clk_add() to the function .init.text:clkdev_add_table() The function s3c2440_clk_add() references the function __init clkdev_add_table(). This is often because s3c2440_clk_add lacks a __init annotation or the annotation of clkdev_add_table is wrong. I'm not sure this is the right thing to do. I suspect this comes from the stupidly complex samsung code, and that this is actually safe - I suspect that s3c2440_clk_add() needs to be appropriately marked, but then you end up having to trace its call path through various structures etc. -- 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] Boottime: A tool for automatic measurement of kernel/bootloader boot time
On Fri, Oct 12, 2012 at 02:45:42PM +0100, Lee Jones wrote:
That's odd. I get:
root@ME:/ cat sys/boottime/bootloader
0
root@ME:/ cat sys/boottime/kernel
4276
root@ME:/ cat /sys/kernel/debug/boottime/summary
kernel: 4276 msecs
total: 4276 msecs
root@ME:/ cat /sys/kernel/debug/boottime/bootgraph
[0.185254] calling splash+0x0/0x0
[2.984335] initcall splash+0x0/0x0 returned 0 after 2799 msecs.
[2.984335] calling autoboot_delay+0x0/0x0
[4.089513] initcall autoboot_delay+0x0/0x0 returned 0 after 1105 msecs.
[4.089513] calling load_kernel+0x0/0x0
[4.239174] initcall load_kernel+0x0/0x0 returned 0 after 149 msecs.
[4.239174] calling boot_kernel+0x0/0x0
[4.276260] initcall boot_kernel+0x0/0x0 returned 0 after 37 msecs.
[4.276260] calling uncompress_ll_init+0x0/0x0
[4.276260] initcall uncompress_ll_init+0x0/0x0 returned 0 after 0 msecs.
[4.276260] Freeing init memory: 0K
Umm, what happened to sysfs not becoming procfs v2? I thought we had
a fairly strict requirement for one value per file and not nicely
formatted for sysfs?
Author: Lee Jones lee.jo...@linaro.org
Date: Wed Jun 30 14:00:40 2010 +0200
Boottime: A tool for automatic measurement of kernel/bootloader boot
time
The overhead is very low and the results will be found under
sysfs/bootime, as well as detailed results in debugfs under
boottime/. The bootgraph* files are compatible with
scripts/bootgraph.pl. The reason for this patch is to provide
data (sysfs/boottime) suitable for automatic test-cases as
well as help for developers to reduce the boot time (debugfs).
Based heavily on the original driver by Jonas Aaberg.
Cc: Russell King li...@arm.linux.org.uk
Cc: Will Deacon will.dea...@arm.com
Signed-off-by: Jonas Aaberg jonas.ab...@stericsson.com
Signed-off-by: Mian Yousaf Kaukab mian.yousaf.kau...@stericsson.com
Signed-off-by: Lee Jones lee.jo...@linaro.org
Reviewed-by: Srinidhi KASAGAR srinidhi.kasa...@stericsson.com
Tested-by: Mian Yousaf KAUKAB mian.yousaf.kau...@stericsson.com
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index e8a4e58..8522356 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_SHARP_PARAM)+= sharpsl_param.o
obj-$(CONFIG_SHARP_SCOOP)+= scoop.o
obj-$(CONFIG_PCI_HOST_ITE8152) += it8152.o
obj-$(CONFIG_ARM_TIMER_SP804)+= timer-sp.o
+obj-$(CONFIG_BOOTTIME)+= boottime.o
diff --git a/arch/arm/common/boottime.c b/arch/arm/common/boottime.c
new file mode 100644
index 000..73e9e04
--- /dev/null
+++ b/arch/arm/common/boottime.c
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2009-2010
+ *
+ * Author: Jonas Aaberg jonas.ab...@stericsson.com for ST-Ericsson
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ *
+ * Store boot times measured during for example u-boot startup.
+ */
+
+#include linux/kernel.h
+#include linux/init.h
+#include linux/boottime.h
+#include linux/string.h
+#include asm/setup.h
+
+static u32 bootloader_idle;
+static u32 bootloader_total;
+
+static int __init boottime_parse_tag(const struct tag *tag)
+{
+ int i;
+ char buff[BOOTTIME_MAX_NAME_LEN];
+
+ bootloader_idle = tag-u.boottime.idle;
+ bootloader_total = tag-u.boottime.total;
+
+ for (i = 0; i tag-u.boottime.num; i++) {
+ snprintf(buff, BOOTTIME_MAX_NAME_LEN, %s+0x0/0x0,
+ tag-u.boottime.entry[i].name);
+ buff[BOOTTIME_MAX_NAME_LEN - 1] = '\0';
+ boottime_mark_wtime(buff, tag-u.boottime.entry[i].time);
+ }
+
+ return 0;
+}
+
+__tagtable(ATAG_BOOTTIME, boottime_parse_tag);
+
+int boottime_bootloader_idle(void)
+{
+ if (bootloader_total == 0)
+ return 0;
+
+ return (int) ((bootloader_idle) / (bootloader_total / 100));
+}
diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
index 24d284a..e8da062 100644
--- a/arch/arm/include/asm/setup.h
+++ b/arch/arm/include/asm/setup.h
@@ -143,6 +143,23 @@ struct tag_memclk {
__u32 fmemclk;
};
+/* for automatic boot timing testcases */
+#define ATAG_BOOTTIME 0x41000403
+#define BOOTTIME_MAX_NAME_LEN 64
+#define BOOTTIME_MAX 10
+
+struct boottime_entry {
+ u32 time; /* in us */
+ u8 name[BOOTTIME_MAX_NAME_LEN];
+};
+
+struct tag_boottime {
+ struct boottime_entry entry[BOOTTIME_MAX];
+ u32 idle; /* in us */
+ u32 total; /* in us */
+ u8 num;
+};
+
struct tag {
struct tag_header hdr;
union {
@@ -165,6 +182,10 @@ struct tag {
* DC21285 specific
*/
struct tag_memclk memclk;
+ /*
+ * Boot time
+ */
+ struct tag_boottime boottime;
} u;
Re: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote: Sourav sourav.pod...@ti.com writes: diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 6ede6fd..3fbc7f7 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct platform_device *pdev) INIT_WORK(up-qos_work, serial_omap_uart_qos_work); platform_set_drvdata(pdev, up); + pm_runtime_set_active(pdev-dev); NAK. This will obviously break platforms where the UARTs are not active before driver loads. I thought I had proposed a solution for this issue, which was this sequence: omap_device_enable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); Yes, I can understand people not liking the omap_device_enable() there, but I also notice that the email suggesting that never got a reply either - not even a I tried this and it doesn't work or it does work. As such, it seems this issue isn't making any progress as we had already established that merely doing a pm_runtime_set_active() before pm_runtime_enable() was going to break other platforms. -- 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: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Fri, Oct 12, 2012 at 05:29:55PM +, Poddar, Sourav wrote: Hi Russell, From: Russell King - ARM Linux [li...@arm.linux.org.uk] Sent: Friday, October 12, 2012 10:12 PM To: Kevin Hilman Cc: Poddar, Sourav; Paul Walmsley; Balbi, Felipe; gre...@linuxfoundation.org; t...@atomide.com; linux-kernel@vger.kernel.org; Shilimkar, Santosh; linux-ser...@vger.kernel.org; linux-o...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; a...@linux.intel.com Subject: Re: [RFT/PATCH] serial: omap: prevent resume if device is not suspended. On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote: Sourav sourav.pod...@ti.com writes: diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 6ede6fd..3fbc7f7 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct platform_device *pdev) INIT_WORK(up-qos_work, serial_omap_uart_qos_work); platform_set_drvdata(pdev, up); + pm_runtime_set_active(pdev-dev); NAK. This will obviously break platforms where the UARTs are not active before driver loads. I thought I had proposed a solution for this issue, which was this sequence: omap_device_enable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); Yes, I can understand people not liking the omap_device_enable() there, but I also notice that the email suggesting that never got a reply either - not even a I tried this and it doesn't work or it does work. Sorry for the late reply on this. I tried this sequence and it worked perfectly fine on panda and beagle. As such, it seems this issue isn't making any progress as we had already established that merely doing a pm_runtime_set_active() before pm_runtime_enable() was going to break other platforms. I was trying to analyse your explanations on this and since omap_device_enable is not generally recommended, I was trying to see if anything else can be done to get around this. Right, so what you need is explanation about why I believe the above sequence to be necessary. What is happening is that we're starting from a device in unknown state. We don't know whether it is enabled or disabled. We don't know the state of the clocks or the power domain. PM runtime state is initialized at device creation in the device is suspended state. If we merely enable PM runtime from that state, we are telling the PM runtime subsystem that the device is _indeed_ suspended (disabled) at boot time. So, that causes the first pm_runtime_get() call to resume the device. Due to the OMAP runtime PM hooks at the bus layer, this causes _od_runtime_resume() to be called. _od_runtime_resume() does two things. It calls omap_device_enable() to ensure that the device is woken up (such as, ensuring that the power domain is on, and turning on the clocks etc.) It then goes on to call the device PM layers to call the driver specific runtime PM resume hook. So, in summary, what this sequence does is: pm_runtime_enable(pdev-dev); pm_runtime_use_autosuspend(pdev-dev); pm_runtime_set_autosuspend_delay(pdev-dev, omap_up_info-autosuspend_timeout); pm_runtime_irq_safe(pdev-dev); pm_runtime_get_sync(pdev-dev); is, at the last call, it calls: _od_runtime_resume() omap_device_enable() serial_omap_runtime_resume() Your original patch at the head of this thread says that the driver specific runtime resume call causes a problem for N800 - because the device is already enabled and setup. Okay, so the initial device state does not match the runtime PM state. So, what happens if we _do_ make it match your state - as required by the runtime PM documentation - by adding a call before the sequence: pm_runtime_set_active(pdev-dev); pm_runtime_enable(pdev-dev); pm_runtime_use_autosuspend(pdev-dev); pm_runtime_set_autosuspend_delay(pdev-dev, omap_up_info-autosuspend_timeout); pm_runtime_irq_safe(pdev-dev); pm_runtime_get_sync(pdev-dev); Right, now runtime PM knows that the device is enabled and alive prior to that pm_runtime_get_sync() call, and it will _not_ call the runtime resume hooks. However, this breaks beaglebone, because the device is disabled when this driver probes. So, we have exactly the opposite problem here - the device is disabled, but runtime PM thinks it is enabled. The _two_ problems are precisely the same problem: the runtime PM state does not accurately reflect the actual hardware state - again, as required by the runtime PM documentation. The only sane solution to this is to ensure that the hardware _is_ in a known state prior to enabling runtime PM. How do we do that? Well, the clue
Re: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Fri, Oct 12, 2012 at 10:59:22AM -0700, Kevin Hilman wrote: Russell King - ARM Linux li...@arm.linux.org.uk writes: On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote: Sourav sourav.pod...@ti.com writes: diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 6ede6fd..3fbc7f7 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct platform_device *pdev) INIT_WORK(up-qos_work, serial_omap_uart_qos_work); platform_set_drvdata(pdev, up); + pm_runtime_set_active(pdev-dev); NAK. This will obviously break platforms where the UARTs are not active before driver loads. I thought I had proposed a solution for this issue, which was this sequence: omap_device_enable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); Yes, I can understand people not liking the omap_device_enable() there, but I also notice that the email suggesting that never got a reply either - not even a I tried this and it doesn't work or it does work. Yes, that solution would work (though I didn't actually try it.) However, we can't use omap_device_enable() in the driver. We're trying to clean all the drivers of OMAP-specific APIs. That being said, something similar could be done in the device/board init code to ensure the UART HW is in the state that the driver is expecting it, but again, that would just mask the real problem which is that a (re)init of the console UART on 2420/n800 is causing output to disappear. See my more expansive suggestion just posted. Whatever way, this discrepancy between runtime PM state and actual device state is what is biting you, and it is that which needs fixing. It's fairly easy to fix given the right design, one which several other bus types are already using. Given the route that OMAP went down when adopting runtime PM, it's going to be a big change across many drivers, because there's no way to gradually transition them, but that's unfortunately one of the results of ignoring requirements of the layers being used. Sooner or later the oversights come back to haunt. Just make sure it's not the ghost of Jaws. -- 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: [PULL REQ] IXP4xx changes for Linux 3.7
On Thu, Oct 18, 2012 at 12:01:17AM +0200, Krzysztof Halasa wrote: Hi, Arnd Bergmann a...@arndb.de writes: Also, never rebase your tree immediately before sending a pull request. I did not, of course. My mail stated: Build-tested for now. This is based on your current tree tip because it depends on commits following 3.6 release. You're lucky that you didn't get flamed by Linus himself for that, as others _have_ been in the past. Normally I wouldn't rebase, but had to (as you well knew) - because you commited a conflicting patch to this very IXP4xx arch. Using your logic, you were supposed to get an Ack from me (or from Imre) for this patch. If you had *bothered* asking the arm-soc people to pull your tree _instead_ of Linus, then that problem becomes the arm-soc's problem, not yours. That means _you_ end up with _less_ work to do. Yet, instead of seeing that benefit, whenever you've been asked to send your tree via arm-soc, you throw your toys out of your pram and basically refuse. So, you're making *more* work for yourself by not participating in arm-soc (as I've explained to you before.) The _ONLY_ thing you have to do is send your pull request to the arm-soc people instead of Linus before the merge window opens. You don't need to rebase your stuff on a different tree, you can still use Linus' tree as a basis. You have offered no technical reason why you can't participate in arm-soc which has stood up to screutiny. The only reasons you've offered seem to be: 1. it'll be more work (untrue) 2. you look after platforms which aren't in mainline and you're not submitting to mainline. Both of these a total nonsense arguments when it comes to the _route_ that your patches make their way into mainline. They have absolutely no bearing on the path your changes take AT ALL. Don't get me wrong. If I had time for this it could be different. Unfortunately IXP4xx is a legacy arch, and for me it's simply a hobby at this point. Given the raised barriers to participate, probably aimed at paid maintainers, I have to quit doing this. As you're being difficult and not willing to co-operate, and for whatever reason building this issue into a mountain, this unfortunately sounds to me like a good thing. Hopefully, a more co-operative maintainer will step up in your place who can see the benefits. -- 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] ARM: EXYNOS: use BUG_ON where possible
On Mon, Nov 12, 2012 at 04:12:29PM +0100, Maarten Lankhorst wrote:
Op 08-11-12 21:23, Sasha Levin schreef:
@@ -465,10 +465,8 @@ static void __init combiner_cascade_irq(unsigned int
combiner_nr, unsigned int i
else
max_nr = EXYNOS4_MAX_COMBINER_NR;
- if (combiner_nr = max_nr)
- BUG();
- if (irq_set_handler_data(irq, combiner_data[combiner_nr]) != 0)
- BUG();
+ BUG_ON(combiner_nr = max_nr);
+ BUG_ON(irq_set_handler_data(irq, combiner_data[combiner_nr]) != 0);
Is it really a good idea to put functions that perform work in a BUG_ON?
I don't know, but for some reason it just feels wrong. I'd expect code to
compile fine if BUG_ON was a noop, so doing verification calls only, not
actual work..
Well, it is currently defined as:
include/asm-generic/bug.h:#define BUG_ON(condition) do { if
(unlikely(condition)) BUG(); } while(0)
include/asm-generic/bug.h:#define BUG_ON(condition) do { if (condition) ; }
while(0)
but as these can be overridden, I don't think relying on those
implementations is a good idea; to do so would be fragile. Eg, what if
the BUG_ON() implementation becomes just:
#define BUG_ON(x)
then the function call itself vanishes. So, only put the actual bug test
inside a BUG_ON(), not the functional part which must always be executed.
--
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] ARM: add get_user() support for 8 byte types
On Fri, Nov 09, 2012 at 03:17:33PM -0600, Rob Clark wrote:
From: Rob Clark r...@ti.com
A new atomic modeset/pageflip ioctl being developed in DRM requires
get_user() to work for 64bit types (in addition to just put_user()).
NAK.
(I did write a better email explaining all the ins and outs of why this
won't work and why 64-bit get_user isn't possible, but my editor crapped
out and lost all that well written message; I don't fancy typing it all
out again.)
Nevertheless,
int test_ptr(unsigned int **v, unsigned int **p)
{
return get_user(*v, p);
}
produces a warning, and you can't get away from that if you stick 64-bit
support into get_user().
Sorry, 64-bit get_user() is a no-no.
--
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] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 01:58:32PM -0600, Rob Clark wrote:
On Mon, Nov 12, 2012 at 1:27 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Fri, Nov 09, 2012 at 03:17:33PM -0600, Rob Clark wrote:
From: Rob Clark r...@ti.com
A new atomic modeset/pageflip ioctl being developed in DRM requires
get_user() to work for 64bit types (in addition to just put_user()).
NAK.
(I did write a better email explaining all the ins and outs of why this
won't work and why 64-bit get_user isn't possible, but my editor crapped
out and lost all that well written message; I don't fancy typing it all
out again.)
Nevertheless,
int test_ptr(unsigned int **v, unsigned int **p)
{
return get_user(*v, p);
}
produces a warning, and you can't get away from that if you stick 64-bit
support into get_user().
Actually, it seems like using 'register typeof(x) __r2 asm(r2);'
does avoid that warning..
That seems to pass the checks I've done on it so far, and seems rather
obvious (there's been a number of people looking at this, none of whom
have come up with that solution). Provided the final cast is kept
(which is there to ensure proper typechecking), it seems like it might
be a solution.
--
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] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 05:33:41PM -0600, Rob Clark wrote: I'm sort of thinking maybe we want to change 'switch (sizeof(*(__p)))' with 'switch (sizeof(typeof(x)))' in case someone ignores the compiler warning when they try something like: Definitely not. Ttype of access is controlled by the pointer, not by the size of what it's being assigned to. Switching that around is likely to break stuff hugely. Consider this: unsigned char __user *p; int val; get_user(val, p); If the pointer type is used to determine the access size, a char will be accessed. This is legal - because we end up assigning an unsigned character to an int. If the size of the destination was used, we'd access an int instead, which is larger than the pointer, and probably the wrong thing to do anyway. Think of get_user(a, b) as being a special accessor having the ultimate semantics of: a = *b; but done in a safe way with error checking. uint32_t x; uint64_t *p = ...; get_user(x, p); that was my one concern about 'register typeof(x) __r2 ...', but I think just changing the switch condition is enough. But maybe good to have some eyes on in case there is something else I'm not thinking of. And what should happen in the above is exactly the same as what happens if you do: x = *p; with those types. For ARM, that would be a 64-bit access (if the compiler decides not to optimize away the upper 32-bit access) followed by a narrowing cast down to 32-bit. With get_user() of course, there's no option not to optimize it away. However, this _does_ reveal a bug with your approach. With sizeof(*p) being 8, and the type of __r2 being a 32-bit quantity, the compiler will choose the 64-bit accessor, which will corrupt r3 - and the compiler won't know that r3 has been corrupted. That's too unsafe. I just tried a variant of your approach, but got lots of warnings again: register unsigned long long __r2 asm(r2); __get_user_x(__r2, __p, __e, 8, lr); x = (typeof(*(__p))) ((typeof(x))__r2); because typeof(x) in the test_ptr() case ends up being a pointer itself. So, back to the drawing board, and I think back to the original position of we don't support this. -- 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] ARM: add get_user() support for 8 byte types
On Mon, Nov 12, 2012 at 06:31:50PM -0600, Rob Clark wrote:
right, that is what I was worried about.. but what about something
along the lines of:
case 8: { \
if (sizeof(x) 8) \
__get_user_x(__r2, __p, __e, __l, 4); \
else\
__get_user_x(__r2, __p, __e, __l, 8); \
break; \
} \
maybe we need a special variant of __get_user_8() instead to get the
right 32bits on big vs little endian systems, but I think something
roughly along these lines could work.
The problem with that is... big endian systems - where the 32-bit word we
want is the second one, so you can't just reduce that down to a 32-bit
access like that. You also need to add an offset to the pointer in the BE
case (which can be done.)
I'd suggest calling the 4-byte version in there __get_user_xb() and doing
the 4-byte offset for BE inside that (or aliasing it to __get_user_x for
LE systems).
--
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] ARM: add get_user() support for 8 byte types
On Tue, Nov 13, 2012 at 09:11:09AM +, Arnd Bergmann wrote:
On Tuesday 13 November 2012, Rob Clark wrote:
right, that is what I was worried about.. but what about something
along the lines of:
case 8: { \
if (sizeof(x) 8) \
__get_user_x(__r2, __p, __e, __l, 4); \
else\
__get_user_x(__r2, __p, __e, __l, 8); \
break; \
} \
I guess that's still broken if x is 8 or 16 bits wide.
Actually, it isn't - because if x is 8 or 16 bits wide, and we load
a 32-bit quantity, all that follows is a narrowing cast which is exactly
what happens today. We don't have a problem with register allocation
like we have in the 32-bit x vs 64-bit pointer target type, which is what
the above code works around.
maybe we need a special variant of __get_user_8() instead to get the
right 32bits on big vs little endian systems, but I think something
roughly along these lines could work.
Or maybe in sizeof(x)8 case, we just __get_user_bad().. I'm not 100%
sure on whether this is supposed to be treated as an error case at
compile time or not.
We know that nobody is using that at the moment, so we could define
it to be a compile-time error.
But I still think this is a pointless exercise, a number of people have
concluded independently that it's not worth trying to come up with a
solution, whether one exists or not. Why can't you just use copy_from_user()
anyway?
You're missing something; that is one of the greatest powers of open
source. The many eyes (and minds) effect. Someone out there probably
has a solution to whatever problem, the trick is to find that person. :)
I think we have a working solution for this for ARM. It won't be suitable
for every arch, where they have 8-bit and 16-bit registers able to be
allocated by the compiler, but for architectures where the minimum register
size is 32-bit, what we have below should work.
In other words, I don't think this will work for x86-32 where ax, al, ah
as well as eax are still available.
What I have currently in my test file, which appears to work correctly,
is (bear in mind this is based upon an older version of get_user() which
pre-dates Will's cleanups):
#define __get_user_x(__r2,__p,__e,__s,__i...) \
__asm__ __volatile__ ( \
bl __get_user_ #__s \
: =r (__e), =r (__r2) \
: 0 (__p) \
: __i, cc)
#ifdef BIG_ENDIAN
#define __get_user_xb(__r2,__p,__e,__s,__i...) \
__get_user_x(__r2,(uintptr_t)__p + 4,__s,__i)
#else
#define __get_user_xb __get_user_x
#endif
#define get_user(x,p) \
({ \
register const typeof(*(p)) __user *__p asm(r0) = (p);\
register int __e asm(r0); \
register typeof(x) __r2 asm(r2); \
switch (sizeof(*(__p))) { \
case 1: \
__get_user_x(__r2, __p, __e, 1, lr); \
break; \
case 2: \
__get_user_x(__r2, __p, __e, 2, r3, lr);\
break; \
case 4: \
__get_user_x(__r2, __p, __e, 4, lr); \
break; \
case 8: \
if (sizeof((x)) 8)\
__get_user_xb(__r2, __p, __e, 4, lr); \
else\
__get_user_x(__r2, __p, __e, 8, lr); \
break; \
default: __e = __get_user_bad(); break; \
} \
x = (typeof(*(__p))) __r2; \
__e;\
})
Tested
Re: [git pull] signals pile 3
On Sun, Oct 14, 2012 at 05:35:23PM +0200, Daniel Mack wrote: I rebased my ARM development branch and figured that your patch 9fff2fa (arm: switch to saner kernel_execve() semantics) breaks the boot on my board right after init is invoked via NFS: Ok, I'm not going to assign blame to Al's commits (I never reviewed his stuff before they hit mainline - patches never posted to the ARM mailing list, and the development actually happened within the merge window, all things we tell people not to do...) I _still_ haven't reviewed that stuff yet. But... nevertheless... [4.682072] VFS: Mounted root (nfs filesystem) on device 0:12. [4.690744] devtmpfs: mounted [4.694395] Freeing init memory: 172K [5.291417] Internal error: Oops - undefined instruction: 0 [#1] SMP THUMB2 Ok, so this tells us the kernel was built using Thumb2 ISA. [5.298734] Modules linked in: [5.301952] CPU: 0Not tainted (3.6.0-11053-g56c8535 #128) [5.308071] PC is at cpsw_probe+0x422/0x9ac PC is not word aligned, so it can't be running in the ARM ISA. [5.312459] LR is at trace_hardirqs_on_caller+0x8f/0xfc [5.317934] pc : [c03493de]lr : [c005e81f]psr: 6113 Note that this reconfirms the above (well, it should do, it's the same value.) [5.317934] sp : cf055fb0 ip : fp : [5.329944] r10: r9 : r8 : [5.335413] r7 : r6 : r5 : c034458d r4 : [5.342244] r3 : cf057a40 r2 : r1 : 0001 r0 : [5.349078] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user And this tells us that we're running in ARM mode, not Thumb mode. [5.356546] Control: 50c5387d Table: 8f434019 DAC: 0015 [5.362562] Process init (pid: 1, stack limit = 0xcf054240) [5.368395] Stack: (0xcf055fb0 to 0xcf056000) [5.372961] 5fa0: 0001 [5.381525] 5fc0: cf055fb0 c000d1a8 [5.390091] 5fe0: bee83f10 b6fdedd0 0010 bfaf a8babbaa No stack backtrace (and it's silent about why that is). The other strange thing here is that the stack dump above is showing that the stack is completely empty - which shouldn't be the case if we're in a driver probe function - driver probe functions are called via the driver model layers... [5.398664] Code: 2206a010 718ef508 0184f8da f8b1f65d (3070f8d8) And now we come to the Code: line, which makes no sense as an ARM ISA: 0: 2206a010andcs sl, r6, #16 4: 718ef508orrvc pc, lr, r8, lsl #10 8: 0184f8daldrdeq pc, [r4, sl] c: f8b1f65d; UNDEFINED instruction: 0xf8b1f65d 10: 3070f8d8ldrsbtccpc, [r0], #-136 ; 0xff78; UNPREDICTABLE But as Thumb, it looks more reasonable: 0: a010add r0, pc, #64 ; (adr r0, 44 foo+0x44) 2: 2206movsr2, #6 4: f508 718e add.w r1, r8, #284; 0x11c 8: f8da 0184 ldr.w r0, [sl, #388] ; 0x184 c: f65d f8b1 bl ffe5d172 foo+0xffe5d172 10: f8d8 3070 ldr.w r3, [r8, #112] ; 0x70 I don't have any further comments to make on this yet, as I've no idea what state stuff is in, but the above oops dump to me suggests that we've randomly jumped into some part of the kernel which just happens to be cpsw_probe(). Please send me (in private mail) your vmlinux file and a corresponding oops dump from that same kernel, and I'll dig and try and work out what's going on... This kind of investigation reminds me of those I did back in the 1990s when stuff was rather unstable and ARM was a young architecture. Now all we need is for an ARM platform to dump its entire memory out the ethernet port, bringing an university department network to a halt (I did that once - back in the 1990s - sorry Tim!) -- 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: [git pull] signals pile 3
Okay, here's the post-mortem diagnosis.
What's happening is as follows (I'm very certain of this.)
We come through the usual init, and issue (see init/main.c):
kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
This creates a new thread, which falls through to the ret_from_fork
assembly, with r4 set NULL and r5 set to kernel_init. You can see
this in your oops dump register set - r5 is 0xc0344555, which is the
address of kernel_init plus 1 which marks the function as Thumb code.
Now, let's look at this code a little closer - this is what the
disassembly looks like:
c000d180 ret_from_fork:
c000d180: f03a fe08 bl c0047d94 schedule_tail
c000d184: 2d00cmp r5, #0
c000d186: bf1eitttne
c000d188: 4620movne r0, r4
c000d18a: 46femovne lr, pc -- XXX
c000d18c: 46afmovne pc, r5
c000d18e: 46e9mov r9, sp
c000d190: ea4f 3959 mov.w r9, r9, lsr #13
c000d194: ea4f 3949 mov.w r9, r9, lsl #13
c000d198: e7c8b.n c000d12c ret_to_user
c000d19a: bf00nop
c000d19c: f3af 8000 nop.w
I have marked one instruction, and it's the significant one.
Eventually, having had a successful call to kernel_execve(), kernel_init()
returns zero.
In returning, it uses the value in 'lr' which was set by the instruction
I marked above. Unfortunately, this causes lr to contain 0xc000d18e -
an even address. This switches the ISA to ARM on return but with a non
word aligned PC value.
So, what do we end up executing? Well, not the instructions above - yes
the opcodes, but they don't mean the same thing in ARM mode. In ARM mode,
it looks like this instead:
c000d18c: 46e946afstrbtmi r4, [r9], pc, lsr #13
c000d190: 3959ea4fldmdbcc r9, {r0, r1, r2, r3, r6, r9, fp, sp,
lr, pc}^
c000d194: 3949ea4fstmdbcc r9, {r0, r1, r2, r3, r6, r9, fp, sp,
lr, pc}^
c000d198: bf00e7c8svclt 0xe7c8
c000d19c: 8000f3afandhi pc, r0, pc, lsr #7
c000d1a0: e88db092stm sp, {r1, r4, r7, ip, sp, pc}
c000d1a4: 46e81fff; UNDEFINED instruction:
0x46e81fff
c000d1a8: 8a00f3efbhi 0xc004a16c
c000d1ac: 0a0cf08abeq 0xc03493dc
I have included more above, because it's relevant. The PSR flags which we
can see in the oops dump are nZCv, so Z and C are set.
All the above ARM instructions are not executed, except for two. c000d1a0,
which has no writeback, and writes below the current stack pointer (and
that data is lost when we take the next exception.) The other instruction
which is executed is c000d1ac, which takes us to... 0xc03493dc. However,
remember that bit 1 of the PC got set. So that makes it 0xc03493de.
And that value is the value we find in the oops dump for PC. What is the
instruction here when interpreted in ARM mode?
0: f71e150c; UNDEFINED instruction: 0xf71e150c
and there we have our undefined instruction (remember that the 'never'
condition code, 0xf, has been deprecated and is now always executed.)
So, what we have above is a consistent and sane story for how we ended up
at such a strange place in the kernel with such an odd register dump - with
no unanswered questions about what happened to get us there.
In light of this, I'm 100% certain that the patch below will fix the issue
you're seeing - please test this and get back to me ASAP, thanks.
arch/arm/kernel/entry-common.S |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 417bac1..3471175 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -88,9 +88,9 @@ ENTRY(ret_from_fork)
bl schedule_tail
cmp r5, #0
movne r0, r4
- movne lr, pc
+ adrne lr, BSYM(1f)
movne pc, r5
- get_thread_info tsk
+1: get_thread_info tsk
b ret_slow_syscall
ENDPROC(ret_from_fork)
--
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: [git pull] signals pile 3
On Mon, Oct 15, 2012 at 12:39:40AM +0200, Daniel Mack wrote:
Tested-by: Daniel Mack zon...@gmail.com
Many thanks for the very prompt response!
Thanks Daniel.
I've also tested this on my OMAP4430 board running in ARM mode, so that
still works - we've covered the possibilities between us here between
ARM mode and Thumb mode, so...
Linus, could you merge this patch please, thanks.
8===
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] ARM: fix oops on initial entry to userspace with Thumb2 kernels
Daniel Mack reports an oops at boot with the latest kernels:
[4.896717] Internal error: Oops - undefined instruction: 0 [#1] SMP THUMB2
[4.904034] Modules linked in:
[4.907253] CPU: 0Not tainted (3.6.0-11057-g584df1d #145)
[4.913372] PC is at cpsw_probe+0x45a/0x9ac
[4.917760] LR is at trace_hardirqs_on_caller+0x8f/0xfc
[4.923235] pc : [c03493de]lr : [c005e81f]psr: 6113
[4.923235] sp : cf055fb0 ip : fp :
[4.935246] r10: r9 : r8 :
[4.940715] r7 : r6 : r5 : c0344555 r4 :
[4.947548] r3 : cf057a40 r2 : r1 : 0001 r0 :
[4.954383] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[4.961853] Control: 50c5387d Table: 8f3f4019 DAC: 0015
[4.967868] Process init (pid: 1, stack limit = 0xcf054240)
[4.973702] Stack: (0xcf055fb0 to 0xcf056000)
[4.978269] 5fa0: 0001
[4.986836] 5fc0: cf055fb0 c000d1a8
[4.995403] 5fe0: be9b3f10 b6f6add0 0010
bfaf a8babbaa
The analysis of this is as follows. In init/main.c, we issue:
kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
This creates a new thread, which falls through to the ret_from_fork
assembly, with r4 set NULL and r5 set to kernel_init. You can see
this in your oops dump register set - r5 is 0xc0344555, which is the
address of kernel_init plus 1 which marks the function as Thumb code.
Now, let's look at this code a little closer - this is what the
disassembly looks like:
c000d180 ret_from_fork:
c000d180: f03a fe08 bl c0047d94 schedule_tail
c000d184: 2d00cmp r5, #0
c000d186: bf1eitttne
c000d188: 4620movne r0, r4
c000d18a: 46femovne lr, pc -- XXX
c000d18c: 46afmovne pc, r5
c000d18e: 46e9mov r9, sp
c000d190: ea4f 3959 mov.w r9, r9, lsr #13
c000d194: ea4f 3949 mov.w r9, r9, lsl #13
c000d198: e7c8b.n c000d12c ret_to_user
c000d19a: bf00nop
c000d19c: f3af 8000 nop.w
This code was introduced in 9fff2fa0db911 (arm: switch to saner
kernel_execve() semantics). I have marked one instruction, and it's
the significant one - I'll come back to that later.
Eventually, having had a successful call to kernel_execve(), kernel_init()
returns zero.
In returning, it uses the value in 'lr' which was set by the instruction
I marked above. Unfortunately, this causes lr to contain 0xc000d18e -
an even address. This switches the ISA to ARM on return but with a non
word aligned PC value.
So, what do we end up executing? Well, not the instructions above - yes
the opcodes, but they don't mean the same thing in ARM mode. In ARM mode,
it looks like this instead:
c000d18c: 46e946afstrbtmi r4, [r9], pc, lsr #13
c000d190: 3959ea4fldmdbcc r9, {r0, r1, r2, r3, r6, r9, fp, sp,
lr, pc}^
c000d194: 3949ea4fstmdbcc r9, {r0, r1, r2, r3, r6, r9, fp, sp,
lr, pc}^
c000d198: bf00e7c8svclt 0xe7c8
c000d19c: 8000f3afandhi pc, r0, pc, lsr #7
c000d1a0: e88db092stm sp, {r1, r4, r7, ip, sp, pc}
c000d1a4: 46e81fff; UNDEFINED instruction:
0x46e81fff
c000d1a8: 8a00f3efbhi 0xc004a16c
c000d1ac: 0a0cf08abeq 0xc03493dc
I have included more above, because it's relevant. The PSR flags which we
can see in the oops dump are nZCv, so Z and C are set.
All the above ARM instructions are not executed, except for two. c000d1a0,
which has no writeback, and writes below the current stack pointer (and
that data is lost when we take the next exception.) The other instruction
which is executed is c000d1ac, which takes us to... 0xc03493dc. However,
remember that bit 1 of the PC got set. So that makes the PC value
0xc03493de.
And that value is the value we find in the oops dump for PC. What is the
instruction here when interpreted in ARM mode?
0: f71e150c; UNDEFINED instruction: 0xf71e150c
and there we have our undefined instruction (remember that the 'never'
condition code, 0xf, has been deprecated and is now always
Re: [Linaro-mm-sig] [RFC 0/2] DMA-mapping IOMMU - physically contiguous allocations
On Tue, Oct 16, 2012 at 09:04:34AM +0300, Hiroshi Doyu wrote: In addition to those contiguous/discontiguous page allocation, is there any way to _import_ anonymous pages allocated by a process to be used in dma-mapping API later? I'm considering the following scenario, an user process allocates a buffer by malloc() in advance, and then it asks some driver to convert that buffer into IOMMU'able/DMA'able ones later. In this case, pages are discouguous and even they may not be yet allocated at malloc()/mmap(). That situation is covered. It's the streaming API you're wanting for that. dma_map_sg() - but you may need additional cache handling via flush_dcache_page() to ensure that your code is safe for all CPU cache architectures. Remember that pages allocated into userspace will be cacheable, so a cache flush is required before they can be DMA'd. Hence the streaming API. -- 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: [Linaro-mm-sig] [RFC 0/2] DMA-mapping IOMMU - physically contiguous allocations
On Tue, Oct 16, 2012 at 07:12:49PM +0900, Inki Dae wrote: Hi Hiroshi, I'm not sure I understand what you mean but we had already tried this way and for this, you can refer to below link, http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg22555.html but this way had been pointed out by drm guys because the pages could be used through gem object after that pages had been freed by free() anyway their pointing was reasonable and I'm trying another way, this is the way that the pages to user space has same life time with dma operation. in other word, if dma completed access to that pages then also that pages will be freed. actually drm-based via driver of mainline kernel is using same way I don't know about Hiroshi, but the above sentence - and I mean the 7 line sentence - is very difficult to understand and wears readers out. If your GPU hardware has a MMU, then the problem of dealing with userspace pages is very easy. Do it the same way that the i915 driver and the rest of DRM does. Use shmem backed memory. I'm doing that for the Dove DRM driver and it works a real treat, and as the pages are backed by page cache pages, you can use all the normal page refcounting on them to prevent them being freed until your DMA has completed. All my X pixmaps are shmem backed drm objects, except for the scanout buffers which are dumb drm objects (because they must be contiguous.) In fact, get_user_pages() will take the reference for you before you pass them over to dma_map_sg(). On completion of DMA, you just need to use dma_unmap_sg() and release each page. If you don't want to use get_user_pages() (which other drivers don't) then you need to following the i915 example and get each page out of shmem individually. (My situation on the Dove hardware is a little different, because the kernel DRM driver isn't involved with the GPU - it merely provides the memory for pixmaps. The GPU software stack, being a chunk of closed source userspace library with open source kernel driver, means that things are more complicated; the kernel side GPU driver uses get_user_pages() to pin them prior to building the GPU's MMU table.) -- 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: [Linaro-mm-sig] [RFC 0/2] DMA-mapping IOMMU - physically contiguous allocations
On Tue, Oct 16, 2012 at 12:27:55PM +0200, Hiroshi Doyu wrote: Hi Russell, Russell King - ARM Linux li...@arm.linux.org.uk wrote @ Tue, 16 Oct 2012 10:59:28 +0200: On Tue, Oct 16, 2012 at 09:04:34AM +0300, Hiroshi Doyu wrote: In addition to those contiguous/discontiguous page allocation, is there any way to _import_ anonymous pages allocated by a process to be used in dma-mapping API later? I'm considering the following scenario, an user process allocates a buffer by malloc() in advance, and then it asks some driver to convert that buffer into IOMMU'able/DMA'able ones later. In this case, pages are discouguous and even they may not be yet allocated at malloc()/mmap(). That situation is covered. It's the streaming API you're wanting for that. dma_map_sg() - but you may need additional cache handling via flush_dcache_page() to ensure that your code is safe for all CPU cache architectures. Remember that pages allocated into userspace will be cacheable, so a cache flush is required before they can be DMA'd. Hence the streaming API. Is the syscall cacheflush() supposed to be the knob for that? Or is there any other ones to have more precise control, clean, invalidate and flush, from userland in generic way? No other syscalls are required - this sequence will do everything you need to perform DMA on pages mapped into userspace: get_user_pages() convert array of struct page * to scatterlist dma_map_sg() perform DMA dma_unmap_sg() for each page in sg() page_cache_release(page); If you get the list of pages some other way (eg, via shmem_read_mapping_page_gfp) then additional maintanence may be required (though that may be a bug in shmem - remember this stuff hasn't been well tested on ARM before.) -- 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: [git pull] signals pile 3
On Tue, Oct 16, 2012 at 04:04:14PM +0200, Uwe Kleine-König wrote: I used: movne r0, r4 - movne lr, pc - movne pc, r5 + blxne r5 get_thread_info tsk but I assume Russell's patch is better. (Probably because blx doesn't exist everywhere?!) Correct. So if it's not too late yet: Tested-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de It is, because it's already in Linus' tree. -- 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] genirq: provide means to retrigger parent
On Tue, Oct 16, 2012 at 03:07:49PM -0700, Kevin Hilman wrote: From: Thomas Gleixner t...@linutronix.de Attempts to retrigger nested threaded IRQs currently fail because they have no primary handler. In order to support retrigger of nested IRQs, the parent IRQ needs to be retriggered. To fix, when an IRQ needs to be resent, if the interrupt has a parent IRQ and runs in the context of the parent IRQ, then resend the parent. Also, handle_nested_irq() needs to clear the replay flag like the other handlers, otherwise check_irq_resend() will set it and it will never be cleared. Without clearing, it results in the first resend working fine, but check_irq_resend() returning early on subsequent resends because the replay flag is still set. Problem discovered on ARM/OMAP platforms where a nested IRQ that's also a wakeup IRQ happens late in suspend and needed to be retriggered during the resume process. Reported-by: Kevin Hilman khil...@ti.com Tested-by: Kevin Hilman khil...@ti.com [khil...@ti.com: changelog edits, clear IRQS_REPLAY in handle_nested_irq()] Signed-off-by: Thomas Gleixner t...@linutronix.de Umm, we also have the converse situation. We have platforms where the resend has to be done from the child IRQ, and the parent must not be touched. I hope that doesn't break those. -- 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] dma: add new DMA control commands
On Thu, Oct 18, 2012 at 02:45:41PM +0800, Huang Shijie wrote: 于 2012年10月18日 14:18, Vinod Koul 写道: Why cant you do start (prepare clock etc) when you submit the descriptor to dmaengine. Can be done in tx_submit callback. Similarly remove the clock when dma transaction gets completed. I ever thought this method too. But it will become low efficient in the following case: Assuming the gpmi-nand driver has to read out 1024 pages in one _SINGLE_ read operation. The gpmi-nand will submit the descriptor to dmaengine per page. So with your method, the system will repeat the enable/disable dma clock 1024 time. At every enable/disable dma clock, the system has to enable the clock chain and it's parents ... And what if you stop using clk_prepare_enable(), and prepare the clock when the channel is requested and only use clk_enable() in the tx_submit method? -- 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 02/03] CM-x270: ITE 8152 PCI bridge support
On Fri, Sep 21, 2007 at 12:42:55PM +0200, Guennadi Liakhovetski wrote: On Thu, 20 Sep 2007, Mike Rapoport wrote: This patch provides driver for ITE 8152 PCI bridge. Signed-off-by: Mike Rapoport [EMAIL PROTECTED] arch/arm/common/Makefile |1 + arch/arm/common/it8152.c | 387 + Shouldn't it go to drivers/pci and define a platform driver? it8152 is not an ARM specific controller. drivers/pci is currently only used for the PCI bus infrastructure. The setup of host bridges is very platform dependent. Also note that you should *avoid* DMA bounce wherever possible. The code is broken as designed. Ensure that the DMA mask is set appropriately, and then moan elsewhere if things don't work. - 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] mmc: mmci: Allow MMCI to request channels with information acquired from DT
On Thu, Apr 18, 2013 at 09:02:38AM +0100, Lee Jones wrote:
@@ -321,19 +323,21 @@ static void mmci_dma_setup(struct mmci_host *host)
* attempt to use it bidirectionally, however if it is
* is specified but cannot be located, DMA will be disabled.
*/
- if (plat-dma_rx_param) {
- host-dma_rx_channel = dma_request_channel(mask,
-plat-dma_filter,
-plat-dma_rx_param);
+ if (plat-dma_rx_param || np) {
+ host-dma_rx_channel = dma_request_slave_channel_compat(mask,
+
plat-dma_filter,
+
plat-dma_rx_param,
+ dev-dev,
rx);
/* E.g if no DMA hardware is present */
if (!host-dma_rx_channel)
dev_err(mmc_dev(host-mmc), no RX DMA channel\n);
I don't think this is right - I think Arnd has been leading you up the
garden path saying that this can be simplified. Why?
If you look at what this code does, the DMA channels are optional. If
they're not provided, then you don't get an error or a warning printk from
the code. However, after your conversion, if you use DT and avoid giving
the DMA information (which you have to avoid on the majority of ARM
platforms) then np will be non-NULL, and
dma_request_slave_channel_compat() will return NULL, causing the error
and/or warning to be printed.
So, we really need to know whether the DMA channel information has been
provided in DT in the upper level if statement, and _not_ use
dma_request_slave_channel_compat().
--
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] serial: pl011: Add Device Tree support to request DMA channels
On Thu, Apr 18, 2013 at 09:14:16AM +0100, Lee Jones wrote:
@@ -269,7 +270,10 @@ static void pl011_dma_probe_initcall(struct
uart_amba_port *uap)
dma_cap_zero(mask);
dma_cap_set(DMA_SLAVE, mask);
- chan = dma_request_channel(mask, plat-dma_filter, plat-dma_tx_param);
+ chan = dma_request_slave_channel_compat(mask,
+ (plat) ? plat-dma_filter : NULL,
+ (plat) ? plat-dma_tx_param : NULL,
+ uap-port.dev, tx);
if (!chan) {
dev_err(uap-port.dev, no TX DMA channel!\n);
return;
This suffers the same problem with your MMCI patch. If you're using DT and
don't provide the DMA information, you get errors printed. That's not on
for an optional driver feature, especially when that feature causes
functional difficulties on various platforms and so is _purposely_ omitted.
--
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] serial: pl011: Add Device Tree support to request DMA channels
On Thu, Apr 18, 2013 at 09:39:22AM +0100, Lee Jones wrote:
On Thu, 18 Apr 2013, Russell King - ARM Linux wrote:
This suffers the same problem with your MMCI patch. If you're using DT and
don't provide the DMA information, you get errors printed. That's not on
for an optional driver feature, especially when that feature causes
functional difficulties on various platforms and so is _purposely_ omitted.
How does that differ from using pdata and not passing DMA information?
If you have pdata but no DMA information then, this gets triggered:
if (!plat || !plat-dma_filter) {
dev_info(uap-port.dev, no DMA platform data\n);
return;
}
However, with your change, this becomes:
if (!(plat plat-dma_filter) !np) {
dev_info(uap-port.dev, no DMA platform data or DT\n);
return;
}
wihch means it is _unconditionally_ bypassed if there is a DT node. So,
if the MMC device has been created from DT, we will drop through to the
following code irrespective of whether there is any DMA data passed.
Hence, we get to here unconditionally in the DT case:
chan = dma_request_slave_channel_compat(mask,
(plat) ? plat-dma_filter : NULL,
(plat) ? plat-dma_tx_param : NULL,
uap-port.dev, tx);
if (!chan) {
dev_err(uap-port.dev, no TX DMA channel!\n);
return;
and if the DT transmit DMA information is not provided, chan will be NULL,
which means we produce an error level no TX DMA channel! message. That
message is supposed to indicate only when it has not been possible to
request the channel, not when there has been no DMA information provided.
This is where AMBA drivers differ from the majority of other drivers -
DMA is *strictly* optional for them, and that must remain the case as long
as we have platforms where AMBA devices are not hooked up to DMA engines
or where the DMA engines they are hooked up to are buggy, or the entire
DMA design (PL08x connected to PL18x) is buggy.
--
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] ARM: sched_clock: Add more notrace to prevent recursion
On Wed, Apr 17, 2013 at 05:34:45PM -0700, Stephen Boyd wrote: On 03/26/13 10:35, Stephen Boyd wrote: On 03/21/13 10:49, Stephen Boyd wrote: On 03/14/13 17:08, Stephen Boyd wrote: cyc_to_sched_clock() is called by sched_clock() and cyc_to_ns() is called by cyc_to_sched_clock(). I suspect that some compilers inline both of these functions into sched_clock() and so we've been getting away without having a notrace marking. It seems that my compiler isn't inlining cyc_to_sched_clock() though, so I'm hitting a recursion bug when I enable the function graph tracer, causing my system to crash. Marking these functions notrace fixes it. Technically cyc_to_ns() doesn't need the notrace because it's already marked inline, but let's just add it so that if we ever remove inline from that function it doesn't blow up. Anyone else seeing this problem? Russell, should I put this into the patch tracker? I'll throw this into the patch tracker tomorrow if nobody complains. Or you could do so today. -- 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] mmc: mmci: Allow MMCI to request channels with information acquired from DT
On Thu, Apr 18, 2013 at 11:25:44AM +0200, Arnd Bergmann wrote:
+static void mmci_dma_setup(struct device *dev, struct mmci_host *host)
+{
+ const char *rxname, *txname;
+
+ host-dma_rx_channel = dma_request_slave_channel(dev, rx);
+ host-dma_tx_channel = dma_request_slave_channel(dev, tx);
+
+ if (!host-dma_rx_channel !host-dma_tx_channel) {
+ if (host-plat host-plat-dma_filter)
+ mmci_dma_plat_setup(host);
+ else
+ dev_info(mmc_dev(host-mmc), no DMA platform data\n);
+ return;
You may like to fix the sillies in this patch... but yes, the above
would be better.
--
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 03/32] dmaengine: ste_dma40: Use the BIT macro to replace ugly '(1 x)'s
On Thu, Apr 18, 2013 at 11:11:45AM +0100, Lee Jones wrote: The aim is to make the code that little more readable. Maybe we should also invent the MULT() macro too, because a * b is just too ugly as well? Maybe ADD(), SUB() and DIV() macros as well... Think of all those extra parens you could use! :) @@ -1448,7 +1448,7 @@ static u32 d40_residue(struct d40_chan *d40c) D40_SREG_ELEM_PHY_ECNT_POS; } - return num_elt * (1 d40c-dma_cfg.dst_info.data_width); + return num_elt * BIT(d40c-dma_cfg.dst_info.data_width); This should be: return num_elt d40c-dma_cfg.dst_info.data_width; -- 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 15/32] dmaengine: ste_dma40: Separate Logical Global Interrupt Mask (GIM) unmasking
On Thu, Apr 18, 2013 at 11:11:57AM +0100, Lee Jones wrote:
+void d40_log_gim_unmask(u32 *src_cfg, u32 *dst_cfg) {
+
+ *src_cfg |= 1 D40_SREG_CFG_LOG_GIM_POS;
+ *dst_cfg |= 1 D40_SREG_CFG_LOG_GIM_POS;
So, in patch 3, you started using the BIT() macro. But here it's fine
not to because... ?
Use one notation or the other, don't mix them together.
--
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: [RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove uart_console defintion
On Wed, Apr 17, 2013 at 05:04:23PM +0530, Sourav Poddar wrote: Since uart_console definition is now moved to serial core haeder file . Serial drivers need not define them. This needs to be part of patch 1. Having it separate may provoke compiler warnings. -- 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] pinctrl/pinconfig: add debug interface
On Thu, Apr 18, 2013 at 12:35:14PM +0200, Linus Walleij wrote: + if (map-type != dbg-map_type) + continue; + if (!!strcmp(map-dev_name, dbg-dev_name)) !! is pointless in this context - if() evaluates any non-zero value as true, so there's no point making the code more complicated by trying to convert it to a false/true value first. -- 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 08/32] dmaengine: ste_dma40: Optimise local MAX() macro
On Thu, Apr 18, 2013 at 12:46:03PM +0200, Arnd Bergmann wrote: On Thursday 18 April 2013, Lee Jones wrote: The current implementation of the DMA40's local MAX() macro evaluates its arguments more times than is necessary. This patch strips it optimises it to only evaluate what's appropriate. No, it does not. index b21a8a3..7b451b2 100644 --- a/drivers/dma/ste_dma40.c +++ b/drivers/dma/ste_dma40.c @@ -53,7 +53,7 @@ #define D40_ALLOC_PHY BIT(30) #define D40_ALLOC_LOG_FREE BIT(0) -#define MAX(a, b) (((a) (b)) ? (b) : (a)) +#define MAX(a, b) ((a b) ? a : b) This just makes the macro worse in case you pass a complex expression in, which can now have additional unwanted effects. Just drop this patch. Never got the original patch... A much better idea is to get rid of that buggy MAX() macro altogether and use the macros already provided by the kernel, which are safe from side effects - but more importantly are _type_ _safe_. The above goes wrong when you consider 'a' and 'b' may have different signed-ness. Consider: int val_in = -5; unsigned val = MAX(val_in, 5U); The resulting value is (unsigned)-5, not (unsigned)5. Best use the kernel's max() or max_t() _everywhere_. -- 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/2] Allow constructor name selection by architecture.
Looks to me like this has died again. Unless Sam responds shortly, I'm going to ask for this to be put into the patch system and I'll just apply it for 3.10 and be done with it. On Mon, Apr 01, 2013 at 06:21:51PM -0400, George G. Davis wrote: On Mon, Apr 1, 2013 at 5:58 PM, Sam Ravnborg s...@ravnborg.org wrote: On Mon, Apr 01, 2013 at 05:47:38PM -0400, George G. Davis wrote: On Jun 6, 2012, at 6:12 AM, Russell King - ARM Linux wrote: On Tue, May 29, 2012 at 10:06:14AM +0100, Russell King - ARM Linux wrote: On Mon, May 28, 2012 at 10:30:05PM +0200, Sam Ravnborg wrote: On Mon, May 28, 2012 at 07:33:37PM +0100, Vincent Sanders wrote: From: Vincent Sanders vi...@collabora.co.uk The constructor symbol name is different between platforms. Allow this to be selected by configuration and set suitable default values. Signed-off-by: Vincent Sanders vincent.sand...@collabora.co.uk --- include/asm-generic/vmlinux.lds.h |6 +++--- init/Kconfig |6 ++ kernel/module.c |2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 8aeadf6..fd34808 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -471,9 +471,9 @@ } #ifdef CONFIG_CONSTRUCTORS -#define KERNEL_CTORS() . = ALIGN(8); \ -VMLINUX_SYMBOL(__ctors_start) = .; \ -*(.ctors) \ +#define KERNEL_CTORS() . = ALIGN(8); \ +VMLINUX_SYMBOL(__ctors_start) = .; \ +*(CONFIG_CONSTRUCTORS_NAME) \ VMLINUX_SYMBOL(__ctors_end) = .; What is wrong with adding both standard names for ctors uncnditionally? Like this: *(.ctors) \ +*(.init_array) \ That doesn't get rid of CONFIG_CONSTRUCTORS_NAME, because it's needed in the module code. Do you have a suggestion to solve that as well? Ping. Pinging this back to life. I'd like to see GCOV for ARM eABI finally make it upstream. So, any objections to this? Should it be resubmitted? Why is CONFIG_CONSTRUCTORS_NAME needed in module code? Because ARM eABI uses the .init_array section for C++ constructors, rather than .ctors. So the patch defined CONFIG_CONSTRUCTORS_NAME to set the correct section name for ARM eABI while leaving non-ARM-eABI as before, .ctors. Here are the changes from the patch for reference: diff --git a/init/Kconfig b/init/Kconfig index 6cfd71d..52181a1 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -20,6 +20,12 @@ config CONSTRUCTORS bool depends on !UML +config CONSTRUCTORS_NAME + string + depends on CONSTRUCTORS + default .init_array if ARM AEABI + default .ctors + config HAVE_IRQ_WORK bool diff --git a/kernel/module.c b/kernel/module.c index 78ac6ec..e5fad5e 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2600,7 +2600,7 @@ static void find_module_sections(struct module *mod, struct load_info *info) mod-unused_gpl_crcs = section_addr(info, __kcrctab_unused_gpl); #endif #ifdef CONFIG_CONSTRUCTORS - mod-ctors = section_objs(info, .ctors, + mod-ctors = section_objs(info, CONFIG_CONSTRUCTORS_NAME, sizeof(*mod-ctors), mod-num_ctors); #endif W/o the above, GCOV does not work on ARM eABI for kernel modules. Meanwhile, it still works as before for non-ARM-eABI kernel modules. Thanks! -- Regards, George -- 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: [PATCHSET v2] arch: unify task dump debug info
On Wed, Apr 03, 2013 at 12:14:50PM -0700, Tejun Heo wrote: Andrew, I think it's about ready and nobody seems to be against the proposed changes. Can you please take these into -mm? This v2 of this patchset. Changes from the last posting[1] are, So, I've just tried to apply this patch to my current testing tree, and it fails because I'm not using -mm... That means I can't put these in my nightly testing tree. -- 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 v6 1/4] arm: introduce psci_smp_ops
On Fri, Apr 05, 2013 at 02:11:32PM +0100, Stefano Stabellini wrote:
+ psci_init();
#ifdef CONFIG_SMP
if (is_smp()) {
- smp_set_ops(mdesc-smp);
+ if (mdesc-smp)
+ smp_set_ops(mdesc-smp);
+ else if (psci_smp_available())
+ smp_set_ops(psci_smp_ops);
So, I have a vague recollection that the ordering of the above got discussed
but I can't find it amongst the 21k of messages so far this year.
The above looks weird to me. Surely this should be:
if (psci_smp_available())
smp_set_ops(psci_smp_ops);
else if (mdesc-smp)
smp_set_ops(mdesc-ops);
This means that if PSCI is available, and provides a set of operations,
we override whatever the platform has statically provided.
Remember, we're trying to move away from using mdescs for platform
stuff, relying on things like DT and such like. We really should not
be going for mdesc-overriding-newstuff but newstuff-overriding-mdesc.
Now, if the psci stuff can't be relied upon to provide the correct
functionality, then that's a separate problem which needs addressing
differently.
This should allow the Xen problem to be resolved, because Xen will
provide the PSCI operations, and it's correct in that case to override
the platform's SMP operations.
--
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 1/3] pstore-ram: use write-combine mappings
On Tue, Apr 09, 2013 at 08:53:18PM -0700, Colin Cross wrote:
On Tue, Apr 9, 2013 at 8:08 PM, Rob Herring robherri...@gmail.com wrote:
- return ioremap(start, size);
+ return ioremap_wc(start, size);
ioremap_wc corresponds to MT_DEVICE_WC, which is still device memory,
so I don't see how this helps solve the problem in the commit message.
In reality it isn't, because there's no such thing as write combining
device memory in the ARM memory model.
There are three major memory types: strongly ordered, device and normal
memory. Only normal memory can be cached in any way, which includes
using write combining.
#define ioremap_wc(cookie,size) __arm_ioremap((cookie), (size), MT_DEVICE_WC)
[MT_DEVICE_WC] = { /* ioremap_wc */
.prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_WC,
* n TR IR OR
* BUFFERABLE 001 10 00 00
* DEV_WC 001 10
(see arch/arm/mm/proc-v7-2level.S for the rest of the table and its
description.)
So, DEV_WC is an alias for BUFFERABLE, which is normal memory,
uncacheable in both inner and outer caches. This means that at the
moment, ioremap_wc() memory has the same properties as system memory
- with all the out of ordering effects you get there.
I don't put any guarantee on this though - we may end up having to
change it if we find a SoC needing this to really be device memory...
--
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 PATCHv2 arm: initial TI-Nspire support]
On Thu, Apr 11, 2013 at 09:51:43PM +1000, Daniel Tang wrote: + .cntl = (CNTL_BGR | CNTL_LCDTFT | CNTL_LCDVCOMP(1) | + CNTL_LCDBPP16_565), NAK. Use the panel capabilities instead. -- 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] ARM: PL011: add support for extended FIFO-size of PL011-r1p5
On Fri, Apr 12, 2013 at 06:18:47PM +0900, Jongsung Kim wrote:
+static unsigned int get_fifosize_arm(unsigned int periphid)
+{
+ unsigned int rev = (periphid 20) 0xf;
+ return rev 3 ? 16 : 32;
Don't we have a macro to get the revision given the amba device?
amba_rev().
--
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 4/4] ARM: mmp: add SMP support for pxa988
On Sat, Apr 13, 2013 at 09:08:12PM +0800, Haojian Zhuang wrote:
On Thu, Apr 11, 2013 at 11:39 AM, Neil Zhang zhan...@marvell.com wrote:
+ /*
+* Synchronise with the boot thread.
+*/
+ spin_lock(boot_lock);
+ spin_unlock(boot_lock);
Lock unlock without protecting anything. If so, you can remove this.
... which means you don't understand what is going on in this code,
and probably didn't read the comment above this fragment. The above
is to synchronise with the code below - again, read the comment at
the spin unlock in this function, remembering that the above code
and the code below runs concurrently on two different CPUs:
+static int __cpuinit mmp_boot_secondary(unsigned int cpu, struct
task_struct *idle)
+{
+ unsigned long timeout;
+
+ /*
+* Avoid timer calibration on slave cpus. Use the value calibrated
+* on master cpu. Referenced from tegra3
+*/
+ preset_lpj = loops_per_jiffy;
+
+ /*
+* set synchronisation state between this boot processor
+* and the secondary one
+*/
+
+ spin_lock(boot_lock);
+
+ /*
+* The secondary processor is waiting to be released from
+* the holding pen - release it, then wait for it to flag
+* that it has been released by resetting pen_release.
+*
+* Note that pen_release is the hardware CPU ID, whereas
+* cpu is Linux's internal ID.
+*/
+ write_pen_release(cpu);
+
+ /* reset the cpu, let it branch to the kernel entry */
+ mmp_cpu_power_up(cpu);
+
+ timeout = jiffies + (1 * HZ);
+ while (time_before(jiffies, timeout)) {
+ smp_rmb();
+ if (pen_release == -1)
+ break;
+
+ udelay(10);
+ }
+
+ /*
+* now the secondary core is starting up let it run its
+* calibrations, then wait for it to finish
+*/
+ spin_unlock(boot_lock);
+
+ return pen_release != -1 ? -ENOSYS : 0;
+}
--
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/
