| Message ID | 20201014145727.338773481@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | UBS: Cleanup in_interupt/in_irq/in_atomic() usage | expand |
On Wed, Oct 14, 2020 at 04:52:18PM +0200, Thomas Gleixner wrote: > From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Having two copies of the same code doesn't make the code more readable and > allocating a buffer of 1 byte for a synchronous operation is a pointless > exercise. Not so. In fact, it is required, because a portion of a structure cannot be mapped for DMA unless it is aligned at a cache line boundary. > Add a byte buffer to struct keyspan_pda_private which can be used > instead. The buffer is only used in open() and tty->write(). This won't work. > Console writes > are not calling into the query. open() obviously happens before write() and > the writes are serialized by bit 0 of port->write_urbs_free which protects > also the transaction itself. > > Move the actual query into a helper function and cleanup the usage sites in > keyspan_pda_write() and keyspan_pda_open(). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Johan Hovold <johan@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > --- > drivers/usb/serial/keyspan_pda.c | 102 ++++++++++++++++----------------------- > 1 file changed, 43 insertions(+), 59 deletions(-) > > --- a/drivers/usb/serial/keyspan_pda.c > +++ b/drivers/usb/serial/keyspan_pda.c > @@ -47,6 +47,7 @@ struct keyspan_pda_private { > struct work_struct unthrottle_work; > struct usb_serial *serial; > struct usb_serial_port *port; > + u8 query_buf; > }; > > > @@ -436,6 +437,31 @@ static int keyspan_pda_tiocmset(struct t > return rc; > } > > +/* > + * Using priv->query_buf is safe here because this is only called for TTY > + * operations open() and write(). write() comes post open() obviously and > + * write() itself is serialized via bit 0 of port->write_urbs_free. Console > + * writes are never calling into this. > + */ > +static int keyspan_pda_query_room(struct usb_serial *serial, > + struct keyspan_pda_private *priv) > +{ > + int res; > + > + res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), > + 6, /* write_room */ > + USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN, > + 0, /* value */ > + 0, /* index */ > + &priv->query_buf, > + 1, > + 2000); Instead, consider using the new usb_control_msg_recv() API. But it might be better to allocate the buffer once and for all. Alan Stern
On Wed, Oct 14 2020 at 12:14, Alan Stern wrote: > On Wed, Oct 14, 2020 at 04:52:18PM +0200, Thomas Gleixner wrote: >> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> >> Having two copies of the same code doesn't make the code more readable and >> allocating a buffer of 1 byte for a synchronous operation is a pointless >> exercise. > > Not so. In fact, it is required, because a portion of a structure > cannot be mapped for DMA unless it is aligned at a cache line boundary. > >> Add a byte buffer to struct keyspan_pda_private which can be used >> instead. The buffer is only used in open() and tty->write(). > > This won't work. Ok. >> + res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), >> + 6, /* write_room */ >> + USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN, >> + 0, /* value */ >> + 0, /* index */ >> + &priv->query_buf, >> + 1, >> + 2000); > > Instead, consider using the new usb_control_msg_recv() API. But it > might be better to allocate the buffer once and for all. Let me have a look. Thanks, tglx
On 2020-10-14 12:14:33 [-0400], Alan Stern wrote: > Instead, consider using the new usb_control_msg_recv() API. But it > might be better to allocate the buffer once and for all. This will still allocate and free buffer on each invocation. What about moving the query_buf to the begin of the struct / align it? > Alan Stern Sebastian
On Wed, Oct 14, 2020 at 06:27:14PM +0200, Sebastian Andrzej Siewior wrote: > On 2020-10-14 12:14:33 [-0400], Alan Stern wrote: > > Instead, consider using the new usb_control_msg_recv() API. But it > > might be better to allocate the buffer once and for all. > > This will still allocate and free buffer on each invocation. What about Yes. That's why I suggesting doing a single buffer allocation at the start and using it for each I/O transfer. (But I'm not familiar with this code, and I don't know if there might be multiple transfers going on concurrently.) > moving the query_buf to the begin of the struct / align it? No, thank won't work either. The key to the issue is that while some memory is mapped for DMA, the CPU must not touch it or anything else in the same cache line. If a field is a member of a data structure, the CPU might very well access a neighboring member while this one is mapped, thereby messing up the cache line. Alan Stern
On 2020-10-14 12:34:25 [-0400], Alan Stern wrote: > On Wed, Oct 14, 2020 at 06:27:14PM +0200, Sebastian Andrzej Siewior wrote: > > On 2020-10-14 12:14:33 [-0400], Alan Stern wrote: > > > Instead, consider using the new usb_control_msg_recv() API. But it > > > might be better to allocate the buffer once and for all. > > > > This will still allocate and free buffer on each invocation. What about > > Yes. That's why I suggesting doing a single buffer allocation at the > start and using it for each I/O transfer. (But I'm not familiar with > this code, and I don't know if there might be multiple transfers going > on concurrently.) There are no concurrent transfer. There is a bit used as a lock. The first one does the transfer, the other wait. > > moving the query_buf to the begin of the struct / align it? > > No, thank won't work either. The key to the issue is that while some > memory is mapped for DMA, the CPU must not touch it or anything else in > the same cache line. If a field is a member of a data structure, the > CPU might very well access a neighboring member while this one is > mapped, thereby messing up the cache line. that is unfortunately true. Let me do the single buffer. > Alan Stern Sebastian
--- a/drivers/usb/serial/keyspan_pda.c +++ b/drivers/usb/serial/keyspan_pda.c @@ -47,6 +47,7 @@ struct keyspan_pda_private { struct work_struct unthrottle_work; struct usb_serial *serial; struct usb_serial_port *port; + u8 query_buf; }; @@ -436,6 +437,31 @@ static int keyspan_pda_tiocmset(struct t return rc; } +/* + * Using priv->query_buf is safe here because this is only called for TTY + * operations open() and write(). write() comes post open() obviously and + * write() itself is serialized via bit 0 of port->write_urbs_free. Console + * writes are never calling into this. + */ +static int keyspan_pda_query_room(struct usb_serial *serial, + struct keyspan_pda_private *priv) +{ + int res; + + res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), + 6, /* write_room */ + USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN, + 0, /* value */ + 0, /* index */ + &priv->query_buf, + 1, + 2000); + if (res != 1) + return res < 0 ? res : -EIO; + + return (unsigned int)priv->query_buf; +} + static int keyspan_pda_write(struct tty_struct *tty, struct usb_serial_port *port, const unsigned char *buf, int count) { @@ -483,39 +509,16 @@ static int keyspan_pda_write(struct tty_ * usage because the console write can't sleep. */ if (count > priv->tx_room && tty) { - u8 *room; - - room = kmalloc(1, GFP_KERNEL); - if (!room) { - rc = -ENOMEM; - goto exit; - } - - rc = usb_control_msg(serial->dev, - usb_rcvctrlpipe(serial->dev, 0), - 6, /* write_room */ - USB_TYPE_VENDOR | USB_RECIP_INTERFACE - | USB_DIR_IN, - 0, /* value: 0 means "remaining room" */ - 0, /* index */ - room, - 1, - 2000); - if (rc > 0) { - dev_dbg(&port->dev, "roomquery says %d\n", *room); - priv->tx_room = *room; - } - kfree(room); + rc = keyspan_pda_query_room(serial, priv); if (rc < 0) { - dev_dbg(&port->dev, "roomquery failed\n"); - goto exit; - } - if (rc == 0) { - dev_dbg(&port->dev, "roomquery returned 0 bytes\n"); - rc = -EIO; /* device didn't return any data */ + dev_dbg(&port->dev, "roomquery failed %d\n", rc); goto exit; } + + dev_dbg(&port->dev, "roomquery says %d\n", rc); + priv->tx_room = rc; } + if (count > priv->tx_room) { /* we're about to completely fill the Tx buffer, so we'll be throttled afterwards. */ @@ -615,45 +618,26 @@ static int keyspan_pda_open(struct tty_s struct usb_serial_port *port) { struct usb_serial *serial = port->serial; - u8 *room; - int rc = 0; struct keyspan_pda_private *priv; + int rc; - /* find out how much room is in the Tx ring */ - room = kmalloc(1, GFP_KERNEL); - if (!room) - return -ENOMEM; + priv = usb_get_serial_port_data(port); - rc = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), - 6, /* write_room */ - USB_TYPE_VENDOR | USB_RECIP_INTERFACE - | USB_DIR_IN, - 0, /* value */ - 0, /* index */ - room, - 1, - 2000); + /* find out how much room is in the Tx ring */ + rc = keyspan_pda_query_room(serial, priv); if (rc < 0) { - dev_dbg(&port->dev, "%s - roomquery failed\n", __func__); - goto error; - } - if (rc == 0) { - dev_dbg(&port->dev, "%s - roomquery returned 0 bytes\n", __func__); - rc = -EIO; - goto error; + dev_dbg(&port->dev, "roomquery failed %d\n", rc); + return rc; } - priv = usb_get_serial_port_data(port); - priv->tx_room = *room; - priv->tx_throttled = *room ? 0 : 1; + + priv->tx_room = rc; + priv->tx_throttled = rc ? 0 : 1; /*Start reading from the device*/ rc = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL); - if (rc) { + if (rc) dev_dbg(&port->dev, "%s - usb_submit_urb(read int) failed\n", __func__); - goto error; - } -error: - kfree(room); + return rc; } static void keyspan_pda_close(struct usb_serial_port *port)