[PATCH 1/4] firmware: arm_scmi: Remove zero-length array in SCMI Notifications

[PATCH 1/4] firmware: arm_scmi: Remove zero-length array in SCMI Notifications

[Cristian Marussi]

Substitute zero-length array defined in scmi_base_error_report with
a flexible length array definition.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/scmi_protocol.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 46d98be92466..7d4348fb7330 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -421,7 +421,7 @@ struct scmi_base_error_report {
 	u32 agent_id;
 	bool fatal;
 	u16 cmd_count;
-	u64 reports[0];
+	u64 reports[];
 };
 
 #endif /* _LINUX_SCMI_PROTOCOL_H */
-- 
2.17.1

[PATCH 1/4] firmware: arm_scmi: Remove zero-length array in SCMI Notifications

[Cristian Marussi]

Substitute zero-length array defined in scmi_base_error_report with
a flexible length array definition.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/scmi_protocol.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 46d98be92466..7d4348fb7330 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -421,7 +421,7 @@ struct scmi_base_error_report {
 	u32 agent_id;
 	bool fatal;
 	u16 cmd_count;
-	u64 reports[0];
+	u64 reports[];
 };
 
 #endif /* _LINUX_SCMI_PROTOCOL_H */
-- 
2.17.1


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

[PATCH 2/4] firmware: arm_scmi: Remove unneeded __packed attribute

[Cristian Marussi]

Remove __packed attribute from struct scmi_event_header.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/notify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index c4d006cfde88..752415367305 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -258,7 +258,7 @@ struct scmi_event_header {
 	u8	evt_id;
 	size_t	payld_sz;
 	u8	payld[];
-} __packed;
+};
 
 struct scmi_registered_event;
 
-- 
2.17.1

[PATCH 2/4] firmware: arm_scmi: Remove unneeded __packed attribute

[Cristian Marussi]

Remove __packed attribute from struct scmi_event_header.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/notify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index c4d006cfde88..752415367305 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -258,7 +258,7 @@ struct scmi_event_header {
 	u8	evt_id;
 	size_t	payld_sz;
 	u8	payld[];
-} __packed;
+};
 
 struct scmi_registered_event;
 
-- 
2.17.1


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

[PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Cristian Marussi]

Drop size_t in favour of fixed size u32 for consistency and shuffle
around fields definitions to minimize implicit padding.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/notify.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index 752415367305..f441da28d91f 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -254,10 +254,10 @@ struct events_queue {
  * queueing it on the related &struct events_queue.
  */
 struct scmi_event_header {
-	u64	timestamp;
-	u8	evt_id;
-	size_t	payld_sz;
-	u8	payld[];
+	u64 timestamp;
+	u32 payld_sz;
+	u8 evt_id;
+	u8 payld[];
 };
 
 struct scmi_registered_event;
-- 
2.17.1

[PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Cristian Marussi]

Drop size_t in favour of fixed size u32 for consistency and shuffle
around fields definitions to minimize implicit padding.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/notify.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index 752415367305..f441da28d91f 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -254,10 +254,10 @@ struct events_queue {
  * queueing it on the related &struct events_queue.
  */
 struct scmi_event_header {
-	u64	timestamp;
-	u8	evt_id;
-	size_t	payld_sz;
-	u8	payld[];
+	u64 timestamp;
+	u32 payld_sz;
+	u8 evt_id;
+	u8 payld[];
 };
 
 struct scmi_registered_event;
-- 
2.17.1


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

Re: [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Arnd Bergmann]

On Wed, Jul 8, 2020 at 2:24 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Drop size_t in favour of fixed size u32 for consistency and shuffle
> around fields definitions to minimize implicit padding.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

As you still have implicit padding at the end, I'd either make
that explicit now, or leave the __packed attribute.

The payld_sz is not actually force to be misaligned with the
reordered layout, which is what's most important.

     Arnd

Re: [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Arnd Bergmann]

On Wed, Jul 8, 2020 at 2:24 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Drop size_t in favour of fixed size u32 for consistency and shuffle
> around fields definitions to minimize implicit padding.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

As you still have implicit padding at the end, I'd either make
that explicit now, or leave the __packed attribute.

The payld_sz is not actually force to be misaligned with the
reordered layout, which is what's most important.

     Arnd

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

Re: [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Cristian Marussi]

On Wed, Jul 08, 2020 at 10:38:08PM +0200, Arnd Bergmann wrote:
> On Wed, Jul 8, 2020 at 2:24 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > Drop size_t in favour of fixed size u32 for consistency and shuffle
> > around fields definitions to minimize implicit padding.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> As you still have implicit padding at the end, I'd either make
> that explicit now, or leave the __packed attribute.

Do you mean expliciting that with a comment, right ? being the last member 'payld'
a flexible array must be the last in order to even compile.

I'm a bit confused anyway on how the trailing padding works on a struct like
this which ends with a flexible array definition, so I was expecting that the
trailing pads would have made no difference, given that it's used to basically
give some know layout to a blob of data via casting...

Thanks

Cristian

> 
> The payld_sz is not actually force to be misaligned with the
> reordered layout, which is what's most important.
> 
>      Arnd

Re: [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing

[Cristian Marussi]

On Wed, Jul 08, 2020 at 10:38:08PM +0200, Arnd Bergmann wrote:
> On Wed, Jul 8, 2020 at 2:24 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > Drop size_t in favour of fixed size u32 for consistency and shuffle
> > around fields definitions to minimize implicit padding.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> As you still have implicit padding at the end, I'd either make
> that explicit now, or leave the __packed attribute.

Do you mean expliciting that with a comment, right ? being the last member 'payld'
a flexible array must be the last in order to even compile.

I'm a bit confused anyway on how the trailing padding works on a struct like
this which ends with a flexible array definition, so I was expecting that the
trailing pads would have made no difference, given that it's used to basically
give some know layout to a blob of data via casting...

Thanks

Cristian

> 
> The payld_sz is not actually force to be misaligned with the
> reordered layout, which is what's most important.
> 
>      Arnd

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

[PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports

[Cristian Marussi]

Event reports are used to convey information describing events to the
registered user-callbacks: they are necessarily derived from the underlying
raw SCMI events' messages but they are not meant to expose or directly
mirror any of those messages data layout, which belong to the protocol
layer.

Using fixed size fields mirroring messages structure is at odd with this:
get rid of them using more generic, equivalent, typing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/scmi_protocol.h | 52 +++++++++++++++++------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 7d4348fb7330..e51f392f464b 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -381,47 +381,47 @@ enum scmi_notification_events {
 };
 
 struct scmi_power_state_changed_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 power_state;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		power_state;
 };
 
 struct scmi_perf_limits_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 range_max;
-	u32 range_min;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		range_max;
+	unsigned int		range_min;
 };
 
 struct scmi_perf_level_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 performance_level;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		performance_level;
 };
 
 struct scmi_sensor_trip_point_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 sensor_id;
-	u32 trip_point_desc;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		sensor_id;
+	unsigned int		trip_point_desc;
 };
 
 struct scmi_reset_issued_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 reset_state;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		reset_state;
 };
 
 struct scmi_base_error_report {
-	u64 timestamp;
-	u32 agent_id;
-	bool fatal;
-	u16 cmd_count;
-	u64 reports[];
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	bool			fatal;
+	unsigned int		cmd_count;
+	unsigned long long	reports[];
 };
 
 #endif /* _LINUX_SCMI_PROTOCOL_H */
-- 
2.17.1

[PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports

[Cristian Marussi]

Event reports are used to convey information describing events to the
registered user-callbacks: they are necessarily derived from the underlying
raw SCMI events' messages but they are not meant to expose or directly
mirror any of those messages data layout, which belong to the protocol
layer.

Using fixed size fields mirroring messages structure is at odd with this:
get rid of them using more generic, equivalent, typing.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/linux/scmi_protocol.h | 52 +++++++++++++++++------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 7d4348fb7330..e51f392f464b 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -381,47 +381,47 @@ enum scmi_notification_events {
 };
 
 struct scmi_power_state_changed_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 power_state;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		power_state;
 };
 
 struct scmi_perf_limits_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 range_max;
-	u32 range_min;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		range_max;
+	unsigned int		range_min;
 };
 
 struct scmi_perf_level_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 performance_level;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		performance_level;
 };
 
 struct scmi_sensor_trip_point_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 sensor_id;
-	u32 trip_point_desc;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		sensor_id;
+	unsigned int		trip_point_desc;
 };
 
 struct scmi_reset_issued_report {
-	u64 timestamp;
-	u32 agent_id;
-	u32 domain_id;
-	u32 reset_state;
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	unsigned int		domain_id;
+	unsigned int		reset_state;
 };
 
 struct scmi_base_error_report {
-	u64 timestamp;
-	u32 agent_id;
-	bool fatal;
-	u16 cmd_count;
-	u64 reports[];
+	unsigned long long	timestamp;
+	unsigned int		agent_id;
+	bool			fatal;
+	unsigned int		cmd_count;
+	unsigned long long	reports[];
 };
 
 #endif /* _LINUX_SCMI_PROTOCOL_H */
-- 
2.17.1


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

Re: [PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports

[Arnd Bergmann]

On Wed, Jul 8, 2020 at 2:25 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Event reports are used to convey information describing events to the
> registered user-callbacks: they are necessarily derived from the underlying
> raw SCMI events' messages but they are not meant to expose or directly
> mirror any of those messages data layout, which belong to the protocol
> layer.
>
> Using fixed size fields mirroring messages structure is at odd with this:
> get rid of them using more generic, equivalent, typing.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Ok, I was expecting you would go with fixed-size structures without
implied padding, but either way makes it more consistent, so this
version is good, too.

       Arnd

Re: [PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports

[Arnd Bergmann]

On Wed, Jul 8, 2020 at 2:25 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Event reports are used to convey information describing events to the
> registered user-callbacks: they are necessarily derived from the underlying
> raw SCMI events' messages but they are not meant to expose or directly
> mirror any of those messages data layout, which belong to the protocol
> layer.
>
> Using fixed size fields mirroring messages structure is at odd with this:
> get rid of them using more generic, equivalent, typing.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Ok, I was expecting you would go with fixed-size structures without
implied padding, but either way makes it more consistent, so this
version is good, too.

       Arnd

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