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

Tue Jul 7 05:37:47 EDT 2020

On Tue, Jul 7, 2020 at 10:33 AM Cristian Marussi
<cristian.marussi at arm.com> wrote:
> On Tue, Jul 07, 2020 at 09:04:10AM +0100, Sudeep Holla wrote:
> > 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 at 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.
> >
>
> This structure is used only internally to the SCMI Notifications machinery to
> describe and push the events from the RX ISR to the deferred worked through the
> kfifos, so the size_t seemed a good fit given it represents a length and the struct
> is just an internal helper.
> The reason for the unneeded __packed was because it seemed odd to me to push
> around padding through the internal fifos, but it is not needed and it would
> hurt perfomance indeed due to the forced misalignment: I'll drop it.

I would suggest first changing the structure to avoid the internal padding by
moving the payld_sz ahead of the evt_id, or by changing evt_id and payld_sz
to both to be u32 members.

The size_t seemed mostly confusing because you went with the fixed-size 'u64'
for the timestamp instead of using the abstract ktime_t type (which is
also 64 bit
but signed).

> > > 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.
>
> Correct, these structs are just common per-event descriptors built by the
> notifications core while dispatching events and passed to the interested users
> (SCMI drivers) as an argument to their notifier_block registered callback to
> provide specific info about the received event.
>
> Not sure though, Arnd, if you added the padding comments above just to
> highlight the possible issue here or if you want them added ?

Both, kind of. Again, using only u64/u32 types in a structure tells the reader
that this is supposed to be a fixed structure layout even if that's not what
you meant, but having implied padding makes it look like you got that wrong
by accident.

Using the abstract ktime_t here or 'unsigned int' or 'unsigned long' types
would again make it clearer to a casual reader that this is a kernel-internal
structure with no constraints on the layout, while adding a comment about
padding or something like a 'u32 __pad' member would convey that you
have thought about the layout and intentionally left it at that.

I think these structures are more the second type, since they are in a
header called scmi_protocol.h and passed around with an explicit
length, so explicit padding is probably best. I would also try to avoid
the 'bool' type for the same reason and make that a 'u8' member.

     Arnd