On Wed, Dec 21, 2022 at 12:13:08AM +0100, Ricardo Ribalda wrote: > Make the developer aware of the requirements of transfer buffer. > > The buffer must be DMAble, if the developer uses an invalid buffer, data > corruption might happen. > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > --- > USB: Improve usb_fill_* documentation > > After trying to "cleanup" the uvc code, I was patiently explained about > the requirements of the urb transfer buffers. > > Lets make this explicit, so other developers do not make the same mistake. > > To: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> > To: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > To: Christoph Hellwig <hch@xxxxxx> > To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Cc: linux-usb@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > --- > Changes in v2: > - s/allocatiing/allocating/ Thanks Randy > - Link to v1: https://lore.kernel.org/r/20221220-usb-dmadoc-v1-0-28386d2eb6cd@xxxxxxxxxxxx > --- > include/linux/usb.h | 27 ++++++++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 7d5325d47c45..1144ef6e4151 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1627,13 +1627,20 @@ struct urb { > * @dev: pointer to the struct usb_device for this urb. > * @pipe: the endpoint pipe > * @setup_packet: pointer to the setup_packet buffer > - * @transfer_buffer: pointer to the transfer buffer > + * @transfer_buffer: pointer to the transfer buffer. Must be suitable for DMA. For control URBs, the setup_packet must also be suitable for DMA. It's a little less critical, though, because the setup_packet is only used for DMA out, never DMA in. > * @buffer_length: length of the transfer buffer > * @complete_fn: pointer to the usb_complete_t function > * @context: what to set the urb context to. > * > * Initializes a control urb with the proper information needed to submit > * it to a device. > + * > + * The transfer buffer might be filled via DMA. "might" is too weak. For nonzero-length IN transfers, the transfer buffer is certain to be filled via DMA (except for the very few cases of USB controller hardware using PIO instead of DMA). > The simplest way to get > + * a buffer that can be DMAed to, is allocating it via kmalloc() or > + * equivalent, even for very small buffers. If transfer_buffer is embedded > + * in a bigger structure, there is a risk that the previous and following > + * fields are left in a corrupted state by the DMA engine, if the platform > + * is not cache coherent. There is also a risk that data read from the device is corrupted. And accesses of the surrounding fields may well be slowed down because the DMA mapping evicts them from the cache. > */ > static inline void usb_fill_control_urb(struct urb *urb, > struct usb_device *dev, > @@ -1658,13 +1665,20 @@ static inline void usb_fill_control_urb(struct urb *urb, > * @urb: pointer to the urb to initialize. > * @dev: pointer to the struct usb_device for this urb. > * @pipe: the endpoint pipe > - * @transfer_buffer: pointer to the transfer buffer > + * @transfer_buffer: pointer to the transfer buffer. Must be suitable for DMA. > * @buffer_length: length of the transfer buffer > * @complete_fn: pointer to the usb_complete_t function > * @context: what to set the urb context to. > * > * Initializes a bulk urb with the proper information needed to submit it > * to a device. > + * > + * The transfer buffer might be filled via DMA. The simplest way to get > + * a buffer that can be DMAed to, is allocating it via kmalloc() or > + * equivalent, even for very small buffers. If transfer_buffer is embedded > + * in a bigger structure, there is a risk that the previous and following > + * fields are left in a corrupted state by the DMA engine, if the platform > + * is not cache coherent. I see no point in repeating exactly the same text multiple times. Instead, just put a reference to the original occurrence of this warning. Alan Stern > */ > static inline void usb_fill_bulk_urb(struct urb *urb, > struct usb_device *dev, > @@ -1687,7 +1701,7 @@ static inline void usb_fill_bulk_urb(struct urb *urb, > * @urb: pointer to the urb to initialize. > * @dev: pointer to the struct usb_device for this urb. > * @pipe: the endpoint pipe > - * @transfer_buffer: pointer to the transfer buffer > + * @transfer_buffer: pointer to the transfer buffer. Must be suitable for DMA. > * @buffer_length: length of the transfer buffer > * @complete_fn: pointer to the usb_complete_t function > * @context: what to set the urb context to. > @@ -1697,6 +1711,13 @@ static inline void usb_fill_bulk_urb(struct urb *urb, > * Initializes a interrupt urb with the proper information needed to submit > * it to a device. > * > + * The transfer buffer might be filled via DMA. The simplest way to get > + * a buffer that can be DMAed to, is allocating it via kmalloc() or > + * equivalent, even for very small buffers. If transfer_buffer is embedded > + * in a bigger structure, there is a risk that the previous and following > + * fields are left in a corrupted state by the DMA engine, if the platform > + * is not cache coherent. > + * > * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic > * encoding of the endpoint interval, and express polling intervals in > * microframes (eight per millisecond) rather than in frames (one per > > --- > base-commit: b6bb9676f2165d518b35ba3bea5f1fcfc0d969bf > change-id: 20221220-usb-dmadoc-29384acebd48 > > Best regards, > -- > Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
