[PATCH 0/5] tty_port_open
Alan Cox
drivers, which nicely fixes a couple of races and cleans up the code.
---
Alan Cox (5):
opticon: Fix resume logic
usb_serial: Kill port mutex
usb_serial: Use the shutdown() operation
tty_port: coding style cleaning pass
tty_port: add "tty_port_open" helper
drivers/char/tty_port.c | 50 ++++++++++++++++++++---
drivers/usb/serial/opticon.c | 7 ++-
drivers/usb/serial/usb-serial.c | 83 +++++++++++++--------------------------
include/linux/tty.h | 10 ++++-
include/linux/usb/serial.h | 3 -
5 files changed, 83 insertions(+), 70 deletions(-)
--
The Zenburger: One with everything
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Alan Cox
don't need the one in the USB serial layer any more.
Signed-off-by: Alan Cox <al...@linux.intel.com>
---
drivers/usb/serial/opticon.c | 4 ++--
drivers/usb/serial/usb-serial.c | 1 -
include/linux/usb/serial.h | 3 ---
3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index 1085a57..c3a9ce2 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -498,12 +498,12 @@ static int opticon_resume(struct usb_interface *intf)
struct usb_serial_port *port = serial->port[0];
int result;
- mutex_lock(&port->mutex);
+ mutex_lock(&port->port.mutex);
if (port->port.count)
result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);
else
result = 0;
- mutex_unlock(&port->mutex);
+ mutex_unlock(&port->port.mutex);
return result;
}
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index e189338..421ef1f 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -896,7 +896,6 @@ int usb_serial_probe(struct usb_interface *interface,
spin_lock_init(&port->lock);
/* Keep this for private driver use for the moment but
should probably go away */
- mutex_init(&port->mutex);
INIT_WORK(&port->work, usb_serial_port_work);
serial->port[i] = port;
port->dev.parent = &interface->dev;
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index c17eb64..ad24c8c 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -39,8 +39,6 @@ enum port_dev_state {
* @serial: pointer back to the struct usb_serial owner of this port.
* @port: pointer to the corresponding tty_port for this port.
* @lock: spinlock to grab when updating portions of this structure.
- * @mutex: mutex used to synchronize serial_open() and serial_close()
- * access for this port.
* @number: the number of the port (the minor number).
* @interrupt_in_buffer: pointer to the interrupt in buffer for this port.
* @interrupt_in_urb: pointer to the interrupt in struct urb for this port.
@@ -77,7 +75,6 @@ struct usb_serial_port {
struct usb_serial *serial;
struct tty_port port;
spinlock_t lock;
- struct mutex mutex;
unsigned char number;
unsigned char *interrupt_in_buffer;
Alan Cox
Signed-off-by: Alan Cox <al...@linux.intel.com>
---
drivers/char/tty_port.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/char/tty_port.c b/drivers/char/tty_port.c
index 2512262..c16ba2d 100644
--- a/drivers/char/tty_port.c
+++ b/drivers/char/tty_port.c
@@ -199,7 +199,7 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
* management of these lines. Note that the dtr/rts raise is done each
* iteration as a hangup may have previously dropped them while we wait.
*/
-
+
int tty_port_block_til_ready(struct tty_port *port,
struct tty_struct *tty, struct file *filp)
{
@@ -248,7 +248,8 @@ int tty_port_block_til_ready(struct tty_port *port,
tty_port_raise_dtr_rts(port);
prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
- /* Check for a hangup or uninitialised port. Return accordingly */
+ /* Check for a hangup or uninitialised port.
+ Return accordingly */
if (tty_hung_up_p(filp) || !(port->flags & ASYNC_INITIALIZED)) {
if (port->flags & ASYNC_HUP_NOTIFY)
retval = -EAGAIN;
@@ -280,11 +281,11 @@ int tty_port_block_til_ready(struct tty_port *port,
port->flags |= ASYNC_NORMAL_ACTIVE;
spin_unlock_irqrestore(&port->lock, flags);
return retval;
-
}
EXPORT_SYMBOL(tty_port_block_til_ready);
-int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct file *filp)
+int tty_port_close_start(struct tty_port *port,
+ struct tty_struct *tty, struct file *filp)
{
unsigned long flags;
@@ -294,7 +295,7 @@ int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct f
return 0;
}
- if( tty->count == 1 && port->count != 1) {
+ if (tty->count == 1 && port->count != 1) {
printk(KERN_WARNING
"tty_port_close_start: tty->count = 1 port count = %d.\n",
port->count);
@@ -326,8 +327,8 @@ int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct f
long timeout;
if (bps > 1200)
- timeout = max_t(long, (HZ * 10 * port->drain_delay) / bps,
- HZ / 10);
+ timeout = max_t(long,
+ (HZ * 10 * port->drain_delay) / bps, HZ / 10);
else
timeout = 2 * HZ;
schedule_timeout_interruptible(timeout);
@@ -378,7 +379,7 @@ void tty_port_close(struct tty_port *port, struct tty_struct *tty,
EXPORT_SYMBOL(tty_port_close);
int tty_port_open(struct tty_port *port, struct tty_struct *tty,
- struct file *filp)
+ struct file *filp)
{
spin_lock_irq(&port->lock);
if (!tty_hung_up_p(filp))
@@ -398,14 +399,13 @@ int tty_port_open(struct tty_port *port, struct tty_struct *tty,
if (port->ops->activate) {
int retval = port->ops->activate(port, tty);
if (retval) {
- mutex_unlock(&port->mutex);
- return retval;
- }
- }
+ mutex_unlock(&port->mutex);
+ return retval;
+ }
+ }
set_bit(ASYNCB_INITIALIZED, &port->flags);
}
mutex_unlock(&port->mutex);
return tty_port_block_til_ready(port, tty, filp);
-}
-
+}
EXPORT_SYMBOL(tty_port_open);
Oliver Neukum
> +++ b/drivers/usb/serial/opticon.c
> @@ -498,12 +498,12 @@ static int opticon_resume(struct usb_interface *intf)
> struct usb_serial_port *port = serial->port[0];
> int result;
>
> - mutex_lock(&port->mutex);
> + mutex_lock(&port->port.mutex);
> if (port->port.count)
> result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);
> else
> result = 0;
> - mutex_unlock(&port->mutex);
> + mutex_unlock(&port->port.mutex);
> return result;
> }
We will need some generic way to autoresume from open.
Resume will need to lock against open() and need to be called
from within open(). Any ideas for an unugly interface?
Regards
Oliver
Alan Stern
> We will need some generic way to autoresume from open.
> Resume will need to lock against open() and need to be called
> from within open(). Any ideas for an unugly interface?
It's not quite that bad. Resume doesn't need to lock against open.
If open is called while resume is running then when it tries to do its
own resume, it will either block (waiting for the pm_mutex) or return
immediately (if it sees the device is already resumed).
A more interesting question is how to synchronize both open/close and
suspend/resume against throttle/unthrottle.
Alan Stern
Oliver Neukum
> On Wed, 7 Oct 2009, Oliver Neukum wrote:
> > We will need some generic way to autoresume from open.
> > Resume will need to lock against open() and need to be called
> > from within open(). Any ideas for an unugly interface?
>
> It's not quite that bad. �Resume doesn't need to lock against open.
> If open is called while resume is running then when it tries to do its
> own resume, it will either block (waiting for the pm_mutex) or return
> immediately (if it sees the device is already resumed).
But resume() needs to know whether the read URBs need to be
submitted or not.
Regards
Oliver
Alan Stern
> Am Mittwoch, 7. Oktober 2009 18:03:08 schrieb Alan Stern:
> > On Wed, 7 Oct 2009, Oliver Neukum wrote:
> > > We will need some generic way to autoresume from open.
> > > Resume will need to lock against open() and need to be called
> > > from within open(). Any ideas for an unugly interface?
> >
> > It's not quite that bad. �Resume doesn't need to lock against open.
> > If open is called while resume is running then when it tries to do its
> > own resume, it will either block (waiting for the pm_mutex) or return
> > immediately (if it sees the device is already resumed).
>
> But resume() needs to know whether the read URBs need to be
> submitted or not.
Given that there are several pathways for turning on or turning off the
read URBs, the best answer is to use a flag.
Alan Stern
Alan Cox
Alan Stern <st...@rowland.harvard.edu> wrote:
> On Wed, 7 Oct 2009, Oliver Neukum wrote:
>
> > We will need some generic way to autoresume from open.
> > Resume will need to lock against open() and need to be called
> > from within open(). Any ideas for an unugly interface?
>
> It's not quite that bad. Resume doesn't need to lock against open.
> If open is called while resume is running then when it tries to do its
> own resume, it will either block (waiting for the pm_mutex) or return
> immediately (if it sees the device is already resumed).
It would probably be cleaner if they could lock against each other
> A more interesting question is how to synchronize both open/close and
> suspend/resume against throttle/unthrottle.
throttle and unthrottle can sleep and obviously have to as they do a fair
bit of work sometimes (xon/xoff, mode line waggling etc)
The current ordering here is quite ugly because we open the ldisc before
the tty which means the ldisc sometimes calls unthrottle before the tty
is opened which is not nice. On the close side we have the same thing via
tty_ldisc_release.
We can take the port->mutex lock in the throttle/unthrottle methods as
far as I can see - there are no obvious problem cases. We do call
->throttle and ->unthrottle from the ldisc open but this occurs outside
of any call to the tty driver open/close method so I don't see any
deadlock.
It adds an ordering of termios lock before port mutex when taking both
but that's not a problem and really implicit in the structure of the code
anyway.
Alan
Alan Stern
> On Wed, 7 Oct 2009 12:03:08 -0400 (EDT)
> Alan Stern <st...@rowland.harvard.edu> wrote:
>
> > On Wed, 7 Oct 2009, Oliver Neukum wrote:
> >
> > > We will need some generic way to autoresume from open.
> > > Resume will need to lock against open() and need to be called
> > > from within open(). Any ideas for an unugly interface?
> >
> > It's not quite that bad. Resume doesn't need to lock against open.
> > If open is called while resume is running then when it tries to do its
> > own resume, it will either block (waiting for the pm_mutex) or return
> > immediately (if it sees the device is already resumed).
>
> It would probably be cleaner if they could lock against each other
What you mean isn't clear. After all, open sometimes has to call
resume. So how could resume lock against open?
> > A more interesting question is how to synchronize both open/close and
> > suspend/resume against throttle/unthrottle.
>
> throttle and unthrottle can sleep and obviously have to as they do a fair
> bit of work sometimes (xon/xoff, mode line waggling etc)
>
> The current ordering here is quite ugly because we open the ldisc before
> the tty which means the ldisc sometimes calls unthrottle before the tty
> is opened which is not nice. On the close side we have the same thing via
> tty_ldisc_release.
>
> We can take the port->mutex lock in the throttle/unthrottle methods as
> far as I can see - there are no obvious problem cases. We do call
> ->throttle and ->unthrottle from the ldisc open but this occurs outside
> of any call to the tty driver open/close method so I don't see any
> deadlock.
>
> It adds an ordering of termios lock before port mutex when taking both
> but that's not a problem and really implicit in the structure of the code
> anyway.
Does this imply that unthrottle should try to autoresume? There does
appear to be a potential race between unthrottle and autosuspend.
Alan Stern
Alan Cox
>
> What you mean isn't clear. After all, open sometimes has to call
> resume. So how could resume lock against open?
Probably it needs a counting lock as the code is currently structured -
which is a bit ugly. What paths do we end up going through the device
open method into resume in the same thread ?
> Does this imply that unthrottle should try to autoresume? There does
> appear to be a potential race between unthrottle and autosuspend.
The more I look at it the more it implies to me that the ldiscs doing
this should instead be taught some better manners instead. The real nasty
is that a driver might not have initialised the locking it needs do that
exclusion until open occurs. I think n_tty is probably the only offender
and if so I'd rather fix that and make it a rule that you don't do that,
trying to fix it other ways is going to be more horrible I imagine.
Alan Stern
> > > It would probably be cleaner if they could lock against each other
> >
> > What you mean isn't clear. After all, open sometimes has to call
> > resume. So how could resume lock against open?
>
> Probably it needs a counting lock as the code is currently structured -
> which is a bit ugly. What paths do we end up going through the device
> open method into resume in the same thread ?
Currently there are no such paths. I keep forgetting that the resume
is done in serial_install() rather than serial_open(). Eventually the
tty_port reorganization will probably force the resume to move into the
activate method.
However in the option and sierra drivers there is a perverse path from
close to resume: Both their close methods call
usb_autopm_get_interface(). This could be removed without much
trouble; perhaps we should do so.
Alan Stern
Oliver Neukum
> However in the option and sierra drivers there is a perverse path from
> close to resume: Both their close methods call
> usb_autopm_get_interface(). �This could be removed without much
> trouble; perhaps we should do so.
I am afraid this won't do in the long run. Some drivers will want to
shut down devices by communicating with them in close().
Regards
Oliver
Alan Cox
Oliver Neukum <oli...@neukum.org> wrote:
> Am Mittwoch, 7. Oktober 2009 20:52:21 schrieb Alan Stern:
> > However in the option and sierra drivers there is a perverse path from
> > close to resume: Both their close methods call
> > usb_autopm_get_interface(). �This could be removed without much
> > trouble; perhaps we should do so.
>
> I am afraid this won't do in the long run. Some drivers will want to
> shut down devices by communicating with them in close().
drivers/serial will need a power management hook to use
tty_port_{open/close} so perhaps that can be covered for both. In the
serial case it needs to kick stuff out of PCI D3 mostly and could
probably be fudged but if USB needs it perhaps it should be explicit.
Alan Stern
> On Wed, 7 Oct 2009 22:56:20 +0200
> Oliver Neukum <oli...@neukum.org> wrote:
>
> > Am Mittwoch, 7. Oktober 2009 20:52:21 schrieb Alan Stern:
> > > However in the option and sierra drivers there is a perverse path from
> > > close to resume: Both their close methods call
> > > usb_autopm_get_interface(). �This could be removed without much
> > > trouble; perhaps we should do so.
> >
> > I am afraid this won't do in the long run. Some drivers will want to
> > shut down devices by communicating with them in close().
>
> drivers/serial will need a power management hook to use
> tty_port_{open/close} so perhaps that can be covered for both. In the
> serial case it needs to kick stuff out of PCI D3 mostly and could
> probably be fudged but if USB needs it perhaps it should be explicit.
I'm losing track of the original point of this thread. IIRC, the
problem is how the resume method should know whether or not to submit
the receive URB(s). It can't afford to acquire the port mutex because
it might be called by open or close, at which time the mutex is already
held.
Other schemes could work, but to me it seems simplest to rely on a flag
protected by a spinlock. The flag would mean "URBs are supposed to be
queued unless we are suspended". It would be set by open and
unthrottle, and cleared by close and throttle.
Alan Stern
Oliver Neukum
> > We can take the port->mutex lock in the throttle/unthrottle methods as
> > far as I can see - there are no obvious problem cases. We do call
> > ->throttle and ->unthrottle from the ldisc open but this occurs outside
> > of any call to the tty driver open/close method so I don't see any
> > deadlock.
> >
> > It adds an ordering of termios lock before port mutex when taking both
> > but that's not a problem and really implicit in the structure of the code
> > anyway.
>
> Does this imply that unthrottle should try to autoresume? �There does
> appear to be a potential race between unthrottle and autosuspend.
The race exists. But I don't think unthrottle should autoresume.
It would be better to just set a flag and defer this to resume.
If the device supports remote wakeup there'll be no need to autoresume,
if not, throttle/unthrottle are too rare to justify the complexity.
Regards
Oliver
Oliver Neukum
> I'm losing track of the original point of this thread. �IIRC, the
> problem is how the resume method should know whether or not to submit
> the receive URB(s). �It can't afford to acquire the port mutex because
> it might be called by open or close, at which time the mutex is already
> held.
>
> Other schemes could work, but to me it seems simplest to rely on a flag
> protected by a spinlock. �The flag would mean "URBs are supposed to be
> queued unless we are suspended". �It would be set by open and
> unthrottle, and cleared by close and throttle.
1. Why a spinlock?
2. Can we get by with only one flag?
Regards
Oliver
Alan Stern
> Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
> > I'm losing track of the original point of this thread. �IIRC, the
> > problem is how the resume method should know whether or not to submit
> > the receive URB(s). �It can't afford to acquire the port mutex because
> > it might be called by open or close, at which time the mutex is already
> > held.
> >
> > Other schemes could work, but to me it seems simplest to rely on a flag
> > protected by a spinlock. �The flag would mean "URBs are supposed to be
> > queued unless we are suspended". �It would be set by open and
> > unthrottle, and cleared by close and throttle.
>
> 1. Why a spinlock?
Because the amount of work involved seems too small for a mutex. But
you could use a mutex if you wanted, since everything occurs in process
context.
> 2. Can we get by with only one flag?
If all you want to do is answer a single question ("Should URBs be
submitted") then a single flag should be all you need. Why, do you
think more information will be necessary? You can always add more.
Alan Stern
Oliver Neukum
> On Thu, 8 Oct 2009, Oliver Neukum wrote:
> > Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
> > > Other schemes could work, but to me it seems simplest to rely on a flag
> > > protected by a spinlock. �The flag would mean "URBs are supposed to be
> > > queued unless we are suspended". �It would be set by open and
> > > unthrottle, and cleared by close and throttle.
> >
> > 1. Why a spinlock?
>
> Because the amount of work involved seems too small for a mutex. But
> you could use a mutex if you wanted, since everything occurs in process
> context.
We have to submit URBs under that lock.
> > 2. Can we get by with only one flag?
>
> If all you want to do is answer a single question ("Should URBs be
> submitted") then a single flag should be all you need. Why, do you
> think more information will be necessary? You can always add more.
We have at least three reasons URBs should not be submitted.
- closure
- throttling
- suspension
Resume() should not submit if either closure or throttling are active,
neither should unthrottle() resubmit if closure or suspension are active.
Regards
Oliver
Alan Stern
> Am Donnerstag, 8. Oktober 2009 16:58:39 schrieb Alan Stern:
> > On Thu, 8 Oct 2009, Oliver Neukum wrote:
> > > Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
>
> > > > Other schemes could work, but to me it seems simplest to rely on a flag
> > > > protected by a spinlock. �The flag would mean "URBs are supposed to be
> > > > queued unless we are suspended". �It would be set by open and
> > > > unthrottle, and cleared by close and throttle.
> > >
> > > 1. Why a spinlock?
> >
> > Because the amount of work involved seems too small for a mutex. But
> > you could use a mutex if you wanted, since everything occurs in process
> > context.
>
> We have to submit URBs under that lock.
Yes. What's your point?
> > > 2. Can we get by with only one flag?
> >
> > If all you want to do is answer a single question ("Should URBs be
> > submitted") then a single flag should be all you need. Why, do you
> > think more information will be necessary? You can always add more.
>
> We have at least three reasons URBs should not be submitted.
> - closure
> - throttling
> - suspension
> Resume() should not submit if either closure or throttling are active,
> neither should unthrottle() resubmit if closure or suspension are active.
True. Nor should open() submit if throttling is active. Feel free to
use three separate flags. :-)
Alan Stern
Alan Stern
> > > > 2. Can we get by with only one flag?
> > >
> > > If all you want to do is answer a single question ("Should URBs be
> > > submitted") then a single flag should be all you need. Why, do you
> > > think more information will be necessary? You can always add more.
> >
> > We have at least three reasons URBs should not be submitted.
> > - closure
> > - throttling
> > - suspension
> > Resume() should not submit if either closure or throttling are active,
> > neither should unthrottle() resubmit if closure or suspension are active.
>
> True. Nor should open() submit if throttling is active. Feel free to
> use three separate flags. :-)
On further thought, unthrottle should autoresume if the device is
open and autosuspended (but it shouldn't do anything if the device is
suspended). After all, the reason for the autosuspend may have been
the lack of activity caused by the throttling.
In practice this isn't likely to come up. It would be surprising if
throttling lasted long enough to cause an autosuspend or if the core
decided to throttle while the device was autosuspended and hence idle.
Oliver Neukum
>
> On further thought, unthrottle should autoresume if the device is
> open and autosuspended (but it shouldn't do anything if the device is
> suspended). After all, the reason for the autosuspend may have been
> the lack of activity caused by the throttling.
>
> In practice this isn't likely to come up. It would be surprising if
> throttling lasted long enough to cause an autosuspend or if the core
> decided to throttle while the device was autosuspended and hence idle.
So you say that throttle() should do an autopm_put?
Regards
Oliver
Alan Cox
Oliver Neukum <oli...@neukum.org> wrote:
> Am Donnerstag, 8. Oktober 2009 22:06:10 schrieb Alan Stern:
>
> >
> > On further thought, unthrottle should autoresume if the device is
> > open and autosuspended (but it shouldn't do anything if the device is
> > suspended). After all, the reason for the autosuspend may have been
> > the lack of activity caused by the throttling.
> >
> > In practice this isn't likely to come up. It would be surprising if
> > throttling lasted long enough to cause an autosuspend or if the core
> > decided to throttle while the device was autosuspended and hence idle.
>
> So you say that throttle() should do an autopm_put?
You need to be very very sure you cannot lose a byte of data or have the
modem lines disrupted in any way if you do that.
Alan Stern
> Am Donnerstag, 8. Oktober 2009 22:06:10 schrieb Alan Stern:
>
> >
> > On further thought, unthrottle should autoresume if the device is
> > open and autosuspended (but it shouldn't do anything if the device is
> > suspended). After all, the reason for the autosuspend may have been
> > the lack of activity caused by the throttling.
> >
> > In practice this isn't likely to come up. It would be surprising if
> > throttling lasted long enough to cause an autosuspend or if the core
> > decided to throttle while the device was autosuspended and hence idle.
>
> So you say that throttle() should do an autopm_put?
The way you've coded the sierra and option drivers, it's not necessary.
Those drivers do an autopm_get_async during submission and an
autopm_put_async after the completion of every output URB (and they
update the last_busy time in the completion of every input URB). When
the driver is throttled no URBs will be submitted, so the usage count
will remain at 0 with no effort on the part of throttle().
For other drivers that use the simpler "autoresume on tty install,
autosuspend on tty cleanup" approach provided by usb-serial.c, the
throttle routines obviously don't need to worry about runtime PM.
Alan Stern
Alan Cox
> the driver is throttled no URBs will be submitted, so the usage count
> will remain at 0 with no effort on the part of throttle().
You can still get modem changes (set_mctrl), break etc with the port
throttled, or indeed output.