linux-kernel - Re: [PATCH] Revert "of: property: fw_devlink: Add support for remote-endpoint"

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAGETcx-3Y3rOSoXu3SbDa6BP_jcT8uSQA+MV55QCY4b0Oe7L-A@mail.gmail.com>
Date:   Mon, 27 Sep 2021 14:58:07 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "of: property: fw_devlink: Add support for remote-endpoint"

On Mon, Sep 27, 2021 at 1:48 PM Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> Since the commit f7514a663016 ("of: property: fw_devlink: Add support
> for remote-endpoint") Linux kernel started parsing and adding devlinks
> for the remote-endpoint properties. However this brings more harm than
> good.
>
> For all the remote-endpoints in the graph two links are created. Thus
> each and every remote-endpoint ends up in the cyclic graph (instead of
> the original intent of catching a cycle of graph + non-graph link):

Yes, I'm well aware of this. I even called this out in the commit
text. This creating of cycles and then catching and relaxing it is
intentional.
https://lore.kernel.org/all/20210330185056.1022008-1-saravanak@google.com/

>
> [    0.381057] OF: remote-endpoint linking /soc@...eniqup@...000/i2c@...000/hdmi-bridge@2b to /soc@...dss@...0000/dsi@...4000/ports/port@...ndpoint
> [    0.394421] OF: remote-endpoint linking /soc@...eniqup@...000/i2c@...000/hdmi-bridge@2b to /hdmi-out/port/endpoint
> [    0.407007] OF: remote-endpoint linking /soc@...hy@...9000 to /soc@...pmi@...0000/pmic@...mic-tcpm/connector/ports/port@...ndpoint@0
> [    0.419648] OF: remote-endpoint linking /soc@...sb@...8800/usb@...0000 to /soc@...pmi@...0000/pmic@...mic-tcpm/ports/port@...ndpoint@0
> [    0.432578] OF: remote-endpoint linking /soc@...ci@...f000/i2c-bus@...am1@c0 to /soc@...amss@...a000/ports/port@...ndpoint
> [    0.444450] OF: remote-endpoint linking /soc@...amss@...a000 to /soc@...ci@...f000/i2c-bus@...am1@...port/endpoint
> [    0.455292] OF: remote-endpoint linking /soc@...dss@...0000/mdp@...1000 to /soc@...dss@...0000/dsi@...4000/ports/port@...ndpoint
> [    0.467210] OF: remote-endpoint linking /soc@...dss@...0000/mdp@...1000 to /soc@...dss@...0000/dsi@...6000/ports/port@...ndpoint
> [    0.479239] OF: remote-endpoint linking /soc@...dss@...0000/dsi@...4000 to /soc@...dss@...0000/mdp@...1000/ports/port@...ndpoint
> [    0.491147] OF: remote-endpoint linking /soc@...dss@...0000/dsi@...4000 to /soc@...eniqup@...000/i2c@...000/hdmi-bridge@...ports/port@...ndpoint
> [    0.504979] OF: remote-endpoint linking /soc@...pmi@...0000/pmic@...ypec@...0 to /soc@...pmi@...0000/pmic@...mic-tcpm/ports/port@...ndpoint
> [    0.517958] OF: remote-endpoint linking /soc@...pmi@...0000/pmic@...dphy@...0 to /soc@...pmi@...0000/pmic@...mic-tcpm/ports/port@...ndpoint
> [    0.565326] OF: remote-endpoint linking /hdmi-out to /soc@...eniqup@...000/i2c@...000/hdmi-bridge@...ports/port@...ndpoint
>
> Under some conditions the device can become it's own supplier,
> preventing this device to be probed at all:

I'm not sure this analysis is correct -- this shouldn't be happening.
If you go to the device link folder and cat "sync_state_only", I
expect it to be "1" in this case. Can you confirm that?

Which means it won't block probing. Yes, the link itself is useless
and it'll get auto deleted once mdss probes and it's easy to not
create it in the first place. But this is definitely not your issue.

> $ ls -l /sys/bus/platform/devices/ae00000.mdss/
> lrwxrwxrwx    1 root     root             0 Aug  4 15:13 consumer:platform:ae00000.mdss -> ../../../virtual/devlink/platform:ae00000.mdss--platform:ae00000.mdss
>
> I think that until of_link can be tought to handle bi-directional links
> on its own, we should not parse remote-endpoint properties. Thus the
> aforementioned commit should be reverted.

Nak. remote-endpoint parsing is working as intended. I don't think the
analysis is correct.

Can you please enable the logs in all these functions and attach the
log so we can see why it's not probing mdss?
device_link_add
device_links_check_suppliers
func fw_devlink_relax_link
fw_devlink_create_devlink

-Saravana