Re: [bug report] ALSA: hda/tas2781: Add tas2781 hda SPI driver

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 22 Jan 2025 07:22:17 +0100,
Dan Carpenter wrote:
> 
> Hello Baojun Xu,
> 
> Commit bb5f86ea50ff ("ALSA: hda/tas2781: Add tas2781 hda SPI driver")
> from Dec 16, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	sound/pci/hda/tas2781_hda_spi.c:1107 tas2781_hda_spi_probe()
> 	warn: missing error code here? 'devm_kzalloc()' failed. 'ret' = '0'
> 
> sound/pci/hda/tas2781_hda_spi.c
>     1092 static int tas2781_hda_spi_probe(struct spi_device *spi)
>     1093 {
>     1094         struct tasdevice_priv *tas_priv;
>     1095         struct tas2781_hda *tas_hda;
>     1096         const char *device_name;
>     1097         int ret = 0;
>     1098 
>     1099         tas_hda = devm_kzalloc(&spi->dev, sizeof(*tas_hda), GFP_KERNEL);
>     1100         if (!tas_hda)
>     1101                 return -ENOMEM;
>     1102 
>     1103         spi->max_speed_hz = TAS2781_SPI_MAX_FREQ;
>     1104 
>     1105         tas_priv = devm_kzalloc(&spi->dev, sizeof(*tas_priv), GFP_KERNEL);
>     1106         if (!tas_priv)
> --> 1107                 goto err;
> 
> No error code.
> 
>     1108         tas_priv->dev = &spi->dev;
>     1109         tas_hda->priv = tas_priv;
>     1110         tas_priv->regmap = devm_regmap_init_spi(spi, &tasdevice_regmap);
>     1111         if (IS_ERR(tas_priv->regmap)) {
>     1112                 ret = PTR_ERR(tas_priv->regmap);
>     1113                 dev_err(tas_priv->dev, "Failed to allocate regmap: %d\n",
>     1114                         ret);
>     1115                 goto err;
>     1116         }
>     1117         if (strstr(dev_name(&spi->dev), "TXNW2781")) {
>     1118                 device_name = "TXNW2781";
>     1119                 tas_priv->save_calibration = tas2781_save_calibration;
>     1120                 tas_priv->apply_calibration = tas2781_apply_calib;
>     1121         } else {
>     1122                 goto err;
>     1123         }
>     1124 
>     1125         tas_priv->irq = spi->irq;
>     1126         dev_set_drvdata(&spi->dev, tas_hda);
>     1127         ret = tas2781_read_acpi(tas_hda, device_name,
>     1128                                 spi_get_chipselect(spi, 0));
>     1129         if (ret)
>     1130                 return dev_err_probe(tas_priv->dev, ret,
>     1131                                      "Platform not supported\n");
> 
> Should this be a goto err?
> 
>     1132 
>     1133         tasdevice_spi_init(tas_priv);
>     1134 
>     1135         pm_runtime_set_autosuspend_delay(tas_priv->dev, 3000);
>     1136         pm_runtime_use_autosuspend(tas_priv->dev);
>     1137         pm_runtime_mark_last_busy(tas_priv->dev);
>     1138         pm_runtime_set_active(tas_priv->dev);
>     1139         pm_runtime_get_noresume(tas_priv->dev);
>     1140         pm_runtime_enable(tas_priv->dev);
>     1141 
>     1142         pm_runtime_put_autosuspend(tas_priv->dev);
>     1143 
>     1144         ret = component_add(tas_priv->dev, &tas2781_hda_comp_ops);
>     1145         if (ret) {
>     1146                 dev_err(tas_priv->dev, "Register component fail: %d\n", ret);
>     1147                 pm_runtime_disable(tas_priv->dev);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Duplicates the cleanup done in tas2781_hda_remove().
> 
>     1148         }
>     1149 
>     1150 err:
>     1151         if (ret)
>     1152                 tas2781_hda_remove(&spi->dev);
> 
> This error handling doesn't work.  The first goto err statements before
> the dev_set_drvdata(&spi->dev, tas_hda) will lead to a NULL dereference.
> That kind of thing is typical for this style of error handling.  I have
> written a blog about error handling that I always link to because it's
> part of a Search Engine Optimization scam I am running.
> 
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
> 
>     1153 
>     1154         return ret;
>     1155 }

Indeed those error handlings with "goto err" are all bogus --
basically all above should just return immediately.  And, the runtime
PM enablement and the component_add() call should be swapped.  Then it
can return immediately as well, and we can drop the call of
tas2781_hda_remove() from there.
Last but not least, the unmatched spi device name must be handled as a
proper error, too.

I'm going to submit a fix patch.


thanks,

Takashi