linux-kernel - Re: [PATCH V4] firmware: arm_scmi: Make scmi core independent of the 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: <82e1181a-b1ff-eccc-d61d-2da0e7afec25@opensynergy.com>
Date:   Fri, 24 Jan 2020 13:15:24 +0100
From:   Peter Hilber <peter.hilber@...nsynergy.com>
To:     Arnd Bergmann <arnd@...db.de>, Sudeep Holla <Sudeep.Holla@....com>,
        "Viresh Kumar" <viresh.kumar@...aro.org>
CC:     Jassi Brar <jassisinghbrar@...il.com>,
        Cristian Marussi <cristian.marussi@....com>,
        <peng.fan@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        ALKML <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH V4] firmware: arm_scmi: Make scmi core independent of the
 transport type

On 22.01.20 03:36, Viresh Kumar 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 on 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.

Sorry for being a bit late with my feedback.

Accessing struct scmi_shared_mem members from driver.c forces any
transport to also use the struct scmi_shared_mem layout (or at least
pretend to do so). IMHO this does not work very well for transports
which are not using a fixed memory region to transmit and receive. But I
think the current approach will still work out.

virtio transfers each message in a separate buffer, and always uses
different parts of the buffer for transmit data and receive data, which
is contrary to the assumptions of the struct scmi_shared_mem.

The virtio transport will probably have no use for the struct
scmi_shared_mem.channel_status and .flags. The check for
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE in scmi_tx_prepare() is not required
for the virtio transport.

I would have preferred (to have an option) to use as data passing
interface to the transport just the struct scmi_xfer. A transport using
this option would not implement ops (read|write)32 and memcpy_(from|to).
The transport would also not call scmi_tx_prepare(), but instead take
data from struct scmi_xfer directly. The transport would use a modified
scmi_rx_callback() to notify that it updated the struct scmi_xfer. A
helper to derive the struct scmi_xfer * from the message header would be
extracted from scmi_rx_callback(). The scmi_xfer_poll_done() would
become an (optional) transport op.

Other remarks:

If staying with this approach, it would be more elegant to add an
abstraction through which the transport can set the
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE bit in the struct scmi_shared_mem.

The SCMI spec allows both interrupt-based and polling-based completion
notification. The transport should be able to indicate which
notification methods it supports. The virtio transport would not want to
support polling.

> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t)

scmi_rx_callback() doesn't need the struct scmi_xfer * parameter any
more ATM (and the transport might also not know about it).

Best regards,

Peter

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>

[tech_days_munchen]

OpenSynergy TechDay München

am 11. Februar 2020, ab 12:00Uhr, im Studio Balan, Moosacherstr. 86.

Anmeldung bitte hier<mailto:sabine.mutumba@...nsynergy.com>