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

[PATCH 1/3] 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/3] 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/3] firmware: arm_scmi: Remove fixed size fields from reports/scmi_event_header

[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 types for report fields, mirroring messages structure,
is at odd with this: get rid of them using more generic, equivalent,
typing.

Substitute scmi_event_header fixed size fields with generic types too and
shuffle around fields definitions to minimize implicit padding while
adapting involved functions.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c    |  2 +-
 drivers/firmware/arm_scmi/driver.c  |  4 +--
 drivers/firmware/arm_scmi/notify.c  | 15 +++++----
 drivers/firmware/arm_scmi/notify.h  |  8 +++--
 drivers/firmware/arm_scmi/perf.c    |  2 +-
 drivers/firmware/arm_scmi/power.c   |  2 +-
 drivers/firmware/arm_scmi/reset.c   |  2 +-
 drivers/firmware/arm_scmi/sensors.c |  2 +-
 include/linux/scmi_protocol.h       | 52 ++++++++++++++---------------
 9 files changed, 46 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 54f378e946f1..9853bd3c4d45 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -273,7 +273,7 @@ static int scmi_base_set_notify_enabled(const struct scmi_handle *handle,
 }
 
 static void *scmi_base_fill_custom_report(const struct scmi_handle *handle,
-					  u8 evt_id, u64 timestamp,
+					  u8 evt_id, ktime_t timestamp,
 					  const void *payld, size_t payld_sz,
 					  void *report, u32 *src_id)
 {
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 19a4287fc0f7..03ec74242c14 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -205,13 +205,13 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
 
 static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
 {
-	u64 ts;
 	struct scmi_xfer *xfer;
 	struct device *dev = cinfo->dev;
 	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
 	struct scmi_xfers_info *minfo = &info->rx_minfo;
+	ktime_t ts;
 
-	ts = ktime_get_boottime_ns();
+	ts = ktime_get_boottime();
 	xfer = scmi_xfer_get(cinfo->handle, minfo);
 	if (IS_ERR(xfer)) {
 		dev_err(dev, "failed to get free message slot (%ld)\n",
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index 752415367305..4731daaacd19 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -80,6 +80,7 @@
 #include <linux/err.h>
 #include <linux/hashtable.h>
 #include <linux/kernel.h>
+#include <linux/ktime.h>
 #include <linux/kfifo.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -246,18 +247,18 @@ struct events_queue {
  * struct scmi_event_header  - A utility header
  * @timestamp: The timestamp, in nanoseconds (boottime), which was associated
  *	       to this event as soon as it entered the SCMI RX ISR
- * @evt_id: Event ID (corresponds to the Event MsgID for this Protocol)
  * @payld_sz: Effective size of the embedded message payload which follows
+ * @evt_id: Event ID (corresponds to the Event MsgID for this Protocol)
  * @payld: A reference to the embedded event payload
  *
  * This header is prepended to each received event message payload before
  * queueing it on the related &struct events_queue.
  */
 struct scmi_event_header {
-	u64	timestamp;
-	u8	evt_id;
-	size_t	payld_sz;
-	u8	payld[];
+	ktime_t timestamp;
+	size_t payld_sz;
+	unsigned char evt_id;
+	unsigned char payld[];
 };
 
 struct scmi_registered_event;
@@ -572,7 +573,7 @@ static void scmi_events_dispatcher(struct work_struct *work)
  * Return: 0 on Success
  */
 int scmi_notify(const struct scmi_handle *handle, u8 proto_id, u8 evt_id,
-		const void *buf, size_t len, u64 ts)
+		const void *buf, size_t len, ktime_t ts)
 {
 	struct scmi_registered_event *r_evt;
 	struct scmi_event_header eh;
@@ -595,7 +596,7 @@ int scmi_notify(const struct scmi_handle *handle, u8 proto_id, u8 evt_id,
 	if (kfifo_avail(&r_evt->proto->equeue.kfifo) < sizeof(eh) + len) {
 		dev_warn(handle->dev,
 			 "queue full, dropping proto_id:%d  evt_id:%d  ts:%lld\n",
-			 proto_id, evt_id, ts);
+			 proto_id, evt_id, ktime_to_ns(ts));
 		return -ENOMEM;
 	}
 
diff --git a/drivers/firmware/arm_scmi/notify.h b/drivers/firmware/arm_scmi/notify.h
index 3791bb7aa79b..3485f20fa70e 100644
--- a/drivers/firmware/arm_scmi/notify.h
+++ b/drivers/firmware/arm_scmi/notify.h
@@ -10,6 +10,7 @@
 #define _SCMI_NOTIFY_H
 
 #include <linux/device.h>
+#include <linux/ktime.h>
 #include <linux/types.h>
 
 #define SCMI_PROTO_QUEUE_SZ	4096
@@ -48,8 +49,9 @@ struct scmi_event_ops {
 	int (*set_notify_enabled)(const struct scmi_handle *handle,
 				  u8 evt_id, u32 src_id, bool enabled);
 	void *(*fill_custom_report)(const struct scmi_handle *handle,
-				    u8 evt_id, u64 timestamp, const void *payld,
-				    size_t payld_sz, void *report, u32 *src_id);
+				    u8 evt_id, ktime_t timestamp,
+				    const void *payld, size_t payld_sz,
+				    void *report, u32 *src_id);
 };
 
 int scmi_notification_init(struct scmi_handle *handle);
@@ -61,6 +63,6 @@ int scmi_register_protocol_events(const struct scmi_handle *handle,
 				  const struct scmi_event *evt, int num_events,
 				  int num_sources);
 int scmi_notify(const struct scmi_handle *handle, u8 proto_id, u8 evt_id,
-		const void *buf, size_t len, u64 ts);
+		const void *buf, size_t len, ktime_t ts);
 
 #endif /* _SCMI_NOTIFY_H */
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 8bcad96e06ca..3e1e87012c95 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -780,7 +780,7 @@ static int scmi_perf_set_notify_enabled(const struct scmi_handle *handle,
 }
 
 static void *scmi_perf_fill_custom_report(const struct scmi_handle *handle,
-					  u8 evt_id, u64 timestamp,
+					  u8 evt_id, ktime_t timestamp,
 					  const void *payld, size_t payld_sz,
 					  void *report, u32 *src_id)
 {
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 4f6757980739..46f213644c49 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -227,7 +227,7 @@ static int scmi_power_set_notify_enabled(const struct scmi_handle *handle,
 }
 
 static void *scmi_power_fill_custom_report(const struct scmi_handle *handle,
-					   u8 evt_id, u64 timestamp,
+					   u8 evt_id, ktime_t timestamp,
 					   const void *payld, size_t payld_sz,
 					   void *report, u32 *src_id)
 {
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index fb7cb517900b..3691bafca057 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -240,7 +240,7 @@ static int scmi_reset_set_notify_enabled(const struct scmi_handle *handle,
 }
 
 static void *scmi_reset_fill_custom_report(const struct scmi_handle *handle,
-					   u8 evt_id, u64 timestamp,
+					   u8 evt_id, ktime_t timestamp,
 					   const void *payld, size_t payld_sz,
 					   void *report, u32 *src_id)
 {
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 2120ac4787c9..1af0ad362e82 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -296,7 +296,7 @@ static int scmi_sensor_set_notify_enabled(const struct scmi_handle *handle,
 }
 
 static void *scmi_sensor_fill_custom_report(const struct scmi_handle *handle,
-					    u8 evt_id, u64 timestamp,
+					    u8 evt_id, ktime_t timestamp,
 					    const void *payld, size_t payld_sz,
 					    void *report, u32 *src_id)
 {
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 7d4348fb7330..7e5dd7d1e221 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;
+	ktime_t		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;
+	ktime_t		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;
+	ktime_t		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;
+	ktime_t		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;
+	ktime_t		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[];
+	ktime_t			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 2/3] firmware: arm_scmi: Remove unneeded __packed attribute

[Steven Price]

On 10/07/2020 14:39, Cristian Marussi wrote:
> Remove __packed attribute from struct scmi_event_header.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

A drive-by review. But this doesn't look safe to me. sizeof(struct 
scmi_event_header) is used in several places and this change will modify 
that from 13 to 16, but leave the structure members at the same offset 
(as the members are naturally aligned). In particular the naïve header 
size is now bigger than the offset to payld.

What is the justification for __packed being 'unneeded'? Perhaps there's 
some reason in the code why this doesn't matter, but if so I feel this 
should be included in the commit message.

Steve

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


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

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

[Steven Price]

On 10/07/2020 14:39, Cristian Marussi wrote:
> Remove __packed attribute from struct scmi_event_header.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

A drive-by review. But this doesn't look safe to me. sizeof(struct 
scmi_event_header) is used in several places and this change will modify 
that from 13 to 16, but leave the structure members at the same offset 
(as the members are naturally aligned). In particular the naïve header 
size is now bigger than the offset to payld.

What is the justification for __packed being 'unneeded'?

Steve

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


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

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

[Sudeep Holla]

On Fri, 10 Jul 2020 14:39:17 +0100, Cristian Marussi wrote:
> Substitute zero-length array defined in scmi_base_error_report with
> a flexible length array definition.


Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/3] firmware: arm_scmi: Remove zero-length array in SCMI notifications
      https://git.kernel.org/sudeep.holla/c/02c003cc18
[2/3] firmware: arm_scmi: Remove unneeded __packed attribute
      https://git.kernel.org/sudeep.holla/c/33ee97f823
[3/3] (korg_sudeep/for-next/scmi) firmware: arm_scmi: Remove fixed size fields from reports/scmi_event_header
      https://git.kernel.org/sudeep.holla/c/72a5eb9d9c

--
Regards,
Sudeep


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

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

[Cristian Marussi]

Hi Steven

thanks for the review.

On Mon, Jul 13, 2020 at 12:20:43PM +0100, Steven Price wrote:
> On 10/07/2020 14:39, Cristian Marussi wrote:
> >Remove __packed attribute from struct scmi_event_header.
> >
> >Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> A drive-by review. But this doesn't look safe to me. sizeof(struct
> scmi_event_header) is used in several places and this change will modify
> that from 13 to 16, but leave the structure members at the same offset (as
> the members are naturally aligned). In particular the naïve header size is
> now bigger than the offset to payld.
> 
> What is the justification for __packed being 'unneeded'?
> 

Arnd pointed out at first that this structure in the original series had a mix of
fixed and non-fixed types in its fields and that the __packed rendered some fields
misaglined.

Removing that as it is, in fact left also some unexplained implicit padding which is
at odd for a struct containing fixed-sized types.

In a following fix in the series I have indeed moved this struct's fields  and others
to generic non fixed type fields and shuffled around the fields to avoid misalignment
and implicit internal padding (except for the trailing padding due to the variable
size array)

It was probably better to squash also this patch in that following patch.

This structure is used internally to push variable-sized (through the means of the payld[])
events descriptors through a fifo from the ISR to the deferred workqueus, so that's whhy I
originally thought to avoid to carry around unneeded padding into the fifos and use the
__packed.

On the correctness side, as you pointed out, the header with padding is now 16 so when
I push thorugh the kfifos this header and the payload there's a hole in the data as
represented in the fifo buffer as such

@end_of hdr+payld kfifo writes:
  kifo_in(fifo, h, sizeof(*h)) + kfifo_in(fifo, payld, h->payld_sz)

0       14   16
+-------+----+------------
|header - pad| payload...
-------------------------
        ^
	|
	.payld

(Note that header and payload comes from two distinct place so I have push it with two kfifo_in()
in order to avoid a redundant memcpy on an intermediate buffer to collate them...thing
that was pointed out as undesirable in a review)

and when I read it back from the fifo such hole is just transparently overwritten:

@header read:
 kfifo_out(fifo, h, sizeof(*h))

0       14   16
+-------+----+--------------
|header - pad| 
----------------------------

@payload_read:
 kfifo_out(fifo, h->payld, h->payld_sz)

0       14   
+-------+----+--------------
|header | payload....
----------------------------
        ^
	|
	.payld

So since anyway the drawback of packing is that the misaglined access potentially slows down the
reads, I was not sure anymore it was worth to pack and misalign, and, given that it seemed not
to be liked so much, I dropped it and moved to generic non-fixed types without packing.

A better (and shorter) explanation of all of the above is possibly needed (but I'd still prefer
the fixed sized typing and __packed 'holeless' approach...)

Thanks

Cristian


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

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

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

[Steven Price]

On 13/07/2020 14:07, Cristian Marussi wrote:
> Hi Steven
> 
> thanks for the review.
> 
> On Mon, Jul 13, 2020 at 12:20:43PM +0100, Steven Price wrote:
>> On 10/07/2020 14:39, Cristian Marussi wrote:
>>> Remove __packed attribute from struct scmi_event_header.
>>>
>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>
>> A drive-by review. But this doesn't look safe to me. sizeof(struct
>> scmi_event_header) is used in several places and this change will modify
>> that from 13 to 16, but leave the structure members at the same offset (as
>> the members are naturally aligned). In particular the na�ve header size is
>> now bigger than the offset to payld.
>>
>> What is the justification for __packed being 'unneeded'?
>>
> 
> Arnd pointed out at first that this structure in the original series had a mix of
> fixed and non-fixed types in its fields and that the __packed rendered some fields
> misaglined.
> 
> Removing that as it is, in fact left also some unexplained implicit padding which is
> at odd for a struct containing fixed-sized types.
> 
> In a following fix in the series I have indeed moved this struct's fields  and others
> to generic non fixed type fields and shuffled around the fields to avoid misalignment
> and implicit internal padding (except for the trailing padding due to the variable
> size array)
> 
> It was probably better to squash also this patch in that following patch.
> 
> This structure is used internally to push variable-sized (through the means of the payld[])
> events descriptors through a fifo from the ISR to the deferred workqueus, so that's whhy I
> originally thought to avoid to carry around unneeded padding into the fifos and use the
> __packed.
> 
> On the correctness side, as you pointed out, the header with padding is now 16 so when
> I push thorugh the kfifos this header and the payload there's a hole in the data as
> represented in the fifo buffer as such
> 
> @end_of hdr+payld kfifo writes:
>    kifo_in(fifo, h, sizeof(*h)) + kfifo_in(fifo, payld, h->payld_sz)
> 
> 0       14   16
> +-------+----+------------
> |header - pad| payload...
> -------------------------
>          ^
> 	|
> 	.payld
> 
> (Note that header and payload comes from two distinct place so I have push it with two kfifo_in()
> in order to avoid a redundant memcpy on an intermediate buffer to collate them...thing
> that was pointed out as undesirable in a review)
> 
> and when I read it back from the fifo such hole is just transparently overwritten:
> 
> @header read:
>   kfifo_out(fifo, h, sizeof(*h))
> 
> 0       14   16
> +-------+----+--------------
> |header - pad|
> ----------------------------
> 
> @payload_read:
>   kfifo_out(fifo, h->payld, h->payld_sz)
> 
> 0       14
> +-------+----+--------------
> |header | payload....
> ----------------------------
>          ^
> 	|
> 	.payld
> 
> So since anyway the drawback of packing is that the misaglined access potentially slows down the
> reads, I was not sure anymore it was worth to pack and misalign, and, given that it seemed not
> to be liked so much, I dropped it and moved to generic non-fixed types without packing.

Thanks for the explanation - it sounds like the change is correct.

However, from the description above it sounds like splitting the header 
and payload into separate types would be clearer. I'm not sure the 
flexible length array is adding to code clarity here. In particular the 
'pad' being put into the fifo is actually going to be a (truncated) copy 
of the payload.

There is also a trick with an unnamed internal struct which gets the 
padding in the correct place...

	struct scmi_event_header {
		struct {
			u64	timestamp;
			u8	evt_id;
			size_t	payld_sz;
		}
		u8	payld[];
	};

...with that then...

	offsetof(struct scmi_event_header, payld) ==
		sizeof(struct scmi_event_header)

...which avoids the need for kfifo_out having to overwrite the padding.

> A better (and shorter) explanation of all of the above is possibly needed (but I'd still prefer
> the fixed sized typing and __packed 'holeless' approach...)

Fixed sized types and __packed is easier to reason about, but obviously 
naturally aligned types do tend to be faster.

Steve

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