[GIT PULL] firmware: arm_scmi: updates for v5.9
Cristian Marussi
cristian.marussi at arm.com
Wed Jul 8 09:41:50 EDT 2020
On Tue, Jul 07, 2020 at 04:12:40PM +0100, Cristian Marussi wrote:
> Hi Arnd,
>
> On Tue, Jul 07, 2020 at 11:37:47AM +0200, Arnd Bergmann wrote:
> > 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).
>
> In fact in an earlier iteration of this series I was using ktime_t, then I had automously
> the brilliant (:D) idea of switching to u64: the (probably bogus) reason for this was that
> I realized that ktime_t was signed and given that this timestamp represent the time, in
> bootime ns, at which the event was received in the RX ISR, I thought that having 64 bits was
> better than 63....but given that this field is not really even architected in the spec but
> simply an implementation aid if the user wanted to measure the latency on his side I think
> I can simply stick to ktime_t or use unsigned long at least.
>
> The reason behind the other fixed-size u8 fields in scmi_event_header is instead directly
> 'inspired' by length of the events SCMI messages as defined by the current spec, but the real
> underlying reason (as for the unneeded __packed) was to stick to the minimum spec-required
> sizes so as to minimize the number of bytes to be memcopied in and out of the kfifo, since,
> during the external review some remarks were raised about one other redundant memcpy that
> would have led to an average unneeded copy of a dozen bytes on each transfer: so, after I
> refactored the code to remove such memcpy, I thought was sensible to shrink the header as
> much as possible to keep memcopied bytes to the minimum.
>
> Probably it's not worth the effort and I could just stick to unsigned char/int/long here
> while still dropping the __packed.
>
> >
> > > > > 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.
> >
>
> Ah, understood.
>
> > 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.
> >
>
> Reviewing this u32/u64 usage here instead I think it's wrong; the whole reason
> of event reports was to convey into the user callbacks some event-specific meaningful
> information in a standard format about the events WITHOUT exposing directly the
> protocol-message structure: so that a standard report is generated during the event-dispatch
> from the raw content in scmi_evemt_header, possibly dropping any non user-interesting event
> fields, and without the user-callback need to have knowledge/decoding of the protocol
> internals but just the report layout.
>
> Using u32/u64 sized report-fields, inspired by the protocol-event-msg spec, is in fact at odd
> with this idea and hurts back compatibility, since if an already released spec should change
> in a some way, (even though we clearly know for sure that in our perfectly civilized
> world this simply cannot happen...o_O), say changing one u16 field into a u8 (or viceversa) we
> would be in trouble supporting this across possible multiple spec-versions, so maybe just
> sticking here to a generic unsigned char/int/long also for the report fields would guard us
> against near-future issues. Does it makes sense ?
>
> Not really a short reply...but thanks to have raised this point so I could rethink to all of this.
>
> Cristian
>
This 4 patches series posted now:
https://lore.kernel.org/linux-arm-kernel/20200708122248.52771-1-cristian.marussi@arm.com/
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-08 12:22 ` [PATCH 1/4] firmware: arm_scmi: Remove zero-length array in SCMI Notifications
2020-07-08 12:22 ` [PATCH 2/4] firmware: arm_scmi: Remove unneeded __packed attribute Cristian Marussi
2020-07-08 12:22 ` [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing Cristian Marussi
2020-07-08 12:22 ` [PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports Cristian Marussi
is meant to address the above issues (hopefully)
Thanks
Cristian
More information about the linux-arm-kernel
mailing list