Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Dec 2022 21:47:28 -0500
From:   Alan Stern <stern@rowland.harvard.edu>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Christoph Hellwig <hch@lst.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] USB: Improve usb_fill_* documentation
Message-ID: <Y6JzwChVdOIx1vPD@rowland.harvard.edu>
References: <20221220-usb-dmadoc-v2-0-4dd4f198113e@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221220-usb-dmadoc-v2-0-4dd4f198113e@chromium.org>
Precedence: bulk

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@chromium.org>
> ---
> 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@infradead.org>
> To: Alan Stern <stern@rowland.harvard.edu>
> To: Christoph Hellwig <hch@lst.de>
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes in v2:
> - s/allocatiing/allocating/ Thanks Randy
> - Link to v1: https://lore.kernel.org/r/20221220-usb-dmadoc-v1-0-28386d2eb6cd@chromium.org
> ---
>  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@chromium.org>