| 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: <f1e40fda-1e71-7b8c-adf3-ffc72897db44@lucaceresoli.net>
Date: Wed, 21 Nov 2018 11:05:00 +0100
From: Luca Ceresoli <luca@...aceresoli.net>
To: jacopo mondi <jacopo@...ndi.org>
Cc: kieran.bingham@...asonboard.com, linux-renesas-soc@...r.kernel.org,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
sakari.ailus@....fi,
Niklas Söderlund <niklas.soderlund@...natech.se>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
linux-kernel@...r.kernel.org,
Jacopo Mondi <jacopo+renesas@...ndi.org>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Niklas Söderlund
<niklas.soderlund+renesas@...natech.se>
Subject: Re: [PATCH v4 3/4] media: i2c: Add MAX9286 driver
Hi Jacopo,
On 20/11/18 18:44, jacopo mondi wrote:
> Hi Luca,
>
> On Tue, Nov 20, 2018 at 11:51:37AM +0100, Luca Ceresoli wrote:
>> Hi Kieran,
>>
>> On 20/11/18 01:32, Kieran Bingham wrote:
>>> Hi Luca,
>>>
>>> My apologies for my travel induced delay in responding here,
>>
>> No problem.
>>
>>> On 14/11/2018 02:04, Luca Ceresoli wrote:
>> [...]
>>>>>>> +static int max9286_probe(struct i2c_client *client,
>>>>>>> + const struct i2c_device_id *did)
>>>>>>> +{
>>>>>>> + struct max9286_device *dev;
>>>>>>> + unsigned int i;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>>>>>> + if (!dev)
>>>>>>> + return -ENOMEM;
>>>>>>> +
>>>>>>> + dev->client = client;
>>>>>>> + i2c_set_clientdata(client, dev);
>>>>>>> +
>>>>>>> + for (i = 0; i < MAX9286_N_SINKS; i++)
>>>>>>> + max9286_init_format(&dev->fmt[i]);
>>>>>>> +
>>>>>>> + ret = max9286_parse_dt(dev);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + dev->regulator = regulator_get(&client->dev, "poc");
>>>>>>> + if (IS_ERR(dev->regulator)) {
>>>>>>> + if (PTR_ERR(dev->regulator) != -EPROBE_DEFER)
>>>>>>> + dev_err(&client->dev,
>>>>>>> + "Unable to get PoC regulator (%ld)\n",
>>>>>>> + PTR_ERR(dev->regulator));
>>>>>>> + ret = PTR_ERR(dev->regulator);
>>>>>>> + goto err_free;
>>>>>>> + }
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * We can have multiple MAX9286 instances on the same physical I2C
>>>>>>> + * bus, and I2C children behind ports of separate MAX9286 instances
>>>>>>> + * having the same I2C address. As the MAX9286 starts by default with
>>>>>>> + * all ports enabled, we need to disable all ports on all MAX9286
>>>>>>> + * instances before proceeding to further initialize the devices and
>>>>>>> + * instantiate children.
>>>>>>> + *
>>>>>>> + * Start by just disabling all channels on the current device. Then,
>>>>>>> + * if all other MAX9286 on the parent bus have been probed, proceed
>>>>>>> + * to initialize them all, including the current one.
>>>>>>> + */
>>>>>>> + max9286_i2c_mux_close(dev);
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * The MAX9286 initialises with auto-acknowledge enabled by default.
>>>>>>> + * This means that if multiple MAX9286 devices are connected to an I2C
>>>>>>> + * bus, another MAX9286 could ack I2C transfers meant for a device on
>>>>>>> + * the other side of the GMSL links for this MAX9286 (such as a
>>>>>>> + * MAX9271). To prevent that disable auto-acknowledge early on; it
>>>>>>> + * will be enabled later as needed.
>>>>>>> + */
>>>>>>> + max9286_configure_i2c(dev, false);
>>>>>>> +
>>>>>>> + ret = device_for_each_child(client->dev.parent, &client->dev,
>>>>>>> + max9286_is_bound);
>>>>>>> + if (ret)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + dev_dbg(&client->dev,
>>>>>>> + "All max9286 probed: start initialization sequence\n");
>>>>>>> + ret = device_for_each_child(client->dev.parent, NULL,
>>>>>>> + max9286_init);
>>>>>>
>>>>>> I can't manage to like this initialization sequence, sorry. If at all
>>>>>> possible, each max9286 should initialize itself independently from each
>>>>>> other, like any normal driver.
>>>>>
>>>>> Yes, I think we're in agreement here, but unfortunately this section is
>>>>> a workaround for the fact that our devices share a common address space.
>>>>>
>>>>> We (currently) *must* disable both devices before we start the
>>>>> initialisation process for either on our platform currently...
>>>>
>>>> The model I proposed in my review to patch 1/4 (split remote physical
>>>> address from local address pool) allows to avoid this workaround.
>>>
>>>
>>> Having just talked this through with Jacopo I think I see that we have
>>> two topics getting intertwined here too.
>>>
>>> - Address translation so that we can separate the camera addressing
>>>
>>> - our 'device_is_bound/device_for_each_child()' workaround which lets
>>> us make sure the buses are closed before we power on any camera
>>> device.
>>>
>>>
>>> For the upstream process of this driver - I will remove the
>>> 'device_is_bound()/device_for_each_child()' workarounds.
>>>
>>>
>>> It is /ugly/ and needs more consideration, but I believe we do still
>>> need it (or something similar) for our platform currently.
>>>
>>>
>>>
>>> The other side of that is the address translation. I think I was wrong
>>> earlier and may have said we have address translation on both sides.
>>>
>>>
>>> I think I now see that we only have some minimal translation for two
>>> addresses on the remote (max9271) side, not the local (max9286) side.
>>
>> Can the remote (max9271) translate addresses for transactions
>> originating from the local side? This would make it possible to do a
>> proper address translation, although 2 addresses is a quite small amount.
>>
>
> Yes, that's true for systems with a single max9286 [1]
>
> We have a system with 2 de-serializers, and what happens is the
> following:
>
> The system starts with the following configuration:
>
> 1)
> +------- max9271@40
> +------- max9271@40
> Soc ----> max9286 --+------- max9271@40
> +------- max9271@40
>
> with a single max9286 it would be easy. We operate on one channel at
> the time, do the reprogramming (or set up the translation, for the TI
> chip use case) when adding the adapter for the channel, and then we
> can talk with all remotes, which now have a different address
>
> 2)
> +-------- max9271@50
> +--- / -- max9271@40
> Soc ----> max9286 --+--- / -- max9271@40
> +--- / -- max9271@40
>
>
> +--- / -- max9271@50
> +-------- max9271@51
> Soc ----> max9286 --+--- / -- max9271@40
> +--- / -- max9271@40
>
> +--- / -- max9271@50
> +--- / -- max9271@51
> Soc ----> max9286 --+-------- max9271@52
> +--- / -- max9271@40
>
> +--- / -- max9271@50
> +--- / -- max9271@51
> Soc ----> max9286 --+--- / -- max9271@52
> +-------- max9271@53
>
> Of course, to do the reprogramming, we need to initially send messages
> to the default 0x40 address each max9271 boots with. If we don't close
> all channels but the one we intend to reprogram, all remotes would
> receive the same message, and thus will be re-programmed to the same
> address (not nice). [2]
>
> Now, if you have two max9286, installed on the same i2c bus, then you
> need to make sure all channels of the 'others' are closed, before you
> can reprogram your remotes, otherwise, you would end up reprogramming
> all the remotes of the 'others' when trying to reprogram yours, as our
> local de-serializers, bounces everything they receives, not directed
> to them, to their remote sides.
This last sentence is the one point that makes things so hard on the
GMSL chips. In my previous email(s) I partially forgot about this, so I
was hoping a better implementation could be possible. Thanks for
re-focusing me.
It would have been lovely if the hardware designers had at least put an
i2c mux between the soc and those chatty deserializers... :-\
> 3)
> +-------- max9271@50
> +--- / -- max9271@40
> Soc --+-> max9286@4c --+--- / -- max9271@40
> | +--- / -- max9271@40
> |
> |-> max9286@6c --+-------- max9271@50 <-- not nice
> +-------- max9271@50
> +-------- max9271@50
> +-------- max9271@50
>
> +--- / -- max9271@50
> +-------- max9271@51
> Soc --+-> max9286@4c --+--- / -- max9271@40
> | +--- / -- max9271@40
> |
> |-> max9286@6c --+-------- max9271@51 <-- not nice
> +-------- max9271@51
> +-------- max9271@51
> +-------- max9271@51
>
> ....
>
> With the (not nice) 'max9286_is_bound()' we make sure we close all
> channels on all max9286 first
>
> 4)
> +--- / -- max9271@40
> +--- / -- max9271@40
> Soc --+-> max9286@4c --+--- / -- max9271@40
> | +--- / -- max9271@40
> |
> |-> max9286@6c --+--- / -- max9271@40
> +--- / -- max9271@40
> +--- / -- max9271@40
> +--- / -- max9271@40
>
> And then only the last one to probe calls the re-programming
> phase for all its fellows de-serializers on the bus.
>
> 5)
> +-------- max9271@50
> +--- / -- max9271@40
> Soc --+-> max9286@4c --+--- / -- max9271@40
> | +--- / -- max9271@40
> |
> |-> max9286@6c --+--- / -- max9271@40
> +--- / -- max9271@40
> +--- / -- max9271@40
> +--- / -- max9271@40
> ....
>
>
> +--- / -- max9271@50
> +--- / -- max9271@51
> Soc --+-> max9286@4c --+--- / -- max9271@52
> | +--- / -- max9271@53
> |
> |-> max9286@6c --+-------- max9271@54
> +--- / -- max9271@40
> +--- / -- max9271@40
> +--- / -- max9271@40
>
> When addr reprogramming is done, we enter the image streaming phase,
> with all channels open, as now, all remotes, have a different i2c
> address assigned.
>
> Suggestions on how to better handle this are very welcome. The point
> here is that, to me, this is a gmsl-specific implementation thing.
>
> Do you think for your chips, if they do translations, can you easy mask
> them with the i2c address you want (being that specified in the remote
> node or selected from an i2c-addr-pool, or something else) without
> having to care about others remotes to be accidentally programmed to
> an i2c address they're not intended to be assigned to.
Yes. The TI chips have a "passthrough-all" option to propagate all
transactions with an unknown address, but it's mostly meant for
debugging. In normal usage the local chip will propagate (with addresses
translated) only transactions coming with a known slave address,
including its own address(es), the remote (de)ser aliases and the remote
chip aliases. All aliases are disabled until programmed.
> Hope this helps clarify your concerns, and I think the actual issue
> to discuss, at least on bindings, would be the i2c-address assignment
> method, as this impacts GMSL, as well as other implementation that
> would use the same binding style as this patches.
Absolutely.
Bye,
--
Luca
Powered by blists - more mailing lists