* [bug report] ALSA: hda/tas2781: Add tas2781 hda SPI driver
@ 2025-01-22 6:22 Dan Carpenter
2025-01-22 8:41 ` Takashi Iwai
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2025-01-22 6:22 UTC (permalink / raw)
To: Baojun Xu; +Cc: linux-sound
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 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] ALSA: hda/tas2781: Add tas2781 hda SPI driver
2025-01-22 6:22 [bug report] ALSA: hda/tas2781: Add tas2781 hda SPI driver Dan Carpenter
@ 2025-01-22 8:41 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2025-01-22 8:41 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Baojun Xu, linux-sound
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-01-22 8:41 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-22 6:22 [bug report] ALSA: hda/tas2781: Add tas2781 hda SPI driver Dan Carpenter
2025-01-22 8:41 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox