* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Rafael J. Wysocki @ 2012-01-19 0:02 UTC (permalink / raw)
To: paulmck; +Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120118224304.GJ2431@linux.vnet.ibm.com>
On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > Hi Paul,
> >
> > On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> > >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> > >>
> > >> Hi Alan, Paul,
> > >>
> > >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> > >> <paulmck@linux.vnet.ibm.com> wrote:
> > >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> > >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> > >> >> Simon Glass <sjg@chromium.org> wrote:
> > >> >>
> > >> >> > Since serial_core now does not make serial ports wake-up capable by
> > >> >> > default, add a parameter to support this feature in the 8250 UART.
> > >> >> > This is the only UART where I think this feature is useful.
> > >> >>
> > >> >> NAK
> > >> >>
> > >> >> Things should just work for users. Magic parameters is not an
> > >> >> improvement. If its a performance problem someone needs to fix the rcu
> > >> >> sync overhead or stop using rcu on that path.
> > >>
> > >> OK fair enough, I agree. Every level I move down the source tree
> > >> affects more people though.
> > >>
> > >> >
> > >> > I must say that I lack context here, even after looking at the patch,
> > >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > >> > of synchronize_rcu() is too large.
> > >> >
> > >>
> > >> Let me provide a bit of context. The serial_core code seems to be the
> > >> only place in the kernel that does this:
> > >>
> > >> device_init_wakeup(tty_dev, 1);
> > >> device_set_wakeup_enable(tty_dev, 0);
> > >>
> > >> The first call makes the device wakeup capable and enables wakeup, The
> > >> second call disabled wakeup.
> > >>
> > >> The code that removes the wakeup source looks like this:
> > >>
> > >> void wakeup_source_remove(struct wakeup_source *ws)
> > >> {
> > >> if (WARN_ON(!ws))
> > >> return;
> > >>
> > >> spin_lock_irq(&events_lock);
> > >> list_del_rcu(&ws->entry);
> > >> spin_unlock_irq(&events_lock);
> > >> synchronize_rcu();
> > >> }
> > >>
> > >> The sync is there because we are about to destroy the actual ws
> > >> structure (in wakeup_source_destroy()). I wonder if it should be in
> > >> wakeup_source_destroy() but that wouldn't help me anyway.
> > >>
> > >> synchronize_rcu_expedited() is a bit faster but not really fast
> > >> enough. Anyway surely people will complain if I put this in the wakeup
> > >> code - it will affect all wakeup users. It seems to me that the right
> > >> solution is to avoid enabling and then immediately disabling wakeup.
> > >
> > > Hmmm... What hardware are you running this one? Normally,
> > > synchronize_rcu_expedited() will be a couple of orders of magnitude
> > > faster than synchronize_rcu().
> > >
> > >> I assume we can't and shouldn't change device_init_wakeup() . We could
> > >> add a call like device_init_wakeup_disabled() which makes the device
> > >> wakeup capable but does not actually enable it. Does that work?
> > >
> > > If the only reason for the synchronize_rcu() is to defer the pair of
> > > kfree()s in wakeup_source_destroy(), then another possible approach
> > > would be to remove the synchronize_rcu() from wakeup_source_remove()
> > > and then use call_rcu() to defer the two kfree()s.
> > >
> > > If this is a reasonable change to make, the approach is as follows:
> > >
> > > 1. Add a struct rcu_head to wakeup_source, call it "rcu".
> > > Or adjust the following to suit your choice of name.
> > >
> > > 2. Replace the pair of kfree()s with:
> > >
> > > call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> > >
> > > 3. Create the wakeup_source_destroy_rcu() as follows:
> > >
> > > static void wakeup_source_destroy_rcu(struct rcu_head *head)
> > > {
> > > struct wakeup_source *ws =
> > > container_of(head, struct wakeup_source, rcu);
> > >
> > > kfree(ws->name);
> > > kfree(ws);
> > > }
> > >
> > > Of course, this assumes that it is OK for wakeup_source_unregister()
> > > to return before the memory is freed up. This often is OK, but there
> > > are some cases where the caller requires that there be no further
> > > RCU readers with access to the old data. In these cases, you really
> > > do need the wait.
> >
> > Thanks very much for that. I'm not sure if it is a reasonable change,
> > but it does bug me that we add it to a data structure knowing that we
> > will immediately remove it!
> >
> > >From what I can see, making a device wakeup-enabled mostly happens on
> > init or in response to a request to the driver (presumably from user
> > space). In the latter case I suspect the synchronise_rcu() is fine. In
> > the former it feels like we should make up our minds which of the
> > three options is required (incapable, capable but not enabled, capable
> > and enabled).
> >
> > I will try a patch first based on splitting the two options (capable
> > and enable) and see if that get a NAK.
> >
> > Then I will come back to your solution - it seems fine to me and not a
> > lot of code. Do we have to worry about someone enabling, disabled,
> > enabling and then disabling wakeup quickly? Will this method break in
> > that case if the second call to call_rcu() uses the same wc->rcu?
>
> There are a couple of questions here, let me take them one at a time:
>
> 1. If you just disabled, can you immediately re-enable?
>
> The answer is "yes". The reason that this works is that you
> allocate a new structure for the re-enabling, and that new
> structure has its own rcu_head field.
>
> 2. If you repeatedly disable and re-enable in a tight loop,
> can this cause problems?
>
> The answer to this is also "yes" -- you can run the system
> out of memory doing that. However, there are a number of
> simple ways to avoid this problem:
>
> a. Do a synchronize_rcu() on every (say) thousandth
> disable operation.
>
> b. As above, but only do the synchronize_rcu() if
> all 1,000 disable operations occurred within
> (say) a second of each other.
>
> c. As above, but actually count the number of
> pending call_rcu() callbacks.
>
> Both (a) and (b) can be carried out on a per-CPU basis if there
> is no convenient locked structure in which to track the state.
> You cannot carry (c) out on a per-CPU basis because RCU callbacks
> can sometimes be invoked on a different CPU from the one that
> call_rcu()ed them. Rare, but it can happen.
>
> I would expect that option (a) would work in almost all cases.
>
> If this can be exercised freely from user space, then you probably
> really do need #2 above.
Yes, you can, but then I'd say it's not necessary for user space to
be able to carry that out in a tight loop. So, it seems, alternatively,
we could make that loop a bit less tight, e.g. by adding an arbitrary
sleep to the user space interface for the "disable" case.
Thanks,
Rafael
^ permalink raw reply
* [PATCH 2/2] serial: Use device_init_wakeup_flag() to make device wakeup-capable
From: Simon Glass @ 2012-01-18 23:07 UTC (permalink / raw)
To: LKML
Cc: Greg Kroah-Hartman, linux-serial, Pavel Machek, Rafael J. Wysocki,
Alan Cox, Paul E. McKenney, Simon Glass
In-Reply-To: <1326928027-20610-1-git-send-email-sjg@chromium.org>
We want the serial device to be wakeup-capable but not actually enable
the wakeup function. The original code results in making the serial
device wakeup-capable, then enabling and then immediately disabling
the actual wakeup feature on the device. This ends up in a
synchronise_rcu() in wakeup_source_remove() which can take some 15ms
to run, yet is entirely avoidable. The cost over 4 ports is about 600ms
on my Tegra2x system (with CONFIG_PREEMPT disabled).
Using the new function avoids this problem.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/tty/serial/serial_core.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c7bf31a..a8b01db 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2348,11 +2348,11 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
*/
tty_dev = tty_register_device(drv->tty_driver, uport->line, uport->dev);
if (likely(!IS_ERR(tty_dev))) {
- device_init_wakeup(tty_dev, 1);
- device_set_wakeup_enable(tty_dev, 0);
- } else
+ device_init_wakeup_flag(tty_dev, PM_WAKEUP_CAP);
+ } else {
printk(KERN_ERR "Cannot register tty device on line %d\n",
uport->line);
+ }
/*
* Ensure UPF_DEAD is not set.
--
1.7.7.3
^ permalink raw reply related
* [PATCH 1/2] power: Add function to init wakeup capability without enabling
From: Simon Glass @ 2012-01-18 23:07 UTC (permalink / raw)
To: LKML
Cc: Greg Kroah-Hartman, linux-serial, Pavel Machek, Rafael J. Wysocki,
Alan Cox, Paul E. McKenney, Simon Glass
In-Reply-To: <1326826563-32215-3-git-send-email-sjg@chromium.org>
device_init_wakeup() offers two options:
1. Wakeup capable and wakeup enabled
2 Not wakeup capable and not wakeup enabled
serial_core wants a third option:
3. Wakeup capable but not wakeup enabled (yet)
Add a new init routine which takes a flag indicating which of the three
options is required. This avoids creating and then destroying the wakeup
source to no purpose, and a sometimes large rcu_synchronise() delay.
Q: What is a good name for the new function?
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/base/power/wakeup.c | 26 ++++++++++++++++++++++----
include/linux/pm_wakeup.h | 22 +++++++++++++++++++---
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index caf995f..ae3b3c5 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -273,7 +273,7 @@ EXPORT_SYMBOL_GPL(device_set_wakeup_capable);
/**
* device_init_wakeup - Device wakeup initialization.
* @dev: Device to handle.
- * @enable: Whether or not to enable @dev as a wakeup device.
+ * @flags: flags PM_WAKEUP_...
*
* By default, most devices should leave wakeup disabled. The exceptions are
* devices that everyone expects to be wakeup sources: keyboards, power buttons,
@@ -281,19 +281,37 @@ EXPORT_SYMBOL_GPL(device_set_wakeup_capable);
* own wakeup requests but merely forward requests from one bus to another
* (like PCI bridges) should have wakeup enabled by default.
*/
-int device_init_wakeup(struct device *dev, bool enable)
+int device_init_wakeup_flag(struct device *dev, int flags)
{
int ret = 0;
- if (enable) {
+ if (flags & PM_WAKEUP_CAP) {
device_set_wakeup_capable(dev, true);
- ret = device_wakeup_enable(dev);
+ if (flags & PM_WAKEUP_EN)
+ ret = device_wakeup_enable(dev);
} else {
+ WARN_ON(flags & PM_WAKEUP_EN);
device_set_wakeup_capable(dev, false);
}
return ret;
}
+EXPORT_SYMBOL_GPL(device_init_wakeup_flag);
+
+/**
+ * device_init_wakeup - Device wakeup initialization.
+ * @dev: Device to handle.
+ * @enable: Whether or not to enable @dev as a wakeup device.
+ *
+ * By default, most devices should leave wakeup disabled. The exceptions are
+ * devices that everyone expects to be wakeup sources: keyboards, power buttons,
+ * possibly network interfaces, etc.
+ */
+int device_init_wakeup(struct device *dev, bool enable)
+{
+ return device_init_wakeup_flag(dev, enable ? PM_WAKEUP_CAP_EN :
+ PM_WAKEUP_NONE);
+}
EXPORT_SYMBOL_GPL(device_init_wakeup);
/**
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index a32da96..3035875 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -56,6 +56,14 @@ struct wakeup_source {
unsigned int active:1;
};
+/* Flags for device_init_wakeup_flag() */
+enum {
+ PM_WAKEUP_NONE, /* Not wakeup capable */
+ PM_WAKEUP_CAP, /* Wakeup capable but not enabled */
+ PM_WAKEUP_EN, /* Don't use */
+ PM_WAKEUP_CAP_EN, /* Wakeup capable and enabled */
+};
+
#ifdef CONFIG_PM_SLEEP
/*
@@ -83,6 +91,7 @@ extern int device_wakeup_enable(struct device *dev);
extern int device_wakeup_disable(struct device *dev);
extern void device_set_wakeup_capable(struct device *dev, bool capable);
extern int device_init_wakeup(struct device *dev, bool val);
+extern int device_init_wakeup_flag(struct device *dev, int flags);
extern int device_set_wakeup_enable(struct device *dev, bool enable);
extern void __pm_stay_awake(struct wakeup_source *ws);
extern void pm_stay_awake(struct device *dev);
@@ -139,13 +148,20 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable)
return 0;
}
-static inline int device_init_wakeup(struct device *dev, bool val)
+static inline device_init_wakeup_flag(struct device *dev, int flags)
{
- device_set_wakeup_capable(dev, val);
- device_set_wakeup_enable(dev, val);
+ device_set_wakeup_capable(dev, flags & PM_WAKEUP_CAP);
+ if (flags & PM_WAKEUP_EN)
+ device_set_wakeup_enable(dev, true);
return 0;
}
+static inline int device_init_wakeup(struct device *dev, bool enable)
+{
+ return device_init_wakeup_flag(enable ? PM_WAKEUP_CAP_EN :
+ PM_WAKEUP_NONE);
+}
+
static inline bool device_may_wakeup(struct device *dev)
{
return dev->power.can_wakeup && dev->power.should_wakeup;
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-18 22:51 UTC (permalink / raw)
To: paulmck; +Cc: Alan Cox, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <20120118224304.GJ2431@linux.vnet.ibm.com>
Hi Paul,
On Wed, Jan 18, 2012 at 2:43 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
>> Hi Paul,
>>
>> On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
>> >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>> >>
>> >> Hi Alan, Paul,
>> >>
>> >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> >> <paulmck@linux.vnet.ibm.com> wrote:
>> >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> >> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> >> >> Simon Glass <sjg@chromium.org> wrote:
>> >> >>
>> >> >> > Since serial_core now does not make serial ports wake-up capable by
>> >> >> > default, add a parameter to support this feature in the 8250 UART.
>> >> >> > This is the only UART where I think this feature is useful.
>> >> >>
>> >> >> NAK
>> >> >>
>> >> >> Things should just work for users. Magic parameters is not an
>> >> >> improvement. If its a performance problem someone needs to fix the rcu
>> >> >> sync overhead or stop using rcu on that path.
>> >>
>> >> OK fair enough, I agree. Every level I move down the source tree
>> >> affects more people though.
>> >>
>> >> >
>> >> > I must say that I lack context here, even after looking at the patch,
>> >> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> >> > of synchronize_rcu() is too large.
>> >> >
>> >>
>> >> Let me provide a bit of context. The serial_core code seems to be the
>> >> only place in the kernel that does this:
>> >>
>> >> device_init_wakeup(tty_dev, 1);
>> >> device_set_wakeup_enable(tty_dev, 0);
>> >>
>> >> The first call makes the device wakeup capable and enables wakeup, The
>> >> second call disabled wakeup.
>> >>
>> >> The code that removes the wakeup source looks like this:
>> >>
>> >> void wakeup_source_remove(struct wakeup_source *ws)
>> >> {
>> >> if (WARN_ON(!ws))
>> >> return;
>> >>
>> >> spin_lock_irq(&events_lock);
>> >> list_del_rcu(&ws->entry);
>> >> spin_unlock_irq(&events_lock);
>> >> synchronize_rcu();
>> >> }
>> >>
>> >> The sync is there because we are about to destroy the actual ws
>> >> structure (in wakeup_source_destroy()). I wonder if it should be in
>> >> wakeup_source_destroy() but that wouldn't help me anyway.
>> >>
>> >> synchronize_rcu_expedited() is a bit faster but not really fast
>> >> enough. Anyway surely people will complain if I put this in the wakeup
>> >> code - it will affect all wakeup users. It seems to me that the right
>> >> solution is to avoid enabling and then immediately disabling wakeup.
>> >
>> > Hmmm... What hardware are you running this one? Normally,
>> > synchronize_rcu_expedited() will be a couple of orders of magnitude
>> > faster than synchronize_rcu().
>> >
>> >> I assume we can't and shouldn't change device_init_wakeup() . We could
>> >> add a call like device_init_wakeup_disabled() which makes the device
>> >> wakeup capable but does not actually enable it. Does that work?
>> >
>> > If the only reason for the synchronize_rcu() is to defer the pair of
>> > kfree()s in wakeup_source_destroy(), then another possible approach
>> > would be to remove the synchronize_rcu() from wakeup_source_remove()
>> > and then use call_rcu() to defer the two kfree()s.
>> >
>> > If this is a reasonable change to make, the approach is as follows:
>> >
>> > 1. Add a struct rcu_head to wakeup_source, call it "rcu".
>> > Or adjust the following to suit your choice of name.
>> >
>> > 2. Replace the pair of kfree()s with:
>> >
>> > call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
>> >
>> > 3. Create the wakeup_source_destroy_rcu() as follows:
>> >
>> > static void wakeup_source_destroy_rcu(struct rcu_head *head)
>> > {
>> > struct wakeup_source *ws =
>> > container_of(head, struct wakeup_source, rcu);
>> >
>> > kfree(ws->name);
>> > kfree(ws);
>> > }
>> >
>> > Of course, this assumes that it is OK for wakeup_source_unregister()
>> > to return before the memory is freed up. This often is OK, but there
>> > are some cases where the caller requires that there be no further
>> > RCU readers with access to the old data. In these cases, you really
>> > do need the wait.
>>
>> Thanks very much for that. I'm not sure if it is a reasonable change,
>> but it does bug me that we add it to a data structure knowing that we
>> will immediately remove it!
>>
>> >From what I can see, making a device wakeup-enabled mostly happens on
>> init or in response to a request to the driver (presumably from user
>> space). In the latter case I suspect the synchronise_rcu() is fine. In
>> the former it feels like we should make up our minds which of the
>> three options is required (incapable, capable but not enabled, capable
>> and enabled).
>>
>> I will try a patch first based on splitting the two options (capable
>> and enable) and see if that get a NAK.
>>
>> Then I will come back to your solution - it seems fine to me and not a
>> lot of code. Do we have to worry about someone enabling, disabled,
>> enabling and then disabling wakeup quickly? Will this method break in
>> that case if the second call to call_rcu() uses the same wc->rcu?
>
> There are a couple of questions here, let me take them one at a time:
>
> 1. If you just disabled, can you immediately re-enable?
>
> The answer is "yes". The reason that this works is that you
> allocate a new structure for the re-enabling, and that new
> structure has its own rcu_head field.
>
> 2. If you repeatedly disable and re-enable in a tight loop,
> can this cause problems?
>
> The answer to this is also "yes" -- you can run the system
> out of memory doing that. However, there are a number of
> simple ways to avoid this problem:
>
> a. Do a synchronize_rcu() on every (say) thousandth
> disable operation.
>
> b. As above, but only do the synchronize_rcu() if
> all 1,000 disable operations occurred within
> (say) a second of each other.
>
> c. As above, but actually count the number of
> pending call_rcu() callbacks.
>
> Both (a) and (b) can be carried out on a per-CPU basis if there
> is no convenient locked structure in which to track the state.
> You cannot carry (c) out on a per-CPU basis because RCU callbacks
> can sometimes be invoked on a different CPU from the one that
> call_rcu()ed them. Rare, but it can happen.
>
> I would expect that option (a) would work in almost all cases.
>
> If this can be exercised freely from user space, then you probably
> really do need #2 above.
OK I see, thank you. It does sound a bit complicated although the
chances of anyone actually doing this are probably remote.
I will send my patch to avoid getting into this situation and see what
you think.
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-18 22:43 UTC (permalink / raw)
To: Simon Glass
Cc: Alan Cox, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <CAPnjgZ1g0Gf8ZCavAbmxjJXxxk9duf1ybbEUNo9L-DHBxyXx-g@mail.gmail.com>
On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> Hi Paul,
>
> On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> >>
> >> Hi Alan, Paul,
> >>
> >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> >> Simon Glass <sjg@chromium.org> wrote:
> >> >>
> >> >> > Since serial_core now does not make serial ports wake-up capable by
> >> >> > default, add a parameter to support this feature in the 8250 UART.
> >> >> > This is the only UART where I think this feature is useful.
> >> >>
> >> >> NAK
> >> >>
> >> >> Things should just work for users. Magic parameters is not an
> >> >> improvement. If its a performance problem someone needs to fix the rcu
> >> >> sync overhead or stop using rcu on that path.
> >>
> >> OK fair enough, I agree. Every level I move down the source tree
> >> affects more people though.
> >>
> >> >
> >> > I must say that I lack context here, even after looking at the patch,
> >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> >> > of synchronize_rcu() is too large.
> >> >
> >>
> >> Let me provide a bit of context. The serial_core code seems to be the
> >> only place in the kernel that does this:
> >>
> >> device_init_wakeup(tty_dev, 1);
> >> device_set_wakeup_enable(tty_dev, 0);
> >>
> >> The first call makes the device wakeup capable and enables wakeup, The
> >> second call disabled wakeup.
> >>
> >> The code that removes the wakeup source looks like this:
> >>
> >> void wakeup_source_remove(struct wakeup_source *ws)
> >> {
> >> if (WARN_ON(!ws))
> >> return;
> >>
> >> spin_lock_irq(&events_lock);
> >> list_del_rcu(&ws->entry);
> >> spin_unlock_irq(&events_lock);
> >> synchronize_rcu();
> >> }
> >>
> >> The sync is there because we are about to destroy the actual ws
> >> structure (in wakeup_source_destroy()). I wonder if it should be in
> >> wakeup_source_destroy() but that wouldn't help me anyway.
> >>
> >> synchronize_rcu_expedited() is a bit faster but not really fast
> >> enough. Anyway surely people will complain if I put this in the wakeup
> >> code - it will affect all wakeup users. It seems to me that the right
> >> solution is to avoid enabling and then immediately disabling wakeup.
> >
> > Hmmm... What hardware are you running this one? Normally,
> > synchronize_rcu_expedited() will be a couple of orders of magnitude
> > faster than synchronize_rcu().
> >
> >> I assume we can't and shouldn't change device_init_wakeup() . We could
> >> add a call like device_init_wakeup_disabled() which makes the device
> >> wakeup capable but does not actually enable it. Does that work?
> >
> > If the only reason for the synchronize_rcu() is to defer the pair of
> > kfree()s in wakeup_source_destroy(), then another possible approach
> > would be to remove the synchronize_rcu() from wakeup_source_remove()
> > and then use call_rcu() to defer the two kfree()s.
> >
> > If this is a reasonable change to make, the approach is as follows:
> >
> > 1. Add a struct rcu_head to wakeup_source, call it "rcu".
> > Or adjust the following to suit your choice of name.
> >
> > 2. Replace the pair of kfree()s with:
> >
> > call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> >
> > 3. Create the wakeup_source_destroy_rcu() as follows:
> >
> > static void wakeup_source_destroy_rcu(struct rcu_head *head)
> > {
> > struct wakeup_source *ws =
> > container_of(head, struct wakeup_source, rcu);
> >
> > kfree(ws->name);
> > kfree(ws);
> > }
> >
> > Of course, this assumes that it is OK for wakeup_source_unregister()
> > to return before the memory is freed up. This often is OK, but there
> > are some cases where the caller requires that there be no further
> > RCU readers with access to the old data. In these cases, you really
> > do need the wait.
>
> Thanks very much for that. I'm not sure if it is a reasonable change,
> but it does bug me that we add it to a data structure knowing that we
> will immediately remove it!
>
> >From what I can see, making a device wakeup-enabled mostly happens on
> init or in response to a request to the driver (presumably from user
> space). In the latter case I suspect the synchronise_rcu() is fine. In
> the former it feels like we should make up our minds which of the
> three options is required (incapable, capable but not enabled, capable
> and enabled).
>
> I will try a patch first based on splitting the two options (capable
> and enable) and see if that get a NAK.
>
> Then I will come back to your solution - it seems fine to me and not a
> lot of code. Do we have to worry about someone enabling, disabled,
> enabling and then disabling wakeup quickly? Will this method break in
> that case if the second call to call_rcu() uses the same wc->rcu?
There are a couple of questions here, let me take them one at a time:
1. If you just disabled, can you immediately re-enable?
The answer is "yes". The reason that this works is that you
allocate a new structure for the re-enabling, and that new
structure has its own rcu_head field.
2. If you repeatedly disable and re-enable in a tight loop,
can this cause problems?
The answer to this is also "yes" -- you can run the system
out of memory doing that. However, there are a number of
simple ways to avoid this problem:
a. Do a synchronize_rcu() on every (say) thousandth
disable operation.
b. As above, but only do the synchronize_rcu() if
all 1,000 disable operations occurred within
(say) a second of each other.
c. As above, but actually count the number of
pending call_rcu() callbacks.
Both (a) and (b) can be carried out on a per-CPU basis if there
is no convenient locked structure in which to track the state.
You cannot carry (c) out on a per-CPU basis because RCU callbacks
can sometimes be invoked on a different CPU from the one that
call_rcu()ed them. Rare, but it can happen.
I would expect that option (a) would work in almost all cases.
If this can be exercised freely from user space, then you probably
really do need #2 above.
Thanx, Paul
> Regards,
> Simon
> >
> > Thanx, Paul
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-18 22:19 UTC (permalink / raw)
To: Alan Cox; +Cc: paulmck, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <20120118221255.5c14f382@pyramind.ukuu.org.uk>
Hi Alan,
On Wed, Jan 18, 2012 at 2:12 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Let me provide a bit of context. The serial_core code seems to be the
>> only place in the kernel that does this:
>>
>> device_init_wakeup(tty_dev, 1);
>> device_set_wakeup_enable(tty_dev, 0);
>>
>> The first call makes the device wakeup capable and enables wakeup, The
>> second call disabled wakeup.
>
>> I assume we can't and shouldn't change device_init_wakeup() . We could
>> add a call like device_init_wakeup_disabled() which makes the device
>> wakeup capable but does not actually enable it. Does that work?
>
> Is that not
>
> device_init_wakeup(tty_dev, 0)
>
> or am I missing something ?
Bearing in mind that I know very little about this, I think
serial_core wants to have the device wakeup capable in any case, but
not actually enable wakeup on the device until requested. It seems
that drivers rely on that behaviour too.
device_init_wakeup(tty_dev, 0) calls device_set_wakeup_capable(dev,
false) which I think is not what we want.
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-18 22:15 UTC (permalink / raw)
To: paulmck; +Cc: Alan Cox, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <20120118214215.GH2431@linux.vnet.ibm.com>
Hi Paul,
On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
>> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>>
>> Hi Alan, Paul,
>>
>> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> >> Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >> > Since serial_core now does not make serial ports wake-up capable by
>> >> > default, add a parameter to support this feature in the 8250 UART.
>> >> > This is the only UART where I think this feature is useful.
>> >>
>> >> NAK
>> >>
>> >> Things should just work for users. Magic parameters is not an
>> >> improvement. If its a performance problem someone needs to fix the rcu
>> >> sync overhead or stop using rcu on that path.
>>
>> OK fair enough, I agree. Every level I move down the source tree
>> affects more people though.
>>
>> >
>> > I must say that I lack context here, even after looking at the patch,
>> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> > of synchronize_rcu() is too large.
>> >
>>
>> Let me provide a bit of context. The serial_core code seems to be the
>> only place in the kernel that does this:
>>
>> device_init_wakeup(tty_dev, 1);
>> device_set_wakeup_enable(tty_dev, 0);
>>
>> The first call makes the device wakeup capable and enables wakeup, The
>> second call disabled wakeup.
>>
>> The code that removes the wakeup source looks like this:
>>
>> void wakeup_source_remove(struct wakeup_source *ws)
>> {
>> if (WARN_ON(!ws))
>> return;
>>
>> spin_lock_irq(&events_lock);
>> list_del_rcu(&ws->entry);
>> spin_unlock_irq(&events_lock);
>> synchronize_rcu();
>> }
>>
>> The sync is there because we are about to destroy the actual ws
>> structure (in wakeup_source_destroy()). I wonder if it should be in
>> wakeup_source_destroy() but that wouldn't help me anyway.
>>
>> synchronize_rcu_expedited() is a bit faster but not really fast
>> enough. Anyway surely people will complain if I put this in the wakeup
>> code - it will affect all wakeup users. It seems to me that the right
>> solution is to avoid enabling and then immediately disabling wakeup.
>
> Hmmm... What hardware are you running this one? Normally,
> synchronize_rcu_expedited() will be a couple of orders of magnitude
> faster than synchronize_rcu().
>
>> I assume we can't and shouldn't change device_init_wakeup() . We could
>> add a call like device_init_wakeup_disabled() which makes the device
>> wakeup capable but does not actually enable it. Does that work?
>
> If the only reason for the synchronize_rcu() is to defer the pair of
> kfree()s in wakeup_source_destroy(), then another possible approach
> would be to remove the synchronize_rcu() from wakeup_source_remove()
> and then use call_rcu() to defer the two kfree()s.
>
> If this is a reasonable change to make, the approach is as follows:
>
> 1. Add a struct rcu_head to wakeup_source, call it "rcu".
> Or adjust the following to suit your choice of name.
>
> 2. Replace the pair of kfree()s with:
>
> call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
>
> 3. Create the wakeup_source_destroy_rcu() as follows:
>
> static void wakeup_source_destroy_rcu(struct rcu_head *head)
> {
> struct wakeup_source *ws =
> container_of(head, struct wakeup_source, rcu);
>
> kfree(ws->name);
> kfree(ws);
> }
>
> Of course, this assumes that it is OK for wakeup_source_unregister()
> to return before the memory is freed up. This often is OK, but there
> are some cases where the caller requires that there be no further
> RCU readers with access to the old data. In these cases, you really
> do need the wait.
Thanks very much for that. I'm not sure if it is a reasonable change,
but it does bug me that we add it to a data structure knowing that we
will immediately remove it!
From what I can see, making a device wakeup-enabled mostly happens on
init or in response to a request to the driver (presumably from user
space). In the latter case I suspect the synchronise_rcu() is fine. In
the former it feels like we should make up our minds which of the
three options is required (incapable, capable but not enabled, capable
and enabled).
I will try a patch first based on splitting the two options (capable
and enable) and see if that get a NAK.
Then I will come back to your solution - it seems fine to me and not a
lot of code. Do we have to worry about someone enabling, disabled,
enabling and then disabling wakeup quickly? Will this method break in
that case if the second call to call_rcu() uses the same wc->rcu?
Regards,
Simon
>
> Thanx, Paul
>
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Alan Cox @ 2012-01-18 22:12 UTC (permalink / raw)
To: Simon Glass
Cc: paulmck, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <CAPnjgZ1GJatR15dVsq-+hAWhD7eaLhtN737QvBaor_=0+K+6wA@mail.gmail.com>
> Let me provide a bit of context. The serial_core code seems to be the
> only place in the kernel that does this:
>
> device_init_wakeup(tty_dev, 1);
> device_set_wakeup_enable(tty_dev, 0);
>
> The first call makes the device wakeup capable and enables wakeup, The
> second call disabled wakeup.
> I assume we can't and shouldn't change device_init_wakeup() . We could
> add a call like device_init_wakeup_disabled() which makes the device
> wakeup capable but does not actually enable it. Does that work?
Is that not
device_init_wakeup(tty_dev, 0)
or am I missing something ?
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-18 21:42 UTC (permalink / raw)
To: Simon Glass
Cc: Alan Cox, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <CAPnjgZ1GJatR15dVsq-+hAWhD7eaLhtN737QvBaor_=0+K+6wA@mail.gmail.com>
On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>
> Hi Alan, Paul,
>
> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> Simon Glass <sjg@chromium.org> wrote:
> >>
> >> > Since serial_core now does not make serial ports wake-up capable by
> >> > default, add a parameter to support this feature in the 8250 UART.
> >> > This is the only UART where I think this feature is useful.
> >>
> >> NAK
> >>
> >> Things should just work for users. Magic parameters is not an
> >> improvement. If its a performance problem someone needs to fix the rcu
> >> sync overhead or stop using rcu on that path.
>
> OK fair enough, I agree. Every level I move down the source tree
> affects more people though.
>
> >
> > I must say that I lack context here, even after looking at the patch,
> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > of synchronize_rcu() is too large.
> >
>
> Let me provide a bit of context. The serial_core code seems to be the
> only place in the kernel that does this:
>
> device_init_wakeup(tty_dev, 1);
> device_set_wakeup_enable(tty_dev, 0);
>
> The first call makes the device wakeup capable and enables wakeup, The
> second call disabled wakeup.
>
> The code that removes the wakeup source looks like this:
>
> void wakeup_source_remove(struct wakeup_source *ws)
> {
> if (WARN_ON(!ws))
> return;
>
> spin_lock_irq(&events_lock);
> list_del_rcu(&ws->entry);
> spin_unlock_irq(&events_lock);
> synchronize_rcu();
> }
>
> The sync is there because we are about to destroy the actual ws
> structure (in wakeup_source_destroy()). I wonder if it should be in
> wakeup_source_destroy() but that wouldn't help me anyway.
>
> synchronize_rcu_expedited() is a bit faster but not really fast
> enough. Anyway surely people will complain if I put this in the wakeup
> code - it will affect all wakeup users. It seems to me that the right
> solution is to avoid enabling and then immediately disabling wakeup.
Hmmm... What hardware are you running this one? Normally,
synchronize_rcu_expedited() will be a couple of orders of magnitude
faster than synchronize_rcu().
> I assume we can't and shouldn't change device_init_wakeup() . We could
> add a call like device_init_wakeup_disabled() which makes the device
> wakeup capable but does not actually enable it. Does that work?
If the only reason for the synchronize_rcu() is to defer the pair of
kfree()s in wakeup_source_destroy(), then another possible approach
would be to remove the synchronize_rcu() from wakeup_source_remove()
and then use call_rcu() to defer the two kfree()s.
If this is a reasonable change to make, the approach is as follows:
1. Add a struct rcu_head to wakeup_source, call it "rcu".
Or adjust the following to suit your choice of name.
2. Replace the pair of kfree()s with:
call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
3. Create the wakeup_source_destroy_rcu() as follows:
static void wakeup_source_destroy_rcu(struct rcu_head *head)
{
struct wakeup_source *ws =
container_of(head, struct wakeup_source, rcu);
kfree(ws->name);
kfree(ws);
}
Of course, this assumes that it is OK for wakeup_source_unregister()
to return before the memory is freed up. This often is OK, but there
are some cases where the caller requires that there be no further
RCU readers with access to the old data. In these cases, you really
do need the wait.
Thanx, Paul
> Regards,
> Simon
>
> > Thanx, Paul
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-18 21:08 UTC (permalink / raw)
To: paulmck; +Cc: Alan Cox, LKML, Greg Kroah-Hartman, linux-serial,
Rafael J. Wysocki
In-Reply-To: <20120118041720.GA2431@linux.vnet.ibm.com>
[+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
Hi Alan, Paul,
On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> On Tue, 17 Jan 2012 10:56:03 -0800
>> Simon Glass <sjg@chromium.org> wrote:
>>
>> > Since serial_core now does not make serial ports wake-up capable by
>> > default, add a parameter to support this feature in the 8250 UART.
>> > This is the only UART where I think this feature is useful.
>>
>> NAK
>>
>> Things should just work for users. Magic parameters is not an
>> improvement. If its a performance problem someone needs to fix the rcu
>> sync overhead or stop using rcu on that path.
OK fair enough, I agree. Every level I move down the source tree
affects more people though.
>
> I must say that I lack context here, even after looking at the patch,
> but the synchronize_rcu_expedited() primitives can be used if the latency
> of synchronize_rcu() is too large.
>
Let me provide a bit of context. The serial_core code seems to be the
only place in the kernel that does this:
device_init_wakeup(tty_dev, 1);
device_set_wakeup_enable(tty_dev, 0);
The first call makes the device wakeup capable and enables wakeup, The
second call disabled wakeup.
The code that removes the wakeup source looks like this:
void wakeup_source_remove(struct wakeup_source *ws)
{
if (WARN_ON(!ws))
return;
spin_lock_irq(&events_lock);
list_del_rcu(&ws->entry);
spin_unlock_irq(&events_lock);
synchronize_rcu();
}
The sync is there because we are about to destroy the actual ws
structure (in wakeup_source_destroy()). I wonder if it should be in
wakeup_source_destroy() but that wouldn't help me anyway.
synchronize_rcu_expedited() is a bit faster but not really fast
enough. Anyway surely people will complain if I put this in the wakeup
code - it will affect all wakeup users. It seems to me that the right
solution is to avoid enabling and then immediately disabling wakeup.
I assume we can't and shouldn't change device_init_wakeup() . We could
add a call like device_init_wakeup_disabled() which makes the device
wakeup capable but does not actually enable it. Does that work?
Regards,
Simon
> Thanx, Paul
>
--
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] amba-pl011: clear previous interrupts before request_irq
From: Alan Cox @ 2012-01-18 11:41 UTC (permalink / raw)
To: Shreshtha Kumar SAHU
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
rmk+kernel@arm.linux.org.uk, linux-kernel@vger.kernel.org
In-Reply-To: <20120118100410.GB889@bnru02.bnr.st.com>
> /*
> + * Clear previous interrupts before installing interrupt handler
> + */
> + spin_lock_irq(&uap->port.lock);
> + uap->im = 0;
> + writew(uap->im, uap->port.membase + UART011_IMSC);
> + writew(0xffff, uap->port.membase + UART011_ICR);
> + spin_unlock_irq(&uap->port.lock);
I'm confused why you use spin_lock_irq before you've enabled the IRQ in
the first place but looks fine.
Alan
^ permalink raw reply
* Re: [PATCH] amba-pl011: do not disable RTS during shutdown
From: Shreshtha Kumar SAHU @ 2012-01-18 10:23 UTC (permalink / raw)
To: Russell King
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120118101013.GF22472@flint.arm.linux.org.uk>
>From 1b9b0de2d7fd099d775e03065ad96414e7c4ed60 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCH v4] amba-pl011: do not disable RTS during shutdown
In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.
Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..d155e45 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -159,6 +159,7 @@ struct uart_amba_port {
unsigned int fifosize; /* vendor-specific */
unsigned int lcrh_tx; /* vendor-specific */
unsigned int lcrh_rx; /* vendor-specific */
+ unsigned int old_cr; /* state during shutdown */
bool autorts;
char type[12];
bool interrupt_may_hang; /* vendor-specific */
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
barrier();
- cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+ /* restore RTS and DTR */
+ cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
+ cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
static void pl011_shutdown(struct uart_port *port)
{
struct uart_amba_port *uap = (struct uart_amba_port *)port;
+ unsigned int cr;
/*
* disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
/*
* disable the port
+ * disable the port. It should not disable RTS and DTR.
+ * Also RTS and DTR state should be preserved to restore
+ * it during startup().
*/
uap->autorts = false;
- writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+ cr = readw(uap->port.membase + UART011_CR);
+ uap->old_cr = cr;
+ cr &= UART011_CR_RTS | UART011_CR_DTR;
+ cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+ writew(cr, uap->port.membase + UART011_CR);
/*
* disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
uap->vendor = vendor;
uap->lcrh_rx = vendor->lcrh_rx;
uap->lcrh_tx = vendor->lcrh_tx;
+ uap->old_cr = 0;
uap->fifosize = vendor->fifosize;
uap->interrupt_may_hang = vendor->interrupt_may_hang;
uap->port.dev = &dev->dev;
--
1.7.4.3
^ permalink raw reply related
* [PATCH 5/7] serial: sh-sci: prepare for conversion to simple DMA
From: Guennadi Liakhovetski @ 2012-01-18 10:22 UTC (permalink / raw)
To: linux-kernel
Cc: linux-sh, Vinod Koul, Magnus Damm, Shimoda, Yoshihiro,
linux-serial
In-Reply-To: <Pine.LNX.4.64.1201181025280.28782@axis700.grange>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/tty/serial/sh-sci.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index aff9d61..2a8ffda 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1463,9 +1463,9 @@ static bool filter(struct dma_chan *chan, void *slave)
struct sh_dmae_slave *param = slave;
dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
- param->slave_id);
+ param->simple_slave.slave_id);
- chan->private = param;
+ chan->private = ¶m->simple_slave;
return true;
}
@@ -1504,7 +1504,7 @@ static void sci_request_dma(struct uart_port *port)
param = &s->param_tx;
/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_TX */
- param->slave_id = s->cfg->dma_slave_tx;
+ param->simple_slave.slave_id = s->cfg->dma_slave_tx;
s->cookie_tx = -EINVAL;
chan = dma_request_channel(mask, filter, param);
@@ -1532,7 +1532,7 @@ static void sci_request_dma(struct uart_port *port)
param = &s->param_rx;
/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_RX */
- param->slave_id = s->cfg->dma_slave_rx;
+ param->simple_slave.slave_id = s->cfg->dma_slave_rx;
chan = dma_request_channel(mask, filter, param);
dev_dbg(port->dev, "%s: RX: got channel %p\n", __func__, chan);
--
1.7.2.5
^ permalink raw reply related
* [PATCH 0/7] extract a simple dmaengine library from shdma.c
From: Guennadi Liakhovetski @ 2012-01-18 10:22 UTC (permalink / raw)
To: linux-kernel
Cc: alsa-devel, linux-sh, Vinod Koul, Magnus Damm, Shimoda, Yoshihiro,
linux-mmc, linux-serial
This patch series has been triggered by a recent series by Shimoda-san
http://www.spinics.net/lists/linux-sh/index.html#09895
In a follow up to that thread it has been decided to extract a common
library from the current shdma driver to be re-used by other dmaengine
drivers. Primarily this library provides a flexible transfer descriptor
management for hardware, that does not dupport scatter-gather lists
natively. In such cases this library can be used to split transfer
requests into smaller chunks, up to the size, supported by the specific
hardware, enter them onto a linked list, thack their completion and call
user callbacks. This is a first shot, aimed to be used by the SUDMAC
driver. Once it is confirmed, that this version can be conveniently used
with it, I'll do some cosmetic clean up, notably, add / improve
documentation:-)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] amba-pl011: clear previous interrupts before request_irq
From: Russell King @ 2012-01-18 10:15 UTC (permalink / raw)
To: Shreshtha Kumar SAHU
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120118100410.GB889@bnru02.bnr.st.com>
On Wed, Jan 18, 2012 at 03:34:11PM +0530, Shreshtha Kumar SAHU wrote:
>
> >From 0742fc49405b2a6f562981f61d93198e1595f30d Mon Sep 17 00:00:00 2001
> From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> Date: Tue, 17 Jan 2012 12:23:19 +0530
> Subject: [PATCH v2] amba-pl011: clear previous interrupts before request_irq
>
> All previous interrupts should be cleared before installing
> interrupt handler i.e. before request_irq. pl011_shutdown
> clears the interrupt register but there may be case where
> bootloader transfers control to kernel and there are some
> pending interrupts. In this case interrupt handler will get
> called even before interrupt mask is enabled.
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
No sign-off, and I don't understand the problem being aluded to in the
description above - particularly the last sentence.
In theory, we should be prepared for the interrupt handler to be called
immediately after request_irq() returns, and not have the driver misbehave
because of that. Maybe a better solution would be to move request_irq()
a bit later?
But first, having a clearer description of the problem you're seeing
would be beneficial.
> ---
> drivers/tty/serial/amba-pl011.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 6958594..6dafaa2 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1381,6 +1381,15 @@ static int pl011_startup(struct uart_port *port)
> uap->port.uartclk = clk_get_rate(uap->clk);
>
> /*
> + * Clear previous interrupts before installing interrupt handler
> + */
> + spin_lock_irq(&uap->port.lock);
> + uap->im = 0;
> + writew(uap->im, uap->port.membase + UART011_IMSC);
> + writew(0xffff, uap->port.membase + UART011_ICR);
> + spin_unlock_irq(&uap->port.lock);
> +
> + /*
> * Allocate the IRQ
> */
> retval = request_irq(uap->port.irq, pl011_int, 0, "uart-pl011", uap);
> --
> 1.7.4.3
>
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply
* Re: [PATCH] amba-pl011: do not disable RTS during shutdown
From: Russell King @ 2012-01-18 10:10 UTC (permalink / raw)
To: Shreshtha Kumar SAHU
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120118094505.GA889@bnru02.bnr.st.com>
On Wed, Jan 18, 2012 at 03:15:06PM +0530, Shreshtha Kumar SAHU wrote:
> data type of old_cr was bool in patchv2, corrected in this patchv3
This is much better, just a small little niggle with it.
> @@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
> while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
> barrier();
>
> - cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> + /* restore RTS and DTR */
> + cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));
You don't need the outer set of parens around this (additional
unnecessary parens can cause confusion when trying to read code.)
> @@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
>
> /*
> * disable the port
> + * disable the port. It should not disable RTS and DTR.
> + * Also RTS and DTR state should be preserved to restore
> + * it during startup().
> */
> uap->autorts = false;
> - writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
> + cr = readw(uap->port.membase + UART011_CR);
> + uap->old_cr = cr;
> + cr &= (UART011_CR_RTS | UART011_CR_DTR);
Ditto.
Once those are fixed:
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply
* Re: [PATCH] amba-pl011: clear previous interrupts before request_irq
From: Shreshtha Kumar SAHU @ 2012-01-18 10:04 UTC (permalink / raw)
To: gregkh@suse.de, linux-serial@vger.kernel.org,
rmk+kernel@arm.linux.org.uk
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <1326796493-16080-1-git-send-email-shreshthakumar.sahu@stericsson.com>
>From 0742fc49405b2a6f562981f61d93198e1595f30d Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 12:23:19 +0530
Subject: [PATCH v2] amba-pl011: clear previous interrupts before request_irq
All previous interrupts should be cleared before installing
interrupt handler i.e. before request_irq. pl011_shutdown
clears the interrupt register but there may be case where
bootloader transfers control to kernel and there are some
pending interrupts. In this case interrupt handler will get
called even before interrupt mask is enabled.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..6dafaa2 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1381,6 +1381,15 @@ static int pl011_startup(struct uart_port *port)
uap->port.uartclk = clk_get_rate(uap->clk);
/*
+ * Clear previous interrupts before installing interrupt handler
+ */
+ spin_lock_irq(&uap->port.lock);
+ uap->im = 0;
+ writew(uap->im, uap->port.membase + UART011_IMSC);
+ writew(0xffff, uap->port.membase + UART011_ICR);
+ spin_unlock_irq(&uap->port.lock);
+
+ /*
* Allocate the IRQ
*/
retval = request_irq(uap->port.irq, pl011_int, 0, "uart-pl011", uap);
--
1.7.4.3
^ permalink raw reply related
* Re: [PATCH] amba-pl011: do not disable RTS during shutdown
From: Shreshtha Kumar SAHU @ 2012-01-18 9:45 UTC (permalink / raw)
To: Russell King
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120117184047.GE22472@flint.arm.linux.org.uk>
data type of old_cr was bool in patchv2, corrected in this patchv3
>From 5500b7d25b5382026bc9392a9b44c3c69bf172b0 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCHv3] amba-pl011: do not disable RTS during shutdown
In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.
Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..62ff7da 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -159,6 +159,7 @@ struct uart_amba_port {
unsigned int fifosize; /* vendor-specific */
unsigned int lcrh_tx; /* vendor-specific */
unsigned int lcrh_rx; /* vendor-specific */
+ unsigned int old_cr; /* state during shutdown */
bool autorts;
char type[12];
bool interrupt_may_hang; /* vendor-specific */
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
barrier();
- cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+ /* restore RTS and DTR */
+ cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));
+ cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
static void pl011_shutdown(struct uart_port *port)
{
struct uart_amba_port *uap = (struct uart_amba_port *)port;
+ unsigned int cr;
/*
* disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
/*
* disable the port
+ * disable the port. It should not disable RTS and DTR.
+ * Also RTS and DTR state should be preserved to restore
+ * it during startup().
*/
uap->autorts = false;
- writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+ cr = readw(uap->port.membase + UART011_CR);
+ uap->old_cr = cr;
+ cr &= (UART011_CR_RTS | UART011_CR_DTR);
+ cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+ writew(cr, uap->port.membase + UART011_CR);
/*
* disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
uap->vendor = vendor;
uap->lcrh_rx = vendor->lcrh_rx;
uap->lcrh_tx = vendor->lcrh_tx;
+ uap->old_cr = 0;
uap->fifosize = vendor->fifosize;
uap->interrupt_may_hang = vendor->interrupt_may_hang;
uap->port.dev = &dev->dev;
--
1.7.4.3
^ permalink raw reply related
* Re: [PATCH] amba-pl011: do not disable RTS during shutdown
From: Shreshtha Kumar SAHU @ 2012-01-18 9:36 UTC (permalink / raw)
To: Russell King
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120117184047.GE22472@flint.arm.linux.org.uk>
On Tue, Jan 17, 2012 at 19:40:48 +0100, Russell King wrote:
> On Tue, Jan 17, 2012 at 03:59:49PM +0530, Shreshtha Kumar SAHU wrote:
> > From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> >
> > In present driver, shutdown clears RTS in CR register. But the
> > documentation "Documentation/serial/driver" suggests not to
> > disable RTS in shutdown(). Also RTS is preserved between shutdown
> > and startup calls, i.e. it is restored in startup if it was enabled
> > during shutdown. So that if autorts is set and RTS is set using
> > pl011_set_mctrl then it should continue even after shutdown->startup
> > sequence. And hence during set_termios it will enable RTS only if RTS
> > bit is set in UARTx_CR register. For throttling/unthrottling user
> > should call pl011_set_mctrl.
> >
> > Change-Id: I743f33fb10e7e655657cd5dae1ec585e914a65bc
>
> Please get rid of this line.
>
done.
> > Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > drivers/tty/serial/amba-pl011.c | 17 ++++++++++++++++-
> > 1 files changed, 16 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 6958594..46a4690 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -160,6 +160,7 @@ struct uart_amba_port {
> > unsigned int lcrh_tx; /* vendor-specific */
> > unsigned int lcrh_rx; /* vendor-specific */
> > bool autorts;
> > + bool rts_state; /* state during shutdown */
>
> Do we really need a variable to track this?
>
As register content will be reset during low power state after shutdown.
Hence we have to preserve it during shutdown.
> > char type[12];
> > bool interrupt_may_hang; /* vendor-specific */
> > #ifdef CONFIG_DMA_ENGINE
> > @@ -1412,6 +1413,8 @@ static int pl011_startup(struct uart_port *port)
> > barrier();
> >
> > cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> > + if (uap->rts_state)
> > + cr |= UART011_CR_RTS;
> > writew(cr, uap->port.membase + UART011_CR);
>
> Note that this should preserve DTR as well.
>
In the attached patch, DTR is preserved and restored.
> --
> Russell King
> Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
> maintainer of:
Best Regards,
Shreshtha
>From 2a9062f4c3fd198a64a96779d6e74b324ddb81e4 Mon Sep 17 00:00:00 2001
From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Date: Tue, 17 Jan 2012 11:00:26 +0530
Subject: [PATCHv2] amba-pl011: do not disable RTS during shutdown
In present driver, shutdown clears RTS and DTR in CR register. But the
documentation "Documentation/serial/driver" suggests not to disable
RTS and DTR in shutdown(). Also RTS and DTR is preserved between shutdown
and startup calls, i.e. these are restored in startup if they were enabled
while doing shutdown. So that if RTS and DTR are set using pl011_set_mctrl
then it should continue even after shutdown->startup sequence.
For throttling/unthrottling user should call pl011_set_mctrl.
Signed-off-by: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6958594..673be75 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -160,6 +160,7 @@ struct uart_amba_port {
unsigned int lcrh_tx; /* vendor-specific */
unsigned int lcrh_rx; /* vendor-specific */
bool autorts;
+ bool old_cr; /* state during shutdown */
char type[12];
bool interrupt_may_hang; /* vendor-specific */
#ifdef CONFIG_DMA_ENGINE
@@ -1411,7 +1412,9 @@ static int pl011_startup(struct uart_port *port)
while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
barrier();
- cr = UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+ /* restore RTS and DTR */
+ cr = (uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR));
+ cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
writew(cr, uap->port.membase + UART011_CR);
/* Clear pending error interrupts */
@@ -1469,6 +1472,7 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
static void pl011_shutdown(struct uart_port *port)
{
struct uart_amba_port *uap = (struct uart_amba_port *)port;
+ unsigned int cr;
/*
* disable all interrupts
@@ -1488,9 +1492,16 @@ static void pl011_shutdown(struct uart_port *port)
/*
* disable the port
+ * disable the port. It should not disable RTS and DTR.
+ * Also RTS and DTR state should be preserved to restore
+ * it during startup().
*/
uap->autorts = false;
- writew(UART01x_CR_UARTEN | UART011_CR_TXE, uap->port.membase + UART011_CR);
+ cr = readw(uap->port.membase + UART011_CR);
+ uap->old_cr = cr;
+ cr &= (UART011_CR_RTS | UART011_CR_DTR);
+ cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+ writew(cr, uap->port.membase + UART011_CR);
/*
* disable break condition and fifos
@@ -1905,6 +1916,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
uap->vendor = vendor;
uap->lcrh_rx = vendor->lcrh_rx;
uap->lcrh_tx = vendor->lcrh_tx;
+ uap->old_cr = 0;
uap->fifosize = vendor->fifosize;
uap->interrupt_may_hang = vendor->interrupt_may_hang;
uap->port.dev = &dev->dev;
--
1.7.4.3
^ permalink raw reply related
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-18 4:17 UTC (permalink / raw)
To: Alan Cox; +Cc: Simon Glass, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120117201036.6d99f98f@pyramind.ukuu.org.uk>
On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> On Tue, 17 Jan 2012 10:56:03 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
> > Since serial_core now does not make serial ports wake-up capable by
> > default, add a parameter to support this feature in the 8250 UART.
> > This is the only UART where I think this feature is useful.
>
> NAK
>
> Things should just work for users. Magic parameters is not an
> improvement. If its a performance problem someone needs to fix the rcu
> sync overhead or stop using rcu on that path.
I must say that I lack context here, even after looking at the patch,
but the synchronize_rcu_expedited() primitives can be used if the latency
of synchronize_rcu() is too large.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Alan Cox @ 2012-01-17 20:10 UTC (permalink / raw)
To: Simon Glass; +Cc: LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <1326826563-32215-3-git-send-email-sjg@chromium.org>
On Tue, 17 Jan 2012 10:56:03 -0800
Simon Glass <sjg@chromium.org> wrote:
> Since serial_core now does not make serial ports wake-up capable by
> default, add a parameter to support this feature in the 8250 UART.
> This is the only UART where I think this feature is useful.
NAK
Things should just work for users. Magic parameters is not an
improvement. If its a performance problem someone needs to fix the rcu
sync overhead or stop using rcu on that path.
Alan
^ permalink raw reply
* Re: [PATCH 2/3] serial: Make wakeup_capable a flag to reduce boot time
From: Alan Cox @ 2012-01-17 20:09 UTC (permalink / raw)
To: Simon Glass; +Cc: LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <1326826563-32215-2-git-send-email-sjg@chromium.org>
On Tue, 17 Jan 2012 10:56:02 -0800
Simon Glass <sjg@chromium.org> wrote:
> The synchronize_rcu() call resulting from making every serial driver
> wake-up capable (commit b3b708fa) slows boot down by 600ms on my Tegra2x
> system (with CONFIG_PREEMPT disabled).
>
> Waking up the machine from a serial port seems to be an unlikely
> requirement, so make serial_core default to not using the behaviour.
Its quite normal on x86, and very very common on embedded. Sorry someone
needs to fix the root cause not hack the driver.
The other question would be whether in PC space you can deduce the wake
up situation from ACPI data ? One for Len ?
Alan
^ permalink raw reply
* [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-17 18:56 UTC (permalink / raw)
To: LKML; +Cc: Greg Kroah-Hartman, linux-serial, Simon Glass
In-Reply-To: <1326826563-32215-1-git-send-email-sjg@chromium.org>
Since serial_core now does not make serial ports wake-up capable by
default, add a parameter to support this feature in the 8250 UART.
This is the only UART where I think this feature is useful.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/tty/serial/8250.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 54f8920..78feee4 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -65,6 +65,7 @@ static int serial_index(struct uart_port *port)
}
static unsigned int skip_txen_test; /* force skip of txen test at init time */
+static unsigned int wakeup_capable; /* device can wake up system */
/*
* Debugging.
@@ -2786,6 +2787,7 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
if (up->port.flags & UPF_FIXED_TYPE)
serial8250_init_fixed_type_port(up, up->port.type);
+ up->port.wakeup_capable = wakeup_capable;
uart_add_one_port(drv, &up->port);
}
@@ -3219,6 +3221,7 @@ int serial8250_register_port(struct uart_port *port)
uart->port.set_termios = port->set_termios;
if (port->pm)
uart->port.pm = port->pm;
+ uart->port.wakeup_capable = wakeup_capable;
if (serial8250_isa_config != NULL)
serial8250_isa_config(0, &uart->port,
@@ -3252,6 +3255,7 @@ void serial8250_unregister_port(int line)
uart->port.type = PORT_UNKNOWN;
uart->port.dev = &serial8250_isa_devs->dev;
uart->capabilities = uart_config[uart->port.type].flags;
+ uart->port.wakeup_capable = wakeup_capable;
uart_add_one_port(&serial8250_reg, &uart->port);
} else {
uart->port.dev = NULL;
@@ -3355,3 +3359,6 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
#endif
MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
+
+module_param(wakeup_capable, uint, 0644);
+MODULE_PARM_DESC(wakeup_capable, "Allow driver to wake up the system");
--
1.7.7.3
^ permalink raw reply related
* [PATCH 2/3] serial: Make wakeup_capable a flag to reduce boot time
From: Simon Glass @ 2012-01-17 18:56 UTC (permalink / raw)
To: LKML; +Cc: Greg Kroah-Hartman, linux-serial, Simon Glass
In-Reply-To: <1326826563-32215-1-git-send-email-sjg@chromium.org>
The synchronize_rcu() call resulting from making every serial driver
wake-up capable (commit b3b708fa) slows boot down by 600ms on my Tegra2x
system (with CONFIG_PREEMPT disabled).
Waking up the machine from a serial port seems to be an unlikely
requirement, so make serial_core default to not using the behaviour.
Before:
[ 0.338809] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[ 0.951861] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
[ 1.666042] console [ttyS0] enabled
After:
[ 0.338452] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[ 0.339016] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
[ 1.041860] console [ttyS0] enabled
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/tty/serial/serial_core.c | 6 ++++--
include/linux/serial_core.h | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c7bf31a..0129551 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2348,8 +2348,10 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
*/
tty_dev = tty_register_device(drv->tty_driver, uport->line, uport->dev);
if (likely(!IS_ERR(tty_dev))) {
- device_init_wakeup(tty_dev, 1);
- device_set_wakeup_enable(tty_dev, 0);
+ if (uport->wakeup_capable) {
+ device_init_wakeup(tty_dev, 1);
+ device_set_wakeup_enable(tty_dev, 0);
+ }
} else
printk(KERN_ERR "Cannot register tty device on line %d\n",
uport->line);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c91ace7..9a87975 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -377,7 +377,8 @@ struct uart_port {
unsigned char hub6; /* this should be in the 8250 driver */
unsigned char suspended;
unsigned char irq_wake;
- unsigned char unused[2];
+ unsigned char wakeup_capable; /* 1 if wake-up capable */
+ unsigned char unused;
void *private_data; /* generic platform data pointer */
};
--
1.7.7.3
^ permalink raw reply related
* [PATCH 1/3] serial: 8250: Remove trailing space in 8250 driver
From: Simon Glass @ 2012-01-17 18:56 UTC (permalink / raw)
To: LKML; +Cc: Greg Kroah-Hartman, linux-serial, Simon Glass
Trivial change to remove a trailing space.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/tty/serial/8250.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 9f50c4e..54f8920 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -865,7 +865,7 @@ static int broken_efr(struct uart_8250_port *up)
/*
* Exar ST16C2550 "A2" devices incorrectly detect as
* having an EFR, and report an ID of 0x0201. See
- * http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-11/4812.html
+ * http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-11/4812.html
*/
if (autoconfig_read_divisor_id(up) == 0x0201 && size_fifo(up) == 16)
return 1;
--
1.7.7.3
^ 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