Document the transfer buffer requirement. That is, the buffer must be
DMAble - otherwise data corruption might occur.
Acked-by: Randy Dunlap <[email protected]>
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: Bagas Sanjaya <[email protected]>
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 v3:
- Improve commit message. Thanks Bagas!
- Improve field description. Thanks Alan!
- Link to v2: https://lore.kernel.org/r/[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, 23 insertions(+), 4 deletions(-)
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7d5325d47c45..06cde9ddca97 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1626,14 +1626,25 @@ struct urb {
* @urb: pointer to the urb to initialize.
* @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
+ * @setup_packet: pointer to the setup_packet buffer. The buffer must be
+ * suitable for DMA.
+ * @transfer_buffer: pointer to the transfer buffer. The 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 and the setup_packet buffer will most likely 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 the buffers are embedded in a bigger structure, there is a risk that
+ * the buffer itself, the previous field and/or the next field are corrupted
+ * due to cache incoherencies; or slowed down if they are evicted from the
+ * cache.
+ *
*/
static inline void usb_fill_control_urb(struct urb *urb,
struct usb_device *dev,
@@ -1658,13 +1669,17 @@ 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. The 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.
+ *
+ * Please refer to usb_fill_control_urb() for a description of the
+ * requirements for transfer_buffer.
*/
static inline void usb_fill_bulk_urb(struct urb *urb,
struct usb_device *dev,
@@ -1687,7 +1702,8 @@ 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. The 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 +1713,9 @@ 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.
*
+ * Please refer to usb_fill_control_urb() for a description of the
+ * requirements for transfer_buffer.
+ *
* 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 11:15:14AM +0100, Ricardo Ribalda wrote:
> Document the transfer buffer requirement. That is, the buffer must be
> DMAble - otherwise data corruption might occur.
>
> Acked-by: Randy Dunlap <[email protected]>
> 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: Bagas Sanjaya <[email protected]>
> 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 v3:
> - Improve commit message. Thanks Bagas!
> - Improve field description. Thanks Alan!
> - Link to v2: https://lore.kernel.org/r/[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, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 7d5325d47c45..06cde9ddca97 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1626,14 +1626,25 @@ struct urb {
> * @urb: pointer to the urb to initialize.
> * @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
> + * @setup_packet: pointer to the setup_packet buffer. The buffer must be
> + * suitable for DMA.
> + * @transfer_buffer: pointer to the transfer buffer. The 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 and the setup_packet buffer will most likely be filled
> + * via DMA.
No, no! The setup_packet buffer will never be filled via DMA -- it is
_read_ via DMA.
The transfer buffer may be filled via DMA; it depends on the direction
of the transfer. For OUT transfers, the buffer is read via DMA; for IN
transfers it is 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 the buffers are embedded in a bigger structure, there is a risk that
> + * the buffer itself, the previous field and/or the next field are corrupted
> + * due to cache incoherencies; or slowed down if they are evicted from the
> + * cache.
> + *
> */
> static inline void usb_fill_control_urb(struct urb *urb,
> struct usb_device *dev,
> @@ -1658,13 +1669,17 @@ 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. The 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.
> + *
> + * Please refer to usb_fill_control_urb() for a description of the
> + * requirements for transfer_buffer.
_This_ right here is the kerneldoc for usb_fill_control_urb()! You
should have written "Please refer to the kerneldoc for struct urb".
> */
> static inline void usb_fill_bulk_urb(struct urb *urb,
> struct usb_device *dev,
> @@ -1687,7 +1702,8 @@ 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. The 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 +1713,9 @@ 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.
> *
> + * Please refer to usb_fill_control_urb() for a description of the
> + * requirements for transfer_buffer.
Similar to the above.
> + *
> * 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
Why didn't you also make the same change to usb_fill_int_urb()?
Alan Stern
>
> ---
> base-commit: b6bb9676f2165d518b35ba3bea5f1fcfc0d969bf
> change-id: 20221220-usb-dmadoc-29384acebd48
>
> Best regards,
> --
> Ricardo Ribalda <[email protected]>
Hi Alan
On Wed, 21 Dec 2022 at 18:28, Alan Stern <[email protected]> wrote:
>
> On Wed, Dec 21, 2022 at 11:15:14AM +0100, Ricardo Ribalda wrote:
> > Document the transfer buffer requirement. That is, the buffer must be
> > DMAble - otherwise data corruption might occur.
> >
> > Acked-by: Randy Dunlap <[email protected]>
> > 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: Bagas Sanjaya <[email protected]>
> > 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 v3:
> > - Improve commit message. Thanks Bagas!
> > - Improve field description. Thanks Alan!
> > - Link to v2: https://lore.kernel.org/r/[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, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 7d5325d47c45..06cde9ddca97 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1626,14 +1626,25 @@ struct urb {
> > * @urb: pointer to the urb to initialize.
> > * @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
> > + * @setup_packet: pointer to the setup_packet buffer. The buffer must be
> > + * suitable for DMA.
> > + * @transfer_buffer: pointer to the transfer buffer. The 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 and the setup_packet buffer will most likely be filled
> > + * via DMA.
>
> No, no! The setup_packet buffer will never be filled via DMA -- it is
> _read_ via DMA.
>
> The transfer buffer may be filled via DMA; it depends on the direction
> of the transfer. For OUT transfers, the buffer is read via DMA; for IN
> transfers it is filled via DMA.
I tried to have a short sentence :), but you are right, let's make it
explicit, at the end of the day that is the whole purpose of this
patch :)
I have written now: will most likely be filled or read 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 the buffers are embedded in a bigger structure, there is a risk that
> > + * the buffer itself, the previous field and/or the next field are corrupted
> > + * due to cache incoherencies; or slowed down if they are evicted from the
> > + * cache.
> > + *
> > */
> > static inline void usb_fill_control_urb(struct urb *urb,
> > struct usb_device *dev,
> > @@ -1658,13 +1669,17 @@ 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. The 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.
> > + *
> > + * Please refer to usb_fill_control_urb() for a description of the
> > + * requirements for transfer_buffer.
>
> _This_ right here is the kerneldoc for usb_fill_control_urb()! You
> should have written "Please refer to the kerneldoc for struct urb".
>
Please try applying the patch. I believe it is on the right location.
diff does not know about kerneldoc and this is why it copies the
previous header (struct urb), instead of usb_fill_control_urb.
This is why it seems that all the messages are shifted one location up.
I would also prefer to keep the: refer to usb_fill_control_urb, as the
functions are more indexed than the structures: Eg:
https://manpages.debian.org/testing/linux-manual-4.8/usb_fill_control_urb.9.en.html
What about adding a reference to struct urb at usb_fill_control_urb() ?
> > */
> > static inline void usb_fill_bulk_urb(struct urb *urb,
> > struct usb_device *dev,
> > @@ -1687,7 +1702,8 @@ 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. The 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 +1713,9 @@ 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.
> > *
> > + * Please refer to usb_fill_control_urb() for a description of the
> > + * requirements for transfer_buffer.
>
> Similar to the above.
>
> > + *
> > * 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
>
> Why didn't you also make the same change to usb_fill_int_urb()?
>
> Alan Stern
>
> >
> > ---
> > base-commit: b6bb9676f2165d518b35ba3bea5f1fcfc0d969bf
> > change-id: 20221220-usb-dmadoc-29384acebd48
> >
> > Best regards,
> > --
> > Ricardo Ribalda <[email protected]>
--
Ricardo Ribalda
On Wed, Dec 21, 2022 at 08:34:42PM +0100, Ricardo Ribalda wrote:
> Hi Alan
>
> On Wed, 21 Dec 2022 at 18:28, Alan Stern <[email protected]> wrote:
> >
> > On Wed, Dec 21, 2022 at 11:15:14AM +0100, Ricardo Ribalda wrote:
> > > Document the transfer buffer requirement. That is, the buffer must be
> > > DMAble - otherwise data corruption might occur.
> > >
> > > Acked-by: Randy Dunlap <[email protected]>
> > > 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: Bagas Sanjaya <[email protected]>
> > > 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 v3:
> > > - Improve commit message. Thanks Bagas!
> > > - Improve field description. Thanks Alan!
> > > - Link to v2: https://lore.kernel.org/r/[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, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > index 7d5325d47c45..06cde9ddca97 100644
> > > --- a/include/linux/usb.h
> > > +++ b/include/linux/usb.h
> > > @@ -1626,14 +1626,25 @@ struct urb {
> > > * @urb: pointer to the urb to initialize.
> > > * @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
> > > + * @setup_packet: pointer to the setup_packet buffer. The buffer must be
> > > + * suitable for DMA.
> > > + * @transfer_buffer: pointer to the transfer buffer. The 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 and the setup_packet buffer will most likely be filled
> > > + * via DMA.
> >
> > No, no! The setup_packet buffer will never be filled via DMA -- it is
> > _read_ via DMA.
> >
> > The transfer buffer may be filled via DMA; it depends on the direction
> > of the transfer. For OUT transfers, the buffer is read via DMA; for IN
> > transfers it is filled via DMA.
>
> I tried to have a short sentence :), but you are right, let's make it
> explicit, at the end of the day that is the whole purpose of this
> patch :)
>
> I have written now: will most likely be filled or read via DMA.
Okay.
> > > 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 the buffers are embedded in a bigger structure, there is a risk that
> > > + * the buffer itself, the previous field and/or the next field are corrupted
> > > + * due to cache incoherencies; or slowed down if they are evicted from the
> > > + * cache.
> > > + *
> > > */
> > > static inline void usb_fill_control_urb(struct urb *urb,
> > > struct usb_device *dev,
> > > @@ -1658,13 +1669,17 @@ 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. The 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.
> > > + *
> > > + * Please refer to usb_fill_control_urb() for a description of the
> > > + * requirements for transfer_buffer.
> >
> > _This_ right here is the kerneldoc for usb_fill_control_urb()! You
> > should have written "Please refer to the kerneldoc for struct urb".
> >
>
> Please try applying the patch. I believe it is on the right location.
> diff does not know about kerneldoc and this is why it copies the
> previous header (struct urb), instead of usb_fill_control_urb.
>
> This is why it seems that all the messages are shifted one location up.
You're right; sorry for jumping to unwarranted conclusions.
> I would also prefer to keep the: refer to usb_fill_control_urb, as the
> functions are more indexed than the structures: Eg:
> https://manpages.debian.org/testing/linux-manual-4.8/usb_fill_control_urb.9.en.html
>
> What about adding a reference to struct urb at usb_fill_control_urb() ?
That's a good idea.
Alan Stern