| Message ID | 20201014145215.518912759@linutronix.de |
|---|---|
| Headers | show |
| Series | UBS: Cleanup in_interupt/in_irq/in_atomic() usage | expand |
On 10/14/20 8:52 AM, Thomas Gleixner wrote: > From: Ahmed S. Darwish <a.darwish@linutronix.de> > > The usage of in_interrupt() in drivers is phased out and Linus clearly > requested that code which changes behaviour depending on context should > either be separated or the context be conveyed in an argument passed by the > caller, which usually knows the context. > > usbip_recv() uses in_interrupt() to conditionally print context information > for debugging messages. The value is zero as the function is only called > from various *_rx_loop() kthread functions. Remove it. > > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Valentina Manea <valentina.manea.m@gmail.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > > --- > drivers/usb/usbip/usbip_common.c | 5 ----- > 1 file changed, 5 deletions(-) > > --- a/drivers/usb/usbip/usbip_common.c > +++ b/drivers/usb/usbip/usbip_common.c > @@ -324,11 +324,6 @@ int usbip_recv(struct socket *sock, void > } while (msg_data_left(&msg)); > > if (usbip_dbg_flag_xmit) { > - if (!in_interrupt()) > - pr_debug("%-10s:", current->comm); > - else > - pr_debug("interrupt :"); > - > pr_debug("receiving....\n"); > usbip_dump_buffer(buf, size); > pr_debug("received, osize %d ret %d size %zd total %d\n", > > Looks good to me. Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
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 04:52:26PM +0200, Thomas Gleixner wrote: > From: Ahmed S. Darwish <a.darwish@linutronix.de> > > The usage of in_interrupt() in drivers is phased out for various reasons. > > Various comments use !in_interrupt() to describe calling context for > functions which might sleep. That's wrong because the calling context has > to be preemptible task context, which is not what !in_interrupt() > describes. > > Replace !in_interrupt() with more accurate plain text descriptions. > > The comment for usb_hcd_poll_rh_status() is misleading as this function is > called from all kinds of contexts including preemptible task > context. Remove it as there is obviously no restriction. > > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > > --- > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h > * Root Hub interrupt transfers are polled using a timer if the > * driver requests it; otherwise the driver is responsible for > * calling usb_hcd_poll_rh_status() when an event occurs. > - * > - * Completions are called in_interrupt(), but they may or may not > - * be in_irq(). This comment should not be removed; instead it should be changed to say that completion handlers are called with interrupts disabled. > @@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned > * @hcd: host controller returning the URB > * @urb: urb being returned to the USB device driver. > * @status: completion status code for the URB. > - * Context: in_interrupt() The comment should be changed to say that the routine runs in a BH handler (or however you want to express it). > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb > /* > * usb_set_isoch_delay - informs the device of the packet transmit delay > * @dev: the device whose delay is to be informed > - * Context: !in_interrupt() > + * Context: can sleep Why is this comment different from all the others? Alan Stern
On 2020-10-14 12:27:21 [-0400], Alan Stern wrote: > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > @@ -746,9 +746,6 @@ static int rh_call_control (struct usb_h > > * Root Hub interrupt transfers are polled using a timer if the > > * driver requests it; otherwise the driver is responsible for > > * calling usb_hcd_poll_rh_status() when an event occurs. > > - * > > - * Completions are called in_interrupt(), but they may or may not > > - * be in_irq(). > > This comment should not be removed; instead it should be changed to say > that completion handlers are called with interrupts disabled. The timer callback: rh_timer_func() -> usb_hcd_poll_rh_status() invokes the function with enabled interrupts. > > @@ -1691,7 +1690,6 @@ static void usb_giveback_urb_bh(unsigned > > * @hcd: host controller returning the URB > > * @urb: urb being returned to the USB device driver. > > * @status: completion status code for the URB. > > - * Context: in_interrupt() > > The comment should be changed to say that the routine runs in a BH > handler (or however you want to express it). Do you mean usb_hcd_giveback_urb() runs in BH context or that the completion callback of the URB runs in BH context? The completion callback of the URB may run in BH or IRQ context depending on HCD. > > --- a/drivers/usb/core/message.c > > +++ b/drivers/usb/core/message.c > > > @@ -934,7 +939,7 @@ int usb_get_device_descriptor(struct usb > > /* > > * usb_set_isoch_delay - informs the device of the packet transmit delay > > * @dev: the device whose delay is to be informed > > - * Context: !in_interrupt() > > + * Context: can sleep > > Why is this comment different from all the others? It says !in_interrupt() which is also true for preempt-disabled regions. But the caller must not have preemption disabled. "can sleep" is more obvious as what it needs. > Alan Stern Sebastian
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
> The usage of such constructs in USB is rather limited. Most of it is in > debug code and (misleading) comments. But of course there are also a few > few bugs including one unfixable. The "UBS" typo in the subject got me confused for a while. Best regards, Pavel