Message-ID: <20251117110457.GN3568724@e132581.arm.com>
Date: Mon, 17 Nov 2025 11:04:57 +0000
From: Leo Yan <leo.yan@....com>
To: Mike Leach <mike.leach@...aro.org>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, suzuki.poulose@....com,
	james.clark@...aro.org
Subject: Re: [PATCH v2 1/1] coresight: fix issue where coresight component
 has no claimtags

Hi Mike,

On Fri, Nov 14, 2025 at 06:03:25PM +0000, Mike Leach wrote:

[...]

> > > +static bool coresight_claim_tags_implemented_unlocked(struct csdev_access *csa)
> > > +{
> > > +     u32 claim_bits_impl = FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
> > > +                      csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> > > +     return ((claim_bits_impl & CORESIGHT_CLAIM_BIT_PROTOCOL_HI) != 0);
> >
> > How about a simple one?
> >
> >         return !!FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
> >                            csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> 
> Cannot use this as it is valid, if unlikely, to have 1 claim bit which
> would give a false positive as we need 2 bits for the existing
> protocol.

Understand now.  As the protocol works with multiple bits, I would
check CLAIM_MASK instead:

    u32 claim_bits = FIELD_GET(CORESIGHT_CLAIM_MASK,
                        csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
    return claim_bits == CORESIGHT_CLAIM_MASK;

[...]

> > >  void coresight_clear_self_claim_tag_unlocked(struct csdev_access *csa)
> > >  {
> > > +     if (!coresight_claim_tags_implemented_unlocked(csa))
> > > +             return;
> >
> > Here it does not call coresight_check_use_claim_tag_unlocked(), which
> > means the cached flag is not used. I think we should be consistent
> > across all claim tag functions.
> 
> the relevant csdev may not yet exist, and is only used once in init -
> see below for further explanation
> 
> >
> > This is why I suggested to put the flag into csdev_access rather than
> > in csdev. Seems to me, it is nature to use csdev_access to store this
> > flag, as it presents a attribute for register access (we can consider
> > csdev presents a device on bus).
> 
> Not used here as these claim tag functions are not consistent
> themselves - some use a csdev, some use a csa.

Yes.  I have noticed that the clear claim functions use "csa" as its
parameter when I reviewed the change.  This is not bad for me, we
might consolidate all claim function for using "csa" parameter.

> The ones that use csa are exclusively used in clearing down a stale
> claim tag during initialization of a device in a probe function or sub
> function and called only once per device - sometimes  before a csdev
> is in existence. This is safe in cases where claim tags do not exist
> or are not operating correctly as writes will be ignored. This
> therefore only represents a single write per device, and not something
> that occurs multiple times and might benefit from caching the claim
> tag usage status.

When the probe() in CoreSight drivers uses a temporary "csa" to clear
claim tags, in the end, the temporary "csa" will be assigned to
csdev->access when invoke coresight_register().  So it is not true for
"use csa in a probe does not benefit from caching the claim tag usage
status".

A corner case is coresight-etm3x-core.c, the "drvdata->csa" is used
to clear claim tag but not synced to csdev.  We can use a separate patch
to fix it.

> All other uses of claim tags in the drivers use the claim/disclaim
> functions using the csdev as a parameter, after probing and when the
> claim infrastructure is being used.

This is not a barrier.  We can easily dereference the flag via csdev:

    if (csdev->access.claim_tag_impl)
         ...

> Having the claimtag information in the csdev is natural as this has
> all the other general cs information - such as cs type, subtype etc -
> describing the coresight features of the hardware.

> The csa is a structure describing only how the registers are accessed.
> with no register or device specifics. This should remain device/
> register agnostic and avoid coupling in device/register specific
> parameters.

csdev presents a device on CoreSight bus, so it makes sense to store
device type and sub type.  It is not necessary to store every hardware
specific info into it.  More imporatnt, a well organized structure can
avoid complexity in flows.

If store flag in csdev, you will face dilemma when exploring a neat
flow.  For instance, during ETMv4's per CPU init, the csdev is not ready
because it has not registered on the bus yet.  If using csdev to store
the cached flag, we will run into a chicken-and-egg problem, on the
other hand, it would be much easier if we store the flag in "csa", as
"csa" is prepared much earlier.

> An additional use of the flag for claimtag usage state (beyond the
> slight gain in avoiding a read per call to the claim tag API), is to
> allows customers whose claim tags are non-operative, or as in an
> actual case - also give a false positive of
> operationally sufficient claim tags - to set the test to "no claim
> tags" during the probe sequence, so as to ensure the test is skipped
> and claim tags are never attempted to be used during enable / disable
> device
> 
> The clearing down of the self claim tag on probe is to remove stale
> self claim tags when a kernel is restarted without disclaiming
> hardware (e.g. debugging/crash etc) and without repowering the device.
> From a kernel perspective, this does not have to happen until the
> first claim attempt is made for the device. Given that on the first
> claim call we now do a one time check, this one time clearing of
> lingering claim tags could be done here, and remove the clear tag apis
> completely simplifying the code  - though this would leave the devices
> apparently claimed until that first use.

Now we already have callsites in device probes for clear claims, seems
to me, we can simply reuse these callsites for claim tag init.  (Just
rename to new functions, coresight_claim_tag_init{_unlocked}()).

I am fine with using the deferring method if the logic is simple
enough.  However, I don't like the idea of separating the claim tag
clearing and the initialization flag - why not maintain the init code
in single place?

Thanks,
Leo

Powered by blists - more mailing lists