Hi Dan,
On 25/10/2022 15:00, Dan Carpenter wrote:
> Hello Peter Ujfalusi,
>
> The patch 73c091a2fe96: "ASoC: SOF: ipc4-loader: Support for loading
> external libraries" from Oct 20, 2022, leads to the following Smatch
> static checker warning:
>
> sound/soc/sof/ipc4-loader.c:207 sof_ipc4_load_library_by_uuid()
> warn: 'payload_offset' unsigned <= 0
>
> sound/soc/sof/ipc4-loader.c
> 167 static int sof_ipc4_load_library_by_uuid(struct snd_sof_dev *sdev,
> 168 unsigned long lib_id, const guid_t *uuid)
> 169 {
> 170 struct sof_ipc4_fw_data *ipc4_data = sdev->private;
> 171 struct sof_ipc4_fw_library *fw_lib;
> 172 const char *fw_filename;
> 173 size_t payload_offset;
> ^^^^^^^^^^^^^^^^^^^^^
>
> 174 int ret, i, err;
> 175
> 176 if (!sdev->pdata->fw_lib_prefix) {
> 177 dev_err(sdev->dev,
> 178 "Library loading is not supported due to not set library path\n");
> 179 return -EINVAL;
> 180 }
> 181
> 182 if (!ipc4_data->load_library) {
> 183 dev_err(sdev->dev, "Library loading is not supported on this platform\n");
> 184 return -EOPNOTSUPP;
> 185 }
> 186
> 187 fw_lib = devm_kzalloc(sdev->dev, sizeof(*fw_lib), GFP_KERNEL);
> 188 if (!fw_lib)
> 189 return -ENOMEM;
> 190
> 191 fw_filename = kasprintf(GFP_KERNEL, "%s/%pUL.bin",
> 192 sdev->pdata->fw_lib_prefix, uuid);
> 193 if (!fw_filename) {
> 194 ret = -ENOMEM;
> 195 goto free_fw_lib;
> 196 }
> 197
> 198 ret = request_firmware(&fw_lib->sof_fw.fw, fw_filename, sdev->dev);
> 199 if (ret < 0) {
> 200 dev_err(sdev->dev, "Library file '%s' is missing\n", fw_filename);
> 201 goto free_filename;
> 202 } else {
> 203 dev_dbg(sdev->dev, "Library file '%s' loaded\n", fw_filename);
> 204 }
> 205
> 206 payload_offset = sof_ipc4_fw_parse_ext_man(sdev, fw_lib);
> --> 207 if (payload_offset <= 0) {
> ^^^^^^^^^^^^^^^^^^^
> sof_ipc4_fw_parse_ext_man() returns negative error codes but as size_t.
> It should just return int. If it returns > INT_MAX that can't work on
> 32bit systems.
Right, this is not looking good.
I think the root of the issue is:
61bafd1c4571 ("ASoC: SOF: Introduce IPC dependent ops for firmware
handling, loading")
Where I have size_t as return value for the parse_ext_manifest callback...
Let me send a quick fix for this alone and prepare a bigger one to sort
out the rest.
> 208 if (!payload_offset)
> 209 ret = -EINVAL;
> 210 else
> 211 ret = payload_offset;
> 212
> 213 goto release;
> 214 }
> 215
> 216 fw_lib->sof_fw.payload_offset = payload_offset;
> 217 fw_lib->id = lib_id;
> 218
> 219 /* Fix up the module ID numbers within the library */
> 220 for (i = 0; i < fw_lib->num_modules; i++)
> 221 fw_lib->modules[i].man4_module_entry.id |= (lib_id << SOF_IPC4_MOD_LIB_ID_SHIFT);
> 222
> 223 /*
> 224 * Make sure that the DSP is booted and stays up while attempting the
> 225 * loading the library for the first time
> 226 */
> 227 ret = pm_runtime_resume_and_get(sdev->dev);
> 228 if (ret < 0 && ret != -EACCES) {
> 229 dev_err_ratelimited(sdev->dev, "%s: pm_runtime resume failed: %d\n",
> 230 __func__, ret);
> 231 goto release;
> 232 }
> 233
> 234 ret = ipc4_data->load_library(sdev, fw_lib, false);
> 235
> 236 pm_runtime_mark_last_busy(sdev->dev);
> 237 err = pm_runtime_put_autosuspend(sdev->dev);
> 238 if (err < 0)
> 239 dev_err_ratelimited(sdev->dev, "%s: pm_runtime idle failed: %d\n",
> 240 __func__, err);
> 241
> 242 if (ret)
> 243 goto release;
> 244
> 245 ret = xa_insert(&ipc4_data->fw_lib_xa, lib_id, fw_lib, GFP_KERNEL);
> 246 if (unlikely(ret))
> 247 goto release;
> 248
> 249 kfree(fw_filename);
> 250
> 251 return 0;
> 252
> 253 release:
> 254 release_firmware(fw_lib->sof_fw.fw);
> 255 /* Allocated within sof_ipc4_fw_parse_ext_man() */
> 256 devm_kfree(sdev->dev, fw_lib->modules);
> 257 free_filename:
> 258 kfree(fw_filename);
> 259 free_fw_lib:
> 260 devm_kfree(sdev->dev, fw_lib);
> 261
> 262 return ret;
> 263 }
>
> regards,
> dan carpenter
--
Péter
