| Message ID | 20200710133919.39792-2-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 33ee97f823cc5b3d03c9910c1b8dbe193a21056b |
| Headers | show |
| Series | [1/3] firmware: arm_scmi: Remove zero-length array in SCMI Notifications | expand |
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; > >
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; > >
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; > > >
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
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;
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(-)