[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-media
Subject:    Re: [PATCH 1/3] media: Zero-initialize all structures passed to subdev pad operations
From:       Laurent Pinchart <laurent.pinchart () ideasonboard ! com>
Date:       2023-02-28 23:58:26
Message-ID: Y/6VInMEEPhpMlxd () pendragon ! ideasonboard ! com
[Download RAW message or body]

On Wed, Mar 01, 2023 at 01:55:49AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Feb 28, 2023 at 12:05:03PM +0200, Sakari Ailus wrote:
> > On Wed, Feb 15, 2023 at 06:50:19PM +0200, Laurent Pinchart wrote:
> > > Several drivers call subdev pad operations, passing structures that are
> > > not fully zeroed. While the drivers initialize the fields they care
> > > about explicitly, this results in reserved fields having uninitialized
> > > values. Future kernel API changes that make use of those fields thus
> > > risk breaking proper driver operation in ways that could be hard to
> > > detect.
> > > 
> > > To avoid this, make the code more robust by zero-initializing all the
> > > structures passed to subdev pad operation. Maintain a consistent coding
> > > style by preferring designated initializers (which zero-initialize all
> > > the fields that are not specified) over memset() where possible, and
> > > make variable declarations local to inner scopes where applicable. One
> > > notable exception to this rule is in the ipu3 driver, where a memset()
> > > is needed as the structure is not a local variable but a function
> > > parameter provided by the caller.
> > > 
> > > Not all fields of those structures can be initialized when declaring the
> > > variables, as the values for those fields are computed later in the
> > > code. Initialize the 'which' field in all cases, and other fields when
> > > the variable declaration is so close to the v4l2_subdev_call() call that
> > > it keeps all the context easily visible when reading the code, to avoid
> > > hindering readability.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > ...
> > 
> > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c \
> > > b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c index \
> > >                 3b76a9d0383a..3c84cb121632 100644
> > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> > > @@ -1305,6 +1305,7 @@ static int cio2_subdev_link_validate_get_format(struct \
> > > media_pad *pad,  struct v4l2_subdev *sd =
> > > 			media_entity_to_v4l2_subdev(pad->entity);
> > > 
> > > +		memset(fmt, 0, sizeof(*fmt));
> > > 		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > 		fmt->pad = pad->index;
> > > 		return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt);
> > 
> > Instead I'd merge this with its only caller.
> > 
> > I can submit a patch on top of this one as it's just a small cleanup.
> 
> I'd prefer that, to keep this series as little intrusive as possible.
> 
> > For the set:
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Thank you.
> 
> > The second latter of the subject of the 3 patch should be lower case.

What ? :-)

-- 
Regards,

Laurent Pinchart


[prev in list] [next in list] [prev in thread] [next in thread]