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 <[email protected]>
---
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 <[email protected]>
To: Alan Stern <[email protected]>
To: Christoph Hellwig <[email protected]>
To: Laurent Pinchart <[email protected]>
To: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Changes in v2:
- s/allocatiing/allocating/ Thanks Randy
- Link to v1: https://lore.kernel.org/r/[email protected]
---
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.
* @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. 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.
*/
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.
*/
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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> To: Alan Stern <[email protected]>
> To: Christoph Hellwig <[email protected]>
> To: Laurent Pinchart <[email protected]>
> To: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> Changes in v2:
> - s/allocatiing/allocating/ Thanks Randy
> - Link to v1: https://lore.kernel.org/r/[email protected]
> ---
> 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 <[email protected]>
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.
Better say:
```
Document the transfer buffer requirement. That is, the buffer must be
DMAble - otherwise data corruption might occur.
```
> - * @transfer_buffer: pointer to the transfer buffer
> + * @transfer_buffer: pointer to the transfer buffer. Must be suitable for DMA.
"... The buffer must be ..."
> - * @transfer_buffer: pointer to the transfer buffer
> + * @transfer_buffer: pointer to the transfer buffer. Must be suitable for DMA.
Same here.
The rest is LGTM, thanks.
--
An old man doll... just what I always wanted! - Clara