| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a8ea673c-a519-81e8-35b1-9d4a224dcbf5@mentor.com>
Date: Tue, 25 Sep 2018 14:04:21 -0700
From: Steve Longerbeam <steve_longerbeam@...tor.com>
To: Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
Steve Longerbeam <slongerbeam@...il.com>
CC: <linux-media@...r.kernel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Niklas Söderlund
<niklas.soderlund+renesas@...natech.se>,
Hans Verkuil <hans.verkuil@...co.com>,
Sebastian Reichel <sre@...nel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 02/17] media: v4l2: async: Allow searching for asd of
any type
Hi Mauro,
On 09/24/2018 10:06 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 9 Jul 2018 15:39:02 -0700
> Steve Longerbeam <slongerbeam@...il.com> escreveu:
>
>> Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
>> searching for any type of async subdev, not just fwnodes. Rename to
>> v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@...tor.com>
>> ---
>> Changes since v5:
>> - none
>> Changes since v4:
>> - none
>> Changes since v3:
>> - removed TODO to support asd compare with CUSTOM match type in
>> asd_equal().
>> Changes since v2:
>> - code optimization in asd_equal(), and remove unneeded braces,
>> suggested by Sakari Ailus.
>> Changes since v1:
>> - none
>> ---
>> drivers/media/v4l2-core/v4l2-async.c | 73 +++++++++++++++++++++---------------
>> 1 file changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 2b08d03..0e7e529 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -124,6 +124,31 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>> return NULL;
>> }
>>
>> +/* Compare two asd's for equivalence */
> Please, on comments, instead of "asd" prefer to use what this 3 random
> letters mean, e. g.:
> asd -> asynchronous subdevice
Ok, I will change the comment to read:
/* Compare two async subdevice descriptors for equivalence */
>
>> +static bool asd_equal(struct v4l2_async_subdev *asd_x,
>> + struct v4l2_async_subdev *asd_y)
>> +{
>> + if (asd_x->match_type != asd_y->match_type)
>> + return false;
>> +
>> + switch (asd_x->match_type) {
>> + case V4L2_ASYNC_MATCH_DEVNAME:
>> + return strcmp(asd_x->match.device_name,
>> + asd_y->match.device_name) == 0;
>> + case V4L2_ASYNC_MATCH_I2C:
>> + return asd_x->match.i2c.adapter_id ==
>> + asd_y->match.i2c.adapter_id &&
>> + asd_x->match.i2c.address ==
>> + asd_y->match.i2c.address;
>> + case V4L2_ASYNC_MATCH_FWNODE:
>> + return asd_x->match.fwnode == asd_y->match.fwnode;
>> + default:
>> + break;
>> + }
>> +
>> + return false;
>> +}
>> +
>> /* Find the sub-device notifier registered by a sub-device driver. */
>> static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
>> struct v4l2_subdev *sd)
>> @@ -308,29 +333,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>> notifier->parent = NULL;
>> }
>>
>> -/* See if an fwnode can be found in a notifier's lists. */
>> -static bool __v4l2_async_notifier_fwnode_has_async_subdev(
>> - struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
>> +/* See if an async sub-device can be found in a notifier's lists. */
>> +static bool __v4l2_async_notifier_has_async_subdev(
>> + struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
> This is a minor issue, but checkpatch complains (with reason)
> (with --strict) about the above:
>
> CHECK: Lines should not end with a '('
> #63: FILE: drivers/media/v4l2-core/v4l2-async.c:337:
> +static bool __v4l2_async_notifier_has_async_subdev(
>
> Better to declare it, instead, as:
>
> static bool
> __v4l2_async_notifier_has_async_subdev(struct v4l2_async_notifier *notifier,
> struct v4l2_async_subdev *asd)
>
> Similar warnings appear on other places:
> CHECK: Lines should not end with a '('
> #102: FILE: drivers/media/v4l2-core/v4l2-async.c:362:
> +static bool v4l2_async_notifier_has_async_subdev(
>
> CHECK: Lines should not end with a '('
> #141: FILE: drivers/media/v4l2-core/v4l2-async.c:410:
> + if (v4l2_async_notifier_has_async_subdev(
Will fix.
>
> Btw, Checkpatch also complains that the author's email is different
> than the SOB's one:
>
> WARNING: Missing Signed-off-by: line by nominal patch author 'Steve Longerbeam <slongerbeam@...il.com>'
>
> (the some comes with Signed-off-by: Steve Longerbeam <steve_longerbeam@...tor.com>)
>
> I suspect that other patches on this series will suffer from the same issue.
Will fix when submitting v7.
> <snip>
>
>> @@ -392,12 +406,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>> case V4L2_ASYNC_MATCH_CUSTOM:
>> case V4L2_ASYNC_MATCH_DEVNAME:
>> case V4L2_ASYNC_MATCH_I2C:
>> - break;
>> case V4L2_ASYNC_MATCH_FWNODE:
>> - if (v4l2_async_notifier_fwnode_has_async_subdev(
>> - notifier, asd->match.fwnode, i)) {
>> + if (v4l2_async_notifier_has_async_subdev(
>> + notifier, asd, i)) {
>> dev_err(dev,
>> - "fwnode has already been registered or in notifier's subdev list\n");
>> + "asd has already been registered or in notifier's subdev list\n");
> Please, never use "asd" on messages printed to the user. While someone
> may understand it while reading the source code, for a poor use,
> "asd" is just a random sequence of 3 characters.
I will change the message to read:
"subdev descriptor already listed in this or other notifiers".
Steve
Powered by blists - more mailing lists