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

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

[Sudeep Holla]

Hi ARM SoC Team,

Please pull !

Regards,
Sudeep

-->8

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.

----------------------------------------------------------------
Cristian Marussi (10):
      firmware: arm_scmi: Fix SCMI genpd domain probing
      firmware: arm_scmi: Add notification protocol-registration
      firmware: arm_scmi: Add notification callbacks-registration
      firmware: arm_scmi: Add notification dispatch and delivery
      firmware: arm_scmi: Enable notification core
      firmware: arm_scmi: Add power notifications support
      firmware: arm_scmi: Add perf notifications support
      firmware: arm_scmi: Add sensor notifications support
      firmware: arm_scmi: Add reset notifications support
      firmware: arm_scmi: Add base notifications support

Nicola Mazzucato (2):
      firmware: arm_scmi: Add fast_switch_possible() interface
      cpufreq: arm_scmi: Set fast_switch_possible conditionally

Sudeep Holla (2):
      firmware: arm_scmi: Use signed integer to report transfer status
      firmware: arm_scmi: Use HAVE_ARM_SMCCC_DISCOVERY instead of ARM_PSCI_FW

 drivers/cpufreq/scmi-cpufreq.c             |    3 +-
 drivers/firmware/arm_scmi/Makefile         |    4 +-
 drivers/firmware/arm_scmi/base.c           |  108 +-
 drivers/firmware/arm_scmi/common.h         |    4 +
 drivers/firmware/arm_scmi/driver.c         |   15 +-
 drivers/firmware/arm_scmi/notify.c         | 1525 ++++++++++++++++++++
 drivers/firmware/arm_scmi/notify.h         |   66 ++
 drivers/firmware/arm_scmi/perf.c           |  151 ++-
 drivers/firmware/arm_scmi/power.c          |   92 +-
 drivers/firmware/arm_scmi/reset.c          |   96 +-
 drivers/firmware/arm_scmi/scmi_pm_domain.c |   12 +-
 drivers/firmware/arm_scmi/sensors.c        |   69 +-
 include/linux/scmi_protocol.h              |  110 +-
 include/trace/events/scmi.h                |    6 +-
 14 files changed, 2217 insertions(+), 44 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/notify.c
 create mode 100644 drivers/firmware/arm_scmi/notify.h

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

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

[Arnd Bergmann]

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

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

[Sudeep Holla]

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

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

[Cristian Marussi]

Hi

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

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

By the way there's also a residual zero-lenght array definition in base report,
I'll remove that too.

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

Thanks

Cristian

> --
> Regards,
> Sudeep

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

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

[Arnd Bergmann]

On Tue, Jul 7, 2020 at 10:33 AM Cristian Marussi
<cristian.marussi@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@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

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

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

[Cristian Marussi]

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


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

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

[Cristian Marussi]

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


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