On Tue, Jul 07, 2026 at 09:59:36AM +0100, Yeoreum Yun wrote:
On Tue, Jul 07, 2026 at 09:25:10AM +0100, Leo Yan wrote:
On Mon, Jul 06, 2026 at 07:16:42PM +0100, Yeoreum Yun wrote:
[...]
I try best to not paste any non-sense AI reviews, record one from Sashiko [1]:
| If a user writes to a sysfs attribute like pe_sel_store(), it acquires | drvdata->spinlock without disabling interrupts. If an IPI is then handled | on the same CPU, etm4_enable_sysfs_smp_call() will call | cscfg_csdev_enable_active_config() which attempts to acquire the exact | same spinlock, hanging the CPU."
The flow for acquiring drvdata->spinlock in SMP call is:
etm4_enable_sysfs_smp_call() `> cscfg_csdev_enable_active_config() `> cscfg_csdev_enable_config() `> cscfg_prog_config() `> cscfg_set_on_enable() `> raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
We might expect the complaint from LOCKDEP with this patch.
Hmm. I miss the this point and there is two solution:
- Remove "feat_csdev->drv_spinlock"
Before this patch, the "feat_csdev->drv_spinlock" is for synchronizing with the "drvdata->config". However, After this patch, the the configfs only show the "active" config and the active config can be used only after taking the "mode". Furthermore, the sysfs only uses the "config" not "active config", there is no race between the sysfs and the configfs. When I check the, "feat_csdev->drv_spinlock" is for the active/deactive the configfs config, so I seems safe to remove.My interpretation is:
After this series, we updates active_config on local CPU within atomic context, it is nature to be protected. So we don't need to use feat_csdev->drv_spinlock when updating active_config.
If so, it is fine for me to remove feat_csdev->drv_spinlock.
Yes. but if I add a little bit more comment. the "active_config" is accessed after "mode changed (perf of sysfs)" it accesses atomically and the "active_config" is refered from the configfs not "config" after this patch. So we can remove the "feat_csdev->drv_spinlock".
- sysfs with _irqsave?
However, this make a latency for the sysfs.I think the (1) seems good.
Any comments? @Suzuki and @Leo?
Yeah, it is good to wait a bit for Suzuki if any ideas.
Okay. If he doesn't have any particular comment, I'll send with next version.
Just for clarification, I'll not remove the drv_spinlock field from cscfg_feature_csdev but to set this field as NULL for etm4 and still make a irqsave/restore in the cscfg_set_on_enable/disable().
Thanks!