From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id D82DD40F72B for ; Tue, 11 Aug 2026 15:45:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786463127; cv=none; b=NZeYoPIGXSaKj7SvdnQ9joFd9gpqGr20sIiplArtS2M4pn7aMmfXKioZC60+TfRULpwK7+xScwMqsHu1kpd7LLeRgaUCY5O+bEbGqFjKJ0ZhTpOC/5DnD+jT8lz4IzLYebyftTL3wrQs9o5A5aKPwqV7dFhGXvW8uZn/py6QYV0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786463127; c=relaxed/simple; bh=le+NNPrAYCUZxz1+o/7VIHlKfNijZaFoZCquij3ucIc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=CzD+xtW33I2ZjAAxG7ipCFCbZNsyDUDpDouhSYP33nN0V9IMBSuRjSNxzZytg/cdAxltDL9CRRDQWY4Jbm79Lk7OkvxA3ehPLoUvpCpbF56h+sh8Eok6ADAOLCBs+IipkO9XP6kCFjdcH6uxESmmjGcLvCpXzg2UvM9E8fhK3Rw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=Mq33hIGc; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="Mq33hIGc" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 235541516; Tue, 11 Aug 2026 08:45:20 -0700 (PDT) Received: from localhost (unknown [10.2.196.114]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B5B0B3F632; Tue, 11 Aug 2026 08:45:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1786463124; bh=le+NNPrAYCUZxz1+o/7VIHlKfNijZaFoZCquij3ucIc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Mq33hIGcTmSoNM6mygiLmeEkmMYMUcfhp9X9yt60bvbT/7Au3M6+75VTK8dVnpnYs HzU/5bvz+kdVfsYZYzbVbaIgLDX/tnBL/BtesU5w1x5Q9YojS/t+8uEERwa1YU4qHw qMN+/W9K1PifG/8bOLb8A6bPUut5lkVZVEcwCjrQ= Date: Tue, 11 Aug 2026 16:45:21 +0100 From: Leo Yan To: Yeoreum Yun Cc: coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, suzuki.poulose@arm.com, mike.leach@arm.com, james.clark@linaro.org, alexander.shishkin@linux.intel.com, jie.gan@oss.qualcomm.com Subject: Re: [PATCH v9 04/13] coresight: etm4x: fix inconsistencies with sysfs configuration Message-ID: <20260811154521.GF15499@e132581.arm.com> References: <20260725113645.57519-1-yeoreum.yun@arm.com> <20260725113645.57519-5-yeoreum.yun@arm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260725113645.57519-5-yeoreum.yun@arm.com> 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 > 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) { > + 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 > +#include Alphabet order. Move cleanup.h above coresight.h. > #include > > /* 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) { ... } > + 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. > + } 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(). Otherwise, LGTM. Thanks, Leo