linux-kernel - Re: [PATCH v3 16/22] firmware: arm_scmi: add arm

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]

Date:   Wed, 4 Oct 2017 13:36:39 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     ALKML <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        Roy Franz <roy.franz@...ium.com>,
        Harb Abdulhamid <harba@...eaurora.org>,
        Nishanth Menon <nm@...com>, Loc Ho <lho@....com>,
        Alexey Klimov <alexey.klimov@....com>,
        Ryan Harkin <Ryan.Harkin@....com>,
        Jassi Brar <jassisinghbrar@...il.com>
Subject: Re: [PATCH v3 16/22] firmware: arm_scmi: add arm_mhu specific mailbox interface

On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla <sudeep.holla@....com> wrote:
> This patch adds ARM MHU specific mailbox interface for SCMI.
>
> Cc: Arnd Bergmann <arnd@...db.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>

This clearly needs an explanation why we need another driver.

> +union mhu_data {
> +       void *ptr;
> +       u32 val;
> +};
> +
> +static void mhu_tx_prepare(struct mbox_client *cl, void *m)
> +{
> +       struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> +       union mhu_data tmp;
> +
> +       scmi_generic_tx_prepare(cinfo, m);
> +
> +       /* clear only the relavant bit */
> +       tmp.ptr = cinfo->priv;
> +       *(u32 *)m &= tmp.val;
> +}

Why do you use the first 32 bits of the pointer here? That doesn't
seem to make any sense, in particular on big-endian 64-bit
architectures.

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 97285a22dfaa..bdc9c566e6c1 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -648,6 +648,9 @@ EXPORT_SYMBOL_GPL(devm_scmi_handle_get);
>  /* Each compatible listed below must have descriptor associated with it */
>  static const struct of_device_id scmi_of_match[] = {
>         { .compatible = "arm,scmi", .data = &scmi_generic_desc },
> +#if IS_REACHABLE(CONFIG_ARM_MHU)
> +       { .compatible = "arm,mhu-scmi", .data = &mhu_scmi_desc },
> +#endif
>         { /* Sentinel */ },
>  };
>

This again is a bad abstraction, if the main part needs to know about
each mailbox that it could use.

Turn the registration around so rather than referring to an exported
symbol from mhu_scmi_desc.c in the main driver, you export
a symbol to register the operations and make the mhu
part its own module.

       Arnd