On Tue, Aug 11, 2026 at 04:45:21PM +0100, Leo Yan wrote:
On Sat, Jul 25, 2026 at 12:36:36PM +0100, Yeoreum Yun wrote:
[...]
As the active_config is used for cfg-configfs, etmv4 doesn't need to set its lock for the cfg-configfs since the active_config is proceted by cs_mode otherwise it would make a possible cpu-stall when it get interrupt while setting the sysfs configuration.
Therefore, set the drv_spinlock for cfg-configfs as NULL and let the cfg-configfs disable irq without grap drv_spinlock when it is NULL.
I understand that this patch tries to put all the changes (active_config and drv_spinlock) into a single patch so that it can be backported to stable kernels.
However, the change is now quite large, and I suspect it will also be difficult for maintainers to backport it to stable kernels. The fixes tag 54ff892b76c6 is quite old, while this patch also touches cfgfs which was introduced much later.
Can we treat this as a refactoring instead and split it into at least two patches? This would make it easier to review now and easier to understand later if someone will read the changes.
- Lock refactoring
- SMP call refactoring
- active_config refactoring
It couldn't since separation of Lock and SMP can introduce the bug for that patch. and the Lock and SMP call refactoring isn't meaningful without active_config.
With this perspective, I think it would be better as-is.
static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat_csdev) {
- unsigned long flags; int i;
- raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
- for (i = 0; i < feat_csdev->nr_regs; i++)
cscfg_set_reg(&feat_csdev->regs_csdev[i]);- raw_spin_unlock_irqrestore(feat_csdev->drv_spinlock, flags);
- scoped_guard (feat_csdev_lock, feat_csdev) {
scoped_guard(feat_csdev_lock, feat_csdev) {
for (i = 0; i < feat_csdev->nr_regs; i++)cscfg_set_reg(&feat_csdev->regs_csdev[i]);- } dev_dbg(&feat_csdev->csdev->dev, "Feature %s: %s", feat_csdev->feat_desc->name, "set on enable"); return 0;
@@ -88,13 +87,12 @@ static int cscfg_set_on_enable(struct cscfg_feature_csdev *feat_csdev) /* copy back values from the driver locations referenced in cscfg_reg_csdev */ static void cscfg_save_on_disable(struct cscfg_feature_csdev *feat_csdev) {
- unsigned long flags; int i;
- raw_spin_lock_irqsave(feat_csdev->drv_spinlock, flags);
- for (i = 0; i < feat_csdev->nr_regs; i++)
cscfg_save_reg(&feat_csdev->regs_csdev[i]);- raw_spin_unlock_irqrestore(feat_csdev->drv_spinlock, flags);
- scoped_guard (feat_csdev_lock, feat_csdev) {
scoped_guard(feat_csdev_lock, feat_csdev) {
Thanks. I'll change it.
for (i = 0; i < feat_csdev->nr_regs; i++)cscfg_save_reg(&feat_csdev->regs_csdev[i]);- } dev_dbg(&feat_csdev->csdev->dev, "Feature %s: %s", feat_csdev->feat_desc->name, "save on disable");
} diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h index 90fd937d3bd8..0782db3b1b74 100644 --- a/drivers/hwtracing/coresight/coresight-config.h +++ b/drivers/hwtracing/coresight/coresight-config.h @@ -8,6 +8,7 @@ #define _CORESIGHT_CORESIGHT_CONFIG_H #include <linux/coresight.h> +#include <linux/cleanup.h>
Alphabet order. Move cleanup.h above coresight.h.
okay.
#include <linux/types.h> /* CoreSight Configuration Management - component and system wide configuration */ @@ -259,4 +260,29 @@ void cscfg_csdev_disable_config(struct cscfg_config_csdev *config_csdev); /* reset a feature to default values */ void cscfg_reset_feat(struct cscfg_feature_csdev *feat_csdev); +#define feat_csdev_lock(feat_csdev, flags) \
Could use inline here?
static inline void feat_csdev_lock_irqsave(..., unsigned long *flags) { ... }
I think this is much annyoing. since the deference might add more instruction to save the flags. Otherwise the typecheck is for compilet-time check and not for runtime.
So, it would be better to remain as-is.
- do { \
raw_spinlock_t *__lock = feat_csdev->drv_spinlock; \typecheck(unsigned long, flags); \After using inline, no need typecheck.
if (__lock) \raw_spin_lock_irqsave(__lock, flags); \else \local_irq_save(flags); \If __lock is NULL, do we still need local_irq_save()? Seems to me, if lock is NULL pointer, it means no race condition.
Yeap. When I check again it doesn't need to disable the irq. I'll remove it.
- } while (0)
+#define feat_csdev_unlock(feat_csdev, flags) \
- do { \
raw_spinlock_t *__lock = feat_csdev->drv_spinlock; \typecheck(unsigned long, flags); \if (__lock) \raw_spin_unlock_irqrestore(__lock, flags); \else \local_irq_restore(flags); \- } while (0)
Use inline for feat_csdev_unlock() and rename it to feat_csdev_unlock_irqrestore().
See above please.
Otherwise, LGTM.
Thanks!