* [PATCHv2 1/1] [RFC] uartclk from serial_core exposed to sysfs
From: Tomas Hlavacek @ 2012-08-15 17:12 UTC (permalink / raw)
To: gregkh, alan, linux-serial, linux-kernel; +Cc: marek.vasut, Tomas Hlavacek
In-Reply-To: <1344929718-22736-1-git-send-email-tmshlvck@gmail.com>
Added file /sys/devices/.../tty/ttySX/uartclk to allow read/modify
uartclk value in struct uart_port in serial_core via sysfs.
It simplifies initialization of no-name cards that have non-standard
oscillator speed while having no distinguishing PCI IDs to allow
autodetection.
Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com>
---
drivers/tty/serial/serial_core.c | 55 ++++++++++++++++++++++++++++++++++++++
drivers/tty/tty_io.c | 17 ++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 74 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..0929fe3 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,47 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t uart_get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct uart_state *state = dev_get_drvdata(dev);
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+ return ret;
+}
+
+static ssize_t uart_set_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ struct uart_state *state = dev_get_drvdata(dev);
+ unsigned int val;
+ int ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&state->port.mutex);
+
+ /*
+ * Check value: baud_base does not make sense to be set below 9600
+ * and since uartclock = (baud_base * 16) it has to be equal or greater
+ * than 9600 * 16 = 153600.
+ */
+ if (val >= 153600)
+ state->uart_port->uartclk = val;
+
+ mutex_unlock(&state->port.mutex);
+
+ return count;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, uart_get_attr_uartclk,
+ uart_set_attr_uartclk);
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2355,6 +2396,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
}
/*
+ * Expose uartclk in sysfs. Use driverdata of the tty device for
+ * referencing the UART port.
+ */
+ dev_set_drvdata(tty_dev, state);
+ if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
+ dev_err(tty_dev, "Failed to add uartclk attr\n");
+
+ /*
* Ensure UPF_DEAD is not set.
*/
uport->flags &= ~UPF_DEAD;
@@ -2397,6 +2446,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
/*
+ * Remove uartclk file from sysfs.
+ */
+ device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
+ &dev_attr_uartclk);
+
+ /*
* Remove the devices from the tty layer
*/
tty_unregister_device(drv->tty_driver, uport->line);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..8ea8622 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
}
EXPORT_SYMBOL(tty_unregister_device);
+/*
+ * tty_lookup_device - lookup a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ *
+ * This function returns a struct device pointer when device has
+ * been found and NULL otherwise.
+ *
+ * Locking: ??
+ */
+struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
+{
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+ return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_lookup_device);
+
struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
{
struct tty_driver *driver;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5d408a1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+extern struct device *tty_lookup_device(struct tty_driver *driver,
+ unsigned index);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs
From: Tomas Hlavacek @ 2012-08-15 17:09 UTC (permalink / raw)
To: Marek Vasut; +Cc: gregkh, alan, linux-serial, linux-kernel
In-Reply-To: <201208141450.23453.marek.vasut@gmail.com>
Hello Marek,
On Tue, Aug 14, 2012 at 2:50 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dear Tomas Hlavacek,
>
>> +static ssize_t get_attr_uartclk(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int ret;
>> +
>> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
>
> You don't need this cast here.
Yes, you are right. Thanks.
>
>> + mutex_lock(&state->port.mutex);
>> + ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
>
> Do you really need such a large buffer (PAGE_SIZE) ?
Well no, but I believe that I get the buffer of length equal to
PAGE_SIZE anyway. Documentation/filesystems/sysfs.txt says so on line
179.
>
>> + mutex_unlock(&state->port.mutex);
>> + return ret;
>> +}
>> +
>> +static ssize_t set_attr_uartclk(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
>> + unsigned int val;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 10, &val);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&state->port.mutex);
>> +
>> + /*
>> + * Check value: baud_base has to be more than 9600
>> + * and uartclock = baud_base * 16 .
>> + */
>> + if (val >= 153600)
>> + state->uart_port->uartclk = val;
>
> This magic value here would use some documentation.
OK. Do you think this would be sufficient comment?:
/*
* Check value: baud_base does not make sense to be set below 9600
* and since uartclock = (baud_base * 16) it has to be equal or greater than
* 9600 * 16 = 153600.
*/
PATCHv2 follows.
Tomas
--
Tomáš Hlaváček <tmshlvck@gmail.com>
^ permalink raw reply
* Re: [PATCH 08/14] tty: atmel_serial: add pinctrl support
From: Linus Walleij @ 2012-08-15 9:01 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, Nicolas Ferre, linux-serial
In-Reply-To: <1344603731-32667-8-git-send-email-plagnioj@jcrosoft.com>
On Fri, Aug 10, 2012 at 3:02 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-serial@vger.kernel.org
Looking good,
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: Ryan Mallon @ 2012-08-15 0:17 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <CANq1E4QP8PwQRFBr++b3cu1dM9NWf6+Y45rq4UAirjaAHyeM4Q@mail.gmail.com>
On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.
I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:
static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}
static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}
__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}
static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}
__fblog_unregister(fb);
}
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;
fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);
ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}
fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);
return 0;
}
Functions which do:
foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}
can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.
>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + case FB_EVENT_FB_UNREGISTERED:
>>> + /* This is called when a low-level system driver unregisters a
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.
Fair enough. It might be something to come back to once this gets merged.
>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().
That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.
> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.
Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.
> As you mentioned that this locking is needlessly complex, I have to
> disagree.
Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.
> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).
I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.
> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.
Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.
~Ryan
^ permalink raw reply
* Re: [PATCH] MIPS: OCTEON: Fix breakage due to 8250 changes.
From: Greg Kroah-Hartman @ 2012-08-14 21:46 UTC (permalink / raw)
To: Ralf Baechle; +Cc: David Daney, linux-mips, linux-serial, David Daney, Alan Cox
In-Reply-To: <20120814165607.GA29888@linux-mips.org>
On Tue, Aug 14, 2012 at 06:56:07PM +0200, Ralf Baechle wrote:
> On Tue, Aug 14, 2012 at 09:42:39AM -0700, David Daney wrote:
>
> > From: David Daney <david.daney@cavium.com>
> >
> > The changes in linux-next removing serial8250_register_port() cause
> > OCTEON to fail to compile.
> >
> > Lets make OCTEON use the new serial8250_register_8250_port() instead.
>
> I think this one should go via the serial tree.
>
> Acked-by: Ralf Baechle <ralf@linux-mips.org>
Ok, I'll take it there, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH] MIPS: OCTEON: Fix breakage due to 8250 changes.
From: Ralf Baechle @ 2012-08-14 16:56 UTC (permalink / raw)
To: David Daney
Cc: linux-mips, linux-serial, David Daney, Alan Cox,
Greg Kroah-Hartman
In-Reply-To: <1344962559-6823-1-git-send-email-ddaney.cavm@gmail.com>
On Tue, Aug 14, 2012 at 09:42:39AM -0700, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> The changes in linux-next removing serial8250_register_port() cause
> OCTEON to fail to compile.
>
> Lets make OCTEON use the new serial8250_register_8250_port() instead.
I think this one should go via the serial tree.
Acked-by: Ralf Baechle <ralf@linux-mips.org>
Thanks,
Ralf
^ permalink raw reply
* [PATCH] MIPS: OCTEON: Fix breakage due to 8250 changes.
From: David Daney @ 2012-08-14 16:42 UTC (permalink / raw)
To: linux-mips, ralf, linux-serial; +Cc: David Daney, Alan Cox, Greg Kroah-Hartman
From: David Daney <david.daney@cavium.com>
The changes in linux-next removing serial8250_register_port() cause
OCTEON to fail to compile.
Lets make OCTEON use the new serial8250_register_8250_port() instead.
Signed-off-by: David Daney <david.daney@cavium.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Perhaps we can get an Acked-by from Ralf, and then merge this along
with the 8250 patches that caused the breakage? What do you think?
arch/mips/cavium-octeon/serial.c | 30 +++++++++++++++---------------
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/mips/cavium-octeon/serial.c b/arch/mips/cavium-octeon/serial.c
index 138b221..569f41b 100644
--- a/arch/mips/cavium-octeon/serial.c
+++ b/arch/mips/cavium-octeon/serial.c
@@ -47,40 +47,40 @@ static int __devinit octeon_serial_probe(struct platform_device *pdev)
{
int irq, res;
struct resource *res_mem;
- struct uart_port port;
+ struct uart_8250_port up;
/* All adaptors have an irq. */
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
- memset(&port, 0, sizeof(port));
+ memset(&up, 0, sizeof(up));
- port.flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
- port.type = PORT_OCTEON;
- port.iotype = UPIO_MEM;
- port.regshift = 3;
- port.dev = &pdev->dev;
+ up.port.flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
+ up.port.type = PORT_OCTEON;
+ up.port.iotype = UPIO_MEM;
+ up.port.regshift = 3;
+ up.port.dev = &pdev->dev;
if (octeon_is_simulation())
/* Make simulator output fast*/
- port.uartclk = 115200 * 16;
+ up.port.uartclk = 115200 * 16;
else
- port.uartclk = octeon_get_io_clock_rate();
+ up.port.uartclk = octeon_get_io_clock_rate();
- port.serial_in = octeon_serial_in;
- port.serial_out = octeon_serial_out;
- port.irq = irq;
+ up.port.serial_in = octeon_serial_in;
+ up.port.serial_out = octeon_serial_out;
+ up.port.irq = irq;
res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res_mem == NULL) {
dev_err(&pdev->dev, "found no memory resource\n");
return -ENXIO;
}
- port.mapbase = res_mem->start;
- port.membase = ioremap(res_mem->start, resource_size(res_mem));
+ up.port.mapbase = res_mem->start;
+ up.port.membase = ioremap(res_mem->start, resource_size(res_mem));
- res = serial8250_register_port(&port);
+ res = serial8250_register_8250_port(&up);
return res >= 0 ? 0 : res;
}
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] Powerpc 8xx CPM_UART delay in receive
From: Alan Cox @ 2012-08-14 14:52 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alan Cox, Vitaly Bordug, Marcelo Tosatti, linux-kernel,
linux-serial, linuxppc-dev
In-Reply-To: <201208141426.q7EEQSPc003956@localhost.localdomain>
On Tue, 14 Aug 2012 16:26:28 +0200
Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Hello,
>
> I'm not sure who to address this Patch to either
>
> It fixes a delay issue with CPM UART driver on Powerpc MPC8xx.
> The problem is that with the actual code, the driver waits 32 IDLE patterns before returning the received data to the upper level. It means for instance about 1 second at 300 bauds.
> This fix limits to one byte the waiting period.
Take a look how the 8250 does it - I think you want to set the value
based upon the data rate. Your patch will break it for everyone doing
high seed I/O.
Alan
^ permalink raw reply
* [PATCH] Powerpc 8xx CPM_UART delay in receive
From: Christophe Leroy @ 2012-08-14 14:26 UTC (permalink / raw)
To: Alan Cox, Vitaly Bordug, Marcelo Tosatti
Cc: linux-kernel, linux-serial, linuxppc-dev
Hello,
I'm not sure who to address this Patch to either
It fixes a delay issue with CPM UART driver on Powerpc MPC8xx.
The problem is that with the actual code, the driver waits 32 IDLE patterns before returning the received data to the upper level. It means for instance about 1 second at 300 bauds.
This fix limits to one byte the waiting period.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
--- linux-3.5-vanilla/drivers/tty/serial/cpm_uart/cpm_uart_core.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-3.5/drivers/tty/serial/cpm_uart/cpm_uart_core.c 2012-08-09 17:38:37.000000000 +0200
@@ -798,7 +799,7 @@
cpm_set_scc_fcr(sup);
out_be16(&sup->scc_genscc.scc_mrblr, pinfo->rx_fifosize);
- out_be16(&sup->scc_maxidl, pinfo->rx_fifosize);
+ out_be16(&sup->scc_maxidl, 1);
out_be16(&sup->scc_brkcr, 1);
out_be16(&sup->scc_parec, 0);
out_be16(&sup->scc_frmec, 0);
@@ -872,7 +873,7 @@
/* Using idle character time requires some additional tuning. */
out_be16(&up->smc_mrblr, pinfo->rx_fifosize);
- out_be16(&up->smc_maxidl, pinfo->rx_fifosize);
+ out_be16(&up->smc_maxidl, 1);
out_be16(&up->smc_brklen, 0);
out_be16(&up->smc_brkec, 0);
out_be16(&up->smc_brkcr, 1);
^ permalink raw reply
* [PATCH] Powerpc 8xx CPM_UART desynchronisation
From: Christophe Leroy @ 2012-08-14 14:26 UTC (permalink / raw)
To: Alan Cox, Vitaly Bordug, Marcelo Tosatti
Cc: linux-kernel, linux-serial, linuxppc-dev
Hello,
I'm not sure who to address this Patch to.
It fixes a desynchronisation problem with CPM UART driver on Powerpc MPC8xx.
The problem happens if data is received before the device is open by the user application.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
--- linux-3.5-vanilla/drivers/tty/serial/cpm_uart/cpm_uart_core.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-3.5/drivers/tty/serial/cpm_uart/cpm_uart_core.c 2012-08-09 17:38:37.000000000 +0200
@@ -417,6 +417,7 @@
clrbits32(&pinfo->sccp->scc_gsmrl, SCC_GSMRL_ENR);
clrbits16(&pinfo->sccp->scc_sccm, UART_SCCM_RX);
}
+ cpm_uart_initbd(pinfo);
cpm_line_cr_cmd(pinfo, CPM_CR_INIT_TRX);
}
/* Install interrupt handler. */
^ permalink raw reply
* Re: [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs
From: Marek Vasut @ 2012-08-14 12:50 UTC (permalink / raw)
To: Tomas Hlavacek; +Cc: gregkh, alan, linux-serial, linux-kernel
In-Reply-To: <1344929718-22736-1-git-send-email-tmshlvck@gmail.com>
Dear Tomas Hlavacek,
> Support for read/modify of uartclk via sysfs added.
> It may prove useful with some no-name cards that
> has different oscillator speeds and no distinguishing
> PCI IDs to allow autodetection. It allows better integration
> with udev and/or init scripts.
>
> Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com>
> ---
> drivers/tty/serial/serial_core.c | 54
> ++++++++++++++++++++++++++++++++++++++ drivers/tty/tty_io.c |
> 17 ++++++++++++
> include/linux/tty.h | 2 ++
> 3 files changed, 73 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c index a21dc8e..059b438 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2293,6 +2293,46 @@ struct tty_driver *uart_console_device(struct
> console *co, int *index) return p->tty_driver;
> }
>
> +static ssize_t get_attr_uartclk(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int ret;
> +
> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
You don't need this cast here.
> + mutex_lock(&state->port.mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
Do you really need such a large buffer (PAGE_SIZE) ?
> + mutex_unlock(&state->port.mutex);
> + return ret;
> +}
> +
> +static ssize_t set_attr_uartclk(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&state->port.mutex);
> +
> + /*
> + * Check value: baud_base has to be more than 9600
> + * and uartclock = baud_base * 16 .
> + */
> + if (val >= 153600)
> + state->uart_port->uartclk = val;
This magic value here would use some documentation.
> + mutex_unlock(&state->port.mutex);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, get_attr_uartclk,
> + set_attr_uartclk);
> +
> /**
> * uart_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> @@ -2355,6 +2395,14 @@ int uart_add_one_port(struct uart_driver *drv,
> struct uart_port *uport) }
>
> /*
> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> + * referencing the UART port.
> + */
> + dev_set_drvdata(tty_dev, state);
> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> + dev_err(tty_dev, "Failed to add uartclk attr\n");
> +
> + /*
> * Ensure UPF_DEAD is not set.
> */
> uport->flags &= ~UPF_DEAD;
> @@ -2397,6 +2445,12 @@ int uart_remove_one_port(struct uart_driver *drv,
> struct uart_port *uport) mutex_unlock(&port->mutex);
>
> /*
> + * Remove uartclk file from sysfs.
> + */
> + device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
> + &dev_attr_uartclk);
> +
> + /*
> * Remove the devices from the tty layer
> */
> tty_unregister_device(drv->tty_driver, uport->line);
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index b425c79..8ea8622 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver
> *driver, unsigned index) }
> EXPORT_SYMBOL(tty_unregister_device);
>
> +/*
> + * tty_lookup_device - lookup a tty device
> + * @driver: the tty driver that describes the tty device
> + * @index: the index in the tty driver for this tty device
> + *
> + * This function returns a struct device pointer when device has
> + * been found and NULL otherwise.
> + *
> + * Locking: ??
> + */
> +struct device *tty_lookup_device(struct tty_driver *driver, unsigned
> index) +{
> + dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
> + return class_find_device(tty_class, NULL, &devt, dev_match_devt);
> +}
> +EXPORT_SYMBOL(tty_lookup_device);
> +
> struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
> {
> struct tty_driver *driver;
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 9f47ab5..5d408a1 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver
> *driver); extern int tty_unregister_driver(struct tty_driver *driver);
> extern struct device *tty_register_device(struct tty_driver *driver,
> unsigned index, struct device *dev);
> +extern struct device *tty_lookup_device(struct tty_driver *driver,
> + unsigned index);
> extern void tty_unregister_device(struct tty_driver *driver, unsigned
> index); extern int tty_read_raw_data(struct tty_struct *tty, unsigned char
> *bufp, int buflen);
^ permalink raw reply
* Re: [PATCH V2] [tty] Fix possible race in n_tty_read()
From: Stanislav Kozina @ 2012-08-14 11:15 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Alan Cox, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120813152654.GA5091@redhat.com>
> I looked a bit more at this. Excluding memory corruption which could
> zero tty struct, the only possibility to nullify tty->read_buf is
> call to n_tty_close(). So NULL pointer dereference on n_tty_read,
> in "while (nr&& tty->read_cnt)" loop can only be caused by calling
> n_tty_close(), while performing n_tty_read().
Correct.
> Stanislav patch solve that problem because we do not touch tty->read_buf
> any longer once tty->read_cnt become 0, and because n_tty_close() clear
> tty->read_cnt (by calling n_tty_flush_buffer() -> reset_buffer_flags()).
Correct.
> However looks like main problem persist, we should never do
> n_tty_read() after/during n_tty_close() and before n_tty_open(). That
> must be an issue in upper layer i.e. tty_io and tty_ldisc, which should
> give guarantee about ->close(), ->read(), ->open() ordering.
Correct.
> I'm going
> to look at that more closely.
Thanks a lot;-)
Regards,
-Stanislav
^ permalink raw reply
* Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
From: David Herrmann @ 2012-08-14 11:07 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <5028447E.7000208@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:04 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> +static ssize_t fblog_dev_active_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + !!test_bit(FBLOG_OPEN, &fb->flags));
>
> Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
>
>> +}
>> +
>> +static ssize_t fblog_dev_active_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> + unsigned long num;
>> + int ret = 0;
>> +
>> + num = simple_strtoul(buf, NULL, 10);
>
> kstrtoul is preferred these days I think, it also catches errors.
>
>> +
>> + mutex_lock(&fb->info->lock);
>> + if (num)
>> + ret = fblog_open(fb);
>> + else
>> + fblog_close(fb, false);
>> + mutex_unlock(&fb->info->lock);
>> +
>> + return ret ? ret : count;
>
> Nitpick, you can use gcc's shortcut form of the ? operator here:
>
> return ret ?: count;
>
>> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>> }
>>
>> - fblog_open(fb);
>> + ret = device_create_file(&fb->dev, &dev_attr_active);
>> + if (ret) {
>> + pr_err("fblog: cannot create sysfs entry");
>
> Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
>
>> + /* do not open fb if we cannot create control file */
>> + do_open = false;
>> + }
>> +
>> + if (!activate_on_hotplug)
>> + do_open = false;
>> + if (main_only && info->node != 0)
>> + do_open = false;
>> +
>> + if (do_open)
>> + fblog_open(fb);
>> }
>>
>> static void fblog_register(struct fb_info *info, bool force)
All four fixes make sense, thanks!
David
^ permalink raw reply
* Re: [PATCH 06/11] fblog: open fb on registration
From: David Herrmann @ 2012-08-14 11:05 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284383.7020100@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> /*
>> + * fblog_open/close()
>> + * These functions manage access to the underlying framebuffer. While opened, we
>> + * have a valid reference to the fb and can use it for drawing operations. When
>> + * the fb is unregistered, we drop our reference and close the fb so it can get
>> + * deleted properly. We also mark it as dead so no further fblog_open() call
>> + * will succeed.
>> + * Both functions must be called with the fb->info->lock mutex held! But make
>> + * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
>> + * will dead-lock with fb-registration.
>> + */
>> +
>> +static int fblog_open(struct fblog_fb *fb)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&fb->lock);
>> +
>> + if (test_bit(FBLOG_KILLED, &fb->flags)) {
>> + ret = -ENODEV;
>> + goto unlock;
>> + }
>> +
>> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
>> + ret = 0;
>> + goto unlock;
>> + }
>> +
>> + if (!try_module_get(fb->info->fbops->owner)) {
>> + ret = -ENODEV;
>> + goto out_killed;
>> + }
>> +
>> + if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
>
> Should propagate the error here:
>
> if (fb->info->fbops->fb_open) {
> ret = fb->info->fbops->fb_open(fb->info, 0);
> if (ret)
> gotou out_unref;
> }
Nice catch, thanks.
>> +/*
>> * fblog framebuffer list
>> * The fblog_fbs[] array contains all currently registered framebuffers. If a
>> * framebuffer is in that list, we always must make sure that we own a reference
>> @@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
>>
>> fblog_fbs[info->node] = NULL;
>>
>> + fblog_close(fb, true);
>> device_del(&fb->dev);
>> put_device(&fb->dev);
>
> device_unregister?
Heh, you already mentioned this in the previous patch. I definitely
need to fix that.
>> }
>> @@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>>
>> fb->info = info;
>> + mutex_init(&fb->lock);
>> __module_get(THIS_MODULE);
>> device_initialize(&fb->dev);
>> fb->dev.class = fb_class;
>> @@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> put_device(&fb->dev);
>> return;
>> }
>> +
>> + fblog_open(fb);
>> }
>
> This function should really return an errno value.
I never used the return value in my code but as mentioned in the
previous patches, fblog_scan() should check them. So yes, I will add
this.
Thanks!
David
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: David Herrmann @ 2012-08-14 11:01 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284234.40306@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 195 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index fb39737..279f4d8 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -23,15 +23,210 @@
>> * all fblog instances before running other graphics applications.
>> */
>>
>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/fb.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> +
>> +enum fblog_flags {
>> + FBLOG_KILLED,
>> +};
>> +
>> +struct fblog_fb {
>> + unsigned long flags;
>
> Are more flags added in later patches? If not, why not just have:
>
> bool is_killed;
>
> ?
Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
FBLOG_SUSPENDED, FBLOG_BLANKED.
>> +static void fblog_release(struct device *dev)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + kfree(fb);
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static void fblog_do_unregister(struct fb_info *info)
>> +{
>> + struct fblog_fb *fb;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (!fb || fb->info != info)
>> + return;
>> +
>> + fblog_fbs[info->node] = NULL;
>> +
>> + device_del(&fb->dev);
>> + put_device(&fb->dev);
>
> device_unregister?
Right, I will replace it.
>> +}
>> +
>> +static void fblog_do_register(struct fb_info *info, bool force)
>> +{
>> + struct fblog_fb *fb;
>> + int ret;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (fb && fb->info != info) {
>> + if (!force)
>> + return;
>> +
>> + fblog_do_unregister(fb->info);
>> + }
>> +
>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>> + if (!fb)
>> + return;
>> +
>> + fb->info = info;
>> + __module_get(THIS_MODULE);
>> + device_initialize(&fb->dev);
>> + fb->dev.class = fb_class;
>> + fb->dev.release = fblog_release;
>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>> + fblog_fbs[info->node] = fb;
>> +
>> + ret = device_add(&fb->dev);
>> + if (ret) {
>> + fblog_fbs[info->node] = NULL;
>> + set_bit(FBLOG_KILLED, &fb->flags);
>> + put_device(&fb->dev);
>
> kfree(fb); ?
No. See device_initialize() in ./drivers/base/core.c. After a call to
device_initialize() the object is ref-counted so put_device() will
invoke the fblog_release() callback which will call kfree(fb) itself.
>> + return;
>> + }
>> +}
>> +
>> +static void fblog_register(struct fb_info *info, bool force)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_register(info, force);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>> +
>> +static void fblog_unregister(struct fb_info *info)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_unregister(info);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>
> This locking is needlessly heavy, and could easily pushed down into the
> fb_do_(un)register functions. It would also help make it clear exactly
> what the lock is protecting.
I need to call fblog_do_unregister() from within fblog_do_register().
I cannot release the locks while calling fblog_do_unregister() so I
need the unlocked fblog_do_unregister() function. So the locking must
be in a wrapper function.
See below for an explanation of the locks.
>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>> + void *data)
>> +{
>> + struct fb_event *event = data;
>> + struct fb_info *info = event->info;
>> +
>> + switch(action) {
>> + case FB_EVENT_FB_REGISTERED:
>> + /* This is called when a low-level system driver registers a new
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held when this is called. */
>
> Nitpick:
>
> /*
> * The Linux kernel multi-line
> * comment style looks like
> * this.
> */
I was confused by a recent discussion on the LKML:
http://comments.gmane.org/gmane.linux.kernel/1282421
However, turns out they didn't add this to CodingStyle so I will adopt
the old style again. Will be fixed in the next revision, thanks.
>> + fblog_register(info, true);
>> + break;
>> + case FB_EVENT_FB_UNREGISTERED:
>> + /* This is called when a low-level system driver unregisters a
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held. */
>> + fblog_unregister(info);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void fblog_scan(void)
>> +{
>> + unsigned int i;
>> + struct fb_info *info, *tmp;
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>
> Nitpick:
>
> if (IS_ERR_OR_NULL(info))
Didn't know of this macro, thanks.
>> + continue;
>> +
>> + fblog_register(info, false);
>
> This function should really return a value to indicate if it failed.
> There is no point continuing if it didn't register anything.
Indeed.
>> + /* There is a very subtle race-condition. Even though we might
>> + * own a reference to the fb, it may still get unregistered
>> + * between our call from get_fb_info() and fblog_register().
>> + * Therefore, we simply check whether the same fb still is
>> + * registered by calling get_fb_info() again. Only if they
>> + * differ we know that it got unregistered, therefore, we
>> + * call fblog_unregister() with the old pointer. */
>> +
>> + tmp = get_fb_info(i);
>> + if (tmp && !IS_ERR(tmp))
>> + put_fb_info(tmp);
>> + if (tmp != info)
>> + fblog_unregister(info);
>
> It would be better to fix this issue properly. Calling fblog_unregister
> here also looks unsafe if the call to fblog_register above failed.
fblog_unregister() does nothing if the passed "info" does not match
the found "info" so this is safe. And as we are holding a reference to
"info" here, the pointer is always valid and cannot be used by other
FBs.
Fixing this properly means changing the locking of fbdev. This can
either be done by exporting the fbdev-registration-lock (which I want
to avoid as locking should never be exported in an API) or by changing
fbdev to provide an scan/enumeration function itself. However, in my
opinion both would be uglier than using this race-condition-check
here.
The best way would be redesigning the fbdev in-kernel API. But that
means checking that fbcon will not break and this is something I
really don't want to touch.
>> + /* Here we either called fblog_unregister() and therefore do not
>> + * need any reference to the fb, or we can be sure that the FB
>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>> + * before the last reference is dropped. Hence, we can drop our
>> + * reference here. */
>
> This seems a slightly odd reasoning. Why would you not hold a reference
> to something you are using?
It's because get_fb_info() takes the fbdev-registration-lock so we
cannot call it from fblog_event(). And fblog_event() calls
fblog_register() for hotplugged framebuffers so we cannot get a refcnt
for hotplugged framebuffers.
Now I can either increase the fbdev-refcount manually without calling
get_fb_info() in fblog_register() or I can simply drop the framebuffer
when the fbdev-core notifies me that it is gone. I chose the latter
one.
>> + put_fb_info(info);
>> + }
>> +}
>> +
>> +static struct notifier_block fblog_notifier = {
>> + .notifier_call = fblog_event,
>> +};
>>
>> static int __init fblog_init(void)
>> {
>> + int ret;
>> +
>> + ret = fb_register_client(&fblog_notifier);
>> + if (ret) {
>> + pr_err("cannot register framebuffer notifier\n");
>> + return ret;
>> + }
>> +
>> + fblog_scan();
>> +
>> return 0;
>> }
>>
>> static void __exit fblog_exit(void)
>> {
>> + unsigned int i;
>> + struct fb_info *info;
>> +
>> + fb_unregister_client(&fblog_notifier);
>> +
>> + /* We scan through the whole registered_fb array here instead of
>> + * fblog_fbs because we need to get the device lock _before_ the
>> + * fblog-registration-lock. */
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>> + continue;
>> +
>> + fblog_unregister(info);
>
> Given the description of the get_fb_info/fblog_register race above, can
> this unregister the wrong framebuffer?
No. fblog_unregister() will do nothing if the "info" pointers do not
match. Moreover, this unregisters _all_ framebuffers, so I don't
understand how this can unregister the "wrong" framebuffer?
But I just noticed a race here. If the fbdev core unregisters a
framebuffer during my loop, I will not call fblog_unregister() on it,
as it does not exist anymore. Therefore, I will not free it in my
fblog_fbs array as the fblog_notifier has already been unregistered.
So I need to set a global "exiting" flag and fblog_event() shall only
handle the UNBIND/UNREGISTER events during exit. Then I simply move
the fb_unregister_client() call below this loop.
>> + put_fb_info(info);
>> + }
>> }
>>
>> module_init(fblog_init);
>>
>
A short explanation for all locks:
First of all, I spent hours getting this right. The easy way would be
removing all locks and relying on the fbdev locking (fbcon does this).
But obviously, that doesn't work as fbdev has no safe way of
scanning/enumerating all existing devices. And this is needed as fblog
can be compiled as a module.
fbdev has a core registration-lock and each framebuffer has its own
fb-lock. fblog_event() is sometimes called with these locks held,
sometimes without any locks held (see the comments in this function)
and I need to work around this.
So fblog_event() as entry point for hotplugging is called with the
fbdev-registration-lock held. And it calls fblog_(un)register() which
uses its own locks. So when calling this from fblog_scan(), I need to
make sure to guarantee that the locks are acquired in the same order
as in fblog_event(), otherwise I might have deadlocks. But I have no
outside access to the fbdev-registration-lock, therefore, the
fblog-registration-lock is needed and fblog_scan() needs to check for
those ugly races.
As you mentioned that this locking is needlessly complex, I have to
disagree. I use one lock to protect the registration
(fblog_registration_lock) and one lock to protect each registered
framebuffer from concurrent access (struct fblog_fb->lock). This is
the most common way to protect hotplugged devices and I don't see how
this can be done with less locks? (these are the only locks that are
added by fblog).
Normally, this would be all locks I have to access. However, the
fbdev-core wasn't designed as in-kernel API and thus has very
inconsistent locking. And all entry points to fblog that are not
through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
acquire the same locks as the fbdev-core to avoid races with the
fbdev-core. As this is not possible, because the fbdev-locks are not
exported, I need to carefully use fbdev-functions that guarantee that
I have no races. And I think I found the only way to guarantee this.
If anyone has other ideas, I would be glad to hear them.
Thanks for reviewing the code!
Regards
David
^ permalink raw reply
* Re: [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
From: David Herrmann @ 2012-08-14 9:42 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50283D7A.4090309@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:34 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> > config VGA_CONSOLE
> > bool "VGA text console" if EXPERT || !X86
> > - depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV &&
> > !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE ||
> > ARCH_INTEGRATOR || ARCH_NETWINDER)
> > + depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC &&
> > !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM ||
> > ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> > default y
> > help
> > Saying Y here will allow you to use Linux in text mode through a
> > @@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> > screenfuls of scrollback buffer
>
> You could just place all of the CONFIG_VT options inside an if VT and
> then the new fblog options in the else case rather than modifying all of
> the (already overly long) depends statements.
Indeed, that sounds better.
> > +config FBLOG
> > + tristate "Framebuffer Kernel Log Driver"
> > + depends on !VT && FB
> > + default n
>
> Default n is not required, all options are default n unless otherwise
> specified. I don't think you need the dependency on FB either, since
> this file should only get included if CONFIG_FB is set?
I will remove the default-line, but the FB-dependency is required. The
"console/" directory includes also drivers like vgacon which do not
depend on FB so it gets always included.
> > + help
> > + This driver displays all kernel log messages on all connected
> > + framebuffers. It is mutually exclusive with
> > CONFIG_FRAMEBUFFER_CONSOLE
> > + and CONFIG_VT. It was mainly created for debugging purposes when
> > + CONFIG_VT is not selected but you still want kernel boot
> > messages on
> > + the screen.
>
> Do command line options exist to specify screens to write the debug log
> to? It might be a useful feature to have.
See patch-7. However, I am not entirely happy with it. I will probably
have to rethink the parameters.
Thanks for reviewing. I will fix all these in the next series.
Regards
David
^ permalink raw reply
* [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs
From: Tomas Hlavacek @ 2012-08-14 7:35 UTC (permalink / raw)
To: gregkh, alan, linux-serial, linux-kernel; +Cc: marek.vasut, Tomas Hlavacek
In-Reply-To: <CAEB7QLAJXkK6NDPVDi36n_U_X_DGh5niHJhH6FpqBUZFmXQ2Xg@mail.gmail.com>
Support for read/modify of uartclk via sysfs added.
It may prove useful with some no-name cards that
has different oscillator speeds and no distinguishing
PCI IDs to allow autodetection. It allows better integration
with udev and/or init scripts.
Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com>
---
drivers/tty/serial/serial_core.c | 54 ++++++++++++++++++++++++++++++++++++++
drivers/tty/tty_io.c | 17 ++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 73 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..059b438 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,46 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+ return ret;
+}
+
+static ssize_t set_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
+ unsigned int val;
+ int ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&state->port.mutex);
+
+ /*
+ * Check value: baud_base has to be more than 9600
+ * and uartclock = baud_base * 16 .
+ */
+ if (val >= 153600)
+ state->uart_port->uartclk = val;
+
+ mutex_unlock(&state->port.mutex);
+
+ return count;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, get_attr_uartclk,
+ set_attr_uartclk);
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2355,6 +2395,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
}
/*
+ * Expose uartclk in sysfs. Use driverdata of the tty device for
+ * referencing the UART port.
+ */
+ dev_set_drvdata(tty_dev, state);
+ if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
+ dev_err(tty_dev, "Failed to add uartclk attr\n");
+
+ /*
* Ensure UPF_DEAD is not set.
*/
uport->flags &= ~UPF_DEAD;
@@ -2397,6 +2445,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
/*
+ * Remove uartclk file from sysfs.
+ */
+ device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
+ &dev_attr_uartclk);
+
+ /*
* Remove the devices from the tty layer
*/
tty_unregister_device(drv->tty_driver, uport->line);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..8ea8622 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
}
EXPORT_SYMBOL(tty_unregister_device);
+/*
+ * tty_lookup_device - lookup a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ *
+ * This function returns a struct device pointer when device has
+ * been found and NULL otherwise.
+ *
+ * Locking: ??
+ */
+struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
+{
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+ return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_lookup_device);
+
struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
{
struct tty_driver *driver;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5d408a1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+extern struct device *tty_lookup_device(struct tty_driver *driver,
+ unsigned index);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH V2] [tty] Fix possible race in n_tty_read()
From: Stanislaw Gruszka @ 2012-08-13 15:26 UTC (permalink / raw)
To: Alan Cox; +Cc: Stanislav Kozina, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120809111620.GA3516@redhat.com>
On Thu, Aug 09, 2012 at 01:16:20PM +0200, Stanislaw Gruszka wrote:
> On Wed, Aug 08, 2012 at 04:27:25PM +0100, Alan Cox wrote:
> > On Wed, 08 Aug 2012 16:28:47 +0200
> > Stanislav Kozina <skozina@redhat.com> wrote:
> >
> > > Fix possible panic caused by unlocked access to tty->read_cnt in
> > > while-loop condition in n_tty_read().
> >
> > Should this also be removing the BUG_ON check you noted in the other
> > email was not valid now ?
>
> You talk about
> http://marc.info/?l=linux-serial&m=134318985920881&w=2
>
> Is possible that we can call n_tty_read() after n_tty_close() ? How oterwise
> tty->read_buf could become NULL?
>
> If I understand correctly Stanislav's patch solve below race condtion:
>
> CPU0 CPU1
> n_tty_read: reset_buffer_flags:
>
> while (nr && tty->read_cnt) {
>
> spin_lock_irqsave(&tty->read_lock, flags);
> tty->read_head = tty->read_tail = tty->read_cnt = 0;
> spin_lock_irqsave(&tty->read_lock, flags);
>
> spin_lock_irqsave(&tty->read_lock, flags);
>
> tty->read_cnt--;
>
> spin_lock_irqsave(&tty->read_lock, flags);
>
> /* Now tty->read_cnt is negative */
>
> }
>
> what itself could have varsious nasty consequences, i.e. ininite
> loop. Is also possible that negative tty->read_cnt would result in
> tty->read_buf == NULL ? If so, I'm not quite understand that.
I looked a bit more at this. Excluding memory corruption which could
zero tty struct, the only possibility to nullify tty->read_buf is
call to n_tty_close(). So NULL pointer dereference on n_tty_read,
in "while (nr && tty->read_cnt)" loop can only be caused by calling
n_tty_close(), while performing n_tty_read().
Stanislav patch solve that problem because we do not touch tty->read_buf
any longer once tty->read_cnt become 0, and because n_tty_close() clear
tty->read_cnt (by calling n_tty_flush_buffer() -> reset_buffer_flags()).
However looks like main problem persist, we should never do
n_tty_read() after/during n_tty_close() and before n_tty_open(). That
must be an issue in upper layer i.e. tty_io and tty_ldisc, which should
give guarantee about ->close(), ->read(), ->open() ordering. I'm going
to look at that more closely.
Stanislaw
^ permalink raw reply
* [PATCH 8/8] n_gsm: memory leak in uplink error path
From: Alan Cox @ 2012-08-13 12:45 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Russ Gorby <russ.gorby@intel.com>
Uplink (TX) network data will go through gsm_dlci_data_output_framed
there is a bug where if memory allocation fails, the skb which
has already been pulled off the list will be lost.
In addition TX skbs were being processed in LIFO order
Fixed the memory leak, and changed to FIFO order processing
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Kappel, LaurentX <laurentx.kappel@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Showjumping <stable@kernel.org>
---
drivers/tty/n_gsm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 17c9e94..793bc38 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -868,7 +868,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
/* dlci->skb is locked by tx_lock */
if (dlci->skb == NULL) {
- dlci->skb = skb_dequeue(&dlci->skb_list);
+ dlci->skb = skb_dequeue_tail(&dlci->skb_list);
if (dlci->skb == NULL)
return 0;
first = 1;
@@ -892,8 +892,11 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
/* FIXME: need a timer or something to kick this so it can't
get stuck with no work outstanding and no buffer free */
- if (msg == NULL)
+ if (msg == NULL) {
+ skb_queue_tail(&dlci->skb_list, dlci->skb);
+ dlci->skb = NULL;
return -ENOMEM;
+ }
dp = msg->data;
if (dlci->adaption == 4) { /* Interruptible framed (Packetised Data) */
^ permalink raw reply related
* [PATCH 7/8] n_gsm: replace kfree_skb w/ appropriate dev_* versions
From: Alan Cox @ 2012-08-13 12:45 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Russ Gorby <russ.gorby@intel.com>
Drivers are supposed to use the dev_* versions of the kfree_skb
interfaces. In a couple of cases we were called with IRQs
disabled as well which kfree_skb() does not expect.
Replaced kfree_skb calls w/ dev_kfree_skb and dev_kfree_skb_any
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Grooming <stable@kernel.org>
---
drivers/tty/n_gsm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index e2bdb8b..17c9e94 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -879,7 +879,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
if (len > gsm->mtu) {
if (dlci->adaption == 3) {
/* Over long frame, bin it */
- kfree_skb(dlci->skb);
+ dev_kfree_skb_any(dlci->skb);
dlci->skb = NULL;
return 0;
}
@@ -905,7 +905,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
skb_pull(dlci->skb, len);
__gsm_data_queue(dlci, msg);
if (last) {
- kfree_skb(dlci->skb);
+ dev_kfree_skb_any(dlci->skb);
dlci->skb = NULL;
}
return size;
@@ -1674,7 +1674,7 @@ static void gsm_dlci_free(struct kref *ref)
dlci->gsm->dlci[dlci->addr] = NULL;
kfifo_free(dlci->fifo);
while ((dlci->skb = skb_dequeue(&dlci->skb_list)))
- kfree_skb(dlci->skb);
+ dev_kfree_skb(dlci->skb);
kfree(dlci);
}
@@ -2013,7 +2013,7 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
{
int i;
struct gsm_dlci *dlci = gsm->dlci[0];
- struct gsm_msg *txq, *utxq;
+ struct gsm_msg *txq, *ntxq;
struct gsm_control *gc;
gsm->dead = 1;
^ permalink raw reply related
* [PATCH 6/8] n_gsm: avoid accessing freed memory during CMD_FCOFF condition
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Russ Gorby <russ.gorby@intel.com>
gsm_data_kick was recently modified to allow messages on the
tx queue bound for DLCI0 to flow even during FCOFF conditions.
Unfortunately we introduced a bug discovered by code inspection
where subsequent list traversers can access freed memory if
the DLCI0 messages were not all at the head of the list.
Replaced singly linked tx list w/ a list_head and used
provided interfaces for traversing and deleting members.
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Riding School <stable@kernel.org>
---
drivers/tty/n_gsm.c | 40 +++++++++++++---------------------------
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 51ba2f2..e2bdb8b 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -108,7 +108,7 @@ struct gsm_mux_net {
*/
struct gsm_msg {
- struct gsm_msg *next;
+ struct list_head list;
u8 addr; /* DLCI address + flags */
u8 ctrl; /* Control byte + flags */
unsigned int len; /* Length of data block (can be zero) */
@@ -245,8 +245,7 @@ struct gsm_mux {
unsigned int tx_bytes; /* TX data outstanding */
#define TX_THRESH_HI 8192
#define TX_THRESH_LO 2048
- struct gsm_msg *tx_head; /* Pending data packets */
- struct gsm_msg *tx_tail;
+ struct list_head tx_list; /* Pending data packets */
/* Control messages */
struct timer_list t2_timer; /* Retransmit timer for commands */
@@ -663,7 +662,7 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
m->len = len;
m->addr = addr;
m->ctrl = ctrl;
- m->next = NULL;
+ INIT_LIST_HEAD(&m->list);
return m;
}
@@ -681,16 +680,13 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
static void gsm_data_kick(struct gsm_mux *gsm)
{
- struct gsm_msg *msg = gsm->tx_head;
- struct gsm_msg *free_msg;
+ struct gsm_msg *msg, *nmsg;
int len;
int skip_sof = 0;
- while (msg) {
- if (gsm->constipated && msg->addr) {
- msg = msg->next;
+ list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
+ if (gsm->constipated && msg->addr)
continue;
- }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -718,14 +714,9 @@ static void gsm_data_kick(struct gsm_mux *gsm)
burst */
skip_sof = 1;
- if (gsm->tx_head == msg)
- gsm->tx_head = msg->next;
- free_msg = msg;
- msg = msg->next;
- kfree(free_msg);
+ list_del(&msg->list);
+ kfree(msg);
}
- if (!gsm->tx_head)
- gsm->tx_tail = NULL;
}
/**
@@ -774,11 +765,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
msg->data = dp;
/* Add to the actual output queue */
- if (gsm->tx_tail)
- gsm->tx_tail->next = msg;
- else
- gsm->tx_head = msg;
- gsm->tx_tail = msg;
+ list_add_tail(&msg->list, &gsm->tx_list);
gsm->tx_bytes += msg->len;
gsm_data_kick(gsm);
}
@@ -2026,7 +2013,7 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
{
int i;
struct gsm_dlci *dlci = gsm->dlci[0];
- struct gsm_msg *txq;
+ struct gsm_msg *txq, *utxq;
struct gsm_control *gc;
gsm->dead = 1;
@@ -2061,11 +2048,9 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
if (gsm->dlci[i])
gsm_dlci_release(gsm->dlci[i]);
/* Now wipe the queues */
- for (txq = gsm->tx_head; txq != NULL; txq = gsm->tx_head) {
- gsm->tx_head = txq->next;
+ list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)
kfree(txq);
- }
- gsm->tx_tail = NULL;
+ INIT_LIST_HEAD(&gsm->tx_list);
}
EXPORT_SYMBOL_GPL(gsm_cleanup_mux);
@@ -2176,6 +2161,7 @@ struct gsm_mux *gsm_alloc_mux(void)
}
spin_lock_init(&gsm->lock);
kref_init(&gsm->ref);
+ INIT_LIST_HEAD(&gsm->tx_list);
gsm->t1 = T1;
gsm->t2 = T2;
^ permalink raw reply related
* [PATCH 5/8] n_gsm: added interlocking for gsm_data_lock for certain code paths
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Russ Gorby <russ.gorby@intel.com>
There were some locking holes in the management of the MUX's
message queue for 2 code paths:
1) gsmld_write_wakeup
2) receipt of CMD_FCON flow-control message
In both cases gsm_data_kick is called w/o locking so it can collide
with other other instances of gsm_data_kick (pulling messages tx_tail)
or potentially other instances of __gsm_data_queu (adding messages to tx_head)
Changed to take the tx_lock in these 2 cases
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Yin, Fengwei <fengwei.yin@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Riding School <stable@kernel.org>
---
drivers/tty/n_gsm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9854edf..51ba2f2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1205,6 +1205,8 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
u8 *data, int clen)
{
u8 buf[1];
+ unsigned long flags;
+
switch (command) {
case CMD_CLD: {
struct gsm_dlci *dlci = gsm->dlci[0];
@@ -1225,7 +1227,9 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
gsm->constipated = 0;
gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
+ spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_data_kick(gsm);
+ spin_unlock_irqrestore(&gsm->tx_lock, flags);
break;
case CMD_FCOFF:
/* Modem wants us to STFU */
@@ -2392,12 +2396,12 @@ static void gsmld_write_wakeup(struct tty_struct *tty)
/* Queue poll */
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_data_kick(gsm);
if (gsm->tx_bytes < TX_THRESH_LO) {
- spin_lock_irqsave(&gsm->tx_lock, flags);
gsm_dlci_data_sweep(gsm);
- spin_unlock_irqrestore(&gsm->tx_lock, flags);
}
+ spin_unlock_irqrestore(&gsm->tx_lock, flags);
}
/**
^ permalink raw reply related
* [PATCH 4/8] char: n_gsm: remove message filtering for contipated DLCI
From: Alan Cox @ 2012-08-13 12:44 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: samix.lebsir <samix.lebsir@intel.com>
The design of uplink flow control in the mux driver is
that for constipated channels data will backup into the
per-channel fifos, and any messages that make it to the
outbound message queue will still go out.
Code was added to also stop messages that were in the outbound
queue but this requires filtering through all the messages on the
queue for stopped dlcis and changes some of the mux logic unneccessarily.
The message fiiltering was removed to be in line w/ the original design
as the message filtering does not provide any solution.
Extra debug messages used during investigation were also removed.
Signed-off-by: samix.lebsir <samix.lebsir@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Dressage <stable@kernel.org>
---
drivers/tty/n_gsm.c | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 6651285..9854edf 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -691,10 +691,6 @@ static void gsm_data_kick(struct gsm_mux *gsm)
msg = msg->next;
continue;
}
- if (gsm->dlci[msg->addr]->constipated) {
- msg = msg->next;
- continue;
- }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -748,8 +744,6 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
u8 *dp = msg->data;
u8 *fcs = dp + msg->len;
- WARN_ONCE(dlci->constipated, "%s: queueing from a constipated DLCI",
- __func__);
/* Fill in the header */
if (gsm->encoding == 0) {
if (msg->len < 128)
@@ -956,9 +950,6 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
break;
dlci = gsm->dlci[i];
if (dlci == NULL || dlci->constipated) {
- if (dlci && (debug & 0x20))
- pr_info("%s: DLCI %d is constipated",
- __func__, i);
i++;
continue;
}
@@ -988,12 +979,8 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
unsigned long flags;
int sweep;
- if (dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d is constipated",
- __func__, dlci->addr);
+ if (dlci->constipated)
return;
- }
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
@@ -1069,15 +1056,9 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
/* Flow control/ready to communicate */
fc = (modem & MDM_FC) || !(modem & MDM_RTR);
if (fc && !dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d START constipated (tx_bytes=%d)",
- __func__, dlci->addr, dlci->gsm->tx_bytes);
/* Need to throttle our output on this device */
dlci->constipated = 1;
} else if (!fc && dlci->constipated) {
- if (debug & 0x20)
- pr_info("%s: DLCI %d END constipated (tx_bytes=%d)",
- __func__, dlci->addr, dlci->gsm->tx_bytes);
dlci->constipated = 0;
gsm_dlci_data_kick(dlci);
}
@@ -1241,8 +1222,6 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
break;
case CMD_FCON:
/* Modem can accept data again */
- if (debug & 0x20)
- pr_info("%s: GSM END constipation", __func__);
gsm->constipated = 0;
gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
@@ -1250,8 +1229,6 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
break;
case CMD_FCOFF:
/* Modem wants us to STFU */
- if (debug & 0x20)
- pr_info("%s: GSM START constipation", __func__);
gsm->constipated = 1;
gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
break;
^ permalink raw reply related
* [PATCH 3/8] n_gsm : Flow control handling in Mux driver
From: Alan Cox @ 2012-08-13 12:43 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Frederic Berat <fredericx.berat@intel.com>
- Correcting handling of FCon/FCoff in order to respect 27.010 spec
- Consider FCon/off will overide all dlci flow control except for
dlci0 as we must be able to send control frames.
- Dlci constipated handling according to FC, RTC and RTR values.
- Modifying gsm_dlci_data_kick and gsm_dlci_data_sweep according
to dlci constipated value
Signed-off-by: Frederic Berat <fredericx.berat@intel.com>
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/n_gsm.c | 79 +++++++++++++++++++++++++++++++++++++--------------
1 file changed, 57 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9b0a44d..6651285 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -673,6 +673,8 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
*
* The tty device has called us to indicate that room has appeared in
* the transmit queue. Ram more data into the pipe if we have any
+ * If we have been flow-stopped by a CMD_FCOFF, then we can only
+ * send messages on DLCI0 until CMD_FCON
*
* FIXME: lock against link layer control transmissions
*/
@@ -680,15 +682,19 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
static void gsm_data_kick(struct gsm_mux *gsm)
{
struct gsm_msg *msg = gsm->tx_head;
+ struct gsm_msg *free_msg;
int len;
int skip_sof = 0;
- /* FIXME: We need to apply this solely to data messages */
- if (gsm->constipated)
- return;
-
- while (gsm->tx_head != NULL) {
- msg = gsm->tx_head;
+ while (msg) {
+ if (gsm->constipated && msg->addr) {
+ msg = msg->next;
+ continue;
+ }
+ if (gsm->dlci[msg->addr]->constipated) {
+ msg = msg->next;
+ continue;
+ }
if (gsm->encoding != 0) {
gsm->txframe[0] = GSM1_SOF;
len = gsm_stuff_frame(msg->data,
@@ -711,15 +717,19 @@ static void gsm_data_kick(struct gsm_mux *gsm)
len - skip_sof) < 0)
break;
/* FIXME: Can eliminate one SOF in many more cases */
- gsm->tx_head = msg->next;
- if (gsm->tx_head == NULL)
- gsm->tx_tail = NULL;
gsm->tx_bytes -= msg->len;
- kfree(msg);
/* For a burst of frames skip the extra SOF within the
burst */
skip_sof = 1;
+
+ if (gsm->tx_head == msg)
+ gsm->tx_head = msg->next;
+ free_msg = msg;
+ msg = msg->next;
+ kfree(free_msg);
}
+ if (!gsm->tx_head)
+ gsm->tx_tail = NULL;
}
/**
@@ -738,6 +748,8 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
u8 *dp = msg->data;
u8 *fcs = dp + msg->len;
+ WARN_ONCE(dlci->constipated, "%s: queueing from a constipated DLCI",
+ __func__);
/* Fill in the header */
if (gsm->encoding == 0) {
if (msg->len < 128)
@@ -944,6 +956,9 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
break;
dlci = gsm->dlci[i];
if (dlci == NULL || dlci->constipated) {
+ if (dlci && (debug & 0x20))
+ pr_info("%s: DLCI %d is constipated",
+ __func__, i);
i++;
continue;
}
@@ -973,6 +988,13 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
unsigned long flags;
int sweep;
+ if (dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d is constipated",
+ __func__, dlci->addr);
+ return;
+ }
+
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO);
@@ -1030,6 +1052,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
{
int mlines = 0;
u8 brk = 0;
+ int fc;
/* The modem status command can either contain one octet (v.24 signals)
or two octets (v.24 signals + break signals). The length field will
@@ -1041,19 +1064,27 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
else {
brk = modem & 0x7f;
modem = (modem >> 7) & 0x7f;
- };
+ }
/* Flow control/ready to communicate */
- if (modem & MDM_FC) {
+ fc = (modem & MDM_FC) || !(modem & MDM_RTR);
+ if (fc && !dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d START constipated (tx_bytes=%d)",
+ __func__, dlci->addr, dlci->gsm->tx_bytes);
/* Need to throttle our output on this device */
dlci->constipated = 1;
- }
- if (modem & MDM_RTC) {
- mlines |= TIOCM_DSR | TIOCM_DTR;
+ } else if (!fc && dlci->constipated) {
+ if (debug & 0x20)
+ pr_info("%s: DLCI %d END constipated (tx_bytes=%d)",
+ __func__, dlci->addr, dlci->gsm->tx_bytes);
dlci->constipated = 0;
gsm_dlci_data_kick(dlci);
}
+
/* Map modem bits */
+ if (modem & MDM_RTC)
+ mlines |= TIOCM_DSR | TIOCM_DTR;
if (modem & MDM_RTR)
mlines |= TIOCM_RTS | TIOCM_CTS;
if (modem & MDM_IC)
@@ -1209,17 +1240,21 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
gsm_control_reply(gsm, CMD_TEST, data, clen);
break;
case CMD_FCON:
- /* Modem wants us to STFU */
- gsm->constipated = 1;
- gsm_control_reply(gsm, CMD_FCON, NULL, 0);
- break;
- case CMD_FCOFF:
/* Modem can accept data again */
+ if (debug & 0x20)
+ pr_info("%s: GSM END constipation", __func__);
gsm->constipated = 0;
- gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
+ gsm_control_reply(gsm, CMD_FCON, NULL, 0);
/* Kick the link in case it is idling */
gsm_data_kick(gsm);
break;
+ case CMD_FCOFF:
+ /* Modem wants us to STFU */
+ if (debug & 0x20)
+ pr_info("%s: GSM START constipation", __func__);
+ gsm->constipated = 1;
+ gsm_control_reply(gsm, CMD_FCOFF, NULL, 0);
+ break;
case CMD_MSC:
/* Out of band modem line change indicator for a DLCI */
gsm_control_modem(gsm, data, clen);
@@ -2276,7 +2311,7 @@ static void gsmld_receive_buf(struct tty_struct *tty, const unsigned char *cp,
gsm->error(gsm, *dp, flags);
break;
default:
- WARN_ONCE("%s: unknown flag %d\n",
+ WARN_ONCE(1, "%s: unknown flag %d\n",
tty_name(tty, buf), flags);
break;
}
^ permalink raw reply related
* [PATCH 2/8] n_gsm: uplink SKBs accumulate on list
From: Alan Cox @ 2012-08-13 12:43 UTC (permalink / raw)
To: greg, linux-serial
In-Reply-To: <20120813124254.6125.70371.stgit@localhost.localdomain>
From: Russ Gorby <russ.gorby@intel.com>
gsm_dlci_data_kick will not call any output function if tx_bytes > THRESH_LO
furthermore it will call the output function only once if tx_bytes == 0
If the size of the IP writes are on the order of THRESH_LO
we can get into a situation where skbs accumulate on the outbound list
being starved for events to call the output function.
gsm_dlci_data_kick now calls the sweep function when tx_bytes==0
Signed-off-by: Russ Gorby <russ.gorby@intel.com>
Tested-by: Kappel, LaurentX <laurentx.kappel@intel.com>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Hay and Water <stable@kernel.org>
---
drivers/tty/n_gsm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5c6c2e2..9b0a44d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -971,16 +971,19 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
{
unsigned long flags;
+ int sweep;
spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
/* If we have nothing running then we need to fire up */
+ sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO);
if (dlci->gsm->tx_bytes == 0) {
if (dlci->net)
gsm_dlci_data_output_framed(dlci->gsm, dlci);
else
gsm_dlci_data_output(dlci->gsm, dlci);
- } else if (dlci->gsm->tx_bytes < TX_THRESH_LO)
- gsm_dlci_data_sweep(dlci->gsm);
+ }
+ if (sweep)
+ gsm_dlci_data_sweep(dlci->gsm);
spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
}
^ permalink raw reply related
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