linux-kernel - Re: [PATCH 13/18] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <23cbc632-9a57-ad67-b7f8-61ff0672d070@gmail.com>
Date:   Tue, 1 Dec 2020 23:15:44 +0000
From:   Dan Scally <djrscally@...il.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-media@...r.kernel.org, devel@...ica.org, rjw@...ysocki.net,
        lenb@...nel.org, gregkh@...uxfoundation.org,
        mika.westerberg@...ux.intel.com, andriy.shevchenko@...ux.intel.com,
        linus.walleij@...aro.org, bgolaszewski@...libre.com,
        wsa@...nel.org, yong.zhi@...el.com, sakari.ailus@...ux.intel.com,
        bingbu.cao@...el.com, tian.shu.qiu@...el.com, mchehab@...nel.org,
        robert.moore@...el.com, erik.kaneda@...el.com, pmladek@...e.com,
        rostedt@...dmis.org, sergey.senozhatsky@...il.com,
        linux@...musvillemoes.dk, kieran.bingham+renesas@...asonboard.com,
        jacopo+renesas@...ndi.org,
        laurent.pinchart+renesas@...asonboard.com,
        jorhand@...ux.microsoft.com, kitakar@...il.com,
        heikki.krogerus@...ux.intel.com
Subject: Re: [PATCH 13/18] ipu3-cio2: Add functionality allowing software_node
 connections to sensors on platforms designed for Windows

Hi Laurent
On 01/12/2020 22:30, Laurent Pinchart wrote:
>>>> +}
>>>> +
>>>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	cio2_bridge_init_property_names(sensor);
>>>> +
>>>> +	for (i = 0; i < 4; i++)
>>>> +		sensor->data_lanes[i] = i + 1;
>>> Is there no provision in the SSDB for data lane remapping ?
>> Sorry; don't follow what you mean by data lane remapping here.
> Some CSI-2 receivers can remap data lanes. The routing inside the SoC
> from the data lane input pins to the PHYs is configurable. This makes
> board design easier as you can route the data lanes to any of the
> inputs. That's why the data lanes DT property is a list of lane numbers
> instead of a number of lanes. I'm actually not sure if the CIO2 supports
> this.

I don't see anything in the SSDB that might refer to that, though of
course we're lacking documentation for it so it could be a part that we
don't understand yet.


>>>> +			dev_info(&bridge->cio2->dev,
>>>> +				 "Found supported sensor %s\n",
>>>> +				 acpi_dev_name(adev));
>>>> +
>>>> +			bridge->n_sensors++;
>>> We probably want a check here to avoid overflowing bridge->sensors. The
>>> other option is to make bridge->sensors a struct list_head and allocate
>>> sensors dynamically.
>> Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
>> the maximum. Seems easier to just do a check, unless the wasted memory
>> is enough that it's worth allocating dynamically. I don't mind either
>> approach.
> In theory we could route multiple sensors to the same receiver, as long
> as only one of them drives the lanes at any given time. It's one way to
> support multiple sensors in cheap designs. I doubt we'll ever encounter
> that with the IPU3, so we could just limit the count to 4.
Ah, that's neat though. But I'll leave it at a check at the top of the
loop for now.
>>>> +
>>>> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
>>>> +	if (!fwnode) {
>>>> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
>>>> +		ret = -ENODEV;
>>>> +		goto err_unregister_sensors;
>>> Can this happen ?
>> It _shouldn't_ happen, as long as nothing else is touching the swnodes
>> I've registered or anything. I've never seen it happen. That didn't feel
>> like quite enough to say it can't ever happen - but I'm happy to skip
>> the check if you think thats ok.
> It seems a bit overkill to me, but I'm not a swnode specialist :-)
I'm going to keep it, if you have no strong feelings, partly through
caution but also because the other place swnodes are most heavily used
(drivers/platform/x86/intel_cht_int33fe_typec.c) _does_ perform the
check, so consistency too.
>>>> @@ -0,0 +1,108 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Author: Dan Scally <djrscally@...il.com> */
>>>> +#ifndef __CIO2_BRIDGE_H
>>>> +#define __CIO2_BRIDGE_H
>>>> +
>>>> +#include <linux/property.h>
>>>> +
>>>> +#define CIO2_HID				"INT343E"
>>>> +#define CIO2_NUM_PORTS			  4
>>> There are a few rogue spaces before '4'.
>> Argh, thanks, this is the curse of using VS code on multiple machines...
> I recommend vim ;-)
You're not the only one - maybe I need to spend the time and it'll save
time in the future