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

[Sudeep Holla <sudeep.holla@arm.com>]

Hi Arnd,

On Mon, Jul 06, 2020 at 09:23:46PM +0200, Arnd Bergmann wrote:
> 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.
>

Agreed, my mistake. I did mention to get rid of __packed in earlier version
and completely missed to observe in later versions.

> 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.
>

These structures are not shared across kernel and userspace/firmware. It
is entirely constructed by kernel for other users within kernel.

Cristian, correct me if I am wrong ? Or add more info/clarity if it
helps the discussion here.

> 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.
>

Thanks

--
Regards,
Sudeep

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