On Thu, 2009-08-13 at 16:54 +0100, Alan Cox wrote:
> > +static int qt2_ioctl(struct tty_struct *tty, struct file *file,
> > + unsigned int cmd, unsigned long arg)
>
> This looks pretty bogus.
>
> TIOCMGET/TIOCMSET are not passed to the driver ioctl method but to your
> mget and mset operators so you can dump that lot in favour of the code
> you have later.
OK, the vendor driver not only had it in both places, but didn't do so
by calling the operators - it actually had the same code pasted in both
places. This patch removes the excess code and does some minor clean-up
of the remainder.
> > + } else if (cmd == TIOCMIWAIT) {
> > + dbg("%s() port %d, cmd == TIOCMIWAIT enter",
> > + __func__, port->number);
>
> This one if provided does need to be in the ioctl handler.
>
> > + prev_msr_value = port_extra->shadowMSR & SERIAL_MSR_MASK;
> > + while (1) {
> > + add_wait_queue(&port_extra->wait, &wait);
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule();
> > + dbg("%s(): port %d, cmd == TIOCMIWAIT here\n",
> > + __func__, port->number);
> > + remove_wait_queue(&port_extra->wait, &wait);
> > + /* see if a signal woke us up */
> > + if (signal_pending(current))
> > + return -ERESTARTSYS;
> > + msr_value = port_extra->shadowMSR & SERIAL_MSR_MASK;
> > + if (msr_value == prev_msr_value)
> > + return -EIO; /* no change - error */
>
> Why is this an error and not just a spurious wake up ?
Largely because it's what Greg K-H's example implementation of this function does ...
http://www.linuxjournal.com/article/6226
http://www.makelinux.net/ldd3/chp-18-sect-4.shtml
suggestions of what to return or whether to remove the test welcome, I
don't personally plan on using this ioctl, but as the vendor driver
includes it, I brought it along.
> What about disconnecting the adapter btw - shouldn't that break out of
> the loop with an error ?
I would think so, but I'm not sure how to achieve that. The crude
solution of inserting the line
port_extra->shadowMSR = !(port_extra->shadowMSR);
for each port into the release method, to ensure that the MSR always
changes on detach, comes to mind, but doesn't seem terribly elegant.
I've added a FIXME comment to this one.
Patch is on top of part 3 and the improvements to the write_room
implementation - I will re-send the patches in order once they are
nearer to ready.
Richard
Signed-off-by: Richard Ash <richard@xxxxxxxxxxxxxxxx>
---
Index: linux/drivers/staging/quatech_usb2/quatech_usb2.c
===================================================================
--- linux.orig/drivers/staging/quatech_usb2/quatech_usb2.c
+++ linux/drivers/staging/quatech_usb2/quatech_usb2.c
@@ -860,7 +860,10 @@ static int qt2_chars_in_buffer(struct tt
return chars;
}
-
+/* called when userspace does an ioctl() on the device. Note that
+ * TIOCMGET and TIOCMSET are filtered off to their own methods before they get
+ * here, so we don't have to handle them.
+ */
static int qt2_ioctl(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg)
{
@@ -887,22 +890,12 @@ static int qt2_ioctl(struct tty_struct *
dbg("%s(): port %d, UartNumber %d, tty =0x%p", __func__,
port->number, UartNumber, tty);
- if (cmd == TIOCMGET) {
- return qt2_tiocmget(tty, file);
- /* same as tiocmget function */
- } else if (cmd == TIOCMSET) {
- if (copy_from_user(&value, (unsigned int *)arg,
- sizeof(unsigned int)))
- return -EFAULT;
- return qt2_tiocmset(tty, file, value, 0);
- /* same as tiocmset function */
- } else if (cmd == TIOCMBIS || cmd == TIOCMBIC) {
- status = qt2_box_get_register(port->serial, UartNumber,
- QT2_MODEM_CONTROL_REGISTER, &mcr_value);
- if (status < 0)
+ if (cmd == TIOCMBIS || cmd == TIOCMBIC) {
+ if (qt2_box_get_register(port->serial, UartNumber,
+ QT2_MODEM_CONTROL_REGISTER, &mcr_value) < 0)
return -ESPIPE;
if (copy_from_user(&value, (unsigned int *)arg,
- sizeof(unsigned int)))
+ sizeof(value)))
return -EFAULT;
switch (cmd) {
@@ -925,9 +918,8 @@ static int qt2_ioctl(struct tty_struct *
default:
break;
} /* end of local switch on cmd */
- status = qt2_box_set_register(port->serial, UartNumber,
- QT2_MODEM_CONTROL_REGISTER, mcr_value);
- if (status < 0) {
+ if (qt2_box_set_register(port->serial, UartNumber,
+ QT2_MODEM_CONTROL_REGISTER, mcr_value) < 0) {
return -ESPIPE;
} else {
port_extra->shadowMCR = mcr_value;
@@ -965,6 +957,10 @@ static int qt2_ioctl(struct tty_struct *
return 0;
}
} /* end inifinite while */
+ /* FIXME: This while loop needs a way to break out if the device
+ * is disconnected while a process is waiting for the MSR to
+ * change, because once it's disconnected, it isn't going to
+ * change state ... */
} else {
/* any other ioctls we don't know about come here */
dbg("%s(): No ioctl for that one. port = %d", __func__,
--
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
