Re: [Outreachy kernel] [PATCH] staging: media: omap4iss: Use list_entry instead of container_of

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Himadri,

On Wed, Mar 27, 2019 at 01:13:27AM +0530, Himadri Pandya wrote:
> On Mon, Mar 25, 2019 at 7:17 AM Laurent Pinchart wrote:
> > On Sun, Mar 23, 2019 at 11:19:54PM +0530, Himadri Pandya wrote:
> >> On Sun, Mar 24, 2019 at 10:48 PM Julia Lawall wrote:
> >>> On Sun, 24 Mar 2019, Himadri Pandya wrote:
> >>>> Use list_entry instead of container_of for lists iss_buffer and
> >>>> iss_video.
> >>>>
> >>>> Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> >>>> ---
> >>>>  drivers/staging/media/omap4iss/iss_video.h | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> >>>> index f22489edb562..e8d27b7ac329 100644
> >>>> --- a/drivers/staging/media/omap4iss/iss_video.h
> >>>> +++ b/drivers/staging/media/omap4iss/iss_video.h
> >>>> @@ -118,7 +118,7 @@ struct iss_buffer {
> >>>>       dma_addr_t iss_addr;
> >>>>  };
> >>>>
> >>>> -#define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)
> >>>> +#define to_iss_buffer(buf)   list_entry(buf, struct iss_buffer, vb)
> >>>
> >>> Actually, this macro is no longer used.  Perhaps someone else converted
> >>> the uses to use the proper list operations and forgot to drop the macro
> >>> definition.
> >>
> >> Then should I send a patch to drop these macros?
> > 
> > It would be better to modify the to_iss_buffer macro to convert from a
> > vb2_buffer to a iss_buffer, and use it where the driver currently uses
> > container_of directly.
> 
> I am sorry but I am not sure if I understand what you are suggesting
> correctly. 
> 
> to_iss_buffer is defined as:
> #define to_iss_buffer(buf)   container_of(buf, struct iss_buffer, vb)

to_iss_buffer() casts a vb2_v4l2_buffer pointer to a iss_buffer pointer.

> And it is not used. Instead, container_of is used directly
> static void iss_video_buf_cleanup(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
> 
> static int iss_video_buf_prepare(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }
> 
> static void iss_video_buf_queue(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct iss_video_fh *vfh = vb2_get_drv_priv(vb->vb2_queue);
> struct iss_video *video = vfh->video;
> struct iss_buffer *buffer = container_of(vbuf, struct iss_buffer, vb);
>         ...
> }

These three functions need to cast a vb2_buffer pointer to a iss_buffer
pointer. They do so by first casting to vb2_v4l2_buffer, and then to
iss_buffer with a direct use of container_of. It would be best to
combine the two casts in a single to_iss_buffer macro and use it through
the code.

> If I understand it correctly, these usages are converting vb2_buffer to
> iss_buffer. Which could be done by using the to_iss_buffer(vbuf) as well. Why
> is this macro no longer used? I also don't understand what modification you are
> suggesting. Can you please elaborate?
> 
> >>>>  enum iss_video_dmaqueue_flags {
> >>>>       /* Set if DMA queue becomes empty when ISS_PIPELINE_STREAM_CONTINUOUS */
> >>>> @@ -170,7 +170,7 @@ struct iss_video {
> >>>>       const struct iss_video_operations *ops;
> >>>>  };
> >>>>
> >>>> -#define to_iss_video(vdev)   container_of(vdev, struct iss_video, video)
> >>>> +#define to_iss_video(vdev)   list_entry(vdev, struct iss_video, video)
> >>>
> >>> This doesn't involve a list.  Nor does the previous one.  To use
> >>> list_entry, the third argument should be the name of a field that has
> >>> type list_head.
> >>
> >> Understood. Thank you.
> >>
> >>>>  struct iss_video_fh {
> >>>>       struct v4l2_fh vfh;

-- 
Regards,

Laurent Pinchart