* Vizzini
@ 2012-09-19 15:23 Alan Cox
2012-09-21 15:43 ` Vizzini Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Alan Cox @ 2012-09-19 15:23 UTC (permalink / raw)
To: greg, linux-serial
/* This version of the Linux driver source contains a number of
abominable conditional compilation sections to manage the API
changes between kernel versions 2.6.18, 2.6.25, and the latest
(currently 2.6.27). At some point we'll hand a version of this
driver off to the mainline Linux source tree, and we'll strip
all these sections out. For now it makes it much easier to
keep it all in sync while the driver is being developed. */
It doesn't seem to
#include <linux/usb/cdc.h>
#ifndef CDC_DATA_INTERFACE_TYPE
#define CDC_DATA_INTERFACE_TYPE 0x0a
#endif
I was wondering why this wasn't using CDC-ACM ?
struct vizzini_serial_private {
struct usb_interface *data_interface;
};
Perhaps that can get simplified ?
static struct vizzini_baud_rate vizzini_baud_rates[] = {
Some explanation might be useful ?
static int vizzini_set_baud_rate(struct usb_serial_port *port,
And the actual rate hould get passed back in the termios
(tty_termios_encode*)
static void vizzini_set_termios(struct tty_struct *tty_param,
struct usb_serial_port *port,
struct ktermios *old_termios)
{
struct tty_struct *tty = port->port.tty;
This mistake is all over the driver. port->port.tty is unsafe as it may
change under you. That's *why* we pass a safe tty reference that the
driver then ignores.
If the driver disable/enable can lose bytes it also needs to figure out
if a real hardware change has occurred or some apps will get upset.
} else if ((cflag & CSIZE) == CS5) {
/* Enabling 5-bit mode is really 9-bit mode! */
format_size = UART_FORMAT_SIZE_9;
If this is magic hackery for 9bit char then it needs doing properly,
if it's a clever way to get 5bit then fine.
} else {
format_size = UART_FORMAT_SIZE_8;
}
And for unsupported formats we need to report the format we actually
picked in the tty->termios data.
static int vizzini_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
{
struct usb_serial_port *port = tty->driver_data;
struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
struct serial_struct ss;
dev_dbg(&port->dev, "%s %08x\n", __func__, cmd);
switch (cmd) {
case TIOCGSERIAL:
if (!arg)
return -EFAULT;
memset(&ss, 0, sizeof(ss));
ss.baud_base = portdata->baud_base;
This leaves lots of fields blank rather than correctly filled in as the
apps will expect. If we support it then it needs to support it all for
read even if only writing odd bits.
if (copy_to_user((void __user *)arg, &ss, sizeof(ss)))
return -EFAULT;
break;
case TIOCSSERIAL:
if (!arg)
return -EFAULT;
if (copy_from_user(&ss, (void __user *)arg, sizeof(ss)))
return -EFAULT;
portdata->baud_base = ss.baud_base;
Because setting stuff as non superuser without any validation is good.
Should also support low latency config if we are doing this.
#ifdef VIZZINI_IWA
static const int vizzini_parity[] = {
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0
};
#endif
Doing parity is something I never bothered with but if we do it then we
ought to have a helper in the tty core instead. Also it's stupid to use
ints for this as it just blows more cache. In fact you might as well use
bits and just test_bit() for parity.
static int vizzini_write_room(struct tty_struct *tty)
{
This seems dodgy - it must never report more than it can guarantee will
fit. If it ever reports 2048 it can't un-report them as free until those
2048 bytes are written. Probably it should be using the kfifo code ?
buffer = kmalloc(bufsize, GFP_ATOMIC);
A fifo would also avoid most of these problems as you can just retry
tidily. This whole path is really ugly to be honest as it gets called
with irqs off by code expecting the queueing to be fast.
static void vizzini_in_callback(struct urb *urb)
{
int endpoint = usb_pipeendpoint(urb->pipe);
struct usb_serial_port *port = urb->context;
struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
struct tty_struct *tty = port->port.tty;
No kref taken so unsafe, could also be NULL, so you execute
NULL->functiontptr. Not good on platforms that don't have low memory
mapping blocked !
room = tty_buffer_request_room(tty, length);
if (room != length)
dev_dbg(&port->dev, "Not enough room in TTY buf, dropped %d chars.\n", length - room);
if (room) {
Not worth checking, just shovel it in - the buffer code will do the work
and only fail when its really congested and the box is stuffed up. Plus
your dev_dbg here will deadlock if this is a USB serial console I think ?
static void vizzini_int_callback(struct urb *urb)
{
struct usb_serial_port *port = urb->context;
struct vizzini_port_private *portdata =
usb_get_serial_port_data(port); struct tty_struct *tty =
port->port.tty;
Again kref...
:
dev_dbg(&port->dev, "Resubmitting interrupt IN urb %p\n", urb);
status = usb_submit_urb(urb, GFP_ATOMIC);
if (status)
dev_err(&port->dev, "usb_submit_urb failed with result %d", status);
Whats the error recovery path for this - a single fail kills the port ?
}
static int vizzini_open(struct tty_struct *tty_param, struct
usb_serial_port *port) {
struct vizzini_port_private *portdata;
struct usb_serial *serial = port->serial;
struct tty_struct *tty = port->port.tty;
Again we pass the tty for a reason !
static void vizzini_close(struct usb_serial_port *port)
{
int i;
struct usb_serial *serial = port->serial;
struct vizzini_port_private *portdata;
struct tty_struct *tty = port->port.tty;
You can't safelty reference tty from here, but it's not used anyway so
just kill it off.
portdata = usb_get_serial_port_data(port);
acm_set_control(port, portdata->ctrlout = 0);
if (serial->dev) {
/* Stop reading/writing urbs */
for (i = 0; i < N_IN_URB; i++)
usb_kill_urb(portdata->in_urbs[i]);
}
usb_kill_urb(port->interrupt_in_urb);
tty = NULL; /* FIXME */
Alan
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Vizzini
2012-09-19 15:23 Vizzini Alan Cox
@ 2012-09-21 15:43 ` Greg KH
2012-09-21 22:52 ` Vizzini Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2012-09-21 15:43 UTC (permalink / raw)
To: Alan Cox, Rob Duncan; +Cc: linux-serial
[Rob added, as he wrote this driver.]
On Wed, Sep 19, 2012 at 04:23:49PM +0100, Alan Cox wrote:
>
> /* This version of the Linux driver source contains a number of
> abominable conditional compilation sections to manage the API
> changes between kernel versions 2.6.18, 2.6.25, and the latest
> (currently 2.6.27). At some point we'll hand a version of this
> driver off to the mainline Linux source tree, and we'll strip
> all these sections out. For now it makes it much easier to
> keep it all in sync while the driver is being developed. */
oops, I fixed those up, but forgot to drop this comment, now fixed.
> It doesn't seem to
>
> #include <linux/usb/cdc.h>
> #ifndef CDC_DATA_INTERFACE_TYPE
> #define CDC_DATA_INTERFACE_TYPE 0x0a
> #endif
>
> I was wondering why this wasn't using CDC-ACM ?
Yeah, I don't know either. Rob, any ideas why you can't just use the
cdc-acm driver instead?
> struct vizzini_serial_private {
> struct usb_interface *data_interface;
> };
>
> Perhaps that can get simplified ?
Will do.
> static struct vizzini_baud_rate vizzini_baud_rates[] = {
>
> Some explanation might be useful ?
Seems to be how they set the baud rate in their chips, a static table
isn't very flexable :(
> static int vizzini_set_baud_rate(struct usb_serial_port *port,
>
> And the actual rate hould get passed back in the termios
> (tty_termios_encode*)
Ah, missed that.
> static void vizzini_set_termios(struct tty_struct *tty_param,
> struct usb_serial_port *port,
> struct ktermios *old_termios)
> {
>
> struct tty_struct *tty = port->port.tty;
>
> This mistake is all over the driver. port->port.tty is unsafe as it may
> change under you. That's *why* we pass a safe tty reference that the
> driver then ignores.
>
> If the driver disable/enable can lose bytes it also needs to figure out
> if a real hardware change has occurred or some apps will get upset.
Yes, I'll go fix those up.
> } else if ((cflag & CSIZE) == CS5) {
> /* Enabling 5-bit mode is really 9-bit mode! */
> format_size = UART_FORMAT_SIZE_9;
>
> If this is magic hackery for 9bit char then it needs doing properly,
> if it's a clever way to get 5bit then fine.
I don't know, Rob?
>
> } else {
> format_size = UART_FORMAT_SIZE_8;
> }
>
> And for unsupported formats we need to report the format we actually
> picked in the tty->termios data.
Good point.
> static int vizzini_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> {
> struct usb_serial_port *port = tty->driver_data;
> struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
> struct serial_struct ss;
>
> dev_dbg(&port->dev, "%s %08x\n", __func__, cmd);
>
> switch (cmd) {
> case TIOCGSERIAL:
> if (!arg)
> return -EFAULT;
> memset(&ss, 0, sizeof(ss));
> ss.baud_base = portdata->baud_base;
>
> This leaves lots of fields blank rather than correctly filled in as the
> apps will expect. If we support it then it needs to support it all for
> read even if only writing odd bits.
I'll drop it for now.
> if (copy_to_user((void __user *)arg, &ss, sizeof(ss)))
> return -EFAULT;
> break;
>
> case TIOCSSERIAL:
> if (!arg)
> return -EFAULT;
> if (copy_from_user(&ss, (void __user *)arg, sizeof(ss)))
> return -EFAULT;
> portdata->baud_base = ss.baud_base;
>
> Because setting stuff as non superuser without any validation is good.
> Should also support low latency config if we are doing this.
I'll drop it here also.
> #ifdef VIZZINI_IWA
> static const int vizzini_parity[] = {
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0
> };
> #endif
>
> Doing parity is something I never bothered with but if we do it then we
> ought to have a helper in the tty core instead. Also it's stupid to use
> ints for this as it just blows more cache. In fact you might as well use
> bits and just test_bit() for parity.
This #define isn't enabled, so I really don't know what is going on.
Rob, any ideas?
> static int vizzini_write_room(struct tty_struct *tty)
> {
>
> This seems dodgy - it must never report more than it can guarantee will
> fit. If it ever reports 2048 it can't un-report them as free until those
> 2048 bytes are written. Probably it should be using the kfifo code ?
Yes, odds are the whole driver should be using that instead, I'll see
about porting it to the generic serial buffer handling which does this.
> buffer = kmalloc(bufsize, GFP_ATOMIC);
>
> A fifo would also avoid most of these problems as you can just retry
> tidily. This whole path is really ugly to be honest as it gets called
> with irqs off by code expecting the queueing to be fast.
Agreed.
> static void vizzini_in_callback(struct urb *urb)
> {
> int endpoint = usb_pipeendpoint(urb->pipe);
> struct usb_serial_port *port = urb->context;
> struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
> struct tty_struct *tty = port->port.tty;
>
> No kref taken so unsafe, could also be NULL, so you execute
> NULL->functiontptr. Not good on platforms that don't have low memory
> mapping blocked !
Good point.
> room = tty_buffer_request_room(tty, length);
> if (room != length)
> dev_dbg(&port->dev, "Not enough room in TTY buf, dropped %d chars.\n", length - room);
>
> if (room) {
>
> Not worth checking, just shovel it in - the buffer code will do the work
> and only fail when its really congested and the box is stuffed up. Plus
> your dev_dbg here will deadlock if this is a USB serial console I think ?
Yes, I think it will, but using usb serial consoles is so messy I don't
like thinking about it...
> static void vizzini_int_callback(struct urb *urb)
> {
> struct usb_serial_port *port = urb->context;
> struct vizzini_port_private *portdata =
> usb_get_serial_port_data(port); struct tty_struct *tty =
> port->port.tty;
>
> Again kref...
> :
> dev_dbg(&port->dev, "Resubmitting interrupt IN urb %p\n", urb);
> status = usb_submit_urb(urb, GFP_ATOMIC);
> if (status)
> dev_err(&port->dev, "usb_submit_urb failed with result %d", status);
>
> Whats the error recovery path for this - a single fail kills the port ?
That's pretty normal for usb-serial drivers, I haven't seen a submit
fail ever, unless the device is removed, so I don't think you need to
worry about it.
> }
>
> static int vizzini_open(struct tty_struct *tty_param, struct
> usb_serial_port *port) {
> struct vizzini_port_private *portdata;
> struct usb_serial *serial = port->serial;
> struct tty_struct *tty = port->port.tty;
>
> Again we pass the tty for a reason !
>
> static void vizzini_close(struct usb_serial_port *port)
> {
> int i;
> struct usb_serial *serial = port->serial;
> struct vizzini_port_private *portdata;
> struct tty_struct *tty = port->port.tty;
>
> You can't safelty reference tty from here, but it's not used anyway so
> just kill it off.
Ok I'll fix up both of these.
Thanks so much for the review, I'll work on these, and it would be great
if Rob could answer the above questions as well.
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Vizzini
2012-09-21 15:43 ` Vizzini Greg KH
@ 2012-09-21 22:52 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2012-09-21 22:52 UTC (permalink / raw)
To: Alan Cox, Rob Duncan; +Cc: linux-serial
On Fri, Sep 21, 2012 at 04:43:23PM +0100, Greg KH wrote:
> [Rob added, as he wrote this driver.]
Ok, as Rob's email bounces, I'm just going to drop this driver entirely,
as I don't want to maintain it, and I can't answer any questions about
it.
Thanks for the review,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-09-21 22:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-19 15:23 Vizzini Alan Cox
2012-09-21 15:43 ` Vizzini Greg KH
2012-09-21 22:52 ` Vizzini Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox