Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions include/sound/sof/topology.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ struct sof_ipc_comp {
uint32_t pipeline_id;
uint32_t core;

/* reserved for future use */
uint32_t reserved[1];
/* extended data offset, 0 if no extended data */
uint32_t ext_data_offset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to me like you've re-invented the concept of variable array?

this is exactly like we've done in other places e.g. with
uint32_t data_size;
uint8_t data[];

In both cases, this is a final change to the structure, you cannot add anything after the variable array, so it'd be wise indeed to keep some reserved space before the variable array.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint8_t data[] is an incomplete array type.

A variable array is something like int n; uint8_t data[n] instead.

When it's the last member of a structure, an incomplete array type is more specifically called a flexible array member.

Sorry to be pedantic but the right terms are needed when searching documentation and more generally looking for information. For instance information about best initialization practices (see previous sizeof/memset discussion) or that the kernel is now VLA-free
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kills-The-VLA
https://lwn.net/Articles/749064/

Plus memory (mis)management is where most security issues lie so some pedantism can't hurt there :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for being pedantic @marc-hb, my point remains that it's probably better to be explicit about data structures than implicit. There are tools that will detect that a flexible array is the last in a structure, if it's not even declared we'd be writing them off.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to me like you've re-invented the concept of variable array?

this is exactly like we've done in other places e.g. with
uint32_t data_size;
uint8_t data[];

In both cases, this is a final change to the structure, you cannot add anything after the variable array, so it'd be wise indeed to keep some reserved space before the variable array.

No, it is not the flexible array here, it means explicit one uint32_t member. It used to be reserved[2] and is reserved[1] after .core is introduced.

I think the reason that keeping these 'reserved' member is to avoid bumping MAJOR frequently, once they are used up, we have to extend it with a MAJOR bump when any new member being added.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint8_t data[] is an incomplete array type.

A variable array is something like int n; uint8_t data[n] instead.

When it's the last member of a structure, an incomplete array type is more specifically called a flexible array member.

Sorry to be pedantic but the right terms are needed when searching documentation and more generally looking for information. For instance information about best initialization practices (see previous sizeof/memset discussion) or that the kernel is now VLA-free
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kills-The-VLA
https://lwn.net/Articles/749064/

Plus memory (mis)management is where most security issues lie so some pedantism can't hurt there :-)

@marc-hb thanks for the details! I somehow completely missed the "get rid of VLAs" movement (shame). I'm wondering though: those articles use the expression "VLAs within structures" - are they referring to structures declared within functions like

void foo(int x)
{
	struct {
		int a[x];
	} b;

? or do they mean what you call incomplete / flexible arrays?

Copy link
Collaborator

@marc-hb marc-hb Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering though: those articles use the expression "VLAs within structures" - are they referring to structures declared within functions like

Yes, the former (gcc abomination).

There are a number of differences between Variable Length Arrays and Flexible Array Members. I believe the simplest to remember and also most relevant is:

  1. The variable memory of all VLAs is automatically allocated an de-allocated at runtime by code automatically added by the compiler.
  2. You must manage the changing memory of Flexible Array Members all by yourself.

I didn't know about gcc VLAs in struct, I read about them today. While plain VLAs were bad, gcc VLAs in structs raise Linus' language to yet another level:
http://thelinuxjedi.blogspot.com/2014/02/why-vlais-is-bad.html
So gcc VLAs in struct are a much worse form of standard VLAs with no relation to standard Flexible Array Members. The terminology is consistent, let's stick to it.

As opposed to all VLAs, Flexible Array Members (= what Pierre actually suggested) are totally welcome in the kernel and a great replacement for 0-sized arrays (bad) or 1-sized arrays (a catastrophe)
https://www.phoronix.com/scan.php?page=news_item&px=Linux-5.8-Flexible-Array-Member

PS: maybe not so pedantic after all? ;-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that flexible arrays in IPC can be moved by also having an offset member which says where the array is beginning? This way you don't need to add reserved fields before the array itself, but IPC itself must be aware of this (and made slightly more complex due to it) so it can fill in those intermediate fields with zeros if necessary.

} __packed;

/*
Expand Down Expand Up @@ -300,4 +300,9 @@ enum sof_event_types {
SOF_KEYWORD_DETECT_DAPM_EVENT,
};

/* extended data struct for component new */
struct sof_ipc_comp_new_ext {
uint8_t uuid[16];
} __packed;

#endif
1 change: 1 addition & 0 deletions include/uapi/sound/sof/tokens.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
* #define SOF_TKN_COMP_PRELOAD_COUNT 403
*/
#define SOF_TKN_COMP_CORE_ID 404
#define SOF_TKN_COMP_UUID 405

/* SSP */
#define SOF_TKN_INTEL_SSP_CLKS_CONTROL 500
Expand Down
24 changes: 19 additions & 5 deletions sound/soc/sof/sof-audio.c
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ int sof_restore_pipelines(struct device *dev)
struct snd_sof_route *sroute;
struct sof_ipc_pipe_new *pipeline;
struct snd_sof_dai *dai;
struct sof_ipc_comp_dai *comp_dai;
struct sof_ipc_cmd_hdr *hdr;
struct sof_ipc_comp *comp;
size_t ipc_size;
int ret;

/* restore pipeline components */
Expand All @@ -189,12 +190,25 @@ int sof_restore_pipelines(struct device *dev)
switch (swidget->id) {
case snd_soc_dapm_dai_in:
case snd_soc_dapm_dai_out:
ipc_size = sizeof(struct sof_ipc_comp_dai) +
sizeof(struct sof_ipc_comp_new_ext);
comp = kzalloc(ipc_size, GFP_KERNEL);
if (!comp)
return -ENOMEM;

dai = swidget->private;
comp_dai = &dai->comp_dai;
ret = sof_ipc_tx_message(sdev->ipc,
comp_dai->comp.hdr.cmd,
comp_dai, sizeof(*comp_dai),
/* restore the struct sof_ipc_comp_dai */
memcpy(comp, &dai->comp_dai,
sizeof(struct sof_ipc_comp_dai));

/* append extended data to the end of the component */
memcpy((u8 *)comp + sizeof(struct sof_ipc_comp_dai),
&swidget->comp_ext, sizeof(swidget->comp_ext));

ret = sof_ipc_tx_message(sdev->ipc, comp->hdr.cmd,
comp, ipc_size,
&r, sizeof(r));
kfree(comp);
break;
case snd_soc_dapm_scheduler:

Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/sof-audio.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ struct snd_sof_widget {
struct snd_soc_dapm_widget *widget;
struct list_head list; /* list in sdev widget list */

/* extended data for component new */
struct sof_ipc_comp_new_ext comp_ext;

void *private; /* core does not touch this */
};

Expand Down
Loading