Serial-Core: USB-Serial port current issues.

Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

 Hi Russell,

 I'm working on the port of the USB-Serial layer to the Serial Core [1],
and turns out that most of the USB-Serial drivers does need to sleep in
set_termios(), break_ctl(), get_mctrl() and set_mctrl() calls (which are
not allowed to sleep according to the documentation).

 I took a look in the Serial Core code and didn't see why set_termios()
and break_ctl() (plus tx_empty()) are not allowed to sleep: they doesn't
seem to run in atomic context. So, are they allowed to sleep? Isn't the
documentation out of date? I've even submitted a patch to fix it [2].

 For get_mctrl() and set_mctrl() it seems possible to switch from a spinlock
to a mutex, as they are not called from an interrupt context. Is this
really possible? Would you agree with this change?

 Please, note that your opnion is very important. Both issues makes
the port not possible.

 Thanks.

[1] http://distro2.conectiva.com.br/~lcapitulino/patches/usbserial/2.6.17-rc5/serialcore-port-V0/
[2] http://marc.theaimsgroup.com/?l=linux-kernel&m=114979748706523&w=2

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Russell King]

On Tue, Jun 13, 2006 at 07:28:29PM -0300, Luiz Fernando N. Capitulino wrote:
>  I took a look in the Serial Core code and didn't see why set_termios()
> and break_ctl() (plus tx_empty()) are not allowed to sleep: they doesn't
> seem to run in atomic context. So, are they allowed to sleep? Isn't the
> documentation out of date? I've even submitted a patch to fix it [2].

You are correct - and I will eventually apply your patch.  At the
moment, I'm throttling back on applying patches so that 2.6.17 can
finally appear (I don't want to be responsible for Linus saying
again "too many changes for -final, let's do another -rc".)

>  For get_mctrl() and set_mctrl() it seems possible to switch from a
> spinlock to a mutex, as they are not called from an interrupt context.
> Is this really possible? Would you agree with this change?

I don't know - that depends whether the throttle/unthrottle driver
methods are ever called from interrupt context or not.

What we could do is put a WARN_ON() or might_sleep() in there and
find out over time if they are called from non-process context.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Wed, 14 Jun 2006 16:28:09 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

| On Tue, Jun 13, 2006 at 07:28:29PM -0300, Luiz Fernando N. Capitulino wrote:
| >  I took a look in the Serial Core code and didn't see why set_termios()
| > and break_ctl() (plus tx_empty()) are not allowed to sleep: they doesn't
| > seem to run in atomic context. So, are they allowed to sleep? Isn't the
| > documentation out of date? I've even submitted a patch to fix it [2].
| 
| You are correct - and I will eventually apply your patch.  At the
| moment, I'm throttling back on applying patches so that 2.6.17 can
| finally appear (I don't want to be responsible for Linus saying
| again "too many changes for -final, let's do another -rc".)

 Oh, ok, no worries.

| >  For get_mctrl() and set_mctrl() it seems possible to switch from a
| > spinlock to a mutex, as they are not called from an interrupt context.
| > Is this really possible? Would you agree with this change?
| 
| I don't know - that depends whether the throttle/unthrottle driver
| methods are ever called from interrupt context or not.
| 
| What we could do is put a WARN_ON() or might_sleep() in there and
| find out over time if they are called from non-process context.

 I think BUG_ON(in_interrupt()) does the job. I'll try it here, and
if it doesn't trigger I'll submit a patch to Andrew only for
testing porpuses (ie, not for mainline).

 Thanks a lot for the feedback, Russell.

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Pete Zaitcev]

On Wed, 14 Jun 2006 17:38:24 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

>  I think BUG_ON(in_interrupt()) does the job. I'll try it here, and
> if it doesn't trigger I'll submit a patch to Andrew only for
> testing porpuses (ie, not for mainline).

Luiz, you can't be serious! You have to do a review and write call paths
on a piece of paper or however you prefer to do it. Your testing is
extremely limited as we know, you don't even have a null-modem cable.
So if the patch does not trip in your testing it tells you absolutely
nothing. But even in context of AKPM's tree you can't rely on run-time
checks to pick this sort of things.

And putting a BUG in there is irresponsible too. It's such a critical
subsystem. Drop bytes or return zero modem lines, but do not bug out.

-- Pete

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Wed, 14 Jun 2006 17:53:08 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Wed, 14 Jun 2006 17:38:24 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| >  I think BUG_ON(in_interrupt()) does the job. I'll try it here, and
| > if it doesn't trigger I'll submit a patch to Andrew only for
| > testing porpuses (ie, not for mainline).
| 
| Luiz, you can't be serious! You have to do a review and write call paths
| on a piece of paper or however you prefer to do it. Your testing is
| extremely limited as we know, you don't even have a null-modem cable.
| So if the patch does not trip in your testing it tells you absolutely
| nothing. But even in context of AKPM's tree you can't rely on run-time
| checks to pick this sort of things.

 Hey, take it easy. :)

 I won't test it in my patches. I'll hack the Serial Core code and add
debug code just before every call to those functions we want to know
whether they run in interrupt context or not.

 Yeah, I know, it's still limited because the driver itself can call its
methods directly in interrupt context. But I think it's a good start.

| And putting a BUG in there is irresponsible too. It's such a critical
| subsystem. Drop bytes or return zero modem lines, but do not bug out.

 Well, I want the easier, fastest and non-questionable way to know whether
they are called from an interrupt context or not. The first thing that
came to my mind was: blow up everything if it has been called in
interrupt context.

 But I'm open for suggestions, of course.

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Greg KH]

On Thu, Jun 15, 2006 at 10:29:40AM -0300, Luiz Fernando N. Capitulino wrote:
> On Wed, 14 Jun 2006 17:53:08 -0700
> Pete Zaitcev <zaitcev@redhat.com> wrote:
> 
> | On Wed, 14 Jun 2006 17:38:24 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> | 
> | >  I think BUG_ON(in_interrupt()) does the job. I'll try it here, and
> | > if it doesn't trigger I'll submit a patch to Andrew only for
> | > testing porpuses (ie, not for mainline).
> | 
> | Luiz, you can't be serious! You have to do a review and write call paths
> | on a piece of paper or however you prefer to do it. Your testing is
> | extremely limited as we know, you don't even have a null-modem cable.
> | So if the patch does not trip in your testing it tells you absolutely
> | nothing. But even in context of AKPM's tree you can't rely on run-time
> | checks to pick this sort of things.
> 
>  Hey, take it easy. :)
> 
>  I won't test it in my patches. I'll hack the Serial Core code and add
> debug code just before every call to those functions we want to know
> whether they run in interrupt context or not.
> 
>  Yeah, I know, it's still limited because the driver itself can call its
> methods directly in interrupt context. But I think it's a good start.
> 
> | And putting a BUG in there is irresponsible too. It's such a critical
> | subsystem. Drop bytes or return zero modem lines, but do not bug out.
> 
>  Well, I want the easier, fastest and non-questionable way to know whether
> they are called from an interrupt context or not. The first thing that
> came to my mind was: blow up everything if it has been called in
> interrupt context.
> 
>  But I'm open for suggestions, of course.

WARN_ON(in_interrupt());
is much nicer.  It gives you a full dump, yet lets the machine keep
working so that users can actually give you the bug report.

thanks,

greg k-h

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Thu, 15 Jun 2006 09:07:05 -0700
Greg KH <gregkh@suse.de> wrote:

| On Thu, Jun 15, 2006 at 10:29:40AM -0300, Luiz Fernando N. Capitulino wrote:
| > On Wed, 14 Jun 2006 17:53:08 -0700
| > Pete Zaitcev <zaitcev@redhat.com> wrote:
| > 
| > | On Wed, 14 Jun 2006 17:38:24 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| > | 
| > | >  I think BUG_ON(in_interrupt()) does the job. I'll try it here, and
| > | > if it doesn't trigger I'll submit a patch to Andrew only for
| > | > testing porpuses (ie, not for mainline).
| > | 
| > | Luiz, you can't be serious! You have to do a review and write call paths
| > | on a piece of paper or however you prefer to do it. Your testing is
| > | extremely limited as we know, you don't even have a null-modem cable.
| > | So if the patch does not trip in your testing it tells you absolutely
| > | nothing. But even in context of AKPM's tree you can't rely on run-time
| > | checks to pick this sort of things.
| > 
| >  Hey, take it easy. :)
| > 
| >  I won't test it in my patches. I'll hack the Serial Core code and add
| > debug code just before every call to those functions we want to know
| > whether they run in interrupt context or not.
| > 
| >  Yeah, I know, it's still limited because the driver itself can call its
| > methods directly in interrupt context. But I think it's a good start.
| > 
| > | And putting a BUG in there is irresponsible too. It's such a critical
| > | subsystem. Drop bytes or return zero modem lines, but do not bug out.
| > 
| >  Well, I want the easier, fastest and non-questionable way to know whether
| > they are called from an interrupt context or not. The first thing that
| > came to my mind was: blow up everything if it has been called in
| > interrupt context.
| > 
| >  But I'm open for suggestions, of course.
| 
| WARN_ON(in_interrupt());
| is much nicer.  It gives you a full dump, yet lets the machine keep
| working so that users can actually give you the bug report.

 Ok. Working on it right now.

 BTW, forgot to say, I do have a null-modem cable now. As soon as
ftdi_sio is ported, we can do all the missing tests.

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Wed, 14 Jun 2006 16:28:09 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

| On Tue, Jun 13, 2006 at 07:28:29PM -0300, Luiz Fernando N. Capitulino wrote:
| >  I took a look in the Serial Core code and didn't see why set_termios()
| > and break_ctl() (plus tx_empty()) are not allowed to sleep: they doesn't
| > seem to run in atomic context. So, are they allowed to sleep? Isn't the
| > documentation out of date? I've even submitted a patch to fix it [2].
| 
| You are correct - and I will eventually apply your patch.  At the
| moment, I'm throttling back on applying patches so that 2.6.17 can
| finally appear (I don't want to be responsible for Linus saying
| again "too many changes for -final, let's do another -rc".)
| 
| >  For get_mctrl() and set_mctrl() it seems possible to switch from a
| > spinlock to a mutex, as they are not called from an interrupt context.
| > Is this really possible? Would you agree with this change?
| 
| I don't know - that depends whether the throttle/unthrottle driver
| methods are ever called from interrupt context or not.
| 
| What we could do is put a WARN_ON() or might_sleep() in there and
| find out over time if they are called from non-process context.

 Ok, I've put a WARN_ON(in_interrupt()) (as suggested by Greg) in
all the functions which grabs the 'port->lock' spinlock and didn't
get anything with my tests (PPP through a standard modem, serial
console and other simple tests).

 I could submit that debug patch to Andrew, but now I'm wondering
whether the switch is really possible (considering, of course, that
those functions are not called from interrupt context).

 Pete, was it your original idea to completely move from the spinlock
to a mutex?

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Pete Zaitcev]

On Tue, 20 Jun 2006 16:11:34 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

>  Pete, was it your original idea to completely move from the spinlock
> to a mutex?

I thought it was the cleanest solution. But perhaps I miss something.
I'll look at your reposted patch again, maybe it's all right as it is.

-- Pete

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Tue, 20 Jun 2006 19:32:33 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Tue, 20 Jun 2006 16:11:34 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| >  Pete, was it your original idea to completely move from the spinlock
| > to a mutex?
| 
| I thought it was the cleanest solution. But perhaps I miss something.
| I'll look at your reposted patch again, maybe it's all right as it is.

 Actually, that's the best solution from the USB-Serial's POV.

 The problem is that several serial drivers uses the uart_port's
spinlock to implement their own locking, and some of them acquires the
lock in its interrupt handler...

 Sh*t.

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Russell King]

On Wed, Jun 21, 2006 at 01:35:00PM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 20 Jun 2006 19:32:33 -0700
> Pete Zaitcev <zaitcev@redhat.com> wrote:
> 
> | On Tue, 20 Jun 2006 16:11:34 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> | 
> | >  Pete, was it your original idea to completely move from the spinlock
> | > to a mutex?
> | 
> | I thought it was the cleanest solution. But perhaps I miss something.
> | I'll look at your reposted patch again, maybe it's all right as it is.
> 
>  Actually, that's the best solution from the USB-Serial's POV.
> 
>  The problem is that several serial drivers uses the uart_port's
> spinlock to implement their own locking, and some of them acquires the
> lock in its interrupt handler...
> 
>  Sh*t.

It all depends what you are locking.

In the uart_update_mctrl() case, the purpose of the locking is to
prevent two concurrent changes to the modem control state resulting
in an inconsistency between the hardware and the software state.  If
it's provable that it is always called from process context (and
it isn't called from a lock_kernel()-section or the lock_kernel()
section doesn't mind a rescheduling point being introduced there),
there's no problem converting that to a mutex.

I suspect that it needed to be a spinlock back in the days when
the low latency mode directly called into the ldisc, which could
then call back into the driver again from interrupt mode.

With get_mctrl(), the situation is slightly more complicated, because
we need to atomically update tty->hw_stopped in some circumstances
(that may also be modified from irq context.)  Therefore, to give
the driver a consistent locking picture, the spinlock is _always_
held.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Wed, 21 Jun 2006 17:43:36 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

| In the uart_update_mctrl() case, the purpose of the locking is to
| prevent two concurrent changes to the modem control state resulting
| in an inconsistency between the hardware and the software state.  If
| it's provable that it is always called from process context (and
| it isn't called from a lock_kernel()-section or the lock_kernel()
| section doesn't mind a rescheduling point being introduced there),
| there's no problem converting that to a mutex.

 Ok, then I can submit my debug patch to answer these questions.

 might_sleep() can catch the lock_kernel()-section case right?

| With get_mctrl(), the situation is slightly more complicated, because
| we need to atomically update tty->hw_stopped in some circumstances
| (that may also be modified from irq context.)  Therefore, to give
| the driver a consistent locking picture, the spinlock is _always_
| held.

 Is it too bad (wrong?) to only protect the tty->hw_stopped update
by the spinlock? Then the call to get_mctrl() could be protected by
a mutex, or is it messy?

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Russell King]

On Wed, Jun 21, 2006 at 06:15:13PM -0300, Luiz Fernando N. Capitulino wrote:
> On Wed, 21 Jun 2006 17:43:36 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 
> | In the uart_update_mctrl() case, the purpose of the locking is to
> | prevent two concurrent changes to the modem control state resulting
> | in an inconsistency between the hardware and the software state.  If
> | it's provable that it is always called from process context (and
> | it isn't called from a lock_kernel()-section or the lock_kernel()
> | section doesn't mind a rescheduling point being introduced there),
> | there's no problem converting that to a mutex.
> 
>  Ok, then I can submit my debug patch to answer these questions.
> 
>  might_sleep() can catch the lock_kernel()-section case right?
> 
> | With get_mctrl(), the situation is slightly more complicated, because
> | we need to atomically update tty->hw_stopped in some circumstances
> | (that may also be modified from irq context.)  Therefore, to give
> | the driver a consistent locking picture, the spinlock is _always_
> | held.
> 
>  Is it too bad (wrong?) to only protect the tty->hw_stopped update
> by the spinlock? Then the call to get_mctrl() could be protected by
> a mutex, or is it messy?

Consider this scenario with what you're proposing:

	thread				irq

	take mutex
	get_mctrl
					cts changes state
					take port lock
					mctrl state read
					tty->hw_stopped changed state
					release port lock
	releaes mutex
	take port lock
	update tty->hw_stopped
	release port lock

Now, tty->hw_stopped does not reflect the hardware state, which will be
buggy and can cause a loss of transmission.

I'm not sure what to suggest on this one since for USB drivers you do
need to be able to sleep in this method... but for UARTs you must not.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Thu, 22 Jun 2006 09:29:40 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

| On Wed, Jun 21, 2006 at 06:15:13PM -0300, Luiz Fernando N. Capitulino wrote:
|
| > | With get_mctrl(), the situation is slightly more complicated, because
| > | we need to atomically update tty->hw_stopped in some circumstances
| > | (that may also be modified from irq context.)  Therefore, to give
| > | the driver a consistent locking picture, the spinlock is _always_
| > | held.
| > 
| >  Is it too bad (wrong?) to only protect the tty->hw_stopped update
| > by the spinlock? Then the call to get_mctrl() could be protected by
| > a mutex, or is it messy?
| 
| Consider this scenario with what you're proposing:
| 
| 	thread				irq
| 
| 	take mutex
| 	get_mctrl
| 					cts changes state
| 					take port lock
| 					mctrl state read
| 					tty->hw_stopped changed state
| 					release port lock
| 	releaes mutex
| 	take port lock
| 	update tty->hw_stopped
| 	release port lock
| 
| Now, tty->hw_stopped does not reflect the hardware state, which will be
| buggy and can cause a loss of transmission.
| 
| I'm not sure what to suggest on this one since for USB drivers you do
| need to be able to sleep in this method... but for UARTs you must not.

 Neither do I. :((

 I thought we could move the 'tty->hw_stopped' update to a workqueue
but it has the same problem you explained above...

 Greg, any suggestions?

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Greg KH]

On Fri, Jun 23, 2006 at 02:28:42PM -0300, Luiz Fernando N. Capitulino wrote:
> On Thu, 22 Jun 2006 09:29:40 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 
> | On Wed, Jun 21, 2006 at 06:15:13PM -0300, Luiz Fernando N. Capitulino wrote:
> |
> | > | With get_mctrl(), the situation is slightly more complicated, because
> | > | we need to atomically update tty->hw_stopped in some circumstances
> | > | (that may also be modified from irq context.)  Therefore, to give
> | > | the driver a consistent locking picture, the spinlock is _always_
> | > | held.
> | > 
> | >  Is it too bad (wrong?) to only protect the tty->hw_stopped update
> | > by the spinlock? Then the call to get_mctrl() could be protected by
> | > a mutex, or is it messy?
> | 
> | Consider this scenario with what you're proposing:
> | 
> | 	thread				irq
> | 
> | 	take mutex
> | 	get_mctrl
> | 					cts changes state
> | 					take port lock
> | 					mctrl state read
> | 					tty->hw_stopped changed state
> | 					release port lock
> | 	releaes mutex
> | 	take port lock
> | 	update tty->hw_stopped
> | 	release port lock
> | 
> | Now, tty->hw_stopped does not reflect the hardware state, which will be
> | buggy and can cause a loss of transmission.
> | 
> | I'm not sure what to suggest on this one since for USB drivers you do
> | need to be able to sleep in this method... but for UARTs you must not.
> 
>  Neither do I. :((
> 
>  I thought we could move the 'tty->hw_stopped' update to a workqueue
> but it has the same problem you explained above...
> 
>  Greg, any suggestions?

Nope, sorry, I don't know what to suggest :(

greg k-h

Re: Serial-Core: USB-Serial port current issues.

[Paul Fulghum]

On Mon, 2006-06-26 at 15:26 -0700, Greg KH wrote:
> On Fri, Jun 23, 2006 at 02:28:42PM -0300, Luiz Fernando N. Capitulino wrote:
> > On Thu, 22 Jun 2006 09:29:40 +0100
> > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 
> > | 
> > | Consider this scenario with what you're proposing:
> > | 
> > | 	thread				irq
> > | 
> > | 	take mutex
> > | 	get_mctrl
> > | 					cts changes state
> > | 					take port lock
> > | 					mctrl state read
> > | 					tty->hw_stopped changed state
> > | 					release port lock
> > | 	releaes mutex
> > | 	take port lock
> > | 	update tty->hw_stopped
> > | 	release port lock
> > | 
> > | Now, tty->hw_stopped does not reflect the hardware state, which will be
> > | buggy and can cause a loss of transmission.
> > | 
> > | I'm not sure what to suggest on this one since for USB drivers you do
> > | need to be able to sleep in this method... but for UARTs you must not.

What about this ugly fragment?
(assuming get_mctrl not called from IRQ)

  take mutex
  take port lock
again:
  save local copy of icount
  release port lock
  get_mctrl
  take port lock
  if (icount changed)
    goto again
  update tty->hw_stopped
  release port lock
  release mutex

--
Paul

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

 Hi Paul,

On Mon, 26 Jun 2006 19:49:09 -0500
Paul Fulghum <paulkf@microgate.com> wrote:

| On Mon, 2006-06-26 at 15:26 -0700, Greg KH wrote:
| > On Fri, Jun 23, 2006 at 02:28:42PM -0300, Luiz Fernando N. Capitulino wrote:
| > > On Thu, 22 Jun 2006 09:29:40 +0100
| > > Russell King <rmk+lkml@arm.linux.org.uk> wrote:
| > > 
| > > | 
| > > | Consider this scenario with what you're proposing:
| > > | 
| > > | 	thread				irq
| > > | 
| > > | 	take mutex
| > > | 	get_mctrl
| > > | 					cts changes state
| > > | 					take port lock
| > > | 					mctrl state read
| > > | 					tty->hw_stopped changed state
| > > | 					release port lock
| > > | 	releaes mutex
| > > | 	take port lock
| > > | 	update tty->hw_stopped
| > > | 	release port lock
| > > | 
| > > | Now, tty->hw_stopped does not reflect the hardware state, which will be
| > > | buggy and can cause a loss of transmission.
| > > | 
| > > | I'm not sure what to suggest on this one since for USB drivers you do
| > > | need to be able to sleep in this method... but for UARTs you must not.
| 
| What about this ugly fragment?
| (assuming get_mctrl not called from IRQ)

 What's the problem with get_mctrl() being called from IRQ?

 Note that get_mctrl() is a callback and the driver is free to call _its_
get_mctrl() from IRQ if it wants to.

|   take mutex
|   take port lock
| again:
|   save local copy of icount
|   release port lock
|   get_mctrl
|   take port lock
|   if (icount changed)
|     goto again
|   update tty->hw_stopped
|   release port lock
|   release mutex

 Well, I think it'd work. But how can we keep track of 'icount'?
Should the driver add 1 if it updates 'tty->hw_stopped'?

PS: Sorry for the long delay, I was off last week.

-- 
Luiz Fernando N. Capitulino

Re: Serial-Core: USB-Serial port current issues.

[Pete Zaitcev]

On Tue, 4 Jul 2006 16:42:57 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

>  Note that get_mctrl() is a callback and the driver is free to call
> _its_ get_mctrl() from IRQ if it wants to.

What do you think "a callback" is? The get_mctrl may be a method,
but it's certainly not a callback. A callback is something being
called from bottom to the the top, e.g. (*urb->complete)().

-- Pete

Re: Serial-Core: USB-Serial port current issues.

[Luiz Fernando N. Capitulino]

On Tue, 4 Jul 2006 12:50:46 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Tue, 4 Jul 2006 16:42:57 -0300, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| >  Note that get_mctrl() is a callback and the driver is free to call
| > _its_ get_mctrl() from IRQ if it wants to.
| 
| What do you think "a callback" is? The get_mctrl may be a method,
| but it's certainly not a callback. A callback is something being
| called from bottom to the the top, e.g. (*urb->complete)().

 I thought Paul was referring to the driver's *callback* function, but
now I just realized that he was referring to the Serial Core's get_mctrl()
method.

 Then his comment makes sense.

-- 
Luiz Fernando N. Capitulino

Re: [linux-usb-devel] Serial-Core: USB-Serial port current issues.

[Paul Fulghum]

Luiz Fernando N. Capitulino wrote:
> |   take mutex
> |   take port lock
> | again:
> |   save local copy of icount
> |   release port lock
> |   get_mctrl
> |   take port lock
> |   if (icount changed)
> |     goto again
> |   update tty->hw_stopped
> |   release port lock
> |   release mutex
> 
>  Well, I think it'd work. But how can we keep track of 'icount'?
> Should the driver add 1 if it updates 'tty->hw_stopped'?

The only thing about icount that needs to be
tracked is that it changes, which indicates
an interrupt might have changed hw_stopped.
If icount changes at all, invalidate the last
reading of the state and do it again. The way
icount is incremented is not changed.

Like I said, it is really ugly. I was just looking
for a way of allowing get_mctrl to sleep if necessary.

-- 
Paul Fulghum
Microgate Systems, Ltd.