Re: [PATCH v3] ASoC: soc-generic-dmaengine: Handle DMA channel request failures correctly

[Bui Duc Phuc]

Hi Sebastian, Mark,

Thanks for the report, and apologies for the regression.

>
> The new warning triggers for the Rockchip SPDIF driver [0], which
> is playback only and thus only has a 'tx' DMA channel:
>
> [    2.275100] rockchip-spdif 27ea0000.spdif-tx: DTS/ACPI name 'rx' not found, legacy failed (-19)
>
> This is from RK3576 Sige5, which does not yet have the node enabled
> upstream. But it should be exactly the same for any RK3576, RK3588
> or RK3568 board at least.
>
> [0] sound/soc/rockchip/rockchip_spdif.c


The if (has_fw_node && !name_exists_in_fw) branch is not handled correctly.
For DT-only devices (such as Rockchip), this branch can end up propagating
the -ENODEV error coming from the DTS/ACPI stage itself, i.e. the case where
the name simply doesn't exist in dma-names (which is exactly what
name_exists_in_fw is meant to capture), rather than actually reflecting a
legacy-filter-map failure, as the message currently implies.

The more accurate fix would be to also filter out the -ENODEV returned by
of_dma_request_slave_channel() and acpi_dma_request_slave_chan_by_name()
when the name isn't found. However, acpi_dma_request_slave_chan_by_name()
internally calls acpi_dev_get_resources(), so the returned error isn't
guaranteed to
always be -ENODEV. It could be a different error from the resource-parsing step.

In addition, for genuine legacy-path failures, find_candidate()
already logs the failure itself:

--------------------------------------
if (err) {
        if (err == -ENODEV) {
                dev_dbg(device->dev, "%s: %s module removed\n",
                               __func__, dma_chan_name(chan));
                list_del_rcu(&device->global_node);
        } else
                dev_dbg(device->dev,
                               "%s: failed to get %s: (%d)\n",
                               __func__, dma_chan_name(chan), err);

        if (--device->privatecnt == 0)
               dma_cap_clear(DMA_PRIVATE, device->cap_mask);

        chan = ERR_PTR(err);
}

------------------------------------

So, to avoid duplicate logging as well as logging with an inaccurate
or misleading source,
I'll drop these two branches:

+                       if (has_fw_node && !name_exists_in_fw)
+                               dev_warn(dev, "DTS/ACPI name '%s' not
found, legacy failed (%ld)\n",
+                                        name, PTR_ERR(chan));
+
+                       if (!has_fw_node)
+                               dev_warn(dev, "Legacy DMA channel '%s'
request failed (%ld)\n",
+                                        name, PTR_ERR(chan));
+

and keep only:

+                       if (has_fw_node && name_exists_in_fw)
+                               dev_warn(dev, "DTS/ACPI DMA channel
'%s' request failed (%ld)\n",
+                                        name, PTR_ERR(chan));

which can be simplified to:
------------------------------------------------
+                       if (device_property_match_string(dev,
"dma-names", name) >= 0)
+                               dev_warn(dev, "dma-names has '%s' but
request failed (%ld)\n",
+                                        name, PTR_ERR(chan));
-------------------------------------------------

I'll send a fix for this.

Best regards,
Phuc