* [PATCH 1/2] Add a new helper for sleepable BTM acquirements which is used by tty_write_message()
From: Jeff Liu @ 2012-06-30 9:39 UTC (permalink / raw)
To: linux-serial
Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, gregkh,
Jan Kara, Ted Ts'o
This helper only used by tty_write_message(), it will sleep until BTM lock acquired.
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
drivers/tty/tty_mutex.c | 13 +++++++++++++
include/linux/tty.h | 1 +
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 9ff986c..e638636 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -30,3 +30,16 @@ void __lockfunc tty_unlock(void)
mutex_unlock(&big_tty_mutex);
}
EXPORT_SYMBOL(tty_unlock);
+
+/*
+ * Wait until getting the big tty mutex.
+ * This function only used by tty_write_message().
+ */
+void __lockfunc tty_lock_wait(void)
+{
+ while (!mutex_trylock(&big_tty_mutex)) {
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule();
+ }
+}
+EXPORT_SYMBOL(tty_lock_wait);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5e01e0b 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -607,6 +607,7 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
/* functions for preparation of BKL removal */
extern void __lockfunc tty_lock(void) __acquires(tty_lock);
extern void __lockfunc tty_unlock(void) __releases(tty_lock);
+extern void __lockfunc tty_lock_wait(void) __acquires(tty_lock);
/*
* this shall be called only from where BTM is held (like close)
--
1.7.9
^ permalink raw reply related
* [PATCH 2/2] make both atomic_write_lock and BTM lock acquirement sleepable at tty_write_message()
From: Jeff Liu @ 2012-06-30 9:40 UTC (permalink / raw)
To: linux-serial
Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, gregkh,
Jan Kara, Ted Ts'o
Avoid lockdep warn by having both lock acquirements sleepable.
BTM also have to go to sleep if lock failed at the first time, otherwise it
will race with tty_open()->tty_lock().
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
drivers/tty/tty_io.c | 35 +++++++++++++++++++++++++++++++----
1 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..2b4664d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1098,12 +1098,40 @@ out:
*
* We must still hold the BTM and test the CLOSING flag for the moment.
*/
-
void tty_write_message(struct tty_struct *tty, char *msg)
{
+retry:
if (tty) {
- mutex_lock(&tty->atomic_write_lock);
- tty_lock();
+ /*
+ * tty_write_message() will invoked by print_warning()
+ * at fs/quota/dquot.c if CONFIG_PRINT_QUOTA_WARNING
+ * is enabled when a user running out of disk quota limits.
+ * It will end up call tty_write(). Here is a potential race
+ * situation since tty_write() call copy_from_user() which
+ * might produce page faults and turn to invoke inodes dirty
+ * process on the underlying file systems if needed, and
+ * it will try to acquire JBD/JBD2 lock accordingly. This
+ * might make lockdep unhappy to print dead lock warning.
+ * To solve this issue, we have to go to sleep and relinquish
+ * the CPU power to another process until the atomic_write_lock
+ * became available.
+ */
+ if (!mutex_trylock(&tty->atomic_write_lock)) {
+ DEFINE_WAIT(wait);
+ prepare_to_wait_exclusive(&tty->write_wait, &wait,
+ TASK_INTERRUPTIBLE);
+ schedule();
+ finish_wait(&tty->write_wait, &wait);
+ goto retry;
+ }
+
+ /*
+ * Call tty_lock() directly might race with tty_open() even
+ * if we have already got the atomic_write_lock. However, we
+ * must perform TTY_CLOSING check up with the BTM hold, so call
+ * tty_lock_wait() which is sleepable instead.
+ */
+ tty_lock_wait();
if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) {
tty_unlock();
tty->ops->write(tty, msg, strlen(msg));
@@ -1114,7 +1142,6 @@ void tty_write_message(struct tty_struct *tty, char *msg)
return;
}
-
/**
* tty_write - write method for tty device file
* @file: tty file pointer
--
1.7.9
^ permalink raw reply related
* Re: [PATCH 1/2] Add a new helper for sleepable BTM acquirements which is used by tty_write_message()
From: Alan Cox @ 2012-06-30 12:35 UTC (permalink / raw)
To: jeff.liu
Cc: linux-serial, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, gregkh, Jan Kara, Ted Ts'o
In-Reply-To: <4FEEC96E.9070502@oracle.com>
On Sat, 30 Jun 2012 17:39:58 +0800
Jeff Liu <jeff.liu@oracle.com> wrote:
> This helper only used by tty_write_message(), it will sleep until BTM lock acquired.
This makes no sense at all. mutexes already sleep. Your code is also
wrong for real time processes.
So NAK.
^ permalink raw reply
* Re: [PATCH 2/2] make both atomic_write_lock and BTM lock acquirement sleepable at tty_write_message()
From: Alan Cox @ 2012-06-30 12:44 UTC (permalink / raw)
To: jeff.liu
Cc: linux-serial, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, gregkh, Jan Kara, Ted Ts'o
In-Reply-To: <4FEEC973.4090905@oracle.com>
> + * tty_write_message() will invoked by print_warning()
> + * at fs/quota/dquot.c if CONFIG_PRINT_QUOTA_WARNING
> + * is enabled when a user running out of disk quota limits.
> + * It will end up call tty_write(). Here is a potential race
tty->ops->write is the low level write method, not tty_write.
This appears to be even more wrong than the other one in other ways too -
it uses interruptible sleeps but doesn't handle the signal case so will
spin on a signal and kill the box.
NAK
Looking gat the traces I suspect what you've actually got is a much more
complicated deadlock where a process doing perfectly normal I/O to the
tty has faulted and there is a chain of dependancies through the file
system code to the thread which is doing the dquot_alloc_inode.
If that is the case then dquot_alloc_inode shouldn't be making blocking
calls to tty_write_message and probably the right thing to do is to queue
work for it so the tty_write_message is done asynchronously.
There are a very limited number of events that need reporting so probably
something like a per mount flags and workqueue would allow you to do
set_bit(DQUOT_INODEOVER, &foo->events);
schedule_work()
and the work queue can just xchg the events long for 0 and spew any
messages required.
Alan
^ permalink raw reply
* Re: [PATCH 2/2] make both atomic_write_lock and BTM lock acquirement sleepable at tty_write_message()
From: Jeff Liu @ 2012-06-30 13:35 UTC (permalink / raw)
To: Alan Cox
Cc: linux-serial, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, gregkh, Jan Kara, Ted Ts'o
In-Reply-To: <20120630134412.55eea39a@pyramind.ukuu.org.uk>
Hey Alan,
On 06/30/2012 08:44 PM, Alan Cox wrote:
>> + * tty_write_message() will invoked by print_warning()
>> + * at fs/quota/dquot.c if CONFIG_PRINT_QUOTA_WARNING
>> + * is enabled when a user running out of disk quota limits.
>> + * It will end up call tty_write(). Here is a potential race
>
> tty->ops->write is the low level write method, not tty_write.
I was wondering if below call trace is come from tty_write_message()->tty->ops->write()?
[ 2739.802106] -> #1 (&mm->mmap_sem){++++++}:
[ 2739.802120] [<c10fa825>] lock_acquire+0x14e/0x189
[ 2739.802133] [<c11e3e83>] might_fault+0xbf/0xf8
[ 2739.802154] [<c13bcbdf>] _copy_from_user+0x40/0x8a
[ 2739.802175] [<c14addc3>] copy_from_user+0x16/0x26
[ 2739.802195] [<c14b00b4>] tty_write+0x282/0x3c7
[ 2739.802212] [<c14b02dd>] redirected_tty_write+0xe4/0xfd
[ 2739.802226] [<c1231879>] vfs_write+0xf5/0x1a3
[ 2739.802239] [<c1231bdc>] sys_write+0x6c/0xa9
[ 2739.802253] [<c186281f>] sysenter_do_call+0x12/0x38
>
> This appears to be even more wrong than the other one in other ways too -
> it uses interruptible sleeps but doesn't handle the signal case so will
> spin on a signal and kill the box.
>
> NAK
>
> Looking gat the traces I suspect what you've actually got is a much more
> complicated deadlock where a process doing perfectly normal I/O to the
> tty has faulted and there is a chain of dependancies through the file
> system code to the thread which is doing the dquot_alloc_inode.
>
> If that is the case then dquot_alloc_inode shouldn't be making blocking
> calls to tty_write_message and probably the right thing to do is to queue
> work for it so the tty_write_message is done asynchronously.
>
> There are a very limited number of events that need reporting so probably
> something like a per mount flags and workqueue would allow you to do
>
> set_bit(DQUOT_INODEOVER, &foo->events);
> schedule_work()
>
> and the work queue can just xchg the events long for 0 and spew any
> messages required.
Thanks for the teaching, I'll give a try.
-Jeff
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Alessandro Rubini @ 2012-07-01 10:44 UTC (permalink / raw)
To: hpa
Cc: linux-kernel, giancarlo.asnaghi, alan, linux, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <4FECB95D.7010200@zytor.com>
>> The sta2x11 I/O Hub is a bridge from PCIe to AMBA. It reuses a number
>> of amba drivers and needs to activate core bus support.
> Nacked-by: H. Peter Anvin <hpa@zytor.com>
>
> This brings in tons of code into the x86 all{yes,mod}config builds, and
> as perhaps one could expect some of it doesn't compile. For example:
Ok. I apologize for not checking this. Actually, I only fixed the ones
that I needed to compile.
How should I address the problem? (original code, published on
sourceforge was simply replicating a number of amba drivers into pci
drivers, but I don't think massive code duplication is ever sensible,
thus I preferred to reuse the existing drivers).
I doubt all amba drivers can easily be made x86-compatible.
I see these possible approaces, all of them with their cons
1- add STA2X11 dependencies to ARM_AMBA _and_ the drivers that do work
2- leave CONFIG_ARM_AMBA undefined and play select file in Makefiles
using obj-$(CONFIG_STA2X11)
3- add ARM dependencies to the drivers that do not compile on x86
any other option?
(1) and (2) bring chipset-specific stuff into the amba world, which is
bad, especially since I suspect other similar bridges will appear soon.
(3) looks like a temporary hack and stuff can be fixed over time, but
it has the disadvantage of adding a big number of needless drivers in
the dsitributions' packages -- like my nacked patch did.
thanks
/alessandro
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Russell King - ARM Linux @ 2012-07-01 10:59 UTC (permalink / raw)
To: Alessandro Rubini
Cc: hpa, linux-kernel, giancarlo.asnaghi, alan, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <20120701104401.GA4352@mail.gnudd.com>
On Sun, Jul 01, 2012 at 12:44:01PM +0200, Alessandro Rubini wrote:
> How should I address the problem? (original code, published on
> sourceforge was simply replicating a number of amba drivers into pci
> drivers, but I don't think massive code duplication is ever sensible,
> thus I preferred to reuse the existing drivers).
I think the answer is... those primecell drivers need fixing in some way.
Re-defining CS, DS and ES in drivers is rather silly given that they're
x86 segment register names - so if PL330 can be fixed to make its names
more specific, that sorts it out.
As for PL08x, which depends on asm/hardware/pl080.h, I think that's going
to have to be a config dependency - it would be nice to move that header
into drivers/dma and make it private to the amba-pl08x driver, but we have
definitions in there which platforms supply to the pl08x driver via
platform data. I'd rather not stuff the register definitions into
include/linux/platform_data/...
Note also that hpa has only reported the first errors he encountered, so
there may be more, and that's somehting which needs checking.
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Alessandro Rubini @ 2012-07-01 11:04 UTC (permalink / raw)
To: linux
Cc: hpa, linux-kernel, giancarlo.asnaghi, alan, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <20120701105934.GD16319@n2100.arm.linux.org.uk>
> I think the answer is... those primecell drivers need fixing in some way.
Ok. I'm going to attack them and make a report.
> As for PL08x, which depends on asm/hardware/pl080.h,
We have pl080 under PCI, so we have patches in the queue to fix this
header problem. I just didn't submit them yet, but now they are part
of this fixup effort and I'll check them over.
> Note also that hpa has only reported the first errors he encountered, so
> there may be more, and that's somehting which needs checking.
Yes. I'm going to make a status report soon, counting and classifying
all errors I encounter when enabling all amba drivers under x86.
thanks
/alessandro
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Russell King - ARM Linux @ 2012-07-01 11:11 UTC (permalink / raw)
To: Alessandro Rubini
Cc: hpa, linux-kernel, giancarlo.asnaghi, alan, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <20120701110454.GA17531@mail.gnudd.com>
On Sun, Jul 01, 2012 at 01:04:54PM +0200, Alessandro Rubini wrote:
> > I think the answer is... those primecell drivers need fixing in some way.
>
> Ok. I'm going to attack them and make a report.
>
> > As for PL08x, which depends on asm/hardware/pl080.h,
>
> We have pl080 under PCI, so we have patches in the queue to fix this
> header problem. I just didn't submit them yet, but now they are part
> of this fixup effort and I'll check them over.
>
> > Note also that hpa has only reported the first errors he encountered, so
> > there may be more, and that's somehting which needs checking.
>
> Yes. I'm going to make a status report soon, counting and classifying
> all errors I encounter when enabling all amba drivers under x86.
Note that I have a *big* pile of changes to the PL08x stuff sitting in
my tree - which is waiting for DMA (and OMAP specifically) to stabilize
before I push them out into linux-next. They've been posted to the ARM
and OMAP lists several times already in the last month or two.
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Jassi Brar @ 2012-07-01 11:58 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Alessandro Rubini, linux-arch, giancarlo.asnaghi, arnd, gregkh,
x86, linux-kernel, linux-serial, hpa, linux-arm-kernel, alan
In-Reply-To: <20120701105934.GD16319@n2100.arm.linux.org.uk>
On 1 July 2012 16:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Sun, Jul 01, 2012 at 12:44:01PM +0200, Alessandro Rubini wrote:
>> How should I address the problem? (original code, published on
>> sourceforge was simply replicating a number of amba drivers into pci
>> drivers, but I don't think massive code duplication is ever sensible,
>> thus I preferred to reuse the existing drivers).
>
> I think the answer is... those primecell drivers need fixing in some way.
> Re-defining CS, DS and ES in drivers is rather silly given that they're
> x86 segment register names - so if PL330 can be fixed to make its names
> more specific, that sorts it out.
>
I am OK prefixing the regs with PL330_ or somesuch.
The CS, DS and ES regs were named so as to match exactly the
terminology of the PL330 manual. So apparently even the ARM Ltd didn't
imagine ARM and X86 galaxies colliding so soon :)
Regards.
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Russell King - ARM Linux @ 2012-07-01 12:10 UTC (permalink / raw)
To: Jassi Brar
Cc: Alessandro Rubini, linux-arch, giancarlo.asnaghi, arnd, gregkh,
x86, linux-kernel, linux-serial, hpa, linux-arm-kernel, alan
In-Reply-To: <CAJe_ZheynaBcMiQiwnxPrEoGxhhAJyHDD6VM-uDEN_5Yg1vVpA@mail.gmail.com>
On Sun, Jul 01, 2012 at 05:28:16PM +0530, Jassi Brar wrote:
> On 1 July 2012 16:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Sun, Jul 01, 2012 at 12:44:01PM +0200, Alessandro Rubini wrote:
> >> How should I address the problem? (original code, published on
> >> sourceforge was simply replicating a number of amba drivers into pci
> >> drivers, but I don't think massive code duplication is ever sensible,
> >> thus I preferred to reuse the existing drivers).
> >
> > I think the answer is... those primecell drivers need fixing in some way.
> > Re-defining CS, DS and ES in drivers is rather silly given that they're
> > x86 segment register names - so if PL330 can be fixed to make its names
> > more specific, that sorts it out.
> >
> I am OK prefixing the regs with PL330_ or somesuch.
>
> The CS, DS and ES regs were named so as to match exactly the
> terminology of the PL330 manual. So apparently even the ARM Ltd didn't
> imagine ARM and X86 galaxies colliding so soon :)
Whatever it says in documentation is neither here nor there. We're humans,
we can re-interpret, and we are capable of adding prefixes to names when
they're stupidly chosen.
Notice how I added MMCI_ and MCI_ to the register definitions for PL180
for example. I always do that kind of thing as a rule to avoid these kinds
of problems right from the outset, and I don't understand why others don't
do the same. We've been through soo many "this symbol clashes with
something else in the kernel tree" now that we really should be doing this
automatically, and not waiting for the clash to happen.
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Jassi Brar @ 2012-07-01 13:11 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Alessandro Rubini, linux-arch, giancarlo.asnaghi, arnd, gregkh,
x86, linux-kernel, linux-serial, hpa, linux-arm-kernel, alan
In-Reply-To: <20120701121008.GF16319@n2100.arm.linux.org.uk>
On 1 July 2012 17:40, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Sun, Jul 01, 2012 at 05:28:16PM +0530, Jassi Brar wrote:
>> On 1 July 2012 16:29, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>> > On Sun, Jul 01, 2012 at 12:44:01PM +0200, Alessandro Rubini wrote:
>> >> How should I address the problem? (original code, published on
>> >> sourceforge was simply replicating a number of amba drivers into pci
>> >> drivers, but I don't think massive code duplication is ever sensible,
>> >> thus I preferred to reuse the existing drivers).
>> >
>> > I think the answer is... those primecell drivers need fixing in some way.
>> > Re-defining CS, DS and ES in drivers is rather silly given that they're
>> > x86 segment register names - so if PL330 can be fixed to make its names
>> > more specific, that sorts it out.
>> >
>> I am OK prefixing the regs with PL330_ or somesuch.
>>
>> The CS, DS and ES regs were named so as to match exactly the
>> terminology of the PL330 manual. So apparently even the ARM Ltd didn't
>> imagine ARM and X86 galaxies colliding so soon :)
>
> Whatever it says in documentation is neither here nor there. We're humans,
> we can re-interpret, and we are capable of adding prefixes to names when
> they're stupidly chosen.
>
> Notice how I added MMCI_ and MCI_ to the register definitions for PL180
> for example. I always do that kind of thing as a rule to avoid these kinds
> of problems right from the outset, and I don't understand why others don't
> do the same. We've been through soo many "this symbol clashes with
> something else in the kernel tree" now that we really should be doing this
> automatically, and not waiting for the clash to happen.
>
I fully agree with your point and IIRC I always add some prefix to
definitions in header files.
Private defines in a .c file, without redundant prefixes, sounded like
safe to me at the time, but perhaps I was wrong.
Thanks.
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: H. Peter Anvin @ 2012-07-01 14:13 UTC (permalink / raw)
To: Alessandro Rubini
Cc: linux-kernel, giancarlo.asnaghi, alan, linux, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <20120701104401.GA4352@mail.gnudd.com>
There is no problem with adding ARM or !X86 dependencies to drivers now and fixing them later or as required.
Alessandro Rubini <rubini@gnudd.com> wrote:
>>> The sta2x11 I/O Hub is a bridge from PCIe to AMBA. It reuses a
>number
>>> of amba drivers and needs to activate core bus support.
>
>> Nacked-by: H. Peter Anvin <hpa@zytor.com>
>>
>> This brings in tons of code into the x86 all{yes,mod}config builds,
>and
>> as perhaps one could expect some of it doesn't compile. For example:
>
>Ok. I apologize for not checking this. Actually, I only fixed the ones
>that I needed to compile.
>
>How should I address the problem? (original code, published on
>sourceforge was simply replicating a number of amba drivers into pci
>drivers, but I don't think massive code duplication is ever sensible,
>thus I preferred to reuse the existing drivers).
>
>I doubt all amba drivers can easily be made x86-compatible.
>I see these possible approaces, all of them with their cons
>
> 1- add STA2X11 dependencies to ARM_AMBA _and_ the drivers that do work
>
> 2- leave CONFIG_ARM_AMBA undefined and play select file in Makefiles
> using obj-$(CONFIG_STA2X11)
>
> 3- add ARM dependencies to the drivers that do not compile on x86
>
> any other option?
>
>(1) and (2) bring chipset-specific stuff into the amba world, which is
>bad, especially since I suspect other similar bridges will appear soon.
>
>(3) looks like a temporary hack and stuff can be fixed over time, but
>it has the disadvantage of adding a big number of needless drivers in
>the dsitributions' packages -- like my nacked patch did.
>
>thanks
>/alessandro
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
^ permalink raw reply
* [UTEXAS: SUSPECTED SPAM] Your Funds Notification
From: Western Union, Malaysia @ 2012-07-01 11:00 UTC (permalink / raw)
To: Recipients
Your funds payment notification.
Do send your Name, Address & Phone Number to;
Mrs Franca Lee
+6010 3770 946.
^ permalink raw reply
* Your Funds Notification
From: Western Union, Malaysia @ 2012-07-01 12:29 UTC (permalink / raw)
To: Recipients
Your funds payment notification.
Do send your Name, Address & Phone Number to;
Mrs Franca Lee
+6010 3770 946.
^ permalink raw reply
* Re: [PATCH 0/2] Fix potential dead lock for tty_write_message() if CONFIG_PRINT_QUOTA_WARNING is enabled
From: Jan Kara @ 2012-07-02 9:32 UTC (permalink / raw)
To: Jeff Liu
Cc: linux-serial, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, gregkh, Jan Kara, Ted Ts'o
In-Reply-To: <4FEEC961.7050409@oracle.com>
Hello,
On Sat 30-06-12 17:39:45, Jeff Liu wrote:
> I observed the below lockdep warning info at syslog on 3.5-rc4, it could be simply reproduced by creating
> files more than the soft limits of inode quota on ext4 if CONFIG_PRINT_QUOTA_WARNING is enabled.
>
> jeff@pibroch:~$ uname -a
> Linux pibroch 3.5.0-rc4-dirty #220 SMP Fri Jun 29 11:30:20 CST 2012 i686 GNU/Linux
> jeff@pibroch:~$ quota -u jeff
> Disk quotas for user jeff (uid 1000):
> Filesystem blocks quota limit grace files quota limit grace
> /dev/sda8 0 0 0 1 100 6000
>
> So creating 110 a files could got quota warns.
> $ for((i=0;i<110;i++));do touch "/ext4_mount_path/testme.".$i;done
Yeah, situations like the one reported below can possibly cause problems
(e.g. when you have application with redirected output to /mnt/out and the
application writes to /mnt/b which exceeds quota - you try to write message
to /mnt/out in the middle of writing to /mnt/b). These problems are rather
hard to overcome and that's why CONFIG_PRINT_QUOTA_WARNING is marked as
obsolete and CONFIG_QUOTA_NETLINK_INTERFACE should be used instead.
Honza
> [ 213.324324] =====================================================
> [ 213.324337] [ INFO: possible circular locking dependency detected ]
> [ 213.324353] 3.5.0-rc4 #201 Not tainted
> [ 213.324364] -------------------------------------------------------
> [ 213.324376] touch/2237 is trying to acquire lock:
> [ 213.324413] (&tty->atomic_write_lock){+.+...}, at: [<c14dcc02>] tty_write_message+0x40/0x103
> [ 213.324426] [ 213.324426] but task is already holding lock:
> [ 213.324469] (jbd2_handle){+.+...}, at: [<f8202060>] start_this_handle+0xa19/0xa8a [jbd2]
> [ 213.324482] [ 213.324482] which lock already depends on the new lock.
> [ 213.324482] [ 213.324497] [ 213.324497] the existing dependency chain (in reverse order) is:
> [ 213.324525] [ 213.324525] -> #2 (jbd2_handle){+.+...}:
> [ 213.324546] [<c1101e89>] lock_acquire+0x198/0x1d3
> [ 213.324576] [<f82020a4>] start_this_handle+0xa5d/0xa8a [jbd2]
> [ 213.324608] [<f820247a>] jbd2__journal_start+0x11b/0x187 [jbd2]
> [ 213.324638] [<f8202506>] jbd2_journal_start+0x20/0x30 [jbd2]
> [ 213.324701] [<f8a8028c>] ext4_journal_start_sb+0x280/0x296 [ext4]
> [ 213.324753] [<f8a5a9bd>] ext4_dirty_inode+0x27/0x84 [ext4]
> [ 213.324767] [<c127e24e>] __mark_inode_dirty+0x52/0x2e7
> [ 213.324781] [<c12675fe>] update_time+0x10f/0x126
> [ 213.324794] [<c1267c07>] touch_atime+0x235/0x25e
> [ 213.324841] [<f8a49fac>] ext4_file_mmap+0x5c/0x7d [ext4]
> [ 213.324856] [<c1201bf9>] mmap_region+0x449/0x7c4
> [ 213.324870] [<c1202435>] do_mmap_pgoff+0x4c1/0x4db
> [ 213.324884] [<c11e7bb4>] vm_mmap_pgoff+0x98/0xc7
> [ 213.324899] [<c11ffa5e>] sys_mmap_pgoff+0x16a/0x1bf
> [ 213.324912] [<c18a5224>] syscall_call+0x7/0xb
>
> [ 213.324929] [ 213.324929] -> #1 (&mm->mmap_sem){++++++}:
> [ 213.324943] [<c1101e89>] lock_acquire+0x198/0x1d3
> [ 213.324956] [<c11f3bcb>] might_fault+0xbf/0xf8
> [ 213.324970] [<c13e1cd3>] _copy_from_user+0x40/0x8a
> [ 213.324982] [<c14da3cf>] copy_from_user+0x16/0x26
> [ 213.324994] [<c14dc80b>] tty_write+0x282/0x3c7
> [ 213.325006] [<c14dca34>] redirected_tty_write+0xe4/0xfd
> [ 213.325020] [<c123f955>] vfs_write+0xf5/0x1a3
> [ 213.325033] [<c123fcb8>] sys_write+0x6c/0xa9
> [ 213.325045] [<c18b169f>] sysenter_do_call+0x12/0x38
>
> [ 213.325062] [ 213.325062] -> #0 (&tty->atomic_write_lock){+.+...}:
> [ 213.325078] [<c1100e70>] __lock_acquire+0x15bb/0x1b98
> [ 213.325091] [<c1101e89>] lock_acquire+0x198/0x1d3
> [ 213.325105] [<c189fb16>] __mutex_lock_common+0x52/0x7cd
> [ 213.325118] [<c18a0439>] mutex_lock_nested+0x5c/0x70
> [ 213.325130] [<c14dcc02>] tty_write_message+0x40/0x103
> [ 213.325144] [<c12bf9fd>] flush_warnings+0x17c/0x313
> [ 213.325157] [<c12c47c4>] dquot_alloc_inode+0x1e4/0x1fc
> [ 213.325207] [<f8a4db1e>] ext4_new_inode+0x11f5/0x1576 [ext4]
> [ 213.325262] [<f8a6166d>] ext4_create+0x160/0x230 [ext4]
> [ 213.325276] [<c1253dc2>] vfs_create+0xc4/0x106
> [ 213.325297] [<c1254ee2>] do_last+0x437/0xdcc
> [ 213.325310] [<c125762a>] path_openat+0x10e/0x5a7
> [ 213.325323] [<c1257c57>] do_filp_open+0x39/0xc2
> [ 213.325335] [<c123e362>] do_sys_open+0xb5/0x1bb
> [ 213.325347] [<c123e4a1>] sys_open+0x39/0x5b
> [ 213.325360] [<c18a5224>] syscall_call+0x7/0xb
> [ 213.325368] [ 213.325368] other info that might help us debug this:
> [ 213.325368] [ 213.325391] Chain exists of:
> [ 213.325391] &tty->atomic_write_lock --> &mm->mmap_sem --> jbd2_handle
> [ 213.325391] [ 213.325401] Possible unsafe locking scenario:
> [ 213.325401] [ 213.325410] CPU0 CPU1
> [ 213.325417] ---- ----
> [ 213.325558] lock(jbd2_handle);
> [ 213.325571] lock(&mm->mmap_sem);
> [ 213.325583] lock(jbd2_handle);
> [ 213.325595] lock(&tty->atomic_write_lock);
> [ 213.325602] [ 213.325602] *** DEADLOCK ***
> [ 213.325602] [ 213.325613] 2 locks held by touch/2237:
> [ 213.325639] #0: (&type->i_mutex_dir_key#3){+.+.+.}, at: [<c1254da1>] do_last+0x2f6/0xdcc
> [ 213.325673] #1: (jbd2_handle){+.+...}, at: [<f8202060>] start_this_handle+0xa19/0xa8a [jbd2]
> [ 213.325682] [ 213.325682] stack backtrace:
> [ 213.325693] Pid: 2237, comm: touch Not tainted 3.5.0-rc4 #201
> [ 213.325702] Call Trace:
> [ 213.325717] [<c10faac9>] print_circular_bug+0x3fa/0x412
> [ 213.325732] [<c1100e70>] __lock_acquire+0x15bb/0x1b98
> [ 213.325747] [<c1101e89>] lock_acquire+0x198/0x1d3
> [ 213.325761] [<c14dcc02>] ? tty_write_message+0x40/0x103
> [ 213.325776] [<c189fb16>] __mutex_lock_common+0x52/0x7cd
> [ 213.325789] [<c14dcc02>] ? tty_write_message+0x40/0x103
> [ 213.325804] [<c18a525d>] ? restore_all+0xf/0xf
> [ 213.325819] [<c10b396c>] ? need_resched+0x22/0x3a
> [ 213.325833] [<c18a0439>] mutex_lock_nested+0x5c/0x70
> [ 213.325847] [<c14dcc02>] ? tty_write_message+0x40/0x103
> [ 213.325860] [<c14dcc02>] tty_write_message+0x40/0x103
> [ 213.325874] [<c12bf9fd>] flush_warnings+0x17c/0x313
> [ 213.325888] [<c12c47c4>] dquot_alloc_inode+0x1e4/0x1fc
> [ 213.325941] [<f8a4db1e>] ext4_new_inode+0x11f5/0x1576 [ext4]
> [ 213.326000] [<f8a6166d>] ext4_create+0x160/0x230 [ext4]
> [ 213.326018] [<c1253dc2>] vfs_create+0xc4/0x106
> [ 213.326033] [<c1254ee2>] do_last+0x437/0xdcc
> [ 213.326048] [<c125762a>] path_openat+0x10e/0x5a7
> [ 213.326063] [<c1257c57>] do_filp_open+0x39/0xc2
> [ 213.326077] [<c1101cdc>] ? lock_release+0x4b3/0x4c8
> [ 213.326090] [<c18a5116>] ? _raw_spin_unlock+0x4c/0x5d
> [ 213.326104] [<c126c821>] ? alloc_fd+0x2fe/0x317
> [ 213.326118] [<c123e362>] do_sys_open+0xb5/0x1bb
> [ 213.326132] [<c123e4a1>] sys_open+0x39/0x5b
> [ 213.326145] [<c18a5224>] syscall_call+0x7/0xb
>
> looks this issue is caused by the tty_write_message()->....->tty_write() because it will call copy_from_user() which will
> produce page faults and kick off ext4 inode dirty process on another CPU if possible.
> According to my understood, if the current process was be preempted when its time quantum expires, the need_resched field of
> the current process is set, so the scheduler is invoked and the current process will be re-scheduled to run and call
> next tty_write_message() at dquot.c->print_warning(), and it need to acquire atomic_write_lock again, however, this lock has
> already been hold by another process, so the race situation is occurred.
>
> I know such kind of issue should be better to fix at quota module, I have also tried to fix it there but no luck. :(
> For now, I could only work out a stupid patch set to let lockdep happy by making both tty_write_lock and BTM lock sleepable.
>
>
> Thanks,
> -Jeff
>
>
>
>
>
>
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/2] Fix potential dead lock for tty_write_message() if CONFIG_PRINT_QUOTA_WARNING is enabled
From: Jeff Liu @ 2012-07-02 9:49 UTC (permalink / raw)
To: Jan Kara
Cc: linux-serial, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, gregkh, Ted Ts'o
In-Reply-To: <20120702093245.GB6679@quack.suse.cz>
On 07/02/2012 05:32 PM, Jan Kara wrote:
> Hello,
>
> On Sat 30-06-12 17:39:45, Jeff Liu wrote:
>> I observed the below lockdep warning info at syslog on 3.5-rc4, it could be simply reproduced by creating
>> files more than the soft limits of inode quota on ext4 if CONFIG_PRINT_QUOTA_WARNING is enabled.
>>
>> jeff@pibroch:~$ uname -a
>> Linux pibroch 3.5.0-rc4-dirty #220 SMP Fri Jun 29 11:30:20 CST 2012 i686 GNU/Linux
>> jeff@pibroch:~$ quota -u jeff
>> Disk quotas for user jeff (uid 1000):
>> Filesystem blocks quota limit grace files quota limit grace
>> /dev/sda8 0 0 0 1 100 6000
>>
>> So creating 110 a files could got quota warns.
>> $ for((i=0;i<110;i++));do touch "/ext4_mount_path/testme.".$i;done
> Yeah, situations like the one reported below can possibly cause problems
> (e.g. when you have application with redirected output to /mnt/out and the
> application writes to /mnt/b which exceeds quota - you try to write message
> to /mnt/out in the middle of writing to /mnt/b). These problems are rather
> hard to overcome and that's why CONFIG_PRINT_QUOTA_WARNING is marked as
> obsolete and CONFIG_QUOTA_NETLINK_INTERFACE should be used instead.
Thanks for the clarification, looks this issue can not be solved in a
straightforward way if not touching the underlying file systems code.
Thanks,
-Jeff
>
> Honza
>
>> [ 213.324324] =====================================================
>> [ 213.324337] [ INFO: possible circular locking dependency detected ]
>> [ 213.324353] 3.5.0-rc4 #201 Not tainted
>> [ 213.324364] -------------------------------------------------------
>> [ 213.324376] touch/2237 is trying to acquire lock:
>> [ 213.324413] (&tty->atomic_write_lock){+.+...}, at: [<c14dcc02>] tty_write_message+0x40/0x103
>> [ 213.324426] [ 213.324426] but task is already holding lock:
>> [ 213.324469] (jbd2_handle){+.+...}, at: [<f8202060>] start_this_handle+0xa19/0xa8a [jbd2]
>> [ 213.324482] [ 213.324482] which lock already depends on the new lock.
>> [ 213.324482] [ 213.324497] [ 213.324497] the existing dependency chain (in reverse order) is:
>> [ 213.324525] [ 213.324525] -> #2 (jbd2_handle){+.+...}:
>> [ 213.324546] [<c1101e89>] lock_acquire+0x198/0x1d3
>> [ 213.324576] [<f82020a4>] start_this_handle+0xa5d/0xa8a [jbd2]
>> [ 213.324608] [<f820247a>] jbd2__journal_start+0x11b/0x187 [jbd2]
>> [ 213.324638] [<f8202506>] jbd2_journal_start+0x20/0x30 [jbd2]
>> [ 213.324701] [<f8a8028c>] ext4_journal_start_sb+0x280/0x296 [ext4]
>> [ 213.324753] [<f8a5a9bd>] ext4_dirty_inode+0x27/0x84 [ext4]
>> [ 213.324767] [<c127e24e>] __mark_inode_dirty+0x52/0x2e7
>> [ 213.324781] [<c12675fe>] update_time+0x10f/0x126
>> [ 213.324794] [<c1267c07>] touch_atime+0x235/0x25e
>> [ 213.324841] [<f8a49fac>] ext4_file_mmap+0x5c/0x7d [ext4]
>> [ 213.324856] [<c1201bf9>] mmap_region+0x449/0x7c4
>> [ 213.324870] [<c1202435>] do_mmap_pgoff+0x4c1/0x4db
>> [ 213.324884] [<c11e7bb4>] vm_mmap_pgoff+0x98/0xc7
>> [ 213.324899] [<c11ffa5e>] sys_mmap_pgoff+0x16a/0x1bf
>> [ 213.324912] [<c18a5224>] syscall_call+0x7/0xb
>>
>> [ 213.324929] [ 213.324929] -> #1 (&mm->mmap_sem){++++++}:
>> [ 213.324943] [<c1101e89>] lock_acquire+0x198/0x1d3
>> [ 213.324956] [<c11f3bcb>] might_fault+0xbf/0xf8
>> [ 213.324970] [<c13e1cd3>] _copy_from_user+0x40/0x8a
>> [ 213.324982] [<c14da3cf>] copy_from_user+0x16/0x26
>> [ 213.324994] [<c14dc80b>] tty_write+0x282/0x3c7
>> [ 213.325006] [<c14dca34>] redirected_tty_write+0xe4/0xfd
>> [ 213.325020] [<c123f955>] vfs_write+0xf5/0x1a3
>> [ 213.325033] [<c123fcb8>] sys_write+0x6c/0xa9
>> [ 213.325045] [<c18b169f>] sysenter_do_call+0x12/0x38
>>
>> [ 213.325062] [ 213.325062] -> #0 (&tty->atomic_write_lock){+.+...}:
>> [ 213.325078] [<c1100e70>] __lock_acquire+0x15bb/0x1b98
>> [ 213.325091] [<c1101e89>] lock_acquire+0x198/0x1d3
>> [ 213.325105] [<c189fb16>] __mutex_lock_common+0x52/0x7cd
>> [ 213.325118] [<c18a0439>] mutex_lock_nested+0x5c/0x70
>> [ 213.325130] [<c14dcc02>] tty_write_message+0x40/0x103
>> [ 213.325144] [<c12bf9fd>] flush_warnings+0x17c/0x313
>> [ 213.325157] [<c12c47c4>] dquot_alloc_inode+0x1e4/0x1fc
>> [ 213.325207] [<f8a4db1e>] ext4_new_inode+0x11f5/0x1576 [ext4]
>> [ 213.325262] [<f8a6166d>] ext4_create+0x160/0x230 [ext4]
>> [ 213.325276] [<c1253dc2>] vfs_create+0xc4/0x106
>> [ 213.325297] [<c1254ee2>] do_last+0x437/0xdcc
>> [ 213.325310] [<c125762a>] path_openat+0x10e/0x5a7
>> [ 213.325323] [<c1257c57>] do_filp_open+0x39/0xc2
>> [ 213.325335] [<c123e362>] do_sys_open+0xb5/0x1bb
>> [ 213.325347] [<c123e4a1>] sys_open+0x39/0x5b
>> [ 213.325360] [<c18a5224>] syscall_call+0x7/0xb
>> [ 213.325368] [ 213.325368] other info that might help us debug this:
>> [ 213.325368] [ 213.325391] Chain exists of:
>> [ 213.325391] &tty->atomic_write_lock --> &mm->mmap_sem --> jbd2_handle
>> [ 213.325391] [ 213.325401] Possible unsafe locking scenario:
>> [ 213.325401] [ 213.325410] CPU0 CPU1
>> [ 213.325417] ---- ----
>> [ 213.325558] lock(jbd2_handle);
>> [ 213.325571] lock(&mm->mmap_sem);
>> [ 213.325583] lock(jbd2_handle);
>> [ 213.325595] lock(&tty->atomic_write_lock);
>> [ 213.325602] [ 213.325602] *** DEADLOCK ***
>> [ 213.325602] [ 213.325613] 2 locks held by touch/2237:
>> [ 213.325639] #0: (&type->i_mutex_dir_key#3){+.+.+.}, at: [<c1254da1>] do_last+0x2f6/0xdcc
>> [ 213.325673] #1: (jbd2_handle){+.+...}, at: [<f8202060>] start_this_handle+0xa19/0xa8a [jbd2]
>> [ 213.325682] [ 213.325682] stack backtrace:
>> [ 213.325693] Pid: 2237, comm: touch Not tainted 3.5.0-rc4 #201
>> [ 213.325702] Call Trace:
>> [ 213.325717] [<c10faac9>] print_circular_bug+0x3fa/0x412
>> [ 213.325732] [<c1100e70>] __lock_acquire+0x15bb/0x1b98
>> [ 213.325747] [<c1101e89>] lock_acquire+0x198/0x1d3
>> [ 213.325761] [<c14dcc02>] ? tty_write_message+0x40/0x103
>> [ 213.325776] [<c189fb16>] __mutex_lock_common+0x52/0x7cd
>> [ 213.325789] [<c14dcc02>] ? tty_write_message+0x40/0x103
>> [ 213.325804] [<c18a525d>] ? restore_all+0xf/0xf
>> [ 213.325819] [<c10b396c>] ? need_resched+0x22/0x3a
>> [ 213.325833] [<c18a0439>] mutex_lock_nested+0x5c/0x70
>> [ 213.325847] [<c14dcc02>] ? tty_write_message+0x40/0x103
>> [ 213.325860] [<c14dcc02>] tty_write_message+0x40/0x103
>> [ 213.325874] [<c12bf9fd>] flush_warnings+0x17c/0x313
>> [ 213.325888] [<c12c47c4>] dquot_alloc_inode+0x1e4/0x1fc
>> [ 213.325941] [<f8a4db1e>] ext4_new_inode+0x11f5/0x1576 [ext4]
>> [ 213.326000] [<f8a6166d>] ext4_create+0x160/0x230 [ext4]
>> [ 213.326018] [<c1253dc2>] vfs_create+0xc4/0x106
>> [ 213.326033] [<c1254ee2>] do_last+0x437/0xdcc
>> [ 213.326048] [<c125762a>] path_openat+0x10e/0x5a7
>> [ 213.326063] [<c1257c57>] do_filp_open+0x39/0xc2
>> [ 213.326077] [<c1101cdc>] ? lock_release+0x4b3/0x4c8
>> [ 213.326090] [<c18a5116>] ? _raw_spin_unlock+0x4c/0x5d
>> [ 213.326104] [<c126c821>] ? alloc_fd+0x2fe/0x317
>> [ 213.326118] [<c123e362>] do_sys_open+0xb5/0x1bb
>> [ 213.326132] [<c123e4a1>] sys_open+0x39/0x5b
>> [ 213.326145] [<c18a5224>] syscall_call+0x7/0xb
>>
>> looks this issue is caused by the tty_write_message()->....->tty_write() because it will call copy_from_user() which will
>> produce page faults and kick off ext4 inode dirty process on another CPU if possible.
>> According to my understood, if the current process was be preempted when its time quantum expires, the need_resched field of
>> the current process is set, so the scheduler is invoked and the current process will be re-scheduled to run and call
>> next tty_write_message() at dquot.c->print_warning(), and it need to acquire atomic_write_lock again, however, this lock has
>> already been hold by another process, so the race situation is occurred.
>>
>> I know such kind of issue should be better to fix at quota module, I have also tried to fix it there but no luck. :(
>> For now, I could only work out a stupid patch set to let lockdep happy by making both tty_write_lock and BTM lock sleepable.
>>
>>
>> Thanks,
>> -Jeff
>>
>>
>>
>>
>>
>>
^ permalink raw reply
* [PATCH] pch_uart: Fix missing break for 16 byte fifo
From: Alan Cox @ 2012-07-02 17:51 UTC (permalink / raw)
To: greg, linux-serial
From: Alan Cox <alan@linux.intel.com>
Otherwise we fall back to the wrong value.
Reported-by: <dcb314@hotmail.com>
Resolves-bug: https://bugzilla.kernel.org/show_bug.cgi?id=44091
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/serial/pch_uart.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 4fdec6a..d53621a 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -1263,6 +1263,7 @@ static int pch_uart_startup(struct uart_port *port)
break;
case 16:
fifo_size = PCH_UART_HAL_FIFO16;
+ break;
case 1:
default:
fifo_size = PCH_UART_HAL_FIFO_DIS;
^ permalink raw reply related
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Arnd Bergmann @ 2012-07-02 16:58 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Alessandro Rubini, linux-kernel, giancarlo.asnaghi, alan, linux,
x86, gregkh, linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <2b797c83-6a3c-4051-841a-a4cfa9d1cae1@email.android.com>
On Sunday 01 July 2012, H. Peter Anvin wrote:
>
> There is no problem with adding ARM or !X86 dependencies to drivers
> now and fixing them later or as required.
Right, but if the fix is trivial (e.g. missing #include statement), it's
usually better to just fix the code.
Also, in many cases it's possible to say specifically what some code depends
on. E.g. if a driver uses clk_get/clk_put, it should depend on CLKDEV_LOOKUP,
not on !X86.
Arnd
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Mark Brown @ 2012-07-02 18:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: H. Peter Anvin, Alessandro Rubini, linux-kernel,
giancarlo.asnaghi, alan, linux, x86, gregkh, linux-arm-kernel,
linux-serial, linux-arch
In-Reply-To: <201207021658.27755.arnd@arndb.de>
On Mon, Jul 02, 2012 at 04:58:27PM +0000, Arnd Bergmann wrote:
> Also, in many cases it's possible to say specifically what some code depends
> on. E.g. if a driver uses clk_get/clk_put, it should depend on CLKDEV_LOOKUP,
> not on !X86.
Or we could get the stubs merged, or x86 could enable the clock API...
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: H. Peter Anvin @ 2012-07-02 18:07 UTC (permalink / raw)
To: Mark Brown
Cc: Arnd Bergmann, Alessandro Rubini, linux-kernel, giancarlo.asnaghi,
alan, linux, x86, gregkh, linux-arm-kernel, linux-serial,
linux-arch
In-Reply-To: <20120702180518.GA25995@sirena.org.uk>
On 07/02/2012 11:05 AM, Mark Brown wrote:
> On Mon, Jul 02, 2012 at 04:58:27PM +0000, Arnd Bergmann wrote:
>
>> Also, in many cases it's possible to say specifically what some code depends
>> on. E.g. if a driver uses clk_get/clk_put, it should depend on CLKDEV_LOOKUP,
>> not on !X86.
>
> Or we could get the stubs merged, or x86 could enable the clock API...
>
If there is a dependency there it should be registered, regardless if
x86 enables the clock API.
Last I saw I saw a patch to that effect, asked what the benefit was, and
got no answer.
-hpa
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Mark Brown @ 2012-07-02 18:33 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Arnd Bergmann, Alessandro Rubini, linux-kernel, giancarlo.asnaghi,
alan, linux, x86, gregkh, linux-arm-kernel, linux-serial,
linux-arch
In-Reply-To: <4FF1E37D.7050103@zytor.com>
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
On Mon, Jul 02, 2012 at 11:07:57AM -0700, H. Peter Anvin wrote:
> If there is a dependency there it should be registered, regardless if
> x86 enables the clock API.
Not really, what we *should* have is the clock API available (at least
for build purposes) everywhere. The current situation where the API is
randomly available on some architectures makes it unusuable in generic
code which is nuts and wasting time and effort, you either need ifdefs
or Kconfig hoop jumping in all the users which isn't at all sane and
means if you're doing anything generic you still need a backup plan.
What should be happening with this is the same as happens with all the
other similar APIs - the API should stub itself out where it's not
provided and the default should be that all architectures use the
generic implementation. The overwhelming majority of clock API usage is
just enabling the clock only while the device is running to save power
when it isn't which is totally amenable to stubbing out on platforms
that don't support that level of power management.
> Last I saw I saw a patch to that effect, asked what the benefit was, and
> got no answer.
Are you positive about that? I don't recall you replying any of the
times I sent out the patch and my mail archive isn't contradicting me
either.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: H. Peter Anvin @ 2012-07-02 19:41 UTC (permalink / raw)
To: Mark Brown
Cc: Arnd Bergmann, Alessandro Rubini, linux-kernel, giancarlo.asnaghi,
alan, linux, x86, gregkh, linux-arm-kernel, linux-serial,
linux-arch
In-Reply-To: <20120702183324.GA29030@opensource.wolfsonmicro.com>
On 07/02/2012 11:33 AM, Mark Brown wrote:
>
>> Last I saw I saw a patch to that effect, asked what the benefit
>> was, and got no answer.
>
> Are you positive about that? I don't recall you replying any of
> the times I sent out the patch and my mail archive isn't
> contradicting me either.
>
I said last time I saw a patch to that effect; it might not have been
from you. I might not have seen yours for whatever reason (including
losing it on my end.)
-hpa
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Mark Brown @ 2012-07-03 11:05 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Arnd Bergmann, Alessandro Rubini, linux-kernel, giancarlo.asnaghi,
alan, linux, x86, gregkh, linux-arm-kernel, linux-serial,
linux-arch
In-Reply-To: <4FF1F987.3030501@zytor.com>
[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]
On Mon, Jul 02, 2012 at 12:41:59PM -0700, H. Peter Anvin wrote:
> On 07/02/2012 11:33 AM, Mark Brown wrote:
> >> Last I saw I saw a patch to that effect, asked what the benefit
> >> was, and got no answer.
> > Are you positive about that? I don't recall you replying any of
> > the times I sent out the patch and my mail archive isn't
> > contradicting me either.
> I said last time I saw a patch to that effect; it might not have been
> from you. I might not have seen yours for whatever reason (including
> losing it on my end.)
I'm kind of surprised anyone else has been sending stuff (unless mine
got resent by someone else, I did include it in some of my serises for
the clock API); I know I've posted mine several times now. In any case,
I hope the mail you're replying to answers your question about why it's
useful.
In general I'd probably go further and say that (at least when a generic
implementation is available) there should be a very good reason for not
enabling an API on an architecture rather than the other way around.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH V2 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11
From: Alessandro Rubini @ 2012-07-03 13:00 UTC (permalink / raw)
To: hpa
Cc: linux-kernel, giancarlo.asnaghi, alan, linux, x86, gregkh, arnd,
linux-arm-kernel, linux-serial, linux-arch
In-Reply-To: <2b797c83-6a3c-4051-841a-a4cfa9d1cae1@email.android.com>
Peter Anvin:
> There is no problem with adding ARM or !X86 dependencies to drivers
> now and fixing them later or as required.
Ok. This is my summary of the compilation errors I have when
enabling ARM_AMBA undex x86 and enabling everything that appears
in "make oldconfig":
This is a sum up of the errors in all driver that are
enabled by telling "CONFIG_ARM_AMBA=y" in the x86 config.
drivers/dma/pl330.c: register names conflict with arch symbols
proposed fix: use proper prefix names
drivers/dma/amba-pl08x.c: needs <asm/hardware/pl080.h>
proposed fix: move pl080.h to include/linux
drivers/gpio/gpio-pl061.c: uses chained_irq_enter/exit
proposed fix: depend on CONFIG_ARM (the function only exists in arm)
drivers/mmc/host/mmci.c: uses <asm/sizes.h>
drivers/mmc/host/mmci.c: uses readsl/writesl
proposed fix: use linux/sizes.h and provide readsl/writesl like others do
drivers/spi/spi-pl022.c: warning for an integer size mismatch
proposed fix: none by now
drivers/watchdog/sp805_wdt.c: uses writel_relaxed
proposed fix: depend on CONFIG_ARM (this is a spear-only cell by now)
So, if you agree, I would:
- fix the two dma engines (checking pl080 against russell's pending
patches)
- make pl061 and sp805_wdt.c depends on CONFIG_ARM
- temporarily do the same for mmci, which I'll make work soon as
I have it under my pci bridge
- ignore the warning on spi-pl022 until I find a fix
- resubmit my enabling of arm_amba for x86 and my pci-to-amba bridge
driver.
thanks
/alessandro
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox