* [PATCH v4 2/2] serial: mxs-auart: Allow device tree probing
From: Fabio Estevam @ 2012-06-18 13:06 UTC (permalink / raw)
To: linux-arm-kernel
Cc: shawn.guo, kernel, marex, rob.herring, Fabio Estevam,
Grant Likely, Alan Cox, linux-serial
In-Reply-To: <1339774557-23588-1-git-send-email-fabio.estevam@freescale.com>
Allow device tree probing.
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: <linux-serial@vger.kernel.org>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v3:
- Remove unneeded mxs_auart_devtype
Changes since v2:
- Change compatible string
- Add aliases information in bindings doc
- Remove unneeded mxs_auart_probe_pdev function
- Remove "ifdef CONFIG_OF"
- Remove of_match_ptr wrapper
Changes since v1:
- Merged patches 3 and 5 from v1 into this one
.../bindings/tty/serial/fsl-mxs-auart.txt | 27 +++++++++++++
drivers/tty/serial/mxs-auart.c | 40 +++++++++++++++++++-
2 files changed, 66 insertions(+), 1 deletions(-)
create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
new file mode 100644
index 0000000..2ee903f
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
@@ -0,0 +1,27 @@
+* Freescale MXS Application UART (AUART)
+
+Required properties:
+- compatible : Should be "fsl,<soc>-auart". The supported SoCs include
+ imx23 and imx28.
+- reg : Address and length of the register set for the device
+- interrupts : Should contain the auart interrupt numbers
+
+Example:
+auart0: serial@8006a000 {
+ compatible = "fsl,imx28-auart", "fsl,imx23-auart";
+ reg = <0x8006a000 0x2000>;
+ interrupts = <112 70 71>;
+};
+
+Note: Each auart port should have an alias correctly numbered in "aliases"
+node.
+
+Example:
+
+aliases {
+ serial0 = &auart0;
+ serial1 = &auart1;
+ serial2 = &auart2;
+ serial3 = &auart3;
+ serial4 = &auart4;
+};
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index ec56d83..103087d 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -33,6 +33,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/of_device.h>
#include <asm/cacheflush.h>
@@ -675,6 +676,30 @@ static struct uart_driver auart_driver = {
#endif
};
+/*
+ * This function returns 1 if pdev isn't a device instatiated by dt, 0 if it
+ * could successfully get all information from dt or a negative errno.
+ */
+static int serial_mxs_probe_dt(struct mxs_auart_port *s,
+ struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ if (!np)
+ /* no device tree device */
+ return 1;
+
+ ret = of_alias_get_id(np, "serial");
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to get alias id: %d\n", ret);
+ return ret;
+ }
+ s->port.line = ret;
+
+ return 0;
+}
+
static int __devinit mxs_auart_probe(struct platform_device *pdev)
{
struct mxs_auart_port *s;
@@ -689,6 +714,12 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
goto out;
}
+ ret = serial_mxs_probe_dt(s, pdev);
+ if (ret > 0)
+ s->port.line = pdev->id < 0 ? 0 : pdev->id;
+ else if (ret < 0)
+ goto out_free;
+
pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
if (IS_ERR(pinctrl)) {
ret = PTR_ERR(pinctrl);
@@ -711,7 +742,6 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
s->port.membase = ioremap(r->start, resource_size(r));
s->port.ops = &mxs_auart_ops;
s->port.iotype = UPIO_MEM;
- s->port.line = pdev->id < 0 ? 0 : pdev->id;
s->port.fifosize = 16;
s->port.uartclk = clk_get_rate(s->clk);
s->port.type = PORT_IMX;
@@ -769,12 +799,19 @@ static int __devexit mxs_auart_remove(struct platform_device *pdev)
return 0;
}
+static struct of_device_id mxs_auart_dt_ids[] = {
+ { .compatible = "fsl,imx23-auart", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_auart_dt_ids);
+
static struct platform_driver mxs_auart_driver = {
.probe = mxs_auart_probe,
.remove = __devexit_p(mxs_auart_remove),
.driver = {
.name = "mxs-auart",
.owner = THIS_MODULE,
+ .of_match_table = mxs_auart_dt_ids,
},
};
@@ -807,3 +844,4 @@ module_init(mxs_auart_init);
module_exit(mxs_auart_exit);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Freescale MXS application uart driver");
+MODULE_ALIAS("platform:mxs-auart");
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v4 2/2] serial: mxs-auart: Allow device tree probing
From: Shawn Guo @ 2012-06-18 13:33 UTC (permalink / raw)
To: Fabio Estevam, Greg Kroah-Hartman
Cc: linux-arm-kernel, kernel, marex, rob.herring, Grant Likely,
Alan Cox, linux-serial
In-Reply-To: <1340024769-7122-1-git-send-email-fabio.estevam@freescale.com>
On Mon, Jun 18, 2012 at 10:06:09AM -0300, Fabio Estevam wrote:
> Allow device tree probing.
>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: <linux-serial@vger.kernel.org>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v3:
> - Remove unneeded mxs_auart_devtype
> Changes since v2:
> - Change compatible string
> - Add aliases information in bindings doc
> - Remove unneeded mxs_auart_probe_pdev function
> - Remove "ifdef CONFIG_OF"
> - Remove of_match_ptr wrapper
> Changes since v1:
> - Merged patches 3 and 5 from v1 into this one
Hi Greg,
May I have your ack to have the patch go through arm-soc tree, so that
we can possibly start converting those non-DT board files over to DT?
Regards,
Shawn
> .../bindings/tty/serial/fsl-mxs-auart.txt | 27 +++++++++++++
> drivers/tty/serial/mxs-auart.c | 40 +++++++++++++++++++-
> 2 files changed, 66 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
>
> diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
> new file mode 100644
> index 0000000..2ee903f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
> @@ -0,0 +1,27 @@
> +* Freescale MXS Application UART (AUART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-auart". The supported SoCs include
> + imx23 and imx28.
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain the auart interrupt numbers
> +
> +Example:
> +auart0: serial@8006a000 {
> + compatible = "fsl,imx28-auart", "fsl,imx23-auart";
> + reg = <0x8006a000 0x2000>;
> + interrupts = <112 70 71>;
> +};
> +
> +Note: Each auart port should have an alias correctly numbered in "aliases"
> +node.
> +
> +Example:
> +
> +aliases {
> + serial0 = &auart0;
> + serial1 = &auart1;
> + serial2 = &auart2;
> + serial3 = &auart3;
> + serial4 = &auart4;
> +};
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index ec56d83..103087d 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -33,6 +33,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/of_device.h>
>
> #include <asm/cacheflush.h>
>
> @@ -675,6 +676,30 @@ static struct uart_driver auart_driver = {
> #endif
> };
>
> +/*
> + * This function returns 1 if pdev isn't a device instatiated by dt, 0 if it
> + * could successfully get all information from dt or a negative errno.
> + */
> +static int serial_mxs_probe_dt(struct mxs_auart_port *s,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + if (!np)
> + /* no device tree device */
> + return 1;
> +
> + ret = of_alias_get_id(np, "serial");
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get alias id: %d\n", ret);
> + return ret;
> + }
> + s->port.line = ret;
> +
> + return 0;
> +}
> +
> static int __devinit mxs_auart_probe(struct platform_device *pdev)
> {
> struct mxs_auart_port *s;
> @@ -689,6 +714,12 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
> goto out;
> }
>
> + ret = serial_mxs_probe_dt(s, pdev);
> + if (ret > 0)
> + s->port.line = pdev->id < 0 ? 0 : pdev->id;
> + else if (ret < 0)
> + goto out_free;
> +
> pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> if (IS_ERR(pinctrl)) {
> ret = PTR_ERR(pinctrl);
> @@ -711,7 +742,6 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
> s->port.membase = ioremap(r->start, resource_size(r));
> s->port.ops = &mxs_auart_ops;
> s->port.iotype = UPIO_MEM;
> - s->port.line = pdev->id < 0 ? 0 : pdev->id;
> s->port.fifosize = 16;
> s->port.uartclk = clk_get_rate(s->clk);
> s->port.type = PORT_IMX;
> @@ -769,12 +799,19 @@ static int __devexit mxs_auart_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static struct of_device_id mxs_auart_dt_ids[] = {
> + { .compatible = "fsl,imx23-auart", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_auart_dt_ids);
> +
> static struct platform_driver mxs_auart_driver = {
> .probe = mxs_auart_probe,
> .remove = __devexit_p(mxs_auart_remove),
> .driver = {
> .name = "mxs-auart",
> .owner = THIS_MODULE,
> + .of_match_table = mxs_auart_dt_ids,
> },
> };
>
> @@ -807,3 +844,4 @@ module_init(mxs_auart_init);
> module_exit(mxs_auart_exit);
> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("Freescale MXS application uart driver");
> +MODULE_ALIAS("platform:mxs-auart");
> --
> 1.7.1
>
>
^ permalink raw reply
* Re: [PATCH v4 2/2] serial: mxs-auart: Allow device tree probing
From: Marek Vasut @ 2012-06-18 13:36 UTC (permalink / raw)
To: Shawn Guo
Cc: Fabio Estevam, Greg Kroah-Hartman, linux-arm-kernel, kernel,
rob.herring, Grant Likely, Alan Cox, linux-serial
In-Reply-To: <20120618133338.GD19249@S2101-09.ap.freescale.net>
Dear Shawn Guo,
> On Mon, Jun 18, 2012 at 10:06:09AM -0300, Fabio Estevam wrote:
> > Allow device tree probing.
> >
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Alan Cox <alan@linux.intel.com>
> > Cc: <linux-serial@vger.kernel.org>
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> > Changes since v3:
> > - Remove unneeded mxs_auart_devtype
> > Changes since v2:
> > - Change compatible string
> > - Add aliases information in bindings doc
> > - Remove unneeded mxs_auart_probe_pdev function
> > - Remove "ifdef CONFIG_OF"
> > - Remove of_match_ptr wrapper
> > Changes since v1:
> > - Merged patches 3 and 5 from v1 into this one
>
> Hi Greg,
>
> May I have your ack to have the patch go through arm-soc tree, so that
> we can possibly start converting those non-DT board files over to DT?
>
> Regards,
> Shawn
Good, I should be back on 20th to the denx board.
^ permalink raw reply
* Re: [PATCH v4 2/2] serial: mxs-auart: Allow device tree probing
From: Greg Kroah-Hartman @ 2012-06-18 15:25 UTC (permalink / raw)
To: Shawn Guo
Cc: Fabio Estevam, linux-arm-kernel, kernel, marex, rob.herring,
Grant Likely, Alan Cox, linux-serial
In-Reply-To: <20120618133338.GD19249@S2101-09.ap.freescale.net>
On Mon, Jun 18, 2012 at 09:33:40PM +0800, Shawn Guo wrote:
> On Mon, Jun 18, 2012 at 10:06:09AM -0300, Fabio Estevam wrote:
> > Allow device tree probing.
> >
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Alan Cox <alan@linux.intel.com>
> > Cc: <linux-serial@vger.kernel.org>
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> > Changes since v3:
> > - Remove unneeded mxs_auart_devtype
> > Changes since v2:
> > - Change compatible string
> > - Add aliases information in bindings doc
> > - Remove unneeded mxs_auart_probe_pdev function
> > - Remove "ifdef CONFIG_OF"
> > - Remove of_match_ptr wrapper
> > Changes since v1:
> > - Merged patches 3 and 5 from v1 into this one
>
> Hi Greg,
>
> May I have your ack to have the patch go through arm-soc tree, so that
> we can possibly start converting those non-DT board files over to DT?
Fine with me:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH] drivers/tty/tty_io.c: fix a potential memleak at do_tty_write()
From: Greg KH @ 2012-06-18 15:26 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-serial
In-Reply-To: <4FDF1DDA.3030704@oracle.com>
On Mon, Jun 18, 2012 at 08:23:54PM +0800, Jeff Liu wrote:
> Hello,
>
> Looks there is a potential memory leak at drivers/tty/tty_io.c: do_tty_write().
> It did allocate a buf_chunk if tty->write_cnt < chunk, however, buf_chunk was not
> freed after the writing is done. Below tiny patch could fix it.
>
> Thanks,
> -Jeff
>
Thanks for the patch, but can you resend it with a signed-off-by: line
as described in Documentation/SubmittingPatches so that we can properly
apply the patch?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] drivers/tty/tty_io.c: fix a potential memleak at do_tty_write()
From: Alan Cox @ 2012-06-18 16:04 UTC (permalink / raw)
To: jeff.liu; +Cc: linux-serial
In-Reply-To: <4FDF1DDA.3030704@oracle.com>
On Mon, 18 Jun 2012 20:23:54 +0800
Jeff Liu <jeff.liu@oracle.com> wrote:
> Hello,
>
> Looks there is a potential memory leak at drivers/tty/tty_io.c: do_tty_write().
> It did allocate a buf_chunk if tty->write_cnt < chunk, however, buf_chunk was not
> freed after the writing is done. Below tiny patch could fix it.
Why should it be freed, we still have a reference to it.
Alan
^ permalink raw reply
* Re: [PATCH] drivers/tty/tty_io.c: fix a potential memleak at do_tty_write()
From: Paul Fulghum @ 2012-06-18 18:00 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff.liu, linux-serial
In-Reply-To: <20120618170415.4d035911@pyramind.ukuu.org.uk>
On 6/18/2012 11:04 AM, Alan Cox wrote:
> On Mon, 18 Jun 2012 20:23:54 +0800
> Jeff Liu <jeff.liu@oracle.com> wrote:
>
>> Hello,
>>
>> Looks there is a potential memory leak at drivers/tty/tty_io.c: do_tty_write().
>> It did allocate a buf_chunk if tty->write_cnt < chunk, however, buf_chunk was not
>> freed after the writing is done. Below tiny patch could fix it.
>
> Why should it be freed, we still have a reference to it.
Yeah, it would be messy on the next write()
when the now freed tty->write_buf is accessed ;-)
*boom*
--
Paul Fulghum
MicroGate Systems, Ltd.
^ permalink raw reply
* Re: [PATCH 04/10] fblog: implement fblog_redraw()
From: David Herrmann @ 2012-06-18 18:36 UTC (permalink / raw)
To: Alan Cox
Cc: linux-serial, Florian Tobias Schandinat, linux-fbdev,
linux-kernel, Greg Kroah-Hartman
In-Reply-To: <20120616233526.7f830890@pyramind.ukuu.org.uk>
Hi Alan
On Sun, Jun 17, 2012 at 12:35 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Sun, 17 Jun 2012 00:04:20 +0200
> David Herrmann <dh.herrmann@googlemail.com> wrote:
>
>> This mostly copies the functionality from drivers/video/console/bitblit.c
>> so we can draw the console content on all available framebuffers.
>>
>> All speed optimizations have been removed for simplicity. The original
>> code depends heavily on CONFIG_VT so we cannot share the codebase here.
>
> No. That means we've got two sets of code to maintain not one. Fix the
> dependancy.
>
> Pull the relevant subset of struct vc_data into another struct
> Make struct vc_data be
>
> struct vc_data {
> struct vc_whatever
> rest
> }
It's a bit more complex as we cannot call scr_read() either. Hence, I
need to assemble the array of printed characters before calling the
redraw functions. But that should be feasible, too. I just need to
figure out how to avoid heavy allocations during redraw to avoid
slowdowns.
If there are no objections I will send these patches as a separate
patchset as we can apply it without fblog.
> Alan
Thanks for reviewing
David
^ permalink raw reply
* Re: [PATCH 05/10] fblog: add framebuffer helpers
From: David Herrmann @ 2012-06-18 18:50 UTC (permalink / raw)
To: Bruno Prémont
Cc: linux-serial, Florian Tobias Schandinat, linux-fbdev,
linux-kernel, Greg Kroah-Hartman
In-Reply-To: <20120617003334.765f6226@neptune.home>
Hi Bruno
On Sun, Jun 17, 2012 at 12:33 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> On Sun, 17 June 2012 David Herrmann <dh.herrmann@googlemail.com> wrote:
>> These helpers scan the system for all available framebuffers and register
>> or unregister them. This is needed during startup and stopping fblog so we
>> are aware of all connected displays.
>>
>> The third helper handles mode changes by rescanning the mode and adjusting
>> the buffer size.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>> ---
>> drivers/video/console/fblog.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index e790971..7d4032e 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -399,6 +399,35 @@ static void fblog_unregister(struct fblog_fb *fb)
>> kfree(fb);
>> }
>>
>> +static void fblog_register_all(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < FB_MAX; ++i)
>> + fblog_register(registered_fb[i]);
>
> You should take registration_lock mutex for accessing registered_fb[],
> even better would be to make use of get_fb_info() and put_fb_info()
Indeed, I will change it to use get_fb_info()/put_fb_info().
Btw., is it safe to call console_lock() during
FB_EVENT_FB_(UN)REGISTERED? I definitely need the console lock when
calling fblog_register() but I am not sure whether all fb-drivers lock
the console during "(un)register_framebuffer()". Otherwise, I would
have to avoid redrawing the console inside of fblog_register().
>> +}
>> +
>> +static void fblog_unregister_all(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < FB_MAX; ++i)
>> + fblog_unregister(fblog_info2fb(registered_fb[i]));
>
> Same here.
>
> Though for unregistering I'm wondering why you still scan through
> registered_fb[], you should just scan your fblog_fbs[] array!
Oops, you're right. I will change it.
> But here again, make sure to have proper locking to not get races with
> registration of new framebuffers or removal of existing ones via
> notifications.
>
>> +}
>> +
>> +static void fblog_refresh(struct fblog_fb *fb)
>> +{
>> + unsigned int width, height;
>> +
>> + if (!fb || !fb->font)
>> + return;
>> +
>> + width = fb->info->var.xres / fb->font->width;
>> + height = fb->info->var.yres / fb->font->height;
>> + fblog_buf_resize(&fb->buf, width, height);
>> + fblog_redraw(fb);
>> +}
>> +
>
> All these new functions are still unused, for easier following of your
> patch series it would be nice to have them connected when they are
> introduced as otherwise on has to search all following patches for
> finding possible users.
I have to admit the split was crap. I am sorry. I will recreate the
patchset with a proper split. Thanks!
>> static int __init fblog_init(void)
>> {
>> return 0;
>
> Bruno
Thanks for reviewing, regards
David
^ permalink raw reply
* Re: [RFC 00/10] fblog: framebuffer kernel log driver
From: David Herrmann @ 2012-06-18 19:06 UTC (permalink / raw)
To: linux-serial
Cc: Florian Tobias Schandinat, linux-fbdev, linux-kernel,
Greg Kroah-Hartman, David Herrmann
In-Reply-To: <1339884266-9201-1-git-send-email-dh.herrmann@googlemail.com>
Hi
On Sun, Jun 17, 2012 at 12:04 AM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Hi
>
> As some might know I am working on making CONFIG_VT obsolete. But as a developer
> it is often useful to have a kernel-log on the screen during boot to debug many
> common kernel(-config) errors. However, without CONFIG_VT we cannot use the
> VGA/framebbufer consoles either. Therefore, I am working on a small driver
> called "fblog".
>
> This driver simply writes the kernel log to all connected framebuffers. It works
> similar to fbcon but removes all the complexity of the virtual terminals. There
> is a sysfs attribute called "active" that allows to enable/disable fblog so
> user-space can start an xserver or similar.
>
> The main purpose is debugging kernel boot problems. Therefore, it is not
> optimized for speed and I tried keeping it simple. I splitted the patches
> into 10 small chunks to make review easier.
>
> I would be glad if someone could review this and tell me whether this is
> something we could include in mainline or not.
>
>
> There are still some issues but apart from them it works fine on my
> machine (x86):
> - I register the fblog device during module_init and need to call
> module_get(). However, this means it is impossible to call "rmmod fblog" as
> fblog has a reference to itself. Using "rmmod -f fblog" works fine but is a
> bit ugly. Is there a nice way to fix this? Otherwise I would need to call
> device_get() in module_exit() if there is a pending user of the fblog-device
> even though I unregistered it.
> - I redraw all framebuffers while holding the console-lock. This may slow down
> machines with more than 2 framebuffers (like 10 or 20). However, as this is
> supposed to be a debug driver, I think I can ignore this? If someone wants
> to improve the redraw logic to avoid redrawing the whole screen all the
> time, I would be glad to include it in this patchset :)
> - I am really no expert regarding the framebuffer subsystem. So I would
> appreciate it if someone could comment whether I need to handle the events
> in a different way or whether it is ok the way it is now.
One additional issue:
With udlfb.c we have hotplug capable framebuffers. However, fbcon and
fblog currently never close a framebuffer if not explicitely requested
by user-space. Therefore, if a framebuffer device is removed, the
FB_EVENT_FB_UNREGISTER event will never be sent because fbcon/fblog
still have a reference to the framebuffer(-driver). Therefore, the
number of available fbs will grow until there are no more free
indices.
See dlfb_usb_disconnect() in udlfb.c for an example. It does not
invoke unregister_framebuffer() unless the last user closed the FB.
udlfb disables the console on its framebuffer devices to avoid this,
but this doesn't seem to be a good solution.
How about sending an FB_EVENT_FB_DISCONNECT event during unlink_framebuffer()?
This still doesn't force user-space to close /dev/fbX but it at least
will make it possible to fblog/fbcon to close the framebuffer. fbmem.c
can then still be modified to mark the open file as dead so user-space
will also close the device hopefully.
Regards
David
--
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 v4 2/2] serial: mxs-auart: Allow device tree probing
From: Subodh Nijsure @ 2012-06-18 20:42 UTC (permalink / raw)
To: Fabio Estevam
Cc: marex, kernel, rob.herring, Grant Likely, linux-serial, shawn.guo,
Matt@grid-net.com, linux-arm-kernel, Alan Cox
In-Reply-To: <1340024769-7122-1-git-send-email-fabio.estevam@freescale.com>
If one were to try run console on one of the /dev/ttyAPP* ports, as is
the case for hardware I am working with, console output wouldn't show up
without the following diff:
Let me know if should send formal patch or we can include it as part of v5?
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index ca3d25e..2ced332 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -761,7 +761,7 @@ static int __devinit mxs_auart_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, s);
- auart_port[pdev->id] = s;
+ auart_port[s->port.line] = s;
mxs_auart_reset(&s->port)
-Subodh
On 06/18/2012 06:06 AM, Fabio Estevam wrote:
> Allow device tree probing.
>
> Cc: Grant Likely<grant.likely@secretlab.ca>
> Cc: Rob Herring<rob.herring@calxeda.com>
> Cc: Alan Cox<alan@linux.intel.com>
> Cc:<linux-serial@vger.kernel.org>
> Signed-off-by: Fabio Estevam<fabio.estevam@freescale.com>
> ---
> Changes since v3:
> - Remove unneeded mxs_auart_devtype
> Changes since v2:
> - Change compatible string
> - Add aliases information in bindings doc
> - Remove unneeded mxs_auart_probe_pdev function
> - Remove "ifdef CONFIG_OF"
> - Remove of_match_ptr wrapper
> Changes since v1:
> - Merged patches 3 and 5 from v1 into this one
> .../bindings/tty/serial/fsl-mxs-auart.txt | 27 +++++++++++++
> drivers/tty/serial/mxs-auart.c | 40 +++++++++++++++++++-
> 2 files changed, 66 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
>
> diff --git a/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
> new file mode 100644
> index 0000000..2ee903f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/fsl-mxs-auart.txt
> @@ -0,0 +1,27 @@
> +* Freescale MXS Application UART (AUART)
> +
> +Required properties:
> +- compatible : Should be "fsl,<soc>-auart". The supported SoCs include
> + imx23 and imx28.
> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain the auart interrupt numbers
> +
> +Example:
> +auart0: serial@8006a000 {
> + compatible = "fsl,imx28-auart", "fsl,imx23-auart";
> + reg =<0x8006a000 0x2000>;
> + interrupts =<112 70 71>;
> +};
> +
> +Note: Each auart port should have an alias correctly numbered in "aliases"
> +node.
> +
> +Example:
> +
> +aliases {
> + serial0 =&auart0;
> + serial1 =&auart1;
> + serial2 =&auart2;
> + serial3 =&auart3;
> + serial4 =&auart4;
> +};
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index ec56d83..103087d 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -33,6 +33,7 @@
> #include<linux/delay.h>
> #include<linux/io.h>
> #include<linux/pinctrl/consumer.h>
> +#include<linux/of_device.h>
>
> #include<asm/cacheflush.h>
>
> @@ -675,6 +676,30 @@ static struct uart_driver auart_driver = {
> #endif
> };
>
> +/*
> + * This function returns 1 if pdev isn't a device instatiated by dt, 0 if it
> + * could successfully get all information from dt or a negative errno.
> + */
> +static int serial_mxs_probe_dt(struct mxs_auart_port *s,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + if (!np)
> + /* no device tree device */
> + return 1;
> +
> + ret = of_alias_get_id(np, "serial");
> + if (ret< 0) {
> + dev_err(&pdev->dev, "failed to get alias id: %d\n", ret);
> + return ret;
> + }
> + s->port.line = ret;
> +
> + return 0;
> +}
> +
> static int __devinit mxs_auart_probe(struct platform_device *pdev)
> {
> struct mxs_auart_port *s;
> @@ -689,6 +714,12 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
> goto out;
> }
>
> + ret = serial_mxs_probe_dt(s, pdev);
> + if (ret> 0)
> + s->port.line = pdev->id< 0 ? 0 : pdev->id;
> + else if (ret< 0)
> + goto out_free;
> +
> pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> if (IS_ERR(pinctrl)) {
> ret = PTR_ERR(pinctrl);
> @@ -711,7 +742,6 @@ static int __devinit mxs_auart_probe(struct platform_device *pdev)
> s->port.membase = ioremap(r->start, resource_size(r));
> s->port.ops =&mxs_auart_ops;
> s->port.iotype = UPIO_MEM;
> - s->port.line = pdev->id< 0 ? 0 : pdev->id;
> s->port.fifosize = 16;
> s->port.uartclk = clk_get_rate(s->clk);
> s->port.type = PORT_IMX;
> @@ -769,12 +799,19 @@ static int __devexit mxs_auart_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static struct of_device_id mxs_auart_dt_ids[] = {
> + { .compatible = "fsl,imx23-auart", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_auart_dt_ids);
> +
> static struct platform_driver mxs_auart_driver = {
> .probe = mxs_auart_probe,
> .remove = __devexit_p(mxs_auart_remove),
> .driver = {
> .name = "mxs-auart",
> .owner = THIS_MODULE,
> + .of_match_table = mxs_auart_dt_ids,
> },
> };
>
> @@ -807,3 +844,4 @@ module_init(mxs_auart_init);
> module_exit(mxs_auart_exit);
> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("Freescale MXS application uart driver");
> +MODULE_ALIAS("platform:mxs-auart");
^ permalink raw reply related
* Re: [PATCH v4 2/2] serial: mxs-auart: Allow device tree probing
From: Fabio Estevam @ 2012-06-18 20:50 UTC (permalink / raw)
To: Subodh Nijsure
Cc: Fabio Estevam, marex, kernel, rob.herring, Grant Likely,
linux-serial, shawn.guo, Matt@grid-net.com, linux-arm-kernel,
Alan Cox
In-Reply-To: <4FDF929B.7000809@grid-net.com>
On Mon, Jun 18, 2012 at 5:42 PM, Subodh Nijsure <snijsure@grid-net.com> wrote:
>
> If one were to try run console on one of the /dev/ttyAPP* ports, as is the
> case for hardware I am working with, console output wouldn't show up without
> the following diff:
>
> Let me know if should send formal patch or we can include it as part of v5?
Please send a formal patch. Shawn has already applied my patch into his tree:
http://git.linaro.org/gitweb?p=people/shawnguo/linux-2.6.git;a=shortlog;h=refs/heads/mxs/for-next
^ permalink raw reply
* My pledge as a maintainer to the developers sending me patches.
From: Greg KH @ 2012-06-18 21:15 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b
As I recently posted here:
http://www.kroah.com/log/linux/maintainer_pledge.html
there's been lots of discussion about maintainers, their general
grumpiness, and how to handle this. I don't want to duplicate what was
said in that post, except for the following.
Here's what I, as a kernel subsystem maintainer, will strive to do for
all patches sent to me by developers:
1) I will review your patch within 1-2 weeks (see details below).
2) I will offer semi-constructive criticism of your patches.
3) I will let you know the status of your patch if it is rejected, or if
it is accepted, what tree it has gone into, where you can find it,
and when you can expect to see it merged into Linus's tree.
Notes on the above:
1) Of course, if I'm sick, or traveling on insane Asia trips, my
response time will be delayed, but feel free to email me asking the
status of a patch you have sent me, I'm happy to respond back that I
just haven't gotten to it. I would much rather field these kinds of
inquiries (after waiting a few weeks) then loose patches and make people
mad.
Also, consider the kernel merge window requirements, during the merge
window, I can't accept new patches into my tree that are not bugfixes
for this specific kernel release, so that means there is usually a 3
week window starting about 1 week before Linus's release,
lasting until after a -rc1 kernel is released, before I can get to your
patch. During that time period hundreds of patches accrue, so please
give me some time to dig out from all of them. Usually by -rc3 I'm
caught up, if not, email me and ask.
2) Sorry, it can't always be constructive, but I'll try my best. I'll
also try to not cast aversions about your cat, but if you taunt me, all
bets are off.
3) I have scripts that automatically do this, that were based on Andrew
Morton's older scripts, that any maintainer is more than willing to have
a copy of. Here's the most recent copy at the moment, if you want to
look at it:
https://github.com/gregkh/gregkh-linux/blob/master/work/do.sh
So, that's it, comments? Criticism?
Note, I don't expect anyone else to do this, it's just what I am willing
to do. I know some subsystem maintainers do a much better job than I do
(some subsystem response times to patches is amazing!), but I figured
I'd put this out there if anyone else wanted to rift on it.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Darren Hart @ 2012-06-18 21:41 UTC (permalink / raw)
To: Tomoya MORINAGA
Cc: Linux Kernel Mailing List, Feng Tang, Alexander Stein,
Greg Kroah-Hartman, Alan Cox, linux-serial
In-Reply-To: <CANKRQnj5mxHLprMt4VMMa7sCwLZb=PAktnVVD1MZVkkUG4eLoA@mail.gmail.com>
On 06/05/2012 04:48 PM, Tomoya MORINAGA wrote:
> On Wed, Jun 6, 2012 at 7:07 AM, Darren Hart <dvhart@linux.intel.com> wrote:
>> Are there still concerns about the additional lock? I'll resend V2
>> tomorrow with the single whitespace fix if I don't hear anything back today.
>
> I understand your saying. Looks good.
> However, I am not expert of linux-uart core system.
> So, I'd like UART maintainer to give us your opinion.
Greg, Alan,
any concerns with the locking approach I've adopted in the patch?
Thanks,
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
^ permalink raw reply
* Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Greg Kroah-Hartman @ 2012-06-18 22:21 UTC (permalink / raw)
To: Darren Hart
Cc: Tomoya MORINAGA, Linux Kernel Mailing List, Feng Tang,
Alexander Stein, Alan Cox, linux-serial
In-Reply-To: <4FDFA09A.4030405@linux.intel.com>
On Mon, Jun 18, 2012 at 02:41:46PM -0700, Darren Hart wrote:
>
>
> On 06/05/2012 04:48 PM, Tomoya MORINAGA wrote:
> > On Wed, Jun 6, 2012 at 7:07 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> >> Are there still concerns about the additional lock? I'll resend V2
> >> tomorrow with the single whitespace fix if I don't hear anything back today.
> >
> > I understand your saying. Looks good.
> > However, I am not expert of linux-uart core system.
> > So, I'd like UART maintainer to give us your opinion.
>
> Greg, Alan,
>
> any concerns with the locking approach I've adopted in the patch?
Care to resend the patch, as it was a RFC one, it's no longer in my
queue.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] serial/amba-pl011: use devm_* allocators
From: Greg Kroah-Hartman @ 2012-06-19 0:34 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-serial, linux-arm-kernel, Linus Walleij
In-Reply-To: <1339940648-6847-1-git-send-email-linus.walleij@stericsson.com>
On Sun, Jun 17, 2012 at 03:44:08PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> This switches a bunch of allocation and remapping to use the
> devm_* garbage collected methods and cleans up the error
> path and remove() paths consequently.
>
> devm_ioremap() is only in <linux/io.h> so fix up the
> erroneous <asm/*> include as well.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/tty/serial/amba-pl011.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
This patch doesn't apply, what tree did you make it against?
greg k-h
^ permalink raw reply
* Re: [PATCH] drivers/tty/tty_io.c: fix a potential memleak at do_tty_write()
From: Jeff Liu @ 2012-06-19 3:44 UTC (permalink / raw)
To: Paul Fulghum; +Cc: Alan Cox, linux-serial, gregkh
In-Reply-To: <4FDF6CCE.7090709@microgate.com>
On 06/19/2012 02:00 AM, Paul Fulghum wrote:
> On 6/18/2012 11:04 AM, Alan Cox wrote:
>> On Mon, 18 Jun 2012 20:23:54 +0800
>> Jeff Liu <jeff.liu@oracle.com> wrote:
>>
>>> Hello,
>>>
>>> Looks there is a potential memory leak at drivers/tty/tty_io.c: do_tty_write().
>>> It did allocate a buf_chunk if tty->write_cnt < chunk, however, buf_chunk was not
>>> freed after the writing is done. Below tiny patch could fix it.
>>
>> Why should it be freed, we still have a reference to it.
>
> Yeah, it would be messy on the next write()
> when the now freed tty->write_buf is accessed ;-)
> *boom*
Oops! I took for granted previously. Duh. :(
Sorry for the noise!
-Jeff
^ permalink raw reply
* RE: [PATCH 1/2] serial: samsung: Remove NULL checking for baud clock
From: Kukjin Kim @ 2012-06-19 8:52 UTC (permalink / raw)
To: 'Kyoungil Kim', linux-arm-kernel, linux-samsung-soc,
linux-serial
Cc: 'Alan Cox', 'Russell King'
In-Reply-To: <002d01cd3988$c0d914c0$428b3e40$%kim@samsung.com>
Kyoungil Kim wrote:
>
> Kyoungil Kim wrote:
>
> I missed following.
>
> Russell King suggested:
>
> As I said, drivers have no business interpreting anything but IS_ERR(clk)
> as being an error. They should not make any other
> assumptions.
>
> Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>
>
> > Signed-off-by: Kyoungil Kim <ki0351.kim@samsung.com>
> > ---
> > drivers/tty/serial/samsung.c | 21 ++++++++++++---------
> > 1 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> > index d8b0aee..5668538 100644
> > --- a/drivers/tty/serial/samsung.c
> > +++ b/drivers/tty/serial/samsung.c
> > @@ -529,7 +529,7 @@ static void s3c24xx_serial_pm(struct uart_port
*port,
> unsigned int level,
> >
> > switch (level) {
> > case 3:
> > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL)
> > + if (!IS_ERR(ourport->baudclk))
> > clk_disable(ourport->baudclk);
> >
> > clk_disable(ourport->clk);
> > @@ -538,7 +538,7 @@ static void s3c24xx_serial_pm(struct uart_port
*port,
> unsigned int level,
> > case 0:
> > clk_enable(ourport->clk);
> >
> > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL)
> > + if (!IS_ERR(ourport->baudclk))
> > clk_enable(ourport->baudclk);
> >
> > break;
> > @@ -604,7 +604,6 @@ static unsigned int s3c24xx_serial_getclk(struct
> s3c24xx_uart_port *ourport,
> > char clkname[MAX_CLK_NAME_LENGTH];
> > int calc_deviation, deviation = (1 << 30) - 1;
> >
> > - *best_clk = NULL;
> > clk_sel = (ourport->cfg->clk_sel) ? ourport->cfg->clk_sel :
> > ourport->info->def_clk_sel;
> > for (cnt = 0; cnt < info->num_clks; cnt++) {
> > @@ -613,7 +612,7 @@ static unsigned int s3c24xx_serial_getclk(struct
> s3c24xx_uart_port *ourport,
> >
> > sprintf(clkname, "clk_uart_baud%d", cnt);
> > clk = clk_get(ourport->port.dev, clkname);
> > - if (IS_ERR_OR_NULL(clk))
> > + if (IS_ERR(clk))
> > continue;
> >
> > rate = clk_get_rate(clk);
> > @@ -684,7 +683,7 @@ static void s3c24xx_serial_set_termios(struct
> uart_port *port,
> > {
> > struct s3c2410_uartcfg *cfg = s3c24xx_port_to_cfg(port);
> > struct s3c24xx_uart_port *ourport = to_ourport(port);
> > - struct clk *clk = NULL;
> > + struct clk *clk = ERR_PTR(-EINVAL);
> > unsigned long flags;
> > unsigned int baud, quot, clk_sel = 0;
> > unsigned int ulcon;
> > @@ -705,7 +704,7 @@ static void s3c24xx_serial_set_termios(struct
> uart_port *port,
> > quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
> > if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
> > quot = port->custom_divisor;
> > - if (!clk)
> > + if (IS_ERR(clk))
> > return;
> >
> > /* check to see if we need to change clock source */
> > @@ -713,9 +712,9 @@ static void s3c24xx_serial_set_termios(struct
> uart_port *port,
> > if (ourport->baudclk != clk) {
> > s3c24xx_serial_setsource(port, clk_sel);
> >
> > - if (ourport->baudclk != NULL && !IS_ERR(ourport->baudclk)) {
> > + if (!IS_ERR(ourport->baudclk)) {
> > clk_disable(ourport->baudclk);
> > - ourport->baudclk = NULL;
> > + ourport->baudclk = ERR_PTR(-EINVAL);
> > }
> >
> > clk_enable(clk);
> > @@ -1160,6 +1159,9 @@ static ssize_t s3c24xx_serial_show_clksrc(struct
> device *dev,
> > struct uart_port *port = s3c24xx_dev_to_port(dev);
> > struct s3c24xx_uart_port *ourport = to_ourport(port);
> >
> > + if (IS_ERR(ourport->baudclk))
> > + return -EINVAL;
> > +
> > return snprintf(buf, PAGE_SIZE, "* %s\n", ourport->baudclk->name);
> > }
> >
> > @@ -1200,6 +1202,7 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > + ourport->baudclk = ERR_PTR(-EINVAL);
> > ourport->info = ourport->drv_data->info;
> > ourport->cfg = (pdev->dev.platform_data) ?
> > (struct s3c2410_uartcfg *)pdev->dev.platform_data :
> > @@ -1387,7 +1390,7 @@ s3c24xx_serial_get_options(struct uart_port *port,
> int *baud,
> > sprintf(clk_name, "clk_uart_baud%d", clk_sel);
> >
> > clk = clk_get(port->dev, clk_name);
> > - if (!IS_ERR(clk) && clk != NULL)
> > + if (!IS_ERR(clk))
> > rate = clk_get_rate(clk);
> > else
> > rate = 1;
> > --
> > 1.7.1
Looks ok to me, will apply.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Alan Cox @ 2012-06-19 9:14 UTC (permalink / raw)
To: Darren Hart
Cc: Tomoya MORINAGA, Linux Kernel Mailing List, Feng Tang,
Alexander Stein, Greg Kroah-Hartman, Alan Cox, linux-serial
In-Reply-To: <4FDFA09A.4030405@linux.intel.com>
On Mon, 18 Jun 2012 14:41:46 -0700
Darren Hart <dvhart@linux.intel.com> wrote:
>
>
> On 06/05/2012 04:48 PM, Tomoya MORINAGA wrote:
> > On Wed, Jun 6, 2012 at 7:07 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> >> Are there still concerns about the additional lock? I'll resend V2
> >> tomorrow with the single whitespace fix if I don't hear anything back today.
> >
> > I understand your saying. Looks good.
> > However, I am not expert of linux-uart core system.
> > So, I'd like UART maintainer to give us your opinion.
>
> Greg, Alan,
>
> any concerns with the locking approach I've adopted in the patch?
Only the one I noted in my reply the first time around which is that you
can't permit tty->low_latency=1 unless your tty receive path is not an
IRQ path. From a locking point of view the change makes sense anyway.
Going back over it your console locking also needs care - an oops or
printk within the areas the private lock covers will hang the box. That
should also probably be a trylock style lock as with the other lock on
that path
Alan
^ permalink raw reply
* Re: Questions regarding adding a patch in linux/drivers/char/8250.c
From: Alan Cox @ 2012-06-19 9:18 UTC (permalink / raw)
To: Donald; +Cc: linux-serial
In-Reply-To: <002101cd4cff$74643c70$5d2cb550$@com.tw>
> + struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
> +
> switch (termios->c_cflag & CSIZE) {
> case CS5:
> cval = UART_LCR_WLEN5;
> @@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
> if (up->capabilities & UART_CAP_RTOIE)
> up->ier |= UART_IER_RTOIE;
>
> + if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
> + fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
> + up->ier &= ~UART_IER_RLSI;
> + } else {
> + up->ier |= UART_IER_RLSI;
> + }
> +
It's on my TODO list for the 3.6 merge. Your patch doesn't work (in fact
it crashes in some cases) because it blindly assumes an 8250 port is on
the PCI bus.
However it documents everything I need to know to push an actual fix.
Alan
^ permalink raw reply
* RE: Questions regarding adding a patch in linux/drivers/char/8250.c
From: Donald @ 2012-06-19 12:35 UTC (permalink / raw)
To: 'Alan Cox'; +Cc: linux-serial
In-Reply-To: <20120619101827.150d3968@pyramind.ukuu.org.uk>
Hi Alan,
Thank you for your nice update. I will try to reproduce the crash case on my end and do further analysis. Any update I will keep you
posted. Thanks.
Donald
-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
Sent: Tuesday, June 19, 2012 5:18 PM
To: Donald
Cc: linux-serial@vger.kernel.org
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c
> + struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
> +
> switch (termios->c_cflag & CSIZE) {
> case CS5:
> cval = UART_LCR_WLEN5;
> @@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
> if (up->capabilities & UART_CAP_RTOIE)
> up->ier |= UART_IER_RTOIE;
>
> + if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
> + fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
> + up->ier &= ~UART_IER_RLSI;
> + } else {
> + up->ier |= UART_IER_RLSI;
> + }
> +
It's on my TODO list for the 3.6 merge. Your patch doesn't work (in fact it crashes in some cases) because it blindly assumes an
8250 port is on the PCI bus.
However it documents everything I need to know to push an actual fix.
Alan
^ permalink raw reply
* Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Darren Hart @ 2012-06-19 17:35 UTC (permalink / raw)
To: Alan Cox
Cc: Tomoya MORINAGA, Linux Kernel Mailing List, Feng Tang,
Alexander Stein, Greg Kroah-Hartman, Alan Cox, linux-serial
In-Reply-To: <20120619101447.74cbd9a1@pyramind.ukuu.org.uk>
On 06/19/2012 02:14 AM, Alan Cox wrote:
> On Mon, 18 Jun 2012 14:41:46 -0700
> Darren Hart <dvhart@linux.intel.com> wrote:
>
>>
>>
>> On 06/05/2012 04:48 PM, Tomoya MORINAGA wrote:
>>> On Wed, Jun 6, 2012 at 7:07 AM, Darren Hart <dvhart@linux.intel.com> wrote:
>>>> Are there still concerns about the additional lock? I'll resend V2
>>>> tomorrow with the single whitespace fix if I don't hear anything back today.
>>>
>>> I understand your saying. Looks good.
>>> However, I am not expert of linux-uart core system.
>>> So, I'd like UART maintainer to give us your opinion.
>>
>> Greg, Alan,
>>
>> any concerns with the locking approach I've adopted in the patch?
>
> Only the one I noted in my reply the first time around
Hi Alan,
I've hunted, but I can't seem to find this reply. :-/
> which is that you
> can't permit tty->low_latency=1 unless your tty receive path is not an
> IRQ path. From a locking point of view the change makes sense anyway.
I ran into this on the PREEMPT_RT kernel which always uses
tty->low_latency and converts the interrupt handler into a thread.
There is a follow-on patch for RT only to address a sleeping while
atomic bug in pch_console_write(), but I felt _this_ locking structure
change was appropriate for mainline.
>
> Going back over it your console locking also needs care - an oops or
> printk within the areas the private lock covers will hang the box. That
> should also probably be a trylock style lock as with the other lock on
> that path
I presume you are referring to pch_console_write()?
> static void
> pch_console_write(struct console *co, const char *s, unsigned int count)
> {
> struct eg20t_port *priv;
> unsigned long flags;
> u8 ier;
> int locked = 1;
>
> priv = pch_uart_ports[co->index];
>
> touch_nmi_watchdog();
>
> local_irq_save(flags);
> spin_lock(&priv->lock);
> if (priv->port.sysrq) {
> /* serial8250_handle_port() already took the lock */
> locked = 0;
> } else if (oops_in_progress) {
> locked = spin_trylock(&priv->port.lock);
> } else
> spin_lock(&priv->port.lock);
I see, the oops_in_progress test right? My thinking was that the
oops_in_progress was only relevant to the port.lock as that could be
taken outside of the pch_uart driver, while the priv.lock is only used
within the driver. But, as the oops uses the pch_console_write itself, I
can see the recursive spinlock failure case there.
As for the printk, it seems the 8250 driver would also suffer from that
in the serial8250_console_write function on the port.lock, and it does
not make any allowances for printk.
I would like to hold the priv.lock for a smaller window, but ordering
requires that I take it prior to the port.lock.
So I can test for oops_in_progress on the priv->lock too, but that won't
address the printk issue. Is the oops the bigger concern?
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
^ permalink raw reply
* Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Alan Cox @ 2012-06-19 17:54 UTC (permalink / raw)
To: Darren Hart
Cc: Tomoya MORINAGA, Linux Kernel Mailing List, Feng Tang,
Alexander Stein, Greg Kroah-Hartman, Alan Cox, linux-serial
In-Reply-To: <4FE0B853.5060203@linux.intel.com>
> I see, the oops_in_progress test right? My thinking was that the
> oops_in_progress was only relevant to the port.lock as that could be
> taken outside of the pch_uart driver, while the priv.lock is only used
> within the driver. But, as the oops uses the pch_console_write itself, I
> can see the recursive spinlock failure case there.
Until your driver crashes...
> As for the printk, it seems the 8250 driver would also suffer from that
> in the serial8250_console_write function on the port.lock, and it does
> not make any allowances for printk.
I think 8250 probably wants fixing too then!
>
> I would like to hold the priv.lock for a smaller window, but ordering
> requires that I take it prior to the port.lock.
>
> So I can test for oops_in_progress on the priv->lock too, but that won't
> address the printk issue. Is the oops the bigger concern?
the oops is the main one - a printk would have to be in driver as a
screwup, and you can force an oops on a stall so pick it up later
Alan
^ permalink raw reply
* [PATCH V3] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Darren Hart @ 2012-06-19 21:00 UTC (permalink / raw)
To: Linux Kernel Mailing List
Cc: Darren Hart, Tomoya MORINAGA, Feng Tang, Alexander Stein,
Greg Kroah-Hartman, Alan Cox, linux-serial
pch_uart_interrupt() takes priv->port.lock which leads to two recursive
spinlock calls if low_latency==1 or CONFIG_PREEMPT_RT_FULL=y (one
otherwise):
pch_uart_interrupt
spin_lock_irqsave(priv->port.lock, flags)
case PCH_UART_IID_RDR_TO (data ready)
handle_rx_to
push_rx
tty_port_tty_get
spin_lock_irqsave(&port->lock, flags) <--- already hold this lock
...
tty_flip_buffer_push
...
flush_to_ldisc
spin_lock_irqsave(&tty->buf.lock)
spin_lock_irqsave(&tty->buf.lock)
disc->ops->receive_buf(tty, char_buf)
n_tty_receive_buf
tty->ops->flush_chars()
uart_flush_chars
uart_start
spin_lock_irqsave(&port->lock) <--- already hold this lock
Avoid this by using a dedicated lock to protect the eg20t_port structure
and IO access to its membase. This is more consistent with the 8250
driver. Ensure priv->lock is always take prior to priv->port.lock when
taken at the same time.
V2: Remove inadvertent whitespace change.
V3: Account for oops_in_progress for the private lock in
pch_console_write().
Note: Like the 8250 driver, if a printk is introduced anywhere inside
the pch_console_write() critical section, the kernel will hang
on a recursive spinlock on the private lock. The oops case is
handled by using a trylock in the oops_in_progress case.
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Feng Tang <feng.tang@intel.com>
CC: Alexander Stein <alexander.stein@systec-electronic.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: linux-serial@vger.kernel.org
---
drivers/tty/serial/pch_uart.c | 38 ++++++++++++++++++++++++++------------
1 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 4fdec6a..d291518 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -253,6 +253,9 @@ struct eg20t_port {
dma_addr_t rx_buf_dma;
struct dentry *debugfs;
+
+ /* protect the eg20t_port private structure and io access to membase */
+ spinlock_t lock;
};
/**
@@ -1058,7 +1061,7 @@ static irqreturn_t pch_uart_interrupt(int irq, void *dev_id)
int next = 1;
u8 msr;
- spin_lock_irqsave(&priv->port.lock, flags);
+ spin_lock_irqsave(&priv->lock, flags);
handled = 0;
while (next) {
iid = pch_uart_hal_get_iid(priv);
@@ -1116,7 +1119,7 @@ static irqreturn_t pch_uart_interrupt(int irq, void *dev_id)
handled |= (unsigned int)ret;
}
- spin_unlock_irqrestore(&priv->port.lock, flags);
+ spin_unlock_irqrestore(&priv->lock, flags);
return IRQ_RETVAL(handled);
}
@@ -1226,9 +1229,9 @@ static void pch_uart_break_ctl(struct uart_port *port, int ctl)
unsigned long flags;
priv = container_of(port, struct eg20t_port, port);
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irqsave(&priv->lock, flags);
pch_uart_hal_set_break(priv, ctl);
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock_irqrestore(&priv->lock, flags);
}
/* Grab any interrupt resources and initialise any low level driver state. */
@@ -1376,7 +1379,8 @@ static void pch_uart_set_termios(struct uart_port *port,
baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16);
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irqsave(&priv->lock, flags);
+ spin_lock(&port->lock);
uart_update_timeout(port, termios->c_cflag, baud);
rtn = pch_uart_hal_set_line(priv, baud, parity, bits, stb);
@@ -1389,7 +1393,8 @@ static void pch_uart_set_termios(struct uart_port *port,
tty_termios_encode_baud_rate(termios, baud, baud);
out:
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock(&port->lock);
+ spin_unlock_irqrestore(&priv->lock, flags);
}
static const char *pch_uart_type(struct uart_port *port)
@@ -1538,8 +1543,9 @@ pch_console_write(struct console *co, const char *s, unsigned int count)
{
struct eg20t_port *priv;
unsigned long flags;
+ int priv_locked = 1;
+ int port_locked = 1;
u8 ier;
- int locked = 1;
priv = pch_uart_ports[co->index];
@@ -1547,12 +1553,16 @@ pch_console_write(struct console *co, const char *s, unsigned int count)
local_irq_save(flags);
if (priv->port.sysrq) {
- /* serial8250_handle_port() already took the lock */
- locked = 0;
+ spin_lock(&priv->lock);
+ /* serial8250_handle_port() already took the port lock */
+ port_locked = 0;
} else if (oops_in_progress) {
- locked = spin_trylock(&priv->port.lock);
- } else
+ priv_locked = spin_trylock(&priv->lock);
+ port_locked = spin_trylock(&priv->port.lock);
+ } else {
+ spin_lock(&priv->lock);
spin_lock(&priv->port.lock);
+ }
/*
* First save the IER then disable the interrupts
@@ -1570,8 +1580,10 @@ pch_console_write(struct console *co, const char *s, unsigned int count)
wait_for_xmitr(priv, BOTH_EMPTY);
iowrite8(ier, priv->membase + UART_IER);
- if (locked)
+ if (port_locked)
spin_unlock(&priv->port.lock);
+ if (priv_locked)
+ spin_unlock(&priv->lock);
local_irq_restore(flags);
}
@@ -1669,6 +1681,8 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
pci_enable_msi(pdev);
pci_set_master(pdev);
+ spin_lock_init(&priv->lock);
+
iobase = pci_resource_start(pdev, 0);
mapbase = pci_resource_start(pdev, 1);
priv->mapbase = mapbase;
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH V3] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks
From: Alan Cox @ 2012-06-19 22:04 UTC (permalink / raw)
To: Darren Hart
Cc: Linux Kernel Mailing List, Tomoya MORINAGA, Feng Tang,
Alexander Stein, Greg Kroah-Hartman, Alan Cox, linux-serial
In-Reply-To: <34b5e216a6dbd83d282dad22c2644136c652c34a.1340139586.git.dvhart@linux.intel.com>
On Tue, 19 Jun 2012 14:00:18 -0700
Darren Hart <dvhart@linux.intel.com> wrote:
> pch_uart_interrupt() takes priv->port.lock which leads to two recursive
> spinlock calls if low_latency==1 or CONFIG_PREEMPT_RT_FULL=y (one
> otherwise):
>
> pch_uart_interrupt
> spin_lock_irqsave(priv->port.lock, flags)
> case PCH_UART_IID_RDR_TO (data ready)
> handle_rx_to
> push_rx
> tty_port_tty_get
> spin_lock_irqsave(&port->lock, flags) <--- already hold this lock
> ...
> tty_flip_buffer_push
> ...
> flush_to_ldisc
> spin_lock_irqsave(&tty->buf.lock)
> spin_lock_irqsave(&tty->buf.lock)
> disc->ops->receive_buf(tty, char_buf)
> n_tty_receive_buf
> tty->ops->flush_chars()
> uart_flush_chars
> uart_start
> spin_lock_irqsave(&port->lock) <--- already hold this lock
>
> Avoid this by using a dedicated lock to protect the eg20t_port structure
> and IO access to its membase. This is more consistent with the 8250
> driver. Ensure priv->lock is always take prior to priv->port.lock when
> taken at the same time.
>
> V2: Remove inadvertent whitespace change.
> V3: Account for oops_in_progress for the private lock in
> pch_console_write().
>
> Note: Like the 8250 driver, if a printk is introduced anywhere inside
> the pch_console_write() critical section, the kernel will hang
> on a recursive spinlock on the private lock. The oops case is
> handled by using a trylock in the oops_in_progress case.
>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
> CC: Feng Tang <feng.tang@intel.com>
> CC: Alexander Stein <alexander.stein@systec-electronic.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Alan Cox <alan@linux.intel.com>
> CC: linux-serial@vger.kernel.org
Acked-by: Alan Cox <alan@linux.intel.com>
^ 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