[v3,0/2] media: uvcvideo: Code cleanup for dev->status

Message

Ricardo Ribalda Dec. 15, 2022, 10:57 a.m. UTC

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 <[e-mail]>
Cc: Alan Stern <[e-mail]>
Cc: [e-mail]
To: Ming Lei <[e-mail]>
To: Jonathan Cameron <[e-mail]>
To: Yunke Cao <[e-mail]>
To: Sergey Senozhatsky <[e-mail]>
To: Max Staudt <[e-mail]>
To: Laurent Pinchart <[e-mail]>
To: Mauro Carvalho Chehab <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Ricardo Ribalda <[e-mail]>

---
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/[e-mail]

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/[e-mail]

---
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,

Comments

Alan Stern Dec. 15, 2022, 3:34 p.m. UTC | #1

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.

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.

Alan Stern

Ricardo Ribalda Dec. 16, 2022, 8:55 a.m. UTC | #2

Hi Alan

On Thu, 15 Dec 2022 at 16:34, Alan Stern <[e-mail]> 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?


> 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

Alan Stern Dec. 16, 2022, 3:45 p.m. UTC | #3

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 <[e-mail]> 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