Re: [GIT PULL] firmware: arm_scmi: updates for v5.9

From: Arnd Bergmann <arnd@arndb.de>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Kevin Hilman <khilman@kernel.org>, SoC Team <soc@kernel.org>,
	ARM SoC Team <arm@kernel.org>,
	ALKML <linux-arm-kernel@lists.infradead.org>,
	Olof Johansson <olof@lixom.net>,
	Cristian Marussi <cristian.marussi@arm.com>
Subject: Re: [GIT PULL] firmware: arm_scmi: updates for v5.9
Date: Mon, 6 Jul 2020 21:23:46 +0200	[thread overview]
Message-ID: <CAK8P3a2QicGAfNnykBFDkbCX4TEZ+=AwWHYSQLOvKK14exN6_Q@mail.gmail.com> (raw)
In-Reply-To: <20200706165336.40800-1-sudeep.holla@arm.com>

On Mon, Jul 6, 2020 at 6:53 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407:
>
>   Linux 5.8-rc1 (2020-06-14 12:45:04 -0700)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git tags/scmi-updates-5.9
>
> for you to fetch changes up to 585dfab3fb80e67b3a54790b3d5ef2991feb3950:
>
>   firmware: arm_scmi: Add base notifications support (2020-07-01 17:07:26 +0100)
>
> ----------------------------------------------------------------
> ARM SCMI/SCPI updates for v5.9
>
> The main addition for this time is the support for platform notifications.
> SCMI protocol specification allows the platform to signal events to the
> interested agents via notification messages. We are adding support for
> the dispatch and delivery of such notifications to the interested users
> inside the kernel.
>
> Other than that, there are minor changes like checking and using the
> fast_switch capability quering the firmware instead of doing it
> unconditionally(using polling mode transfer), cosmetic trace update and
> use of HAVE_ARM_SMCCC_DISCOVERY instead of ARM_PSCI_FW.

I haven't pulled this yet, as I noticed one data structure definition that
seems very odd:

struct scmi_event_header {
        u64     timestamp;
        u8      evt_id;
        size_t  payld_sz;
        u8      payld[];
} __packed;

This is an odd mix of fixed-length fields (u64) and variable length
fields (size_t is different on 32-bit machines), out of which at least
one is misaligned because of the __packed attribute.

There are others that are just slightly odd:

struct scmi_reset_issued_report {
       u64 timestamp;
       u32 agent_id;
       u32 domain_id;
       u32 reset_state;
      /* four bytes padding */
};

struct scmi_perf_level_report {
       u64 timestamp;
       u32 agent_id;
       u32 domain_id;
       u32 performance_level;
      /* four bytes padding */
};

struct scmi_base_error_report {
       u64 timestamp;
       u32 agent_id;
       bool fatal;
       /* 1 byte padding */
       u16 cmd_count;
       u64 reports[0];
};

as this includes four implied padding bytes at the end. I could not figure
out exactly what the guarantees for interface stability on either of
them are, but if these get passed between the kernel and some other
code (firmware or user space), or might be in the future, then I'd suggest
redefining them in a way that avoids those oddities.

Once this has been clarified, please just add any further patches
(if needed) on top of the existing branch and send a new pull request.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel