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?
Jan
--
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
