linux-kernel - Re: [PATCH v1 3/3] Revert "Revert "driver core: Set fw

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: <20210427151042.j7hku7pxqz56uyt6@bogus>
Date:   Tue, 27 Apr 2021 16:10:42 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Cristian Marussi <cristian.marussi@....com>
Cc:     Jim Quinlan <james.quinlan@...adcom.com>,
        Sudeep Holla <sudeep.holla@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Saravana Kannan <saravanak@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Al Cooper <alcooperx@...il.com>,
        Michael Walle <michael@...le.cc>,
        Jon Hunter <jonathanh@...dia.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Android Kernel Team <kernel-team@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 3/3] Revert "Revert "driver core: Set fw_devlink=on by
 default""

On Tue, Apr 27, 2021 at 03:11:16PM +0100, Cristian Marussi wrote:
> On Tue, Apr 27, 2021 at 09:33:31AM -0400, Jim Quinlan wrote:
[...]
> > >
> > I believe that the brcmstb-mbox node is in our DT for backwards
> > compatibility with our older Linux only.   Note that  we use the compatible
> > string '"arm,scmi-smc", "arm,scmi"'; the former chooses SMC transport and
> > ignores custom  mailboxes such as brcmstb-mbox.
> >
>
> Right..so it is even more wrong that it is waiting for the mailboxes...but
> looking at the DT:
>
> brcm_scmi_mailbox@0 {
>                 #mbox-cells = <0x01>;
>                 compatible = "brcm,brcmstb-mbox";
>                 status = "disabled";
>                 linux,phandle = <0x04>;
>                 phandle = <0x04>;
>         };
>
> brcm_scmi@0 {
>                 compatible = "arm,scmi-smc\0arm,scmi";
>                 mboxes = <0x04 0x00 0x04 0x01>;
>                 mbox-names = "tx\0rx";
>                 shmem = <0x05>;
>                 status = "disabled";
>                 arm,smc-id = <0x83000400>;
>                 interrupt-names = "a2p";
>                 #address-cells = <0x01>;
>                 #size-cells = <0x00>;
>
> it seems to me that even though you declare an SMC based transport (and in fact
> you define the smc-id too) you also still define mboxes (as a fallback I suppose)
> referring to the brcm_scmi_mailbox phandle, and while this is ignored by the SCMI
> driver (because you have selected a compatible SMC transport) I imagine this dep
> is picked up by fw_devlink which in fact says:
>
> > [    0.300086] platform brcm_scmi@0: Linked as a consumer to brcm_scmi_mailbox@0
>
> and stalls waiting for it. (but I'm not really familiar on how fw_devlink
> internals works really...so I maybe off in these regards)
>

I was about to mention/ask the same when I saw Jim's reply. I see you have
already asked that. Couple of my opinions based on my very limited knowledge
on fw_devlink and how it works.

1. Since we have different compatible for SMC and mailbox, I am not sure
   if it correct to leave mailbox information in scmi node. Once we have
   proper yaml scheme, we must flag that error IMO.

2. IIUC, the fw_devlink might use information from DT to establish the
   dependency and having mailbox information in this context may be
   considered wrong as there is no dependency if it is using SMC.

--
Regards,
Sudeep