| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y6LdrdQ0CDRqkTfh@pendragon.ideasonboard.com>
Date: Wed, 21 Dec 2022 12:19:25 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Bagas Sanjaya <bagasdotme@...il.com>,
Christoph Hellwig <hch@....de>,
Alan Stern <stern@...land.harvard.edu>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Randy Dunlap <rdunlap@...radead.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] USB: Improve usb_fill_* documentation
Hi Ricardo,
Thank you for the patch.
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@...radead.org>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.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: Bagas Sanjaya <bagasdotme@...il.com>
> To: Randy Dunlap <rdunlap@...radead.org>
> To: Alan Stern <stern@...land.harvard.edu>
> To: Christoph Hellwig <hch@....de>
> To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-usb@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
> 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@chromium.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, 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
s/field/fields/g
> + * 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
I would drop "please". Same below.
With or without those changes,
Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> + * 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
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists