On Tue 13 Sep 2022 at 01:56, Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> Hi Jerome, Mark, again
>
>> > > > Current aiu-fifo.c is using kzalloc()/kfree(), but we can replace
>> > > > it by devm_kzalloc(), and remove kfree().
>> > > > This patch do it.
>> >
>> > > I'm not sure about this change Kuninori.
>> >
>> > > This is the dai probe, not the device driver probe.
>> > > If I'm not mistaken it gets called when binding the card.
>> >
>> > > The components and card drivers are different here.
>> >
>> > > If the card probes several times for any reason, EPROBE_DEFER for
>> > > example, wouldn't this allocate the memory several times without
>> > > releasing it ?
>> >
>> > Yes, indeed. You'd need to move the allocation to the device level
>> > probe to convert to devm (which *would* be a good thing to do if
>> > possible).
The AIU is a device that provides multiple ASoC components. I know it is
border line to have this and I'm sorry I was not able to contribute on
this lately.
The device is a bit complex as it is. The allocation in question really
belongs to DAI. Moving it to the device level might be possible but it
would not help make the code easy to follow and maintain.
I usually pick devm_ function whenever possible but here I chose the old
school way on purpose, to make sure the memory is managed correctly.
Beside the general recommendation to use devm when possible, is there an
issue I am missing ?
>>
>> Oh, yes, indeed.
>> I will fixup it in v2.
>
> If there was EPROBE_DEFER issue, snd_soc_bind_card() will return at (A)
> *before* calling probe callback on each DAIs at (B).
> So, I think calling devm_kzalloc() at .probe is maybe no problem.
>
> static int snd_soc_bind_card(...)
> {
> ...
>
> for_each_card_prelinks(card, i, dai_link) {
> (A) ret = snd_soc_add_pcm_runtime(card, dai_link);
> if (ret < 0)
> goto probe_end;
> }
>
> ...
>
> /* probe all DAI links on this card */
> (B) ret = soc_probe_link_dais(card);
> ...
> }
It would still allocate the memory multiple times if the card driver
gets unloaded and reloaded, manually for example.
It would be strange to do so, but it remains incorrect from the driver
to allocate the memory like this.
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
