* 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
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Rafael J. Wysocki @ 2012-01-19 0:08 UTC (permalink / raw)
To: Simon Glass; +Cc: paulmck, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <CAPnjgZ1GJatR15dVsq-+hAWhD7eaLhtN737QvBaor_=0+K+6wA@mail.gmail.com>
On Wednesday, January 18, 2012, 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);
It shouldn't do that.
It should just do device_set_wakeup_capable(tty_dev, true) instead.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 1/2] power: Add function to init wakeup capability without enabling
From: Rafael J. Wysocki @ 2012-01-19 0:10 UTC (permalink / raw)
To: Simon Glass
Cc: LKML, Greg Kroah-Hartman, linux-serial, Pavel Machek, Alan Cox,
Paul E. McKenney
In-Reply-To: <1326928027-20610-1-git-send-email-sjg@chromium.org>
On Thursday, January 19, 2012, Simon Glass wrote:
> 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)
Which is device_set_wakeup_capable(dev, true).
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-19 0:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: paulmck, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <201201190108.32794.rjw@sisk.pl>
Hi Rafael,
On Wed, Jan 18, 2012 at 4:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, January 18, 2012, 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);
>
> It shouldn't do that.
>
> It should just do device_set_wakeup_capable(tty_dev, true) instead.
Ok, that makes sense - thank you for clearing it up. I will do a new patch.
Regards
Simon
>
> Thanks,
> Rafael
--
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-19 1:37 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <201201190102.58788.rjw@sisk.pl>
On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> 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.
Good point, that would work just as well and be simpler.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Simon Glass @ 2012-01-19 2:35 UTC (permalink / raw)
To: paulmck; +Cc: Rafael J. Wysocki, Alan Cox, LKML, Greg Kroah-Hartman,
linux-serial
In-Reply-To: <20120119013731.GK2431@linux.vnet.ibm.com>
Hi Paul,
On Wed, Jan 18, 2012 at 5:37 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
>> 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.
>
> Good point, that would work just as well and be simpler.
OK, well I am expecting that this will now be a very small patch to
change just serial_core.
Thanks for your help with this.
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] amba-pl011: clear previous interrupts before request_irq
From: Shreshtha Kumar SAHU @ 2012-01-19 11:07 UTC (permalink / raw)
To: Russell King
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20120118101555.GG22472@flint.arm.linux.org.uk>
On Wed, Jan 18, 2012 at 11:15:55 +0100, Russell King wrote:
> 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.
>
Sorry for late reply and missing sign-off.
In the setup where console is disabled and logs are diverted to buffer, it is
observed that, continuous uart interrupts are coming. Even before IMSC
(Interrupt mask set/clear register) is enabled for TX or RX interrupts,
it is seen that interrupt handler getting called just after request_irq.
It was found that this situation happens when RIS (raw interrupt register)
has some pending interrupts (e.g. RI, CTS, DCD, RX etc. but IMO RX is main)
when control gets transferred to kernel from bootloader. This refers that
GIC (global interrupt controller) is providing interrupt even if interrupts
for UART are masked/disabled in IMSC register.
In this state problem elivated when cpu is looping in interrupt handler
because of Rx interrupt set but Rx fifo is empty (hence rx_char is simply
returning). How UART goes in to this state is still not completely
understood.
> 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?
>
I tried doing this but problem persists.
I understand that module should be robust enough and should not depend
on state left by previous user of soc. I am still investigating and will
come back with analysis.
> 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:
Thanks and Regards
Shreshtha
^ permalink raw reply
* Re: [PATCH] amba-pl011: clear previous interrupts before request_irq
From: Shreshtha Kumar SAHU @ 2012-01-19 11:15 UTC (permalink / raw)
To: Alan Cox
Cc: gregkh@suse.de, linux-serial@vger.kernel.org,
rmk+kernel@arm.linux.org.uk, linux-kernel@vger.kernel.org
In-Reply-To: <20120118114100.064e2c8f@pyramind.ukuu.org.uk>
On Wed, Jan 18, 2012 at 12:41:00 +0100, Alan Cox wrote:
> > /*
> > + * 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
Yes, spin_lock_irq for the port is not required as request_irq is not yet
called for the port.
Thanks and Regards,
Shreshtha
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-19 19:13 UTC (permalink / raw)
To: Simon Glass
Cc: Rafael J. Wysocki, Alan Cox, LKML, Greg Kroah-Hartman,
linux-serial
In-Reply-To: <CAPnjgZ1zFHzihG51qHM-dk5Ou5vig0JL62LNFATmDwbQo_r+FQ@mail.gmail.com>
On Wed, Jan 18, 2012 at 06:35:56PM -0800, Simon Glass wrote:
> Hi Paul,
>
> On Wed, Jan 18, 2012 at 5:37 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> >> 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.
> >
> > Good point, that would work just as well and be simpler.
>
> OK, well I am expecting that this will now be a very small patch to
> change just serial_core.
>
> Thanks for your help with this.
Glad to help, and even more glad that Alan and Rafael were able to help. ;-)
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
* [PATCH] serial: Fix wakeup init logic to speed up startup
From: Simon Glass @ 2012-01-19 19:28 UTC (permalink / raw)
To: LKML
Cc: Greg Kroah-Hartman, linux-serial, Rafael J. Wysocki, Alan Cox,
Paul E. McKenney, Simon Glass
The synchronize_rcu() call resulting from making every serial driver
wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
(with CONFIG_PREEMPT disabled).
But this is avoidable since it is the device_set_wakeup_enable() and then
subsequence disable which causes the delay. We might as well just make
the device wakeup capable but not actually enable it for wakeup until
needed.
Effectively the current code does this:
device_set_wakeup_capable(dev, 1);
device_set_wakeup_enable(dev, 1);
device_set_wakeup_enable(dev, 0);
We can just drop the last two lines.
Before this change my boot log says:
[ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
after:
[ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
for saving of 450ms.
Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
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..1305618 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_set_wakeup_capable(tty_dev, 1);
+ } 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] serial: trivial checkpatch fixes in drivers/tty/serial/8250.c
From: Simon Glass @ 2012-01-19 19:37 UTC (permalink / raw)
To: LKML; +Cc: Jiri Kosina, Greg Kroah-Hartman, linux-serial, Simon Glass
This makes the file checkpatch clean, fixing these warnings:
warning: drivers/tty/serial/8250.c,86: please, no space before tabs
warning: drivers/tty/serial/8250.c,345: please, no space before tabs
warning: drivers/tty/serial/8250.c,672: line over 80 characters
warning: drivers/tty/serial/8250.c,1070: line over 80 characters
warning: drivers/tty/serial/8250.c,1517: line over 80 characters
warning: drivers/tty/serial/8250.c,1519: line over 80 characters
warning: drivers/tty/serial/8250.c,2009: line over 80 characters
warning: drivers/tty/serial/8250.c,2023: line over 80 characters
warning: drivers/tty/serial/8250.c,2025: line over 80 characters
warning: drivers/tty/serial/8250.c,2230: line over 80 characters
error: drivers/tty/serial/8250.c,2252: code indent should use tabs where
possible
warning: drivers/tty/serial/8250.c,3056: line over 80 characters
warning: drivers/tty/serial/8250.c,3133: line over 80 characters
warning: drivers/tty/serial/8250.c,3337: EXPORT_SYMBOL(foo); should
immediately follow its function/variable
warning: drivers/tty/serial/8250.c,3338: EXPORT_SYMBOL(foo); should
immediately follow its function/variable
warning: drivers/tty/serial/8250.c,3348: line over 80 characters
This warning remains, since I am not sure how to fix it:
error: drivers/tty/serial/8250.c,2929: Macros with complex values should
be enclosed in parenthesis
I am not sure if trivial includes all of the above - please let me know.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
drivers/tty/serial/8250.c | 53 ++++++++++++++++++++++++++------------------
1 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 9f50c4e..5e72f99 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -83,7 +83,7 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
#define PASS_LIMIT 512
-#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
+#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
/*
@@ -342,7 +342,7 @@ static const u8
[UART_SCR] = 0x2c
},
regmap_out[8] = {
- [UART_TX] = 0x04,
+ [UART_TX] = 0x04,
[UART_IER] = 0x0c,
[UART_FCR] = 0x18,
[UART_LCR] = 0x1c,
@@ -669,7 +669,8 @@ static void disable_rsa(struct uart_8250_port *up)
result = !(mode & UART_RSA_MSR_FIFO);
if (!result) {
- serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
+ serial_outp(up, UART_RSA_MSR, mode
+ & ~UART_RSA_MSR_FIFO);
mode = serial_inp(up, UART_RSA_MSR);
result = !(mode & UART_RSA_MSR_FIFO);
}
@@ -865,7 +866,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;
@@ -1067,7 +1068,8 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
return;
DEBUG_AUTOCONF("ttyS%d: autoconf (0x%04lx, 0x%p): ",
- serial_index(&up->port), up->port.iobase, up->port.membase);
+ serial_index(&up->port), up->port.iobase,
+ up->port.membase);
/*
* We really do need global IRQs disabled here - we're going to
@@ -1514,9 +1516,11 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up)
if (status & UART_MSR_DDSR)
up->port.icount.dsr++;
if (status & UART_MSR_DDCD)
- uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
+ uart_handle_dcd_change(&up->port,
+ status & UART_MSR_DCD);
if (status & UART_MSR_DCTS)
- uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
+ uart_handle_cts_change(&up->port,
+ status & UART_MSR_CTS);
wake_up_interruptible(&up->port.state->port.delta_msr_wait);
}
@@ -2006,7 +2010,8 @@ static int serial8250_startup(struct uart_port *port)
*/
if (!(up->port.flags & UPF_BUGGY_UART) &&
(serial_inp(up, UART_LSR) == 0xff)) {
- printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n",
+ printk_ratelimited(KERN_INFO
+ "ttyS%d: LSR safety check engaged!\n",
serial_index(&up->port));
return -ENODEV;
}
@@ -2019,10 +2024,13 @@ static int serial8250_startup(struct uart_port *port)
serial_outp(up, UART_LCR, UART_LCR_CONF_MODE_B);
- fctr = serial_inp(up, UART_FCTR) & ~(UART_FCTR_RX|UART_FCTR_TX);
- serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_RX);
+ fctr = serial_inp(up, UART_FCTR) &
+ ~(UART_FCTR_RX | UART_FCTR_TX);
+ serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD |
+ UART_FCTR_RX);
serial_outp(up, UART_TRG, UART_TRG_96);
- serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_TX);
+ serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD |
+ UART_FCTR_TX);
serial_outp(up, UART_TRG, UART_TRG_96);
serial_outp(up, UART_LCR, 0);
@@ -2247,9 +2255,8 @@ static unsigned int serial8250_get_divisor(struct uart_port *port, unsigned int
return quot;
}
-void
-serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
- struct ktermios *old)
+void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
+ struct ktermios *old)
{
struct uart_8250_port *up =
container_of(port, struct uart_8250_port, port);
@@ -2992,6 +2999,7 @@ void serial8250_suspend_port(int line)
{
uart_suspend_port(&serial8250_reg, &serial8250_ports[line].port);
}
+EXPORT_SYMBOL(serial8250_suspend_port);
/**
* serial8250_resume_port - resume one serial port
@@ -3014,6 +3022,7 @@ void serial8250_resume_port(int line)
}
uart_resume_port(&serial8250_reg, &up->port);
}
+EXPORT_SYMBOL(serial8250_resume_port);
/*
* Register a set of serial devices attached to a platform device. The
@@ -3053,8 +3062,8 @@ static int __devinit serial8250_probe(struct platform_device *dev)
port.irqflags |= irqflag;
ret = serial8250_register_port(&port);
if (ret < 0) {
- dev_err(&dev->dev, "unable to register port at index %d "
- "(IO%lx MEM%llx IRQ%d): %d\n", i,
+ dev_err(&dev->dev, "unable to register port at "
+ "index %d (IO%lx MEM%llx IRQ%d): %d\n", i,
p->iobase, (unsigned long long)p->mapbase,
p->irq, ret);
}
@@ -3130,7 +3139,8 @@ static struct platform_device *serial8250_isa_devs;
*/
static DEFINE_MUTEX(serial_mutex);
-static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port)
+static struct uart_8250_port *serial8250_find_match_or_unused(
+ struct uart_port *port)
{
int i;
@@ -3334,9 +3344,6 @@ static void __exit serial8250_exit(void)
module_init(serial8250_init);
module_exit(serial8250_exit);
-EXPORT_SYMBOL(serial8250_suspend_port);
-EXPORT_SYMBOL(serial8250_resume_port);
-
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
@@ -3345,10 +3352,12 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
" (unsafe)");
module_param(nr_uarts, uint, 0644);
-MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
+MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" \
+ __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
module_param(skip_txen_test, uint, 0644);
-MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
+MODULE_PARM_DESC(skip_txen_test, \
+ "Skip checking for the TXEN bug at init time");
#ifdef CONFIG_SERIAL_8250_RSA
module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH] serial: Fix wakeup init logic to speed up startup
From: Paul E. McKenney @ 2012-01-19 19:42 UTC (permalink / raw)
To: Simon Glass
Cc: LKML, Greg Kroah-Hartman, linux-serial, Rafael J. Wysocki,
Alan Cox
In-Reply-To: <1327001336-28703-1-git-send-email-sjg@chromium.org>
On Thu, Jan 19, 2012 at 11:28:56AM -0800, Simon Glass wrote:
> The synchronize_rcu() call resulting from making every serial driver
> wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
> (with CONFIG_PREEMPT disabled).
>
> But this is avoidable since it is the device_set_wakeup_enable() and then
> subsequence disable which causes the delay. We might as well just make
> the device wakeup capable but not actually enable it for wakeup until
> needed.
>
> Effectively the current code does this:
>
> device_set_wakeup_capable(dev, 1);
> device_set_wakeup_enable(dev, 1);
> device_set_wakeup_enable(dev, 0);
>
> We can just drop the last two lines.
>
> Before this change my boot log says:
> [ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>
> after:
> [ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>
> for saving of 450ms.
You have multiple CPUs running at this point, correct? Before that
second CPU starts up, synchronize_rcu() is a no-op.
The patch looks good to me, but then again, I do not consider myself
qualified to have an opinion on the TTY layer. ;-)
Thanx, Paul
> Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
> 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..1305618 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_set_wakeup_capable(tty_dev, 1);
> + } 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
* Re: [PATCH] serial: trivial checkpatch fixes in drivers/tty/serial/8250.c
From: Joe Perches @ 2012-01-19 19:49 UTC (permalink / raw)
To: Simon Glass; +Cc: LKML, Jiri Kosina, Greg Kroah-Hartman, linux-serial
In-Reply-To: <1327001868-29941-1-git-send-email-sjg@chromium.org>
On Thu, 2012-01-19 at 11:37 -0800, Simon Glass wrote:
> This makes the file checkpatch clean, fixing these warnings:
Some comments...
> This warning remains, since I am not sure how to fix it:
> error: drivers/tty/serial/8250.c,2929: Macros with complex values should
> be enclosed in parenthesis
you can ignore this warning.
> diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
[]
> @@ -669,7 +669,8 @@ static void disable_rsa(struct uart_8250_port *up)
> result = !(mode & UART_RSA_MSR_FIFO);
>
> if (!result) {
> -
> serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
> + serial_outp(up, UART_RSA_MSR, mode
> + & ~UART_RSA_MSR_FIFO);
If it's really necessary to use 80 columns,
I'd prefer this sort of change be:
serial_outp(up, UART_RSA_MSR,
mode & ~UART_RSA_MSR_FIFO);
[]
> @@ -2006,7 +2010,8 @@ static int serial8250_startup(struct uart_port *port)
> */
> if (!(up->port.flags & UPF_BUGGY_UART) &&
> (serial_inp(up, UART_LSR) == 0xff)) {
> - printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n",
> + printk_ratelimited(KERN_INFO
pr_info_ratelimited()
> @@ -2019,10 +2024,13 @@ static int serial8250_startup(struct uart_port *port)
>
> serial_outp(up, UART_LCR, UART_LCR_CONF_MODE_B);
>
> - fctr = serial_inp(up, UART_FCTR) & ~(UART_FCTR_RX|UART_FCTR_TX);
> - serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_RX);
> + fctr = serial_inp(up, UART_FCTR) &
> + ~(UART_FCTR_RX | UART_FCTR_TX);
> + serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD |
> + UART_FCTR_RX);
I think it's better to have all the or's
on the same line where possible.
serial_outp(up, UART_FCTR,
fctr | UART_FCTR_TRGD | UART_FCTR_RX);
> @@ -3130,7 +3139,8 @@ static struct platform_device *serial8250_isa_devs;
> */
> static DEFINE_MUTEX(serial_mutex);
>
> -static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port)
> +static struct uart_8250_port *serial8250_find_match_or_unused(
> + struct uart_port *port)
It might be better to use
static struct uart_8250_port *
serial8250_find_match_or_unused(struct uart_port *port)
{
> @ -3345,10 +3352,12 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
> " (unsafe)");
>
> module_param(nr_uarts, uint, 0644);
> -MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
> +MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" \
> + __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
Broken.
Don't use line continuations with whitespace on the next line.
Better not use line continations at all except in #defines
> module_param(skip_txen_test, uint, 0644);
> -MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
> +MODULE_PARM_DESC(skip_txen_test, \
> + "Skip checking for the TXEN bug at init time");
Useless line continuation.
^ permalink raw reply
* Re: [PATCH] serial: Fix wakeup init logic to speed up startup
From: Simon Glass @ 2012-01-19 19:51 UTC (permalink / raw)
To: paulmck; +Cc: LKML, Greg Kroah-Hartman, linux-serial, Rafael J. Wysocki,
Alan Cox
In-Reply-To: <20120119194242.GE2373@linux.vnet.ibm.com>
Hi Paul,
On Thu, Jan 19, 2012 at 11:42 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jan 19, 2012 at 11:28:56AM -0800, Simon Glass wrote:
>> The synchronize_rcu() call resulting from making every serial driver
>> wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
>> (with CONFIG_PREEMPT disabled).
>>
>> But this is avoidable since it is the device_set_wakeup_enable() and then
>> subsequence disable which causes the delay. We might as well just make
>> the device wakeup capable but not actually enable it for wakeup until
>> needed.
>>
>> Effectively the current code does this:
>>
>> device_set_wakeup_capable(dev, 1);
>> device_set_wakeup_enable(dev, 1);
>> device_set_wakeup_enable(dev, 0);
>>
>> We can just drop the last two lines.
>>
>> Before this change my boot log says:
>> [ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>> [ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>>
>> after:
>> [ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>> [ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>>
>> for saving of 450ms.
>
> You have multiple CPUs running at this point, correct? Before that
> second CPU starts up, synchronize_rcu() is a no-op.
Yes that's right, although I didn't get different behavior with 'nosmp'.
>
> The patch looks good to me, but then again, I do not consider myself
> qualified to have an opinion on the TTY layer. ;-)
Thanks, me neither :-)
Regards,
Simon
>
> Thanx, Paul
>
>> Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
>> 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..1305618 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_set_wakeup_capable(tty_dev, 1);
>> + } else {
>> printk(KERN_ERR "Cannot register tty device on line %d\n",
>> uport->line);
>> + }
>>
>> /*
>> * Ensure UPF_DEAD is not set.
>> --
>> 1.7.7.3
>>
>
--
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] serial: Fix wakeup init logic to speed up startup
From: Paul E. McKenney @ 2012-01-19 20:12 UTC (permalink / raw)
To: Simon Glass
Cc: LKML, Greg Kroah-Hartman, linux-serial, Rafael J. Wysocki,
Alan Cox
In-Reply-To: <CAPnjgZ0kB8L6fFrcjq_0MSoh7KkL-051JHYptNQMaZNi89MavQ@mail.gmail.com>
On Thu, Jan 19, 2012 at 11:51:47AM -0800, Simon Glass wrote:
> Hi Paul,
>
> On Thu, Jan 19, 2012 at 11:42 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, Jan 19, 2012 at 11:28:56AM -0800, Simon Glass wrote:
> >> The synchronize_rcu() call resulting from making every serial driver
> >> wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
> >> (with CONFIG_PREEMPT disabled).
> >>
> >> But this is avoidable since it is the device_set_wakeup_enable() and then
> >> subsequence disable which causes the delay. We might as well just make
> >> the device wakeup capable but not actually enable it for wakeup until
> >> needed.
> >>
> >> Effectively the current code does this:
> >>
> >> device_set_wakeup_capable(dev, 1);
> >> device_set_wakeup_enable(dev, 1);
> >> device_set_wakeup_enable(dev, 0);
> >>
> >> We can just drop the last two lines.
> >>
> >> Before this change my boot log says:
> >> [ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> >> [ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
> >>
> >> after:
> >> [ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> >> [ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
> >>
> >> for saving of 450ms.
> >
> > You have multiple CPUs running at this point, correct? Before that
> > second CPU starts up, synchronize_rcu() is a no-op.
>
> Yes that's right, although I didn't get different behavior with 'nosmp'.
If you are running CONFIG_PREEMPT=y, that is expected behavior, though
that grace period is a bit on the long side. What is the value of HZ?
On the other hand, if you are running CONFIG_PREEMPT=n on recent kernels,
synchronize_rcu() will be a no-op any time that you are running with
only a single CPU.
The reason for this difference is that with CONFIG_PREEMPT=y, there might
be a preempted RCU reader, and RCU must check for this condition, waiting
for any such reader to complete. In contrast, when CONFIG_PREEMPT=n on a
single-CPU system, the fact that you are executing in synchronize_rcu()
guarantees that there are no running RCU readers, so synchronize_rcu()
need do nothing in that case.
Thanx, Paul
> > The patch looks good to me, but then again, I do not consider myself
> > qualified to have an opinion on the TTY layer. ;-)
>
> Thanks, me neither :-)
>
> Regards,
> Simon
>
> >
> > Thanx, Paul
> >
> >> Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> 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..1305618 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_set_wakeup_capable(tty_dev, 1);
> >> + } 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
* Re: [PATCH RFC 2/2] OMAP : serial : Check for error in get_context_loss_count
From: Kevin Hilman @ 2012-01-19 22:59 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-kernel, linux-arm-kernel
In-Reply-To: <1326377019-23688-2-git-send-email-shubhrajyoti@ti.com>
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> In serial_omap_runtime_resume in case of errors returned by
> get_context_loss_count print a warning and do a restore.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
These two patches should be combined into a single patch.
Also, please Cc Govindraj since he's the maintainer of this driver and
should Ack.
Thanks,
Kevin
> ---
> applies on Tony's uart branch
>
> drivers/tty/serial/omap-serial.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 29fe6fd..6008612 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1602,10 +1602,16 @@ static int serial_omap_runtime_resume(struct device *dev)
>
> if (up) {
> if (pdata->get_context_loss_count) {
> - u32 loss_cnt = pdata->get_context_loss_count(dev);
> + int loss_cnt = pdata->get_context_loss_count(dev);
>
> - if (up->context_loss_cnt != loss_cnt)
> + if (loss_cnt < 0) {
> + dev_err(dev,
> + "get_context_loss_count failed : %d\n",
> + loss_cnt);
> serial_omap_restore_context(up);
> + } else if (up->context_loss_cnt != loss_cnt) {
> + serial_omap_restore_context(up);
> + }
> }
>
> /* Errata i291 */
^ permalink raw reply
* Re: [PATCH] serial: Fix wakeup init logic to speed up startup
From: Rafael J. Wysocki @ 2012-01-19 23:46 UTC (permalink / raw)
To: Simon Glass
Cc: LKML, Greg Kroah-Hartman, linux-serial, Alan Cox,
Paul E. McKenney
In-Reply-To: <1327001336-28703-1-git-send-email-sjg@chromium.org>
On Thursday, January 19, 2012, Simon Glass wrote:
> The synchronize_rcu() call resulting from making every serial driver
> wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
> (with CONFIG_PREEMPT disabled).
>
> But this is avoidable since it is the device_set_wakeup_enable() and then
> subsequence disable which causes the delay. We might as well just make
> the device wakeup capable but not actually enable it for wakeup until
> needed.
>
> Effectively the current code does this:
>
> device_set_wakeup_capable(dev, 1);
> device_set_wakeup_enable(dev, 1);
> device_set_wakeup_enable(dev, 0);
>
> We can just drop the last two lines.
>
> Before this change my boot log says:
> [ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>
> after:
> [ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>
> for saving of 450ms.
>
> Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Thanks!
> ---
> 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..1305618 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_set_wakeup_capable(tty_dev, 1);
> + } else {
> printk(KERN_ERR "Cannot register tty device on line %d\n",
> uport->line);
> + }
>
> /*
> * Ensure UPF_DEAD is not set.
>
^ permalink raw reply
* Re: [PATCH] serial: Fix wakeup init logic to speed up startup
From: Simon Glass @ 2012-01-19 23:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: LKML, Greg Kroah-Hartman, linux-serial, Alan Cox,
Paul E. McKenney
In-Reply-To: <201201200046.59766.rjw@sisk.pl>
Hi Rafael,
On Thu, Jan 19, 2012 at 3:46 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 19, 2012, Simon Glass wrote:
>> The synchronize_rcu() call resulting from making every serial driver
>> wake-up capable (commit b3b708fa) slows boot down on my Tegra2x system
>> (with CONFIG_PREEMPT disabled).
>>
>> But this is avoidable since it is the device_set_wakeup_enable() and then
>> subsequence disable which causes the delay. We might as well just make
>> the device wakeup capable but not actually enable it for wakeup until
>> needed.
>>
>> Effectively the current code does this:
>>
>> device_set_wakeup_capable(dev, 1);
>> device_set_wakeup_enable(dev, 1);
>> device_set_wakeup_enable(dev, 0);
>>
>> We can just drop the last two lines.
>>
>> Before this change my boot log says:
>> [ 0.227062] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>> [ 0.702928] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>>
>> after:
>> [ 0.227264] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>> [ 0.227983] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
>>
>> for saving of 450ms.
>>
>> Suggested-by: Rafael J. Wysocki <rjw@sisk.pl>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
>
> Thanks!
Thanks for your help with this.
Regards,
Simon
>
>> ---
>> 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..1305618 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_set_wakeup_capable(tty_dev, 1);
>> + } else {
>> printk(KERN_ERR "Cannot register tty device on line %d\n",
>> uport->line);
>> + }
>>
>> /*
>> * Ensure UPF_DEAD is not set.
>>
>
--
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: Rafael J. Wysocki @ 2012-01-20 0:03 UTC (permalink / raw)
To: paulmck; +Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120119013731.GK2431@linux.vnet.ibm.com>
On Thursday, January 19, 2012, Paul E. McKenney wrote:
> On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
[...]
> > 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.
>
> Good point, that would work just as well and be simpler.
Thanks for the confirmation! :-)
By the way, I wonder, would it help to add synchronize_rcu() to
wakeup_source_add() too? Then, even if device_wakeup_enable() and
device_wakeup_disable() are executed in a tight loop for the same
device, the list_add/list_del operations will always happen in
different RCU cycles (or at least it seems so).
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] serial: trivial checkpatch fixes in drivers/tty/serial/8250.c
From: Simon Glass @ 2012-01-20 0:23 UTC (permalink / raw)
To: Joe Perches; +Cc: LKML, Jiri Kosina, Greg Kroah-Hartman, linux-serial
In-Reply-To: <1327002594.6404.27.camel@joe2Laptop>
Hi Joe,
On Thu, Jan 19, 2012 at 11:49 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2012-01-19 at 11:37 -0800, Simon Glass wrote:
>> This makes the file checkpatch clean, fixing these warnings:
>
> Some comments...
Thanks for the comments - all addressed I think. Will send a V2.
Regards,
Simon
>
>> This warning remains, since I am not sure how to fix it:
>> error: drivers/tty/serial/8250.c,2929: Macros with complex values should
>> be enclosed in parenthesis
>
> you can ignore this warning.
>
>> diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
> []
>> @@ -669,7 +669,8 @@ static void disable_rsa(struct uart_8250_port *up)
>> result = !(mode & UART_RSA_MSR_FIFO);
>>
>> if (!result) {
>> -
>> serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
>> + serial_outp(up, UART_RSA_MSR, mode
>> + & ~UART_RSA_MSR_FIFO);
>
> If it's really necessary to use 80 columns,
> I'd prefer this sort of change be:
> serial_outp(up, UART_RSA_MSR,
> mode & ~UART_RSA_MSR_FIFO);
>
> []
>> @@ -2006,7 +2010,8 @@ static int serial8250_startup(struct uart_port *port)
>> */
>> if (!(up->port.flags & UPF_BUGGY_UART) &&
>> (serial_inp(up, UART_LSR) == 0xff)) {
>> - printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n",
>> + printk_ratelimited(KERN_INFO
>
> pr_info_ratelimited()
>
>
>> @@ -2019,10 +2024,13 @@ static int serial8250_startup(struct uart_port *port)
>>
>> serial_outp(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>
>> - fctr = serial_inp(up, UART_FCTR) & ~(UART_FCTR_RX|UART_FCTR_TX);
>> - serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_RX);
>> + fctr = serial_inp(up, UART_FCTR) &
>> + ~(UART_FCTR_RX | UART_FCTR_TX);
>> + serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD |
>> + UART_FCTR_RX);
>
> I think it's better to have all the or's
> on the same line where possible.
>
> serial_outp(up, UART_FCTR,
> fctr | UART_FCTR_TRGD | UART_FCTR_RX);
>
>
>> @@ -3130,7 +3139,8 @@ static struct platform_device *serial8250_isa_devs;
>> */
>> static DEFINE_MUTEX(serial_mutex);
>>
>> -static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port)
>> +static struct uart_8250_port *serial8250_find_match_or_unused(
>> + struct uart_port *port)
>
> It might be better to use
>
> static struct uart_8250_port *
> serial8250_find_match_or_unused(struct uart_port *port)
> {
>> @ -3345,10 +3352,12 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
>> " (unsafe)");
>>
>> module_param(nr_uarts, uint, 0644);
>> -MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>> +MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" \
>> + __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>
> Broken.
>
> Don't use line continuations with whitespace on the next line.
> Better not use line continations at all except in #defines
>
>
>> module_param(skip_txen_test, uint, 0644);
>> -MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
>> +MODULE_PARM_DESC(skip_txen_test, \
>> + "Skip checking for the TXEN bug at init time");
>
> Useless line continuation.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] serial: trivial checkpatch fixes in drivers/tty/serial/8250.c
From: Simon Glass @ 2012-01-20 0:24 UTC (permalink / raw)
To: LKML
Cc: Jiri Kosina, Greg Kroah-Hartman, linux-serial, Joe Perches,
Simon Glass
In-Reply-To: <1327001868-29941-1-git-send-email-sjg@chromium.org>
This makes the file checkpatch clean, fixing these warnings:
warning: drivers/tty/serial/8250.c,86: please, no space before tabs
warning: drivers/tty/serial/8250.c,345: please, no space before tabs
warning: drivers/tty/serial/8250.c,672: line over 80 characters
warning: drivers/tty/serial/8250.c,1070: line over 80 characters
warning: drivers/tty/serial/8250.c,1517: line over 80 characters
warning: drivers/tty/serial/8250.c,1519: line over 80 characters
warning: drivers/tty/serial/8250.c,2009: line over 80 characters
warning: drivers/tty/serial/8250.c,2023: line over 80 characters
warning: drivers/tty/serial/8250.c,2025: line over 80 characters
warning: drivers/tty/serial/8250.c,2230: line over 80 characters
error: drivers/tty/serial/8250.c,2252: code indent should use tabs where
possible
warning: drivers/tty/serial/8250.c,3056: line over 80 characters
warning: drivers/tty/serial/8250.c,3133: line over 80 characters
warning: drivers/tty/serial/8250.c,3337: EXPORT_SYMBOL(foo); should
immediately follow its function/variable
warning: drivers/tty/serial/8250.c,3338: EXPORT_SYMBOL(foo); should
immediately follow its function/variable
warning: drivers/tty/serial/8250.c,3348: line over 80 characters
This warning remains, and should be ignored:
error: drivers/tty/serial/8250.c,2929: Macros with complex values should
be enclosed in parenthesis
I am not sure if 'trivial' includes all of the above - please let me know.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fixed line continuation problems (and unnecessary ones)
- Tidied up code style in response to comments
- Update commit message to mention I am ignoring one warning
- Use pr_info_ratelimited() instead of printk_ratelimited(KERN_INFO...)
drivers/tty/serial/8250.c | 54 ++++++++++++++++++++++++++------------------
1 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 9f50c4e..c770a0c 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -83,7 +83,7 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
#define PASS_LIMIT 512
-#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
+#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
/*
@@ -342,7 +342,7 @@ static const u8
[UART_SCR] = 0x2c
},
regmap_out[8] = {
- [UART_TX] = 0x04,
+ [UART_TX] = 0x04,
[UART_IER] = 0x0c,
[UART_FCR] = 0x18,
[UART_LCR] = 0x1c,
@@ -669,7 +669,8 @@ static void disable_rsa(struct uart_8250_port *up)
result = !(mode & UART_RSA_MSR_FIFO);
if (!result) {
- serial_outp(up, UART_RSA_MSR, mode & ~UART_RSA_MSR_FIFO);
+ serial_outp(up, UART_RSA_MSR,
+ mode & ~UART_RSA_MSR_FIFO);
mode = serial_inp(up, UART_RSA_MSR);
result = !(mode & UART_RSA_MSR_FIFO);
}
@@ -865,7 +866,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;
@@ -1067,7 +1068,8 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
return;
DEBUG_AUTOCONF("ttyS%d: autoconf (0x%04lx, 0x%p): ",
- serial_index(&up->port), up->port.iobase, up->port.membase);
+ serial_index(&up->port), up->port.iobase,
+ up->port.membase);
/*
* We really do need global IRQs disabled here - we're going to
@@ -1514,9 +1516,11 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up)
if (status & UART_MSR_DDSR)
up->port.icount.dsr++;
if (status & UART_MSR_DDCD)
- uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
+ uart_handle_dcd_change(&up->port,
+ status & UART_MSR_DCD);
if (status & UART_MSR_DCTS)
- uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
+ uart_handle_cts_change(&up->port,
+ status & UART_MSR_CTS);
wake_up_interruptible(&up->port.state->port.delta_msr_wait);
}
@@ -2006,8 +2010,8 @@ static int serial8250_startup(struct uart_port *port)
*/
if (!(up->port.flags & UPF_BUGGY_UART) &&
(serial_inp(up, UART_LSR) == 0xff)) {
- printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n",
- serial_index(&up->port));
+ pr_info_ratelimited("ttyS%d: LSR safety check engaged!\n",
+ serial_index(&up->port));
return -ENODEV;
}
@@ -2019,10 +2023,13 @@ static int serial8250_startup(struct uart_port *port)
serial_outp(up, UART_LCR, UART_LCR_CONF_MODE_B);
- fctr = serial_inp(up, UART_FCTR) & ~(UART_FCTR_RX|UART_FCTR_TX);
- serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_RX);
+ fctr = serial_inp(up, UART_FCTR) &
+ ~(UART_FCTR_RX | UART_FCTR_TX);
+ serial_outp(up, UART_FCTR,
+ fctr | UART_FCTR_TRGD | UART_FCTR_RX);
serial_outp(up, UART_TRG, UART_TRG_96);
- serial_outp(up, UART_FCTR, fctr | UART_FCTR_TRGD | UART_FCTR_TX);
+ serial_outp(up, UART_FCTR,
+ fctr | UART_FCTR_TRGD | UART_FCTR_TX);
serial_outp(up, UART_TRG, UART_TRG_96);
serial_outp(up, UART_LCR, 0);
@@ -2227,7 +2234,8 @@ static void serial8250_shutdown(struct uart_port *port)
serial_unlink_irq_chain(up);
}
-static unsigned int serial8250_get_divisor(struct uart_port *port, unsigned int baud)
+static unsigned int serial8250_get_divisor(struct uart_port *port,
+ unsigned int baud)
{
unsigned int quot;
@@ -2249,7 +2257,7 @@ static unsigned int serial8250_get_divisor(struct uart_port *port, unsigned int
void
serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
- struct ktermios *old)
+ struct ktermios *old)
{
struct uart_8250_port *up =
container_of(port, struct uart_8250_port, port);
@@ -2992,6 +3000,7 @@ void serial8250_suspend_port(int line)
{
uart_suspend_port(&serial8250_reg, &serial8250_ports[line].port);
}
+EXPORT_SYMBOL(serial8250_suspend_port);
/**
* serial8250_resume_port - resume one serial port
@@ -3014,6 +3023,7 @@ void serial8250_resume_port(int line)
}
uart_resume_port(&serial8250_reg, &up->port);
}
+EXPORT_SYMBOL(serial8250_resume_port);
/*
* Register a set of serial devices attached to a platform device. The
@@ -3053,8 +3063,8 @@ static int __devinit serial8250_probe(struct platform_device *dev)
port.irqflags |= irqflag;
ret = serial8250_register_port(&port);
if (ret < 0) {
- dev_err(&dev->dev, "unable to register port at index %d "
- "(IO%lx MEM%llx IRQ%d): %d\n", i,
+ dev_err(&dev->dev, "unable to register port at "
+ "index %d (IO%lx MEM%llx IRQ%d): %d\n", i,
p->iobase, (unsigned long long)p->mapbase,
p->irq, ret);
}
@@ -3130,7 +3140,8 @@ static struct platform_device *serial8250_isa_devs;
*/
static DEFINE_MUTEX(serial_mutex);
-static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port)
+static struct uart_8250_port *
+serial8250_find_match_or_unused(struct uart_port *port)
{
int i;
@@ -3334,9 +3345,6 @@ static void __exit serial8250_exit(void)
module_init(serial8250_init);
module_exit(serial8250_exit);
-EXPORT_SYMBOL(serial8250_suspend_port);
-EXPORT_SYMBOL(serial8250_resume_port);
-
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
@@ -3345,10 +3353,12 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
" (unsafe)");
module_param(nr_uarts, uint, 0644);
-MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
+MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-"
+ __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
module_param(skip_txen_test, uint, 0644);
-MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
+MODULE_PARM_DESC(skip_txen_test,
+ "Skip checking for the TXEN bug at init time");
#ifdef CONFIG_SERIAL_8250_RSA
module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH RFC 2/2] OMAP : serial : Check for error in get_context_loss_count
From: Shubhrajyoti @ 2012-01-20 5:59 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-serial, linux-omap, linux-kernel, linux-arm-kernel
In-Reply-To: <87hazrti1c.fsf@ti.com>
On Friday 20 January 2012 04:29 AM, Kevin Hilman wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
>> In serial_omap_runtime_resume in case of errors returned by
>> get_context_loss_count print a warning and do a restore.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> These two patches should be combined into a single patch.
>
OK I will combine. I had split as the one was a serial driver and one
for omap
maintainer.
However I agree that they should be combined.
> Also, please Cc Govindraj since he's the maintainer of this driver and
> should Ack.
will do that
> Thanks,
>
> Kevin
>
>> ---
>> applies on Tony's uart branch
>>
>> drivers/tty/serial/omap-serial.c | 10 ++++++++--
>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 29fe6fd..6008612 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1602,10 +1602,16 @@ static int serial_omap_runtime_resume(struct device *dev)
>>
>> if (up) {
>> if (pdata->get_context_loss_count) {
>> - u32 loss_cnt = pdata->get_context_loss_count(dev);
>> + int loss_cnt = pdata->get_context_loss_count(dev);
>>
>> - if (up->context_loss_cnt != loss_cnt)
>> + if (loss_cnt < 0) {
>> + dev_err(dev,
>> + "get_context_loss_count failed : %d\n",
>> + loss_cnt);
>> serial_omap_restore_context(up);
>> + } else if (up->context_loss_cnt != loss_cnt) {
>> + serial_omap_restore_context(up);
>> + }
>> }
>>
>> /* Errata i291 */
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-20 6:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <201201200103.34296.rjw@sisk.pl>
On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> [...]
> > > 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.
> >
> > Good point, that would work just as well and be simpler.
>
> Thanks for the confirmation! :-)
>
> By the way, I wonder, would it help to add synchronize_rcu() to
> wakeup_source_add() too? Then, even if device_wakeup_enable() and
> device_wakeup_disable() are executed in a tight loop for the same
> device, the list_add/list_del operations will always happen in
> different RCU cycles (or at least it seems so).
I cannot immediately see how adding a synchronize_rcu() to
wakeup_source_add() would help anything. You only need to wait for a
grace period on removal, not (normally) on addition. The single grace
period during removal will catch up all other asynchronous RCU grace
period requests on that CPU.
Or am I missing your point?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 0/5] uart updates
From: Govindraj @ 2012-01-20 8:45 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1326709360-26020-1-git-send-email-shubhrajyoti@ti.com>
On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> The patch series does the following
>
> - Make the Make the suspend/resume functions depend on
> CONFIG_PM_SLEEP
> - Fix the serial omap probe's error handling
> - Make he context_loss_cnt signed so that error handling is
> possible.
>
>
After fixing Kevin's comment's,
If can add some testing updates it be nice.
like :
boot tested or any other tests done on xxx boards.
--
Thanks,
Govindraj.R
> Shubhrajyoti D (5):
> omap-serial :Make the suspend/resume functions depend on
> CONFIG_PM_SLEEP.
> omap-serial: make serial_omap_restore_context depend on
> CONFIG_PM_RUNTIME
> omap-serial: Fix the error handling in the omap_serial probe
> ARM : OMAP : serial : Make context_loss_cnt signed
> OMAP : serial : Check for error in get_context_loss_count
>
> arch/arm/plat-omap/include/plat/omap-serial.h | 2 +-
> drivers/tty/serial/omap-serial.c | 41 ++++++++++++++++--------
> 2 files changed, 28 insertions(+), 15 deletions(-)
>
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/5] omap-serial: Fix the error handling in the omap_serial probe
From: Govindraj @ 2012-01-20 8:49 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1326709360-26020-4-git-send-email-shubhrajyoti@ti.com>
On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> The patch does the following
>
> - The pm_runtime_disable is called in the remove not in the error
> case of probe.The patch calls the pm_runtime_disable in the error
> case.
> - The up is not freed in the error path. Fix the memory leak by calling
> kfree in the error path.
> - Also the iounmap is not called fix the same by calling iounmap in the
> error case of probe and remove .
> - Make the name of the error tags more meaningful.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/tty/serial/omap-serial.c | 27 +++++++++++++++++----------
> 1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 1c24269..8c6f137 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1369,14 +1369,14 @@ static int serial_omap_probe(struct platform_device *pdev)
>
> dma_rx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "rx");
> if (!dma_rx) {
> - ret = -EINVAL;
> - goto err;
> + ret = -ENXIO;
> + goto do_release_region;
> }
>
> dma_tx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "tx");
> if (!dma_tx) {
> - ret = -EINVAL;
> - goto err;
> + ret = -ENXIO;
> + goto do_release_region;
> }
>
> up = kzalloc(sizeof(*up), GFP_KERNEL);
> @@ -1403,7 +1403,7 @@ static int serial_omap_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
> up->port.line);
> ret = -ENODEV;
> - goto err;
> + goto err_port_line;
> }
>
> sprintf(up->name, "OMAP UART%d", up->port.line);
> @@ -1412,7 +1412,7 @@ static int serial_omap_probe(struct platform_device *pdev)
> if (!up->port.membase) {
> dev_err(&pdev->dev, "can't ioremap UART\n");
> ret = -ENOMEM;
> - goto err;
> + goto err_ioremap;
> }
>
> up->port.flags = omap_up_info->flags;
> @@ -1458,16 +1458,22 @@ static int serial_omap_probe(struct platform_device *pdev)
>
> ret = uart_add_one_port(&serial_omap_reg, &up->port);
> if (ret != 0)
> - goto do_release_region;
> + goto err_add_port;
>
> pm_runtime_put(&pdev->dev);
> platform_set_drvdata(pdev, up);
> return 0;
> -err:
> - dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
> - pdev->id, __func__, ret);
> +
> +err_add_port:
> + pm_runtime_disable(&pdev->dev);
> + iounmap(up->port.membase);
> +err_ioremap:
> +err_port_line:
> + kfree(up);
> do_release_region:
> release_mem_region(mem->start, resource_size(mem));
> + dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
> + pdev->id, __func__, ret);
> return ret;
> }
>
> @@ -1476,6 +1482,7 @@ static int serial_omap_remove(struct platform_device *dev)
> struct uart_omap_port *up = platform_get_drvdata(dev);
>
> if (up) {
> + iounmap(up->port.membase);
you can build omap-serial as module insmod and rmmod
the module and test this patch.
This can be done on zoom board which uses a non-omap uart
as console.
--
Thanks,
Govindraj.R
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox