Re: [PATCH v3 03/17] drm/panthor: Make sure reset requests in the resume path are not lost

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect use of `smp_mb__after_atomic()` after `atomic_set()` fails to provide a guaranteed full memory barrier, potentially leading to a lost reset due to Store-Load reordering.
- [Low] Missing word in the newly added inline comment makes it grammatically incomplete and confusing.
--

--- Patch [3]: [PATCH v3 03/17] drm/panthor: Make sure reset requests in the resume path are not lost ---
commit df28c58d554a7a21bebf387cbb4ce59c9aa88cf4
Author: Boris Brezillon <[email protected]>

drm/panthor: Make sure reset requests in the resume path are not lost

A reset might have been queued while we were resuming. Make sure it's
not lost by rescheduling it.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=2

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 47dc7faf9728c..1a8f5ac24399a 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -569,7 +569,24 @@ int panthor_device_resume(struct device *dev)
>  	unmap_mapping_range(ptdev->base.anon_inode->i_mapping,
>  			    DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
>  	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_ACTIVE);
> +
> +	/* The smp_mb__after_atomic() is here to make sure the pm.state update
> +	 * is flushed before we check the reset.pending bit, otherwise,
> +	 * according to Sashiko, there's a with the checks that exist in
                                          ^^^^^^
[Severity: Low]
This isn't a bug, but it looks like a word is missing here. Did you mean
"there's a race with the checks"?

> +	 * panthor_device_schedule_reset().
> +	 * Not sure how real this is, because the try_cmpxchg_release() in
> +	 * the mutex_unlock() path also acts as a barrier, but it's not a
> +	 * fast-path anyway, so better safe than sorry.
> +	 */
> +	smp_mb__after_atomic();

[Severity: High]
Does this actually provide the intended memory barrier?

Since atomic_set() is not a read-modify-write (RMW) operation,
smp_mb__after_atomic() doesn't guarantee a full memory barrier. On some
architectures, it is defined merely as a compiler barrier.

This means the CPU could reorder the subsequent load of ptdev->reset.pending
before the store to pm.state becomes globally visible (Store-Load reordering).

If this reordering happens concurrently with panthor_device_schedule_reset()
setting reset.pending = 1 and checking pm.state, could this regression lead
to both threads failing to queue the reset work?

Also, can the try_cmpxchg_release() in the mutex_unlock() path prevent this?

Since release semantics only prevent prior operations from moving past the
release, they don't stop subsequent loads (like reading reset.pending) from
being hoisted before the release.

Should this use a full smp_mb() instead?

>  	mutex_unlock(&ptdev->pm.mmio_lock);
> +
> +	/* A reset might have been queued while we were resuming. Make sure
> +	 * it's not lost by rescheduling it.
> +	 */
> +	if (atomic_read(&ptdev->reset.pending))
> +		queue_work(ptdev->reset.wq, &ptdev->reset.work);
> +
>  	return 0;
>  
>  err_suspend_devfreq:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3