linux-kernel - Re: [PATCH] firmware: arm_scmi: Make scmi core independent of transport type

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: <20200109093442.4jt44eu2zlmjaq3f@vireshk-i7>
Date:   Thu, 9 Jan 2020 15:04:42 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Sudeep Holla <sudeep.holla@....com>, peng.fan@....com,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] firmware: arm_scmi: Make scmi core independent of
 transport type

On 09-01-20, 09:18, Arnd Bergmann wrote:
> On Fri, Nov 29, 2019 at 10:32 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> >
> > The SCMI specification is fairly independent of the transport protocol,
> > which can be a simple mailbox (already implemented) or anything else.
> > The current Linux implementation however is very much dependent of the
> > mailbox transport layer.
> >
> > This patch makes the SCMI core code (driver.c) independent of the
> > mailbox transport layer and moves all mailbox related code to a new
> > file: mailbox.c.
> >
> > We can now implement more transport protocols to transport SCMI
> > messages.
> >
> > The transport protocols just need to provide struct scmi_transport_ops,
> > with its version of the callbacks to enable exchange of SCMI messages.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> 
> Conceptually I think this is fine, but as others have said, it would be
> better to have another transport implementation posted along with this
> to see if the interfaces actually work out.

@Sudeep/Vincent: Do you think we can add another transport
implementation something right away for it ?

@Peng ?

> > +/**
> > + * struct scmi_chan_info - Structure representing a SCMI channel information
> > + *
> > + * @payload: Transmit/Receive payload area
> > + * @dev: Reference to device in the SCMI hierarchy corresponding to this
> > + *      channel
> > + * @handle: Pointer to SCMI entity handle
> > + * @transport_info: Transport layer related information
> > + */
> > +struct scmi_chan_info {
> > +       void __iomem *payload;
> > +       struct device *dev;
> > +       struct scmi_handle *handle;
> > +       void *transport_info;
> > +};
> 
> I would assume that with another transport, the 'payload' pointer would
> not be __iomem

Hmm, okay. I just separated things based on the current transport and
didn't add much changes on top of it as I wasn't sure how things are
going to look with next transport and so left the changes for then.

I can now drop it though.

> > +static int scmi_set_transport_ops(struct scmi_info *info)
> > +{
> > +       struct scmi_transport_ops *ops;
> > +       struct device *dev = info->dev;
> > +
> > +       /* Only mailbox method supported for now */
> > +       ops = scmi_mailbox_get_ops(dev);
> > +       if (!ops) {
> > +               dev_err(dev, "Transport protocol not found in %pOF\n",
> > +                       dev->of_node);
> > +               return -EINVAL;
> > +       }
> > +
> > +       info->transport_ops = ops;
> > +       return 0;
> > +}
> 
> This looks odd: rather than guessing the transport type based on
> random DT properties, I would prefer to have it determined by
> the device compatible string, and have different drivers bind
> to one of them each, with each driver linking against a common
> base implementation, either as separate modules or in one file.

Since there are no platforms using the scmi binding in mainline kernel
for now, it won't be difficult to add new compatible strings. So
should this be done like:

        compatible = "arm,scmi", "arm,scmi-mailbox";

or just
        compatible = "arm,scmi-mailbox";

?
-- 
viresh