| Message ID | 20201014145215.518912759@linutronix.de |
|---|---|
| Headers | show |
| Series | UBS: Cleanup in_interupt/in_irq/in_atomic() usage | expand |
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
> 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