Hi Alan On Wed, 21 Dec 2022 at 18:28, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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 <rdunlap@xxxxxxxxxxxxx> > > 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: Bagas Sanjaya <bagasdotme@xxxxxxxxx> > > 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 v3: > > - Improve commit message. Thanks Bagas! > > - Improve field description. Thanks Alan! > > - Link to v2: https://lore.kernel.org/r/20221220-usb-dmadoc-v2-0-4dd4f198113e@xxxxxxxxxxxx > > > > 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, 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 <ribalda@xxxxxxxxxxxx> -- Ricardo Ribalda
