Allocate dai_config dynamically by ranj063 · Pull Request #281 · thesofproject/linux

ranj063 · 2018-11-13T18:43:15Z

No description provided.

ranj063 · 2018-11-13T18:46:43Z

this PR fixes #275

plbossart

I can see how the second patch makes sense, you only pass what is needed for a DAI type.
Changing the allocation as done in the first patch is one bridge too far for me, I remember there was a good reason (which I can't recall) why we moved to statically allocated structures instead of dynamic ones.
Do we really need the first part?

And while I am at it, what is the link with the ABI change that exposed a problem with the DMIC? I am trying not be reply as a grumpy old fart but please give me more technical reasons why this fix is required.

ranj063 · 2018-11-13T20:43:27Z

@plbossart let me try answering in bits:

what is the link with the ABI change that exposed a problem with the DMIC.

Without the reserved bytes in struct sof_ipc_dai_dmic_params, sizeof(struct sof_ipc_dai_config) would evaluate to 104 which is what would be needed for the DMIC dai for the dai config in sof-glk-da7219.tplg with only 1 pdm controller. So we had no issues earlier.

But with the ABI change to add 32 extra bytes in the DMIC params, we need 136 bytes for the DMIC dai_config. So when setting the dai_config, we were missing out on the last 32 bytes which contains the pdm controller config data.

Back to the first question:
With the size of dai_config varying with dai type and actual config data in tplg, I think we should allocate it dynamically. The sof_ipc_dai_config struct has a member that is a union of the 3 types of DAI and the variable length pdm config array in DMIC makes is tricky with a statically allocated member.

plbossart · 2018-11-13T21:26:26Z

@ranj063 sorry, why was 104 ok and 136 not ok. As compared to what?

Also if we move to dynamic allocation then the union can go as well, and we'd need to check the code changes in details to look for memory leaks. I am tired of spending my afternoons unwinding error paths and checking for resources that have not been released to be very honest...

ranj063 · 2018-11-13T21:44:00Z

@ranj063 sorry, why was 104 ok and 136 not ok. As compared to what?

@plbossart I was trying to say that the the static dai_config member in struct sof_ipc_dai_config would have a size of 104 bytes and will not be able to accomodate the DMIC dai_config data (136 bytes). This has be dynamically allocated based on the number of active pdm's in tplg.

Also if we move to dynamic allocation then the union can go as well, and we'd need to check the code changes in details to look for memory leaks. I am tired of spending my afternoons unwinding error paths and checking for resources that have not been released to be very honest...

I allocate memory for the dai_config when we parse topology and free it when the dai widget is unloaded. Let me check if there are any memory leaks.

plbossart · 2018-11-13T21:56:56Z

so I have a simpler fix for now: just add a reserved array of 136 bytes in the union...

ranj063 · 2018-11-13T21:58:04Z

so I have a simpler fix for now: just add a reserved array of 136 bytes in the union...

But then what about other platforms like suecreek where we could have 2 pdm controller configs?
The we would need 156 bytes.

ranj063 · 2018-11-13T22:03:27Z

@plbossart if we really want to stick with a statically allocated dai_config, maybe we could introduce NUM_HW_CONTROLLERS which would specify the size of pdm config array size depending on the platform. The fw does this already.

plbossart · 2018-11-13T22:05:33Z

We are only humans and need to progress one step at a time. First step is to restore DMIC functionality, second step is to revisit dai_config allocations. I don't want to do this latter step, which affects all platforms, while everyone is in the middle of releasing code and stabilizing blocking bugs.
Let's record the issue and do it the right way, not with rushed code reviews that will only amplify the current unreliability of the code...
It's not that I am against your ideas, it's just that I want to make sure they are properly reviewed and tested. In other words it's "not now" rather than "no".

ranj063 · 2018-11-13T22:10:35Z

@plbossart fair enough. Let me send a PR to just patch DMIC functionality then!

ranj063 · 2018-11-27T23:49:17Z

@plbossart I've just re-opened this pull request. I've spent some time thinking about how best to solve the DMIC issue and I think making dai_config's allocation dynamic makes sense to handle the issue.

But I dont think we can get rid of the union in the sof_ipc_dai_config structure. Could you please provide some feedback on this?

plbossart · 2018-11-28T10:21:38Z

But I dont think we can get rid of the union in the sof_ipc_dai_config structure. Could you please provide some feedback on this?

I don't get the question, sorry.

ranj063 · 2018-11-28T18:15:14Z

@plbossart in one of your previous comments you mentioned that if the dai_config member is dynamic the union in the sof_ipc_dai_config struct has to go as well.

I am wondering why that would be the case. Thanks!

plbossart · 2018-11-28T19:26:15Z

@plbossart in one of your previous comments you mentioned that if the dai_config member is dynamic the union in the sof_ipc_dai_config struct has to go as well.

I am wondering why that would be the case. Thanks!

A union will allocate the size needed for the worst case.
If you handle the memory allocation dynamically, you can use a void pointer for all types of DAIs and use typecasts as needed. The union between 3 pointers is no different than a single pointer in terms of memory layout...

Makes sense?

ranj063 · 2018-11-28T19:36:34Z

@plbossart in one of your previous comments you mentioned that if the dai_config member is dynamic the union in the sof_ipc_dai_config struct has to go as well.
I am wondering why that would be the case. Thanks!

A union will allocate the size needed for the worst case.
If you handle the memory allocation dynamically, you can use a void pointer for all types of DAIs and use typecasts as needed. The union between 3 pointers is no different than a single pointer in terms of memory layout...

Makes sense?

@plbossart This was what I started off doing as well but I hit a snag.
If dai params in sof_ipc_dai_config struct is a void pointer will the ipc payload be in contiguous memory? The reason I ask is because when we send the ipc, all we do is copy size number of bytes from the starting address.

plbossart · 2018-11-28T19:43:48Z

If dai params in sof_ipc_dai_config struct is a void pointer will the ipc payload be in contiguous memory? The reason I ask is because when we send the ipc, all we do is copy size number of bytes from the starting address.

Nope. That was one of the issues I wasn't clear on. Your only option then is to allocate a max size array, up to the maximum IPC size and use code to define where the dmic info is. Or use clever type casts?

ranj063 · 2018-11-28T20:15:46Z

If dai params in sof_ipc_dai_config struct is a void pointer will the ipc payload be in contiguous memory? The reason I ask is because when we send the ipc, all we do is copy size number of bytes from the starting address.

Nope. That was one of the issues I wasn't clear on. Your only option then is to allocate a max size array, up to the maximum IPC size and use code to define where the dmic info is. Or use clever type casts?

@plbossart All of the problems we saw earlier were due to the variable length array in dmic_params.
struct sof_ipc_dai_dmic_pdm_ctrl pdm[0]

Can I just make it fixed length of 4 to account for PDM controllers instead? Then maybe we wont need any changes at all.

jajanusz · 2018-11-28T23:18:05Z

@ranj063
Sorry, but I think we shouldn't make it fixed length.
There is no need for that because even without fixed length you can get everything you need to make it work, and why would we limit ourselves to fixed solution when we have it dynamic already ;)

You know what data you receive/send so you can compute size of data for dmic like sizeof(struct sof_ipc_dai_dmic_params) + (sizeof(struct sof_ipc_dai_dmic_pdm_ctrl ) * num_pdm_active).

Calculating size of whole ipc is a bit more problematic, but it is possible.
Lets start with incorrect example:

size_t size = sizeof(struct sof_ipc_dai_config) + (sizeof(struct sof_ipc_dai_dmic_pdm_ctrl ) * num_pdm_active);

It's potential bug - it can only work if sizeof dmic (without last subfield) is bigger than sizeof other structs, because union will give size of biggest struct without flexible arrays.

First of all get ipc size without flexible arrays:

size_t size_without_flex_arrays = sizeof(struct sof_ipc_dai_config);

We also need size of union field:

union {
	struct sof_ipc_dai_ssp_params ssp;
	struct sof_ipc_dai_dmic_params dmic;
	struct sof_ipc_dai_hda_params hda;
} hw_data;

size_t union_field_size = sizeof(hw_data);
// or if you don't to define union locally ->
size_t union_field_size = max(sizeof(cfg.ssp),max(sizeof(cfg.dmic),sizeof(cfg.hda)));
// if we'd have this field named like 'hw_data' instead of anonymous union in sof_ipc_dai_config, we could use something like:
size_t union_field_size = sizeof(cfg.hw_data.ssp);

Now calculate the size of ipc without union:

size_t size_without_union = size_without_flex_arrays - union_field_size;
// or ignore previous steps and do it in hacky way ->
size_t size_without_union = (size_t) (&((struct sof_ipc_dai_config *)NULL)->dmic);

So the size of whole ipc is:

size_t dmic_data_size = sizeof(struct sof_ipc_dai_dmic_params) + (sizeof(struct sof_ipc_dai_dmic_pdm_ctrl ) * num_pdm_active);
size_t ipc_size = size_without_union + dmic_data_size;

ranj063 · 2018-11-28T23:23:31Z

@jajanusz we have calculated the size of the ipc properly already. The problem is not that.

The problem is that struct sof_ipc_dai_config member in struct snd_sof_dai is allocated statically today. So its memory include ipc header size + common params + union size (which is equal to the ssp params size).

Without making the sof_ipc_dai_config member dynamic i cant get it to work properly. This patch takes care of that case.

Alternatively, we could also fix the dmic pdm array length if we dont want to dynamically allocate as here.

jajanusz · 2018-11-28T23:36:56Z

If you can allocate it dynamically I see no problem here ;)

plbossart · 2018-11-29T00:27:35Z

If you can allocate it dynamically I see no problem here ;)

we can't allocate the dmic part dynamically while having it contiguous with the ipc header.

ranj063 · 2018-11-29T03:01:03Z

@plbossart @jajanusz but only sof_ipc_dai_config as a whole is dynamically allocated not just the dmic_params member. Is even that not acceptable?

ranj063 · 2018-11-29T22:02:55Z

@plbossart I did not make any changes to this PR because I think it is a legitimate fix to the problem.

the struct sof_ipc_dai_config dai_config is a member of struct snd_sof_dai which is not part of the ipc payload. And do dynamically allocating dai_config should be fine as it will still be contiguous for the ipc to work correctly.

ranj063 · 2018-11-29T23:29:55Z

@plbossart I have fixed the conflict. I am testing it again to ensure it works.
Also if we are merging this one, we need to revert the previous patch that fixed the pdm array length to 2. Maybe I will add it to the pull request next.

ranj063 · 2018-11-29T23:52:46Z

@plbossart please hold off on merging this one. The latest sof-dev changes have caused an issue with this one. I will update ASAP.

ranj063 · 2018-11-30T00:06:43Z

@plbossart sorry for the false alarm. I have tested it to work on glk.

plbossart

I understand the intent and the flows, but I can't merge the code as is.

My family tells me I am so predictable and boring. That's the same at work.
When I see a kzalloc that is not tested, I will request a change.
When I see an error case with a memory leak, I will request a change.
When I see a size used that doesn't seem to make sense I will request a change.

Please recheck and resubmit, thanks.

plbossart · 2018-11-30T01:33:32Z

that should be dai->dai_config = kmemdup(config, size, GFP_KERNEL)

You also need to check that dai->dai_config != NULL and return an error, so the function type needs to be changed to int.

I don't quite get the loop, can we really have a case where you are going to do this multiple times? I realize you moved existing code from one place to the other, but the intent is not clear.

@plbossart yes we do have a case where we do it multiple times. Esp for the ssp2 dai which is for headset playback and capture. I added it specifically when I made the PM updates because we were only updating one previously and the other was failing during resume.

@plbossart updated to use kmemdup

plbossart · 2018-11-30T02:01:19Z

this function is just totally broken to start with,
sdev->private = kzalloc(sizeof(u32), GFP_KERNEL);

this is no matching free in error cases?

plbossart · 2018-11-30T02:09:25Z

is this correct? The size field is never used in the topology.

I don't see any case where the actual size, e.g.

size = sizeof(*config) + sizeof(struct sof_ipc_dai_dmic_pdm_ctrl) * sizeof(struct sof_ipc_dai_dmic_pdm_ctrl) * config>dmic.num_pdm_active;

is actually stored in that field?

@plbossart this is correct. Line 2021 in dmic_load() in topology.c is where we store the size and this is the sie of the ipc payload. ipc will fail is this field is not set.

where is this set for other DAIs then?

never mind, it's ok

dai_config for DAI's are of variable length depending on the type of DAI. For example, DMIC's have a variable length of pdm config array that are determined while parsing DMIC tokens in topology. So this patch modifies the dai_config member of struct snd_sof_dai to account for these changes. The changes included in the patch are: 1. Modify dai_config in struct snd_sof_dai to a pointer 2. Allocate memory for dai_config based on dai type after parsing the respective tokens for the DAI type 3. Update users of dai_config to use the pointer. 4. free dai_config when the dai widget is unloaded. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

While restoring dai links during resume, use the actual size of the config as they may vary depending on dai type. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

Looks good now. Only one delta between SSP and HDA that it'd like clarified and need your signoff for the revert. please resubmit and i'll merge. Thanks!

plbossart · 2018-11-30T14:11:20Z

+
+		if (link->dpcm_capture)
+			dev_err(sdev->dev, "error: failed to save DAI config for capture HDA%d\n",
+				config->dai_index);


Hum, wondering if I am missing something. Why do you call out playback and capture for HDAudio but not for SSP? Is this intended? Is it needed, if yes should we have this for SSP as well?

@plbossart I added this explicitly because of the previous comment in hda_load that says
"for hda link, playback and capture are supported by different dai in FW."
So to be explicit about which one failed, I separated them out.

plbossart · 2018-11-30T14:12:13Z

never mind, it's ok

plbossart · 2018-11-30T14:13:06Z

need your signoff tag even for reverts. git commit --amend --signoff

@plbossart done!

This reverts commit ddf853b. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Fix double build of kernel.o

ranj063 requested review from lgirdwood, plbossart and singalsu and removed request for lgirdwood and plbossart November 13, 2018 18:43

ranj063 force-pushed the dmic_fix branch from b374567 to 5e12ec6 Compare November 13, 2018 18:45

ranj063 requested a review from keyonjie November 13, 2018 19:08

ranj063 force-pushed the dmic_fix branch from 5e12ec6 to b63d319 Compare November 13, 2018 19:13

ranj063 requested a review from plbossart November 13, 2018 19:13

plbossart reviewed Nov 13, 2018

View reviewed changes

ranj063 closed this Nov 13, 2018

ranj063 reopened this Nov 27, 2018

ranj063 mentioned this pull request Nov 28, 2018

DAI configs: need to rework memory allocation #286

Closed

ranj063 force-pushed the dmic_fix branch from b63d319 to 72ea010 Compare November 29, 2018 23:28

ranj063 changed the title ~~Dmic fix~~ Allocate dai_config dynamically Nov 29, 2018

plbossart requested changes Nov 30, 2018

View reviewed changes

ranj063 force-pushed the dmic_fix branch from 3d8609d to 1fc4725 Compare November 30, 2018 06:46

ranj063 added 2 commits November 29, 2018 22:50

ASoC: SOF: use actual config size while restoring dai links

47cb929

While restoring dai links during resume, use the actual size of the config as they may vary depending on dai type. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the dmic_fix branch from 1fc4725 to a232a67 Compare November 30, 2018 06:58

plbossart reviewed Nov 30, 2018

View reviewed changes

Revert "ASoC: SOF: dmic: fix size of pdm config array"

51333d2

This reverts commit ddf853b. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the dmic_fix branch from a232a67 to 51333d2 Compare November 30, 2018 17:26

plbossart merged commit de1f57a into thesofproject:topic/sof-dev Nov 30, 2018

ranj063 deleted the dmic_fix branch December 3, 2018 06:53

aiChaoSONG pushed a commit to aiChaoSONG/linux that referenced this pull request May 24, 2021

Merge pull request thesofproject#281 from nbdd0121/link-time-panic

ffdacf7

Fix double build of kernel.o

Conversation

ranj063 commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

plbossart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

plbossart commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

plbossart commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

plbossart commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 13, 2018

Uh oh!

ranj063 commented Nov 27, 2018

Uh oh!

plbossart commented Nov 28, 2018

Uh oh!

ranj063 commented Nov 28, 2018

Uh oh!

plbossart commented Nov 28, 2018

Uh oh!

ranj063 commented Nov 28, 2018

Uh oh!

plbossart commented Nov 28, 2018

Uh oh!

ranj063 commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jajanusz commented Nov 28, 2018

Uh oh!

ranj063 commented Nov 28, 2018

Uh oh!

jajanusz commented Nov 28, 2018

Uh oh!

plbossart commented Nov 29, 2018

Uh oh!

ranj063 commented Nov 29, 2018

Uh oh!

ranj063 commented Nov 29, 2018

Uh oh!

ranj063 commented Nov 29, 2018

Uh oh!

ranj063 commented Nov 29, 2018

Uh oh!

ranj063 commented Nov 30, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment •

edited

Loading

ranj063 commented Nov 28, 2018 •

edited

Loading

ranj063 Nov 30, 2018 •

edited

Loading

ranj063 Nov 30, 2018 •

edited

Loading