On Tue, 19 Sep 2023 10:32:09 +0200,
Bard Liao wrote:
>
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> +static char *loadable_patch[SNDRV_CARDS];
Hm, this size looks wrong.
In the code later in this patch, dev_index points to the codec
address, and it's basically irrelevant with SNDRV_CARDS. As
SNDRV_CARDS might be 8 depending on the config, it can go beyond the
array size, too. Use a different array size to match with the codec
address ranges.
> +module_param_array_named(patch, loadable_patch, charp, NULL, 0444);
> +MODULE_PARM_DESC(patch, "Patch file for Intel HD audio interface.");
Better to give a bit more description; when it's a codec address
array, it can be non-zero, and user may wonder why it's not applied.
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> + if (loadable_patch[hda_pvt->dev_index] && *loadable_patch[hda_pvt->dev_index]) {
> + dev_info(&hdev->dev, "Applying patch firmware '%s'\n",
> + loadable_patch[hda_pvt->dev_index]);
> + ret = request_firmware(&hda_pvt->fw, loadable_patch[hda_pvt->dev_index],
> + &hdev->dev);
> + if (ret < 0)
> + goto error_no_pm;
> + if (hda_pvt->fw) {
> + ret = snd_hda_load_patch(hcodec->bus, hda_pvt->fw->size, hda_pvt->fw->data);
> + if (ret < 0) {
> + dev_err(&hdev->dev, "failed to load hda patch %d\n", ret);
> + goto error_no_pm;
> + }
> + release_firmware(hda_pvt->fw);
> + hda_pvt->fw = NULL;
So the hda_pvt->fw is only for a temporary use, already released
already here. Then...
> --- a/sound/soc/codecs/hdac_hda.h
> +++ b/sound/soc/codecs/hdac_hda.h
> @@ -26,6 +26,10 @@ struct hdac_hda_priv {
> struct hda_codec *codec;
> struct hdac_hda_pcm pcm[HDAC_DAI_ID_NUM];
> bool need_display_power;
> + int dev_index;
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> + const struct firmware *fw;
> +#endif
... we don't have to add a new extra field, right?
thanks,
Takashi
