On Thu, May 24, 2012 at 10:40:34AM -0400, Alan Stern wrote:
> [Added Alan Cox to the CC: list]
>
> On Thu, 24 May 2012, Jan Safrata wrote:
>
> > On 17:09 Wed 23 May , Johan Hovold wrote:
> > > On Wed, May 23, 2012 at 07:12:39AM +0200, Jan Safrata wrote:
> > > > On 16:06 Tue 22 May , Johan Hovold wrote:
> > > > > On Tue, May 22, 2012 at 02:04:37PM +0200, Jan Safrata wrote:
> > > > > > Call Trace:
> > > > > > [<f85e7c60>] usb_wwan_dtr_rts+0x60/0xa0 [usb_wwan]
> > > > >
> > > > > This is not right. You're fixing a race with disconnect in option's
> > > > > probe, but this indicates that there's a race with usb_wwan_dtr_rts (as
> > > > > well).
> > > >
> > > > > > @@ -1478,10 +1478,14 @@ static int option_send_setup(struct usb_serial_port *port)
> > > > > > struct usb_wwan_intf_private *intfdata =
> > > > > > (struct usb_wwan_intf_private *) serial->private;
> > > > > > struct usb_wwan_port_private *portdata;
> > > > > > - int ifNum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
> > > > > > + int ifNum;
> > > > > > int val = 0;
> > > > > > dbg("%s", __func__);
> > > > > >
> > > > > > + if (serial->disconnected)
> > > > > > + return -ENODEV;
> > > > > > +
> > > > > > + ifNum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
> > > > > > if (is_blacklisted(ifNum, OPTION_BLACKLIST_SENDSETUP,
> > > > > > (struct option_blacklist_info *) intfdata->private)) {
> > > > > > dbg("No send_setup on blacklisted interface #%d\n", ifNum);
> > > > >
> > > > > This is not quite right. You need to use the disconnect mutex as well,
> > > > > e.g.:
> > > > >
> > > > > mutex_lock(&serial->disc_mutex);
> > > > > if (serial->disconnected) {
> > > > > mutex_unlock(&serial->disc_mutex);
> > > > > return -ENODEV;
> > > > > }
> > > > > ifNum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
> > > > > ...
> > > > > ret = usb_control_msg(serial->dev, ...)
> > > > > mutex_unlock(&serial->disc_mutex);
> > > > >
> > > > > return ret;
> > > > >
> > > > > Johan
> > > >
> > > > The mutex cannot be used in the option_send_setup() function, because it
> > > > is already held in usb_wwan_dtr_rts() which calls the option_send_setup(),
> > > > so simple check of serial->disconnected flag should be ok there.
> > >
> > > Ah. I misread option_probe (only sets the send_setup pointer).
> > >
> > > Hmmm. And you can't push the locking down into send_setup because it's
> > > also called from open (which is also called with disc_mutex held)...
> > >
> > > What about the other call sites in usb_wwan (in set_termios and tiocmset)?
> > > Shouldn't these be protected by the disc_mutex as well? And wouldn't it
> > > then be cleaner to check the disconnected flag where the lock is taken
> > > (i.e., in usb_wwan)?
> >
> > I've also thought about set_termios and tiocmset, but it seems that the
> > oops does not happen with them - I guess that's because tty_vhangup() is
> > called from usb_serial_disconnect(), replacing tty fops with
> > hung_up_tty_fops, so that set_termios and tiocmset could not be called
> > anymore (from tty ioctl).
> >
> > The oops I was trying to fix happens only with open() and only when the
> > tty device is already opened at least one time (as the code to reproduce
> > demonstrates). In that case tty_open() calls tty_release() getting to
> > usb_wwan_dtr_rts() in disconnected state.
> >
> > My understanding of locking in tty layer and it's subdrivers is not so deep
> > so I did not dare to change something in that - it might be that checking of
> > serial->disconnected would not be needed in option_send_setup() nor in
> > usb_wwan_dtr_rts() if the protection via hung_up_tty_fops would not
> > leave a hole in tty_open/tty_release. Maybe the proper fix should be
> > done somewhere in tty_release/tty_port_close_start/tty_port_lower_dtr_rts?
>
> You may very well be right about that. A good indication would be if
> other serial drivers have the same sort of potential race.
>
> Also, wasn't the DTR/RTS stuff modified just recently?
Well tty-core shouldn't handle disconnect (which is usb-serial-
specific), but I see no reason not to move the disconnect handling up to
usb-serial core. It does not make sense for usb-serial drivers to
manipulate modem control-signals after the device is gone. Might as well
deal with in core as we do for open.
As for tiocmset, only pl2303 and opticon currently checks for the
disconnect flag. Either all driver need to, or these do not. Perhaps
Alan Cox could provide some input on this?
I'll respond to this mail with a patch (RFC) which moves the disconnect
handling into usb-serial. I'll have a closer look at tiocmset later and
perhaps prepare something similar for that case depending on what Alan
says.
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
