| Message ID | 20221214-uvc-status-alloc-v3-0-9a67616cc549@chromium.org |
|---|---|
| Headers | show |
| Series | media: uvcvideo: Code cleanup for dev->status | expand |
On Fri, Dec 16, 2022 at 09:55:09AM +0100, Ricardo Ribalda wrote: > Hi Alan > > On Thu, 15 Dec 2022 at 16:34, Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, Dec 15, 2022 at 11:57:17AM +0100, Ricardo Ribalda wrote: > > > There is no need to make a kzalloc just for 16 bytes. Let's embed the data > > > into the main data structure. > > > > > > Now that we are at it, lets remove all the castings and open coding of > > > offsets for it. > > > > > > [Christoph, do you think dma wise we are violating any non written rules? :) thanks] > > > > There _is_ a rule, and it is not exactly unwritten. The kerneldoc for > > the transfer_buffer member of struct urb says: > > > > This buffer must be suitable for DMA; allocate it with > > kmalloc() or equivalent. > > > > Which in general means that the buffer must not be part of a larger > > structure -- not unless the driver can guarantee that the structure will > > _never_ be accessed while a USB transfer to/from the buffer is taking > > place. > > > > Thanks a lot for the clarification. I was mainly looking at the kerneldoc from: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/usb.h#n1687 > > and I could not see any reference to the DMA requirements. > > Mind if I send a patch to add a reference there? Not at all. But if you change the documentation for usb_fill_int_urb() then you should also change it for usb_fill_control_urb() and usb_fill_bulk_urb(). Alan Stern > > There are examples all over the USB subsystem where buffers as small as > > one or two bytes get kmalloc'ed in order to obey this rule. 16 bytes is > > certainly big enough that you shouldn't worry about it being allocated > > separately. > > > Yep, we should keep it malloced. Thanks a lot for looking into this :) > > > > Alan Stern > > > > -- > Ricardo Ribalda
There is no need to make a kzalloc just for 16 bytes. Let's embed the data into the main data structure. Now that we are at it, lets remove all the castings and open coding of offsets for it. [Christoph, do you think dma wise we are violating any non written rules? :) thanks] Cc: Christoph Hellwig <hch@lst.de> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: linux-usb@vger.kernel.org To: Ming Lei <tom.leiming@gmail.com> To: Jonathan Cameron <Jonathan.Cameron@huawei.com> To: Yunke Cao <yunkec@chromium.org> To: Sergey Senozhatsky <senozhatsky@chromium.org> To: Max Staudt <mstaudt@chromium.org> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: linux-media@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- Changes in v3: - Split the patch in two - Add linux-usb, Alan and Christoph for the allocation change. - Link to v2: https://lore.kernel.org/r/20221214-uvc-status-alloc-v2-0-3f1cba6fc734@chromium.org Changes in v2: - using __aligned(), to keep the old alignment - Adding Johnathan Cameron to:, as he has some similar experience with iio - Adding Ming Lei, as this patch kind of revert his patch - Link to v1: https://lore.kernel.org/r/20221214-uvc-status-alloc-v1-0-a0098ddc7c93@chromium.org --- Ricardo Ribalda (2): media: uvcvideo: Remove void casting for the status endpoint media: uvcvideo: Do not alloc dev->status drivers/media/usb/uvc/uvc_status.c | 69 ++++++++++++-------------------------- drivers/media/usb/uvc/uvcvideo.h | 29 ++++++++++++++-- 2 files changed, 47 insertions(+), 51 deletions(-) --- base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c change-id: 20221214-uvc-status-alloc-93becb783898 Best regards,