Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] accel: ethosu: mixing __free() and scoped_guard() with goto cleanup labels --
commit cbd771f7e8b7995eaa7aedcd81a7cde35a4b6ada Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Guard operations after unplug This commit guards ethosu device file open and private ioctl operations with drm_dev_enter() and drm_dev_exit(), and switches device removal from drm_dev_unregister() to drm_dev_unplug(). It also drops drm_dev_enter() from the BO wait ioctl so fence waits do not delay scheduler teardown. > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index 6875ed17a004d..91b2c2ab0a03e 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c [ ... ] > @@ -139,27 +132,34 @@ static int ethosu_ioctl_cmdstream_bo_create(struct > drm_device *ddev, void *data, > > static int ethosu_open(struct drm_device *ddev, struct drm_file *file) > { > - int ret = 0; > + struct ethosu_file_priv __free(kfree) * priv = NULL; > + int cookie, ret; > > if (!try_module_get(THIS_MODULE)) > return -EINVAL; > + if (!drm_dev_enter(ddev, &cookie)) { > + module_put(THIS_MODULE); > + return -ENODEV; > + } > > - struct ethosu_file_priv __free(kfree) *priv = kzalloc_obj(*priv); > + priv = kzalloc_obj(*priv); > if (!priv) { > ret = -ENOMEM; > - goto err_put_mod; > + goto err_dev_exit; > } > priv->edev = to_ethosu_device(ddev); > > ret = ethosu_job_open(priv); > if (ret) > - goto err_put_mod; > + goto err_dev_exit; > > ethosu_perfmon_open_file(priv); > file->driver_priv = no_free_ptr(priv); > + drm_dev_exit(cookie); > return 0; > > -err_put_mod: > +err_dev_exit: > + drm_dev_exit(cookie); > module_put(THIS_MODULE); > return ret; > } [Severity: Medium] In drivers/accel/ethosu/ethosu_drv.c, ethosu_open() declares priv with __free(kfree) = NULL at the top of the function before try_module_get() and drm_dev_enter(), and then mixes __free(kfree) with goto err_dev_exit when kzalloc_obj() or ethosu_job_open() fails. Because scope-based cleanup runs when the function scope exits after the err_dev_exit label executes, drm_dev_exit(cookie) and module_put(THIS_MODULE) will run before kfree(priv), reversing the reverse acquisition (lifo) teardown order and mixing goto error handling with cleanup.h helpers. Should ethosu_open() avoid mixing __free(kfree) and goto labels so that priv is freed before exiting the drm device section and dropping the module reference? > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > index f22cacb19e3dc..8ffdc83d06f98 100644 > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c [ ... ] > @@ -557,26 +558,34 @@ static int ethosu_ioctl_submit_job(struct drm_device > *dev, struct drm_file *file > int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file > *file) > { > struct drm_ethosu_submit *args = data; > - int ret = 0; > + int cookie, ret = 0; > unsigned int i = 0; > > + if (!drm_dev_enter(dev, &cookie)) > + return -ENODEV; > + > struct drm_ethosu_job __free(kvfree) *jobs = > kvmalloc_objs(*jobs, args->job_count); > - if (!jobs) > - return -ENOMEM; > + if (!jobs) { > + ret = -ENOMEM; > + goto out_dev_exit; > + } > > if (copy_from_user(jobs, > (void __user *)(uintptr_t)args->jobs, > args->job_count * sizeof(*jobs))) { > drm_dbg(dev, "Failed to copy incoming job array\n"); > - ret = -EFAULT; > + ret = -EFAULT; > + goto out_dev_exit; > } > > for (i = 0; i < args->job_count; i++) { > ret = ethosu_ioctl_submit_job(dev, file, &jobs[i], > args->perfmon_id); > if (ret) > - return ret; > + goto out_dev_exit; > } > > - return 0; > +out_dev_exit: > + drm_dev_exit(cookie); > + return ret; > } > diff --git a/drivers/accel/ethosu/ethosu_perfmon.c > b/drivers/accel/ethosu/ethosu_perfmon.c > index 2a9b3cc1447bd..8d3635ba3347b 100644 > --- a/drivers/accel/ethosu/ethosu_perfmon.c > +++ b/drivers/accel/ethosu/ethosu_perfmon.c [ ... ] > @@ -260,44 +289,54 @@ int ethosu_ioctl_perfmon_set_global(struct drm_device > *dev, void *data, > struct ethosu_file_priv *ethosu_priv = file_priv->driver_priv; > struct drm_ethosu_perfmon_set_global *req = data; > struct ethosu_device *ethosu = to_ethosu_device(dev); > - struct ethosu_perfmon *perfmon; > + struct ethosu_perfmon *old = NULL, *perfmon; > + int cookie, ret = 0; > > - if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) > - return -EINVAL; > + if (!drm_dev_enter(dev, &cookie)) > + return -ENODEV; > + > + if (req->flags & ~DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) { > + ret = -EINVAL; > + goto out_dev_exit; > + } > > perfmon = ethosu_perfmon_find(ethosu_priv, req->id); > - if (!perfmon) > - return -EINVAL; > + if (!perfmon) { > + ret = -EINVAL; > + goto out_dev_exit; > + } > > /* If the request is to clear the global performance monitor */ > if (req->flags & DRM_ETHOSU_PERFMON_CLEAR_GLOBAL) { > - struct ethosu_perfmon *old; > - > scoped_guard(mutex, ðosu->perfmon_state.lock) { > old = ethosu->global_perfmon; > - if (!old) { > - ethosu_perfmon_put(perfmon); > - return -EINVAL; > + if (old) { > + ethosu->global_perfmon = NULL; > + ethosu_perfmon_stop_locked(ethosu, old, true); > } > + } > > - ethosu->global_perfmon = NULL; > - ethosu_perfmon_stop_locked(ethosu, old, true); > + ethosu_perfmon_put(perfmon); > + if (!old) { > + ret = -EINVAL; > + goto out_dev_exit; > } > > ethosu_perfmon_put(old); > - ethosu_perfmon_put(perfmon); > > - return 0; > + goto out_dev_exit; > } > > scoped_guard(mutex, ðosu->perfmon_state.lock) { > - if (ethosu->perfmon_state.active || ethosu->global_perfmon) { > - ethosu_perfmon_put(perfmon); > - return -EBUSY; > - } > - > - ethosu->global_perfmon = perfmon; > + if (ethosu->perfmon_state.active || ethosu->global_perfmon) > + ret = -EBUSY; > + else > + ethosu->global_perfmon = perfmon; > } > + if (ret) > + ethosu_perfmon_put(perfmon); > > - return 0; > +out_dev_exit: > + drm_dev_exit(cookie); > + return ret; > } [Severity: Medium] Similarly, in drivers/accel/ethosu/ethosu_job.c, ethosu_ioctl_submit() mixes a mid-function __free(kvfree) declaration for jobs with goto out_dev_exit, which causes drm_dev_exit(cookie) to run before kvfree(jobs). In drivers/accel/ethosu/ethosu_perfmon.c, ethosu_ioctl_perfmon_set_global() also mixes scoped_guard(mutex, ðosu->perfmon_state.lock) with goto out_dev_exit labels. Per include/linux/cleanup.h, goto-based cleanup and scope-based cleanup helpers should not be mixed within the same function. Could these functions either use scope-based cleanup consistently or use explicit cleanup along the goto paths? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
