Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Janusz Krzysztofik <jmkrzyszt@gmail.com>
To:     Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Date:   Wed, 15 May 2019 22:56:36 +0200
Message-ID: <1943741.XiKEDqKQ7m@z50>
In-Reply-To: <20190515071601.knfdhwofz6ukjmxt@paasikivi.fi.intel.com>
References: <20190514224823.11564-1-jmkrzyszt@gmail.com> <20190514224823.11564-2-jmkrzyszt@gmail.com> <20190515071601.knfdhwofz6ukjmxt@paasikivi.fi.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Sakari,

On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop 
*crop)
> > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> >  {
> > -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> 
> One more comment.
> 
> The num_pads doesn't really tell whether a given op is valid for a device.
> Well, in this case it would have to be a bug in the driver, but those do
> happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> non-NULL if the entity is registered with a media device, i.e. when these
> callback functions are supposed to be called.

Before I do that, let me undestand your point better.

My intentions were:
1) to provide a check for validity of a pad ID passed to an operation, not ann 
eligibility of a driver to support the operation,
2) to not break drivers which don't set pad_num, especially when building them 
with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.

Since pad IDs are verified against pad_num which may be not set, we should 
obviously check validity of pad_num before comparing against it.  Since media 
controller compatible subdevices need at least one pad, I think the check for 
non-zero pad_num is quite reasonable.

Moreover, old drivers are actually using those pad operations you describe as 
not supposed to be called.  They are using them because they were converted to 
use them in place of former video ops.  Already dealing with pad IDs, they may 
decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for 
example register pads, without implementing fulll media controller support.  
Why should we refuse to perform pad ID verification for them?

Thanks,
Janusz