* [PATCH] serial8250-em: clk_get() IS_ERR() error handling fix
From: Magnus Damm @ 2012-05-09 6:49 UTC (permalink / raw)
To: linux-serial
Cc: horms, arnd, linux-sh, gregkh, swarren, linux-kernel, rjw,
paul.gortmaker, lethal, olof, Magnus Damm, dan.j.williams, alan
From: Magnus Damm <damm@opensource.se>
Update the 8250_em driver to correctly handle the case
where no clock is associated with the device.
The return value of clk_get() needs to be checked with
IS_ERR() to avoid NULL pointer referencing.
Signed-off-by: Magnus Damm <damm@opensource.se>
---
Applies of top of "[PATCH] serial8250-em: Emma Mobile UART driver V2"
included in linux-next 20120508.
drivers/tty/serial/8250/8250_em.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- 0001/drivers/tty/serial/8250/8250_em.c
+++ work/drivers/tty/serial/8250/8250_em.c 2012-05-09 15:35:10.000000000 +0900
@@ -110,8 +110,9 @@ static int __devinit serial8250_em_probe
}
priv->sclk = clk_get(&pdev->dev, "sclk");
- if (!priv->sclk) {
+ if (IS_ERR(priv->sclk)) {
dev_err(&pdev->dev, "unable to get clock\n");
+ ret = PTR_ERR(priv->sclk);
goto err1;
}
^ permalink raw reply
* Re: [PATCH 2/3] pty: Lock the devpts bits privately
From: H. Peter Anvin @ 2012-05-08 20:41 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, linux-serial
In-Reply-To: <20120508214309.2d50e5f0@pyramind.ukuu.org.uk>
On 05/08/2012 01:43 PM, Alan Cox wrote:
>
> One step at a time. I agree entirely that the ideal case is that
> devpts_foo is internally locked and coherent. That is an exercise for
> someone who likes devpts 8)
>
OK. I can look at that later; this should detach it enough from the tty
locking that the rest is easy.
-hpa
^ permalink raw reply
* Re: [PATCH 2/3] pty: Lock the devpts bits privately
From: Alan Cox @ 2012-05-08 20:43 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: linux-kernel, linux-serial
In-Reply-To: <4FA9637B.1060609@zytor.com>
On Tue, 08 May 2012 11:18:35 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:
> On 05/03/2012 02:22 PM, Alan Cox wrote:
> > From: Alan Cox <alan@linux.intel.com>
> >
> > This is a private pty affair, we don't want to tangle it with the tty_lock
> > any more as we know all the other non tty locking is now handled by the vfs
> > so we too can move.
> >
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
>
> > + mutex_lock(&devpts_mutex);
> > devpts_pty_kill(tty->link);
> > + mutex_unlock(&devpts_mutex);
>
> > + mutex_lock(&devpts_mutex);
> > + tty = devpts_get_tty(pts_inode, idx);
> > + mutex_unlock(&devpts_mutex);
>
> > + mutex_lock(&devpts_mutex);
> > tty = tty_init_dev(ptm_driver, index);
> > + mutex_unlock(&devpts_mutex);
>
> Conceptually this seems fine, but it would seem cleaner to me to push
> this mutex into the called functions in devpts; I suspect the lock could
> be eliminated or at least be made per instance there (which would make
> massive-container people happy...)
One step at a time. I agree entirely that the ideal case is that
devpts_foo is internally locked and coherent. That is an exercise for
someone who likes devpts 8)
Alan
^ permalink raw reply
* Re: [PATCH 2/3] pty: Lock the devpts bits privately
From: H. Peter Anvin @ 2012-05-08 18:18 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, linux-serial
In-Reply-To: <20120503212205.568.25804.stgit@bob.linux.org.uk>
On 05/03/2012 02:22 PM, Alan Cox wrote:
> From: Alan Cox <alan@linux.intel.com>
>
> This is a private pty affair, we don't want to tangle it with the tty_lock
> any more as we know all the other non tty locking is now handled by the vfs
> so we too can move.
>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> + mutex_lock(&devpts_mutex);
> devpts_pty_kill(tty->link);
> + mutex_unlock(&devpts_mutex);
> + mutex_lock(&devpts_mutex);
> + tty = devpts_get_tty(pts_inode, idx);
> + mutex_unlock(&devpts_mutex);
> + mutex_lock(&devpts_mutex);
> tty = tty_init_dev(ptm_driver, index);
> + mutex_unlock(&devpts_mutex);
Conceptually this seems fine, but it would seem cleaner to me to push
this mutex into the called functions in devpts; I suspect the lock could
be eliminated or at least be made per instance there (which would make
massive-container people happy...)
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Sasha Levin @ 2012-05-08 18:12 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Alan Cox, linux-kernel, linux-serial, Jiri Slaby
In-Reply-To: <4FA838D2.9000908@suse.cz>
On Mon, May 7, 2012 at 11:04 PM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 05/07/2012 07:00 PM, Sasha Levin wrote:
>>> So whatever your trace is showing, that's not the bug. Something more
>>> complicated would appear to be afoot.
>>
>> Oddly enough, tty != tty->link, but the lockdep warning triggers.
>>
>> Any idea why it might happen?
>
> I think so, both locks are the same lockdep class. So lockdep thinks it
> is the same lock. However this is a false positive. I guess we need
> mutex_lock_nested...
It looks like it causes an actual deadlock, and hung_tasks screams if
left alone for a bit, so it doesn't look like a lockdep issue.
^ permalink raw reply
* Re: [PATCH 00/06] serial8250: DLL/DLM rework, Emma Mobile UART driver
From: Magnus Damm @ 2012-05-08 16:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-serial, horms, linux-sh, gregkh, swarren, linux-kernel, rjw,
paul.gortmaker, lethal, olof, dan.j.williams, alan
In-Reply-To: <201205041628.20024.arnd@arndb.de>
On Sat, May 5, 2012 at 1:28 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 02 May 2012, Magnus Damm wrote:
>> Note that there is no DT support included at this point,
>> but it boils down to a 10 line change. The boot loader on
>> my board does not do DT so I'd like to use kexec for DT
>> development (as usual), but to use kexec I first need to
>> get a non-DT kernel working. Which is basically this. =)
>
> As a follow-up on this, based on my comments to your emma platform
> code, I think it would be easy enough to just use the appended
> dtb support that we have, which allows you to boot a DT-enabled
> kernel with a legacy boot loader.
Thanks for your comments! I have some code going with DT now. Will
post tomorrow.
As for the appended dtb support, do you know the recommended way to
include it in the uImage? It feels a bit odd to recommend customers to
patch their kernel source to build a bootable kernel. I ended up with
this local hackery, but there must be better ways:
--- 0001/arch/arm/boot/Makefile
+++ work/arch/arm/boot/Makefile 2012-05-09 00:40:17.000000000 +0900
@@ -53,6 +53,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image
$(obj)/zImage: $(obj)/compressed/vmlinux FORCE
$(call if_changed,objcopy)
+ cat $(obj)/emev2-kzm9d.dtb >> $@
@echo ' Kernel: $@ is ready'
endif
Cheers,
/ magnus
^ permalink raw reply
* [PATCH] cris: fix missing tty arg in wait_event_interruptible_tty call
From: Paul Gortmaker @ 2012-05-08 15:17 UTC (permalink / raw)
To: linux-serial
Cc: linux-cris-kernel, Paul Gortmaker, Mikael Starvik, Jesper Nilsson,
Alan Cox, Arnd Bergmann, Greg Kroah-Hartman
Commit d29f3ef39be4eec0362b985305fc526d9be318cf
"tty_lock: Localise the lock"
added a tty arg to wait_event_interruptible_tty() but it missed
this arch specific instance for cris, causing a compile failure.
Cc: Mikael Starvik <starvik@axis.com>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
diff --git a/drivers/tty/serial/crisv10.c b/drivers/tty/serial/crisv10.c
index b431a51..7264d4d 100644
--- a/drivers/tty/serial/crisv10.c
+++ b/drivers/tty/serial/crisv10.c
@@ -3976,7 +3976,7 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
*/
if (tty_hung_up_p(filp) ||
(info->flags & ASYNC_CLOSING)) {
- wait_event_interruptible_tty(info->close_wait,
+ wait_event_interruptible_tty(tty, info->close_wait,
!(info->flags & ASYNC_CLOSING));
#ifdef SERIAL_DO_RESTART
if (info->flags & ASYNC_HUP_NOTIFY)
@@ -4115,7 +4115,7 @@ rs_open(struct tty_struct *tty, struct file * filp)
*/
if (tty_hung_up_p(filp) ||
(info->flags & ASYNC_CLOSING)) {
- wait_event_interruptible_tty(info->close_wait,
+ wait_event_interruptible_tty(tty, info->close_wait,
!(info->flags & ASYNC_CLOSING));
#ifdef SERIAL_DO_RESTART
return ((info->flags & ASYNC_HUP_NOTIFY) ?
--
1.7.9.1
^ permalink raw reply related
* Re: What's the rationale behind sending a Xoff character when the port is stopped ?
From: Grant Edwards @ 2012-05-08 14:11 UTC (permalink / raw)
To: linux-serial
In-Reply-To: <CAES2cWxoxNG-L5hK67jH3uug9KScq=rvcqE_f+B3MaRNGDL1FA@mail.gmail.com>
On 2012-05-07, Karthik Manamcheri <karthik.manamcheri@gmail.com> wrote:
> In the case when I am flow controlled (meaning I received a Xoff from
> the other end) and my receive buffer fills up, sending a Xoff to the
> other end might overflow the buffer for the other guy, right?
Not really. The Xoff doesn't go into the other guy's receive buffer.
It's processed by the flow-control software (or hardware) before it
gets to the receive buffer.
--
Grant Edwards grant.b.edwards Yow! I put aside my copy
at of "BOWLING WORLD" and
gmail.com think about GUN CONTROL
legislation...
^ permalink raw reply
* RE: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions
From: Preston Fick @ 2012-05-08 13:56 UTC (permalink / raw)
To: Bjørn Mork, Preston Fick
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
In-Reply-To: <87ipgd3ako.fsf@nemi.mork.no>
Hi Bjorn -
I agree - I was not the original author of this driver, but am helping to bring it up to date to fix some issues and add missing support from our product line. I just simply added this in to stick with the way that it had already been developed, however I can submit another patch to setup those defines using the standard USB definitions. Thanks for the suggestion.
Kind Regards -
Preston
-----Original Message-----
From: Bjørn Mork [mailto:bjorn@mork.no]
Sent: Thursday, May 03, 2012 3:59 AM
To: Preston Fick
Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org; Preston Fick
Subject: Re: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions
Preston Fick <pffick@gmail.com> writes:
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec30f95..e67ccf3 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
> };
>
> /* Config request types */
> -#define REQTYPE_HOST_TO_DEVICE 0x41
> -#define REQTYPE_DEVICE_TO_HOST 0xc1
> +#define REQTYPE_HOST_TO_INTERFACE 0x41
> +#define REQTYPE_INTERFACE_TO_HOST 0xc1
> +#define REQTYPE_HOST_TO_DEVICE 0x40
> +#define REQTYPE_DEVICE_TO_HOST 0xc0
Any particular reason you need to define these instead of just using the
standard flags from linux/usb/ch9.h directly in the requests?:
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN)
If nothing else, using those from the beginning would have avoided the
mis-labelling you are fixing up.
Bjørn
This message (including any attachments) is intended only for the use of the individual or entity to which it is addressed and may contain information that is non-public, proprietary, privileged, confidential, and exempt from disclosure under applicable law or may constitute as attorney work product. If you are not the intended recipient, you are hereby notified that any use, dissemination, distribution, or copying of this communication is strictly prohibited. If you have received this communication in error, notify us immediately by telephone and (i) destroy this message if a facsimile or (ii) delete this message immediately if this is an electronic communication.
Thank you.
^ permalink raw reply
* Re: [PATCH RESEND 1/5] tty: serial: imx: adopt pinctrl support
From: Dong Aisheng @ 2012-05-08 3:43 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-arm-kernel, Arnd Bergmann, Greg Kroah-Hartman, Sascha Hauer,
linux-serial, Olof Johansson, Dong Aisheng
In-Reply-To: <1336352040-28447-2-git-send-email-shawn.guo@linaro.org>
On Mon, May 07, 2012 at 08:53:56AM +0800, Shawn Guo wrote:
> Cc: linux-serial@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> drivers/tty/serial/imx.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
Regards
Dong Aisheng
^ permalink raw reply
* Re: What's the rationale behind sending a Xoff character when the port is stopped ?
From: Karthik Manamcheri @ 2012-05-07 21:47 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-serial
In-Reply-To: <20120507215059.3111018f@pyramind.ukuu.org.uk>
In the case when I am flow controlled (meaning I received a Xoff from
the other end) and my receive buffer fills up, sending a Xoff to the
other end might overflow the buffer for the other guy, right ? Is that
Ok ?
On Mon, May 7, 2012 at 3:50 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Mon, 7 May 2012 15:00:50 -0500
> Karthik Manamcheri <karthik.manamcheri@gmail.com> wrote:
>
>> Hi,
>>
>> I noticed in drivers/tty/serial/8250.c that when we transmit
>> characters (by calling transmit_chars), we check for uart_tx_stopped
>> only after sending the x_char (if any). What's the rationale behind
>> this ? I would expect the uart not to send ANY characters (including a
>> Xoff character) if its throttled (or stopped).
>
> We implement xon/xoff flow control. Therefore we need to send characters
> when throttled.
>
> Think about the case where you are flow controlled for transmit and your
> receive buffer fills up...
>
> Alan
--
Karthik Manamcheri
Software R&D | Instrument Control Products | National Instruments
http://www.karthikmanamcheri.com
http://www.linkedin.com/in/kmanamcheri
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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 3/3] tty_lock: Localise the lock
From: Jiri Slaby @ 2012-05-07 21:04 UTC (permalink / raw)
To: Sasha Levin; +Cc: Alan Cox, linux-kernel, linux-serial, Jiri Slaby
In-Reply-To: <CA+1xoqez0PaX2KBHHN-etztwbFoU1cKx3NwMXX+PO4SguA3bRg@mail.gmail.com>
On 05/07/2012 07:00 PM, Sasha Levin wrote:
>> So whatever your trace is showing, that's not the bug. Something more
>> complicated would appear to be afoot.
>
> Oddly enough, tty != tty->link, but the lockdep warning triggers.
>
> Any idea why it might happen?
I think so, both locks are the same lockdep class. So lockdep thinks it
is the same lock. However this is a false positive. I guess we need
mutex_lock_nested...
regards,
--
js
suse labs
^ permalink raw reply
* Re: What's the rationale behind sending a Xoff character when the port is stopped ?
From: Alan Cox @ 2012-05-07 20:50 UTC (permalink / raw)
To: Karthik Manamcheri; +Cc: linux-serial
In-Reply-To: <CAES2cWwz=JJY14rCX8_GwnD7Mgmweiz9xSUsw6z08YZ2zNHkqw@mail.gmail.com>
On Mon, 7 May 2012 15:00:50 -0500
Karthik Manamcheri <karthik.manamcheri@gmail.com> wrote:
> Hi,
>
> I noticed in drivers/tty/serial/8250.c that when we transmit
> characters (by calling transmit_chars), we check for uart_tx_stopped
> only after sending the x_char (if any). What's the rationale behind
> this ? I would expect the uart not to send ANY characters (including a
> Xoff character) if its throttled (or stopped).
We implement xon/xoff flow control. Therefore we need to send characters
when throttled.
Think about the case where you are flow controlled for transmit and your
receive buffer fills up...
Alan
^ permalink raw reply
* What's the rationale behind sending a Xoff character when the port is stopped ?
From: Karthik Manamcheri @ 2012-05-07 20:00 UTC (permalink / raw)
To: linux-serial
Hi,
I noticed in drivers/tty/serial/8250.c that when we transmit
characters (by calling transmit_chars), we check for uart_tx_stopped
only after sending the x_char (if any). What's the rationale behind
this ? I would expect the uart not to send ANY characters (including a
Xoff character) if its throttled (or stopped).
--
Karthik Manamcheri
Software R&D | Instrument Control Products | National Instruments
http://www.linkedin.com/in/kmanamcheri
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* [PATCH -next] tty/amiserial: Add missing argument for tty_unlock()
From: Geert Uytterhoeven @ 2012-05-07 19:36 UTC (permalink / raw)
To: Alan Cox, Greg Kroah-Hartman
Cc: linux-next, linux-kernel, linux-serial, Geert Uytterhoeven
commit d29f3ef39be4eec0362b985305fc526d9be318cf ("tty_lock: Localise the lock")
missed that d3a7b83f865b46bb7b5e1ed18a129ce1af349db4 ("drivers/tty/amiserial.c:
add missing tty_unlock") just added a new caller of tty_unlock():
drivers/tty/amiserial.c: In function ‘set_serial_info’:
drivers/tty/amiserial.c:1077: error: too few arguments to function ‘tty_unlock’
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
http://kisskb.ellerman.id.au/kisskb/buildresult/6271985/
drivers/tty/amiserial.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index b88a65c..35819e3 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -1074,7 +1074,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_state *state,
(new_serial.xmit_fifo_size != state->xmit_fifo_size) ||
((new_serial.flags & ~ASYNC_USR_MASK) !=
(port->flags & ~ASYNC_USR_MASK))) {
- tty_unlock();
+ tty_unlock(tty);
return -EPERM;
}
port->flags = ((port->flags & ~ASYNC_USR_MASK) |
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Sasha Levin @ 2012-05-07 17:00 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, linux-serial
In-Reply-To: <20120507174240.4209c5cb@pyramind.ukuu.org.uk>
> So whatever your trace is showing, that's not the bug. Something more
> complicated would appear to be afoot.
Oddly enough, tty != tty->link, but the lockdep warning triggers.
Any idea why it might happen?
^ permalink raw reply
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Alan Cox @ 2012-05-07 16:42 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, linux-serial
In-Reply-To: <1336408208.3638.15.camel@lappy>
On Mon, 07 May 2012 18:30:08 +0200
Sasha Levin <levinsasha928@gmail.com> wrote:
> On Mon, 2012-05-07 at 17:11 +0100, Alan Cox wrote:
> > > I don't believe that this change is correct.
> > >
> > > Consider the following scenario:
> > >
> > > tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock
> >
> > We hang up tty->link not tty.
> >
> > It's now a per tty lock. So I think we are ok.
>
> Unless we can cause tty->link == tty, in which case:
We should not be able to cause tty->link == tty. So that's a different
problem altogether.
tty->link is set to point to the other half of the pty in pty_install and
in pty98_unix_install. It's never assigned to the same tty and ptys
simply wouldn't work if this wasn't the case.
So whatever your trace is showing, that's not the bug. Something more
complicated would appear to be afoot.
Alan
^ permalink raw reply
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Sasha Levin @ 2012-05-07 16:30 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, linux-serial
In-Reply-To: <20120507171126.5beddc27@pyramind.ukuu.org.uk>
On Mon, 2012-05-07 at 17:11 +0100, Alan Cox wrote:
> > I don't believe that this change is correct.
> >
> > Consider the following scenario:
> >
> > tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock
>
> We hang up tty->link not tty.
>
> It's now a per tty lock. So I think we are ok.
Unless we can cause tty->link == tty, in which case:
[ 6522.256890] =============================================
[ 6522.257023] [ INFO: possible recursive locking detected ]
[ 6522.257023] 3.4.0-rc6-next-20120507-sasha-00001-g06a300f #175 Tainted: G W
[ 6522.257023] ---------------------------------------------
[ 6522.257023] trinity/18088 is trying to acquire lock:
[ 6522.257023] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80
[ 6522.257023]
[ 6522.257023] but task is already holding lock:
[ 6522.257023] (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80
[ 6522.257023]
[ 6522.257023] other info that might help us debug this:
[ 6522.257023] Possible unsafe locking scenario:
[ 6522.257023]
[ 6522.257023] CPU0
[ 6522.257023] ----
[ 6522.257023] lock(&tty->legacy_mutex);
[ 6522.257023] lock(&tty->legacy_mutex);
[ 6522.257023]
[ 6522.257023] *** DEADLOCK ***
[ 6522.257023]
[ 6522.257023] May be due to missing lock nesting notation
[ 6522.257023]
[ 6522.257023] 1 lock held by trinity/18088:
[ 6522.257023] #0: (&tty->legacy_mutex){+.+.+.}, at: [<ffffffff82d8a6f2>] tty_lock+0x72/0x80
[ 6522.257023]
[ 6522.257023] stack backtrace:
[ 6522.257023] Pid: 18088, comm: trinity Tainted: G W 3.4.0-rc6-next-20120507-sasha-00001-g06a300f #175
[ 6522.257023] Call Trace:
[ 6522.257023] [<ffffffff8111a509>] print_deadlock_bug+0x119/0x140
[ 6522.257023] [<ffffffff8111c6fe>] validate_chain+0x5ee/0x790
[ 6522.257023] [<ffffffff810f1418>] ? sched_clock_cpu+0x108/0x120
[ 6522.257023] [<ffffffff8111ccc3>] __lock_acquire+0x423/0x4c0
[ 6522.257023] [<ffffffff8111ce3c>] lock_acquire+0xdc/0x120
[ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80
[ 6522.257023] [<ffffffff82d86b60>] __mutex_lock_common+0x60/0x590
[ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80
[ 6522.257023] [<ffffffff82d8a6f2>] ? tty_lock+0x72/0x80
[ 6522.257023] [<ffffffff82d871c0>] mutex_lock_nested+0x40/0x50
[ 6522.257023] [<ffffffff82d8a6f2>] tty_lock+0x72/0x80
[ 6522.257023] [<ffffffff81a2ce34>] __tty_hangup+0x74/0x400
[ 6522.257023] [<ffffffff82d8a154>] ? _raw_spin_unlock_irqrestore+0x94/0xc0
[ 6522.257023] [<ffffffff81a2d1e9>] tty_vhangup+0x9/0x10
[ 6522.257023] [<ffffffff81a36264>] pty_close+0x154/0x160
[ 6522.257023] [<ffffffff81a2dfcd>] tty_release+0xed/0x4d0
[ 6522.257023] [<ffffffff8122d0eb>] ? vfs_lock_file+0x3b/0x40
[ 6522.257023] [<ffffffff8122d18e>] ? locks_remove_posix+0x9e/0xe0
[ 6522.257023] [<ffffffff811e13ea>] __fput+0x11a/0x2c0
[ 6522.257023] [<ffffffff811e15a5>] fput+0x15/0x20
[ 6522.257023] [<ffffffff811dd8b2>] filp_close+0x82/0xa0
[ 6522.257023] [<ffffffff810bb914>] close_files+0x1b4/0x200
[ 6522.257023] [<ffffffff810bb760>] ? sys_waitid+0x200/0x200
[ 6522.257023] [<ffffffff810bb981>] put_files_struct+0x21/0x180
[ 6522.257023] [<ffffffff82d8a090>] ? _raw_spin_unlock+0x30/0x60
[ 6522.257023] [<ffffffff810bbb2d>] exit_files+0x4d/0x60
[ 6522.257023] [<ffffffff810bc7b5>] do_exit+0x285/0x460
[ 6522.257023] [<ffffffff810e74e1>] ? get_parent_ip+0x11/0x50
[ 6522.257023] [<ffffffff810bca31>] do_group_exit+0xa1/0xe0
[ 6522.257023] [<ffffffff810cceb8>] get_signal_to_deliver+0x348/0x3a0
[ 6522.257023] [<ffffffff810e855d>] ? finish_task_switch+0x8d/0x110
[ 6522.257023] [<ffffffff8104daf2>] do_signal+0x42/0x120
[ 6522.257023] [<ffffffff810e74e1>] ? get_parent_ip+0x11/0x50
[ 6522.257023] [<ffffffff810e7c1e>] ? sub_preempt_count+0xae/0xf0
[ 6522.257023] [<ffffffff82d8850f>] ? __schedule+0x79f/0x7d0
[ 6522.257023] [<ffffffff82d8a934>] ? retint_restore_args+0x13/0x13
[ 6522.257023] [<ffffffff82d8a9bf>] ? retint_signal+0x11/0x92
[ 6522.257023] [<ffffffff8104dc24>] do_notify_resume+0x54/0xb0
[ 6522.257023] [<ffffffff82d8a9fb>] retint_signal+0x4d/0x92
^ permalink raw reply
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Alan Cox @ 2012-05-07 16:11 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, linux-serial
In-Reply-To: <CA+1xoqezLCQh_fqJ1iNAs25X7Fm24W=-0J8zTZuVVJQnWTtSLg@mail.gmail.com>
> I don't believe that this change is correct.
>
> Consider the following scenario:
>
> tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock
We hang up tty->link not tty.
It's now a per tty lock. So I think we are ok.
Alan
^ permalink raw reply
* Re: [PATCH 3/3] tty_lock: Localise the lock
From: Sasha Levin @ 2012-05-07 16:03 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel, linux-serial
In-Reply-To: <20120503212219.568.15653.stgit@bob.linux.org.uk>
Hi Alan,
On Thu, May 3, 2012 at 11:24 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 5505ffc..d6fa842 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -62,9 +63,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
> mutex_unlock(&devpts_mutex);
> }
> #endif
> - tty_unlock();
> tty_vhangup(tty->link);
> - tty_lock();
> }
> }
I don't believe that this change is correct.
Consider the following scenario:
tty_release -> tty_lock -> pty_close -> tty_vhangup -> tty_lock
Which would cause a deadlock.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3)
From: Ivo Sieben @ 2012-05-07 14:10 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben
In-Reply-To: <1336048663-21882-1-git-send-email-meltedpianoman@gmail.com>
Hi,
2012/5/3 Ivo Sieben <meltedpianoman@gmail.com>:
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -317,12 +317,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
>
> void tty_schedule_flip(struct tty_struct *tty)
> {
> - unsigned long flags;
> - spin_lock_irqsave(&tty->buf.lock, flags);
> - if (tty->buf.tail != NULL)
> - tty->buf.tail->commit = tty->buf.tail->used;
> - spin_unlock_irqrestore(&tty->buf.lock, flags);
> - schedule_work(&tty->buf.work);
> + tty_flip_buffer_push(tty);
> }
> EXPORT_SYMBOL(tty_schedule_flip);
>
I have an additional question on the above change in the patch that I've send:
I found that two functions in drivers/tty/tty_buffer.c implement
almost the same functionality:
- tty_schedule_flip
- tty_flip_buffer_push
Only difference was that tty_schedule_flip() always uses the work
queue, while the tty_flip_buffer_push only uses the work queue in case
of a non prempt_rt system and low_latency flag unset.
But is my change correct? I see that most serial drivers use the
tty_flip_buffer_push() function. But still a number of drivers use the
tty_schedule_flip() function. I even found one driver that uses both
(drivers/staging/serqt_usb2/serqt_usb2.c). Does my patch introduce
bugs to these drivers? Or is the tty_schedule_flip() a legacy
function, and would it be better to remove it completely?
Regards,
Ivo Sieben
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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: [alsa-devel] [PATCH RESEND 0/9] Enable pinctrl support for mach-mxs
From: Dong Aisheng @ 2012-05-07 8:01 UTC (permalink / raw)
To: Shawn Guo
Cc: Dong Aisheng-B29396, linux-arm-kernel@lists.infradead.org,
linux-fbdev@vger.kernel.org, Chris Ball, Arnd Bergmann,
Florian Tobias Schandinat, Artem Bityutskiy, Mark Brown,
linux-mmc@vger.kernel.org, alsa-devel@alsa-project.org,
Wolfram Sang, linux-mtd@lists.infradead.org,
linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org,
Greg Kroah-Hartman, Olof Johansson
In-Reply-To: <20120507074658.GH19389@S2101-09.ap.freescale.net>
On Mon, May 07, 2012 at 03:47:00PM +0800, Shawn Guo wrote:
> On Mon, May 07, 2012 at 03:14:00PM +0800, Dong Aisheng wrote:
> > As IMX, basically i'd prefer to add pinctrl states in dts file at the
> > same time within the patch or using a separate patch to add them before
> > this series to avoid breaking the exist platforms.
> >
> > However i noted that for mxs, most drivers here are still not dt capable,
> > so it may be ok to not add their pinctrl state at this time.
> >
> There no mxs driver on mainline that has been DT aware of. What I'm
> going to do is to ask Arnd abandon the mxs/dt branch I sent him before
> and send him an updated one with the whole mxs DT support based on
> mxs common clk and pinctrl series.
>
> > But for the patch "serial: amba-pl011: adopt pinctrl support" since it is
> > dt capable, so with this patch applied, the mx28 dt boot will fail.
> > Maybe we should at least add pinctrl states for amba-pl011 first.
> >
> The updated mxs/dt will have pinctrl defined in dts for each device
> that is converted to DT.
>
Well, i did not see amba-pl011 pinctrl states defined in this patch.
But it would be ok if you can get it done and applied before this patch.
> > > Shawn Guo (9):
> > > ARM: mxs: enable pinctrl dummy states
> > > serial: amba-pl011: adopt pinctrl support
> > BTW, will this one break other platforms using this driver?
> >
> If the platforms do not turn on CONFIG_PINCTRL, they are fine. If they
> turn on the support, they should provide pinctrl state either dummy or
> real one.
>
Regards
Dong Aisheng
^ permalink raw reply
* Re: [alsa-devel] [PATCH RESEND 0/9] Enable pinctrl support for mach-mxs
From: Shawn Guo @ 2012-05-07 7:47 UTC (permalink / raw)
To: Dong Aisheng
Cc: linux-arm-kernel, linux-fbdev, Chris Ball, Arnd Bergmann,
Florian Tobias Schandinat, Artem Bityutskiy, Mark Brown,
linux-mmc, alsa-devel, Wolfram Sang, linux-mtd, linux-i2c,
linux-serial, Greg Kroah-Hartman, Olof Johansson
In-Reply-To: <20120507071400.GB23607@shlinux2.ap.freescale.net>
On Mon, May 07, 2012 at 03:14:00PM +0800, Dong Aisheng wrote:
> As IMX, basically i'd prefer to add pinctrl states in dts file at the
> same time within the patch or using a separate patch to add them before
> this series to avoid breaking the exist platforms.
>
> However i noted that for mxs, most drivers here are still not dt capable,
> so it may be ok to not add their pinctrl state at this time.
>
There no mxs driver on mainline that has been DT aware of. What I'm
going to do is to ask Arnd abandon the mxs/dt branch I sent him before
and send him an updated one with the whole mxs DT support based on
mxs common clk and pinctrl series.
> But for the patch "serial: amba-pl011: adopt pinctrl support" since it is
> dt capable, so with this patch applied, the mx28 dt boot will fail.
> Maybe we should at least add pinctrl states for amba-pl011 first.
>
The updated mxs/dt will have pinctrl defined in dts for each device
that is converted to DT.
> > Shawn Guo (9):
> > ARM: mxs: enable pinctrl dummy states
> > serial: amba-pl011: adopt pinctrl support
> BTW, will this one break other platforms using this driver?
>
If the platforms do not turn on CONFIG_PINCTRL, they are fine. If they
turn on the support, they should provide pinctrl state either dummy or
real one.
--
Regards,
Shawn
^ permalink raw reply
* Re: [PATCH RESEND 0/5] Adopt pinctrl support for a few outstanding imx drivers
From: Dong Aisheng @ 2012-05-07 7:53 UTC (permalink / raw)
To: Shawn Guo
Cc: Dong Aisheng-B29396, linux-arm-kernel@lists.infradead.org,
Arnd Bergmann, netdev@vger.kernel.org, Sascha Hauer, Wolfram Sang,
linux-can@vger.kernel.org, Grant Likely, Marc Kleine-Budde,
linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org,
Greg Kroah-Hartman, Olof Johansson,
spi-devel-general@lists.sourceforge.net, Dong Aisheng,
David S. Miller
In-Reply-To: <20120507073403.GG19389@S2101-09.ap.freescale.net>
On Mon, May 07, 2012 at 03:34:06PM +0800, Shawn Guo wrote:
> On Mon, May 07, 2012 at 02:50:02PM +0800, Dong Aisheng wrote:
> > Shouldn't we add the pinctrl states in dts file at the same time
> > with this patch series or using another separate patch to add them
> > before this series to avoid breaking the exist mx6q platforms?
> >
> Ah, I just noticed that your patch "ARM: imx: enable pinctrl dummy
> states" did not cover imx6q. I think we should do the same for imx6q,
Yes, doing that was to force people to add pinctrl states in dts file
rather than using dummy state since mx6 supports pinctrl driver.
> so that we can separate dts update from the driver change. When all
> imx6q boards' dts files get updated to have pins defined for the
> devices, we can then remove dummy state for imx6q. Doing so will ease
> the pinctrl migration for those imx6q boards.
>
Well, considering we have several mx6 boards, i think i can also be fine
with this way to ease the mx6q pinctrl migration.
> Will update your patch on my branch to have dummy state enabled for
> imx6q.
>
Then go ahead.
Regards
Dong Aisheng
^ permalink raw reply
* Re: [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3)
From: Ivo Sieben @ 2012-05-07 7:45 UTC (permalink / raw)
To: Greg KH; +Cc: linux-serial, Alan Cox, RT
In-Reply-To: <20120503162529.GB3063@kroah.com>
Hi,
2012/5/3 Greg KH <gregkh@linuxfoundation.org>:
>
> Why, what does this cause to have happen? What's the difference here
> that causes any speedups or latency fixes?
>
This patch solves the following scenario:
- A low priority process starts to send data to serial port A
- In order to do this, the low priority process calls the
tty_ldisc_ref_wait() function to retrieve a reference to its line
discipline.
- In the tty_ldisc_ref_wait() function, the global ldisc spinlock is locked.
- While the spin lock is still locked, a context switch takes place
and a second process on high real-time priority starts reading data
from another serial port B (this is a possible scenario on a
PREEMPT_RT system where a "normal" spin lock behaves like a mutex and
no interrupts are actually disabled, although the "irqsave" postfix in
the spinlock function name suggests otherwise)
- The second project also calls the tty_ldisc_ref_wait() function, and
therefor gets blocked on the global ldisc spin lock.
- As a result priority inversion takes place, and the second process
is scheduled out, the first process is scheduled in on high priority.
After the first process has released the spin lock, the second process
is scheduled again.
The above scenario is not wrong, it works fine... but the scheduling &
mutex handling requires a lot of extra processing time what makes that
a normal read operation of about 50us sometimes can last up to 230us.
By using raw spin locks this situation is prevented. I think it is
legitimate to use raw spin locks because the critical sections in
ldisc are small.
Regards,
Ivo Sieben
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
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