dmic: fix dmic and volume configuration ipc

[dabekjakub]

Fixed ipc data size not beeing corectly passed to dmic and volume.

Signed-off-by: Jakub Dabek jakub.dabek@linux.intel.com

[dabekjakub]

fix for : #717 and #718

[lgirdwood]

@jajanusz has suggested a better fix as this one can crash on ABI updates. The DMIC has a flex array that is being missed in current code.

[lgirdwood]

@dabekjakub since you are still online the fix is to pass _ipc->comp_data to dai_config() directly and to other calls that take variable length flex arrays.

[dabekjakub]

Yes we found it just after pr was created. I am in the process of submitting new one.

[ranj063]

@lgirdwood yes that is indeed a good catch. DMIC does has a variable length array and it needs to be handled carefully especially now that 4ch capture is the requirement.

[lgirdwood]

@dabekjakub great, I think it's only dai_config() and comp_cmd() that use the var flex arrays.
@ranj063 wasn't me, @jajanusz found it :)

[ranj063]

@dabekjakub it seems like we dont really need the data variable at here right? can we just remove it?

[dabekjakub]

it is used later down the line in
component.h:286 static inline int comp_cmd(struct comp_dev *dev, int cmd, void *data)

[mmaka1]

I guess it must be removed in all the places to avoid stop-on-warning when using another compiler.

[mmaka1]

Right, it is used indeed but the original request content stays in the data buffer on stack. If you need a quick fix to the existing version that uses copy, perhaps IPC_COPY_CMD should be called after return from the handler to make sure potential error codes are propagated if there are any.

[mmaka1]

There might be another issue, if there are var-size data indicated by the (_ipc->comp_data).rhdr.hdr.size > fixed sizeof(data), there might be OOB here:

/* write component values to the outbox */mailbox_hostbox_write(0, &data, data.rhdr.hdr.size);

[dabekjakub]

May be relevant to other functions as well. Need to analyze it on Monday.

[lgirdwood]

Agreed, I think we need

struct sof_ipc_ctrl_data *out_data = NULL;
/* get component values */
ret = ipc_comp_cmd(comp_dev->cd, cmd, _ipc->comp_data, &out_data);
if (ret < 0) {
trace_ipc_error("ipc: comp %d cmd %u failed %d", data.comp_id,
data.cmd, ret);
return ret;
}
/* write component values to the outbox */
mailbox_hostbox_write(0, out_data, out_data.rhdr.hdr.size);
rfree(out_data);

Where out_data is allocated in the component but freed here.

[dabekjakub]

Moved

IPC_COPY_CMD(data, _ipc->comp_data);

to position after data is handled.

[ranj063]

Moved

IPC_COPY_CMD(data, _ipc->comp_data);

to position after data is handled.

@dabekjakub sorry I think this would still cause problems. The best thing to do is remove data and just usr _ipc->comp_data everywhere. But we need to find a more graceful fix that changing every single function in handler like we talked about today.

[jajanusz]

The best thing to do is remove data and just usr _ipc->comp_data everywhere.
@ranj063 you're right, it's best what we can do atm for release

@lgirdwood
I don't like current IPC_COPY_CMD implementation. Idea is good overall but it will fail or will need to be super cautious with:

1. Unions
2. Flex arrays
3. IPC cmds specifications (like in *comp_new)

Even if it somehow works now it will cause problems in the future.

3rd issue is literally like casting always to top level type in OO languages - because we use type of variable that is passed by value and the fact that it may be kind of 'subclass' doesn't matter for macro.

We need better way of 'masking', because that's what we do now we kinda mask what we get from driver to what FW may understand, problem is that our mask is only general type (or lacks flexarrays/is extended by unions).

[ranj063]

The best thing to do is remove data and just usr _ipc->comp_data everywhere.
@ranj063 you're right, it's best what we can do atm for release

@lgirdwood
I don't like current IPC_COPY_CMD implementation. Idea is good overall but it will fail or will need to be super cautious with:

1. Unions
2. Flex arrays
3. IPC cmds specifications (like in *comp_new)

Even if it somehow works now it will cause problems in the future.

3rd issue is literally like casting always to top level type in OO languages - because we use type of variable that is passed by value and the fact that it may be kind of 'subclass' doesn't matter for macro.

We need better way of 'masking', because that's what we do now we kinda mask what we get from driver to what FW may understand, problem is that our mask is only general type (or lacks flexarrays/is extended by unions).

@jajanusz agree with your opinion. We do need to fix IPC_CMD_COPY.

[jajanusz]

@lgirdwood@ranj063@dabekjakub

We can limit problem with api compatibility to 2 cases - when drvier is ahead of FW or otherwise.

When driver is ahead - just cut to size of what FW can process.
When fw is ahead - add zeros on fields where ipc from driver is lacking.

That's what IPC_COPY_CMD does now. Problem is that we don't know right size (current macro uses sizeof what doesn't work in some cases). Size may change in 3 ways that I mentioned above, what can be determined by 2 things:

1. Size is designed to by dynamic by using arrays/unions, you can dynamically calculate it by using some type field of ipc for unions and size for arrays.
2. Internal handler that expects specification of some ipc (and higher level knows only generalization).

I think we need something more than 1 simple macro to handle this.
My first idea would be some callbacks array for each cmd type that calculates what size it should be (or sizeof for no handler). However I'd prefer to have something easier to manage/maintain.
Maybe we can have some smart policy for IPCs, idk.
I also don't know how much of IPC handling we want to rework, I bet we can come up with greater things if we could rework more of it. So far we were just patching it. Maybe it's good time to use experience we got and make it right from the ground up.
Definitely we have something to think about.

[ranj063]

@lgirdwood@ranj063@dabekjakub

We can limit problem with api compatibility to 2 cases - when drvier is ahead of FW or otherwise.

When driver is ahead - just cut to size of what FW can process.
When fw is ahead - add zeros on fields where ipc from driver is lacking.

That's what IPC_COPY_CMD does now. Problem is that we don't know right size (current macro uses sizeof what doesn't work in some cases). Size may change in 3 ways that I mentioned above, what can be determined by 2 things:
@jajanusz when the driver sends the ipc, the ipc header member size always contains the right size which includes the header and payload size.

1. Size is designed to by dynamic by using arrays/unions, you can dynamically calculate it by using some type field of ipc for unions and size for arrays.
2. Internal handler that expects specification of some ipc (and higher level knows only generalization).

I think we need something more than 1 simple macro to handle this.
My first idea would be some callbacks array for each cmd type that calculates what size it should be (or sizeof for no handler). However I'd prefer to have something easier to manage/maintain.
Maybe we can have some smart policy for IPCs, idk.
I also don't know how much of IPC handling we want to rework, I bet we can come up with greater things if we could rework more of it. So far we were just patching it. Maybe it's good time to use experience we got and make it right from the ground up.
Definitely we have something to think about.

[jajanusz]

@ranj063
Yea I know. What I meant is that FW doesn't know right size, because it tries to cut ipc down to size that is computed on compilation time, but it's dynamic, what causes problem.

[jajanusz]

@lgirdwood
At the moment IPC_COPY_CMD is good only for local use (the function that is doing copy uses this value because it knows to what type it needs to cut) - we shouldn't cut ipc and propagate them, so for now it would be better to use original _ipc->comp_data in calls instead of copied value - do you agree?

@dabekjakub
Can you do that?

[dabekjakub]

@jajanusz@ranj063@lgirdwood

@dabekjakub sorry I think this would still cause problems. The best thing to do is remove data and just usr _ipc->comp_data everywhere. But we need to find a more graceful fix that changing every single function in handler like we talked about today.

Change made as per @mmaka1 request to ensure that the two way ipc works correctly (example: host asking for volume value). We could change the mailbox to

	mailbox_hostbox_write(0, _ipc->comp_data, ((struct sof_ipc_ctrl_data *)_ipc->comp_data)->rhdr.hdr.size);

and delete data entirely but @mmaka1 had an argument for keeping it. @mmaka1 could You elaborate on this. I don't want to spread misleading info here since conversation took place off github.

@jajanusz@ranj063
We know ipc size since host sends it every time in ipc_cmd_header, so we do not have to relay on anything else if host calculates what it sends. We need to allocate enough memory for structs in handler.c for that matter. Just as i sugested:

	struct sof_ipc_ctrl_data *data = rmalock(RZONE_RUNTIME,SOF_MEM_CAPS_RAM, ((struct sof_ipc_cmd_hdr *)_ipc->comp_data)->size);

@mmaka1 suggested a buffer specifically for ipc handling.

In the end we need to decide if we even want this temporary fix on master or do we scrap it and move on directly to end result.

[plbossart]

On 12/14/18 3:57 PM, Jakub Dabek wrote: @jajanusz <https://github.com/jajanusz> @ranj063 <https://github.com/ranj063> @lgirdwood <https://github.com/lgirdwood> @dabekjakub <https://github.com/dabekjakub> sorry I think this would still cause problems. The best thing to do is remove data and just usr _ipc->comp_data everywhere. But we need to find a more graceful fix that changing every single function in handler like we talked about today. Change made as per @mmaka1 <https://github.com/mmaka1> request to ensure that the two way ipc works correctly (example: host asking for volume value). We could change the mailbox to |mailbox_hostbox_write(0, _ipc->comp_data, ((struct sof_ipc_ctrl_data *)_ipc->comp_data)->rhdr.hdr.size); | and delete data entirely but @mmaka1 <https://github.com/mmaka1> had an argument for keeping it. @mmaka1 <https://github.com/mmaka1> could You elaborate on this. I don't want to spread misleading info here since conversation took place off github. @jajanusz <https://github.com/jajanusz> @ranj063 <https://github.com/ranj063> We know ipc size since host sends it every time in ipc_cmd_header, so we do not have to relay on anything else if host calculates what it sends. We need to allocate enough memory for structs in handler.c for that matter. Just as i sugested: |struct sof_ipc_ctrl_data *data = rmalock(RZONE_RUNTIME,SOF_MEM_CAPS_RAM, ((struct sof_ipc_cmd_hdr *)_ipc->comp_data)->size); | @mmaka1 <https://github.com/mmaka1> suggested a buffer specifically for ipc handling. In the end we need to decide if we even want this temporary fix on master or do we scrap it and move on directly to end result.

You guys have exactly 4 days to agree and close this. the ABI3 will be frozen when I send the next batch of Linux patches upstream. I've waited and waited for ABI updates since I came back, sorry folks, nothing personal, but time is up. Tick Tock.

[michalgrodzicki]

@plbossart I see that you don't want to help, but please don't disturb people.

[plbossart]

On 12/14/18 4:36 PM, michalgrodzicki wrote: @plbossart <https://github.com/plbossart> I see that you don't want to help, but please don't disturb people.

I've been around for longer than you did, and I want to avoid having to maintain code like this: https://github.com/thesofproject/linux/blob/b8db053ebce3636f5d3eca2c25b544cd15ee14ef/sound/soc/intel/skylake/skl-topology.c#L2912 You may not like what I state but without progress we will have to live with the existing solution as legacy. This happened for Skylake/cAVS and is about to happen again. You may chose to ignore the need to close on the ABI, but it'll hit your planning next year when we have to deal with backwards compatibility. Over and out.

[ranj063]

@plbossart I think the discussion on this thread is not proposing any changes to the ABI. So we should be good for ABI3.

It is purely a discussion on how to fix the problem being addressed for dmic and volume today more widely.

[lgirdwood]

@dabekjakub@jajanusz The IPC_COPY_CMD macro is only meant for introspection at the handler level (and to guarantee that introspection will work regardless if new fields are added). The _ipc->comp_data should be passed by handler into client APIs (since clients will deal with the unions/flex arrays). Same goes for opposite data copy direction too from DSP to host.

@dabekjakub I'll be back on later tonight, and will do a PR if I don't see an update from you today.

[lgirdwood]

@dabekjakub I have something cooking now. Will post in a few hours.

[jajanusz]

@dabekjakub@jajanusz The IPC_COPY_CMD macro is only meant for introspection at the handler level (and to guarantee that introspection will work regardless if new fields are added). The _ipc->comp_data should be passed by handler into client APIs (since clients will deal with the unions/flex arrays). Same goes for opposite data copy direction too from DSP to host.

Ok, it makes sense. Just intention wasn't clear, cos of misuse of that macro looked like it was meant to be used everywhere, not just at the handler level.

[dabekjakub]

I am in a position where @mmaka1 asked for one change and @jajanusz@ranj063 for other. In my opinion we should merge this as is at the moment or work on good solution right away. @lgirdwood if You need any help or action on my side i have a setup at home i can get to work with in 3 hours.

[ranj063]

I am in a position where @mmaka1 asked for one change and @jajanusz@ranj063 for other. In my opinion we should merge this as is at the moment or work on good solution right away. @lgirdwood if You need any help or action on my side i have a setup at home i can get to work with in 3 hours.

@dabekjakub I think it would be better to just wait for @lgirdwood 's update rather than merging this as is. I can test Liam's solution tonight too.

[dabekjakub]

Agreed

[lgirdwood]

@ranj063@dabekjakub@jajanusz Please see #730 compile tested only atm. @mengdonglin can someone test this Monday morning PRC time and make any fixes if needed.

Going forward a stream based IPC where every structure has a header with size, type, version and no substructures or flex arrays would be easiest for ABI changes going forward, but this is just too invasive for v1.3, maybe something for v1.4+....

[mmaka1]

@lgirdwood

Going forward a stream based IPC [...]

This is exactly what was mentioned in the thesofproject/linux#414.
Suggested just a one small step, that would make backward compatibility easier, not even started talking about defragmentation on a lower level (required soon, something mentioned in #730 as I understood it) nor data encoding (if required, probably much later).