Re: [PATCH] coresight: tmc-etr: Don't fail probe when non-secure access is disabled

[Yabin Cui]

Thanks for the suggestion of triggering the probe manually. It works.

I feel the problem isn't product-specific. Because DBGEN isn't product
specific.

If you don't want users to misunderstand that ETR works while not,
how about doing the check in tmc_enable_etr_sink() instead?
Or can we use some flag to make the check configurable?

Thanks,
Yabin

On Thu, Aug 31, 2023 at 3:05 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> On 2023-08-30 18:49, Yabin Cui wrote:
> > Hi Suzuki,
> >
> >> Are you not able to build the coresight drivers as modules and load
> >> them after the device has been authenticated and NS access enabled ?
> >> Running a trace session without NS access enabled on a normal device
> >> would be asking for trouble in the "normal world".
> >
> > Theoretically we can load coresight drivers after getting NS access.
> > But in practice,
> > it makes the userspace work more complex. The process will be as below:
> > 1. Use device specific checks to know if we have NS access authorized.
> >      Because we can't use the general coresight sysfs interface to read
> > authstatus.
> > 2. Load coresight driver modules.
> > 3. Use ETM/ETR.
> >
> > It needs to add device specific checks in Android AOSP code (which we
> > don't prefer),
> > and add an extra step to load driver modules. It's more complex no matter we do
> > it in a daemon or want to use ETM/ETR manually.
> >
> > If we can load the coresight drivers at boot time. The process is
> > simplified as below:
> > 1. Use the coresight sysfs interface to read authstatus. It works on
> > all devices.
> > 2. If authorized, use ETM/ETR.
> >
> > The authorization used on Pixel devices can be granted/revoked while running.
> > So not allowing loading coresight drivers doesn't help us. We always need to
> > check authstatus each time before using ETM/ETR. And the check can be
> > easily added in tools using ETM/ETR.
>
> What, and "needing to connect to a server to verify identification after
> booting" isn't already a complex extra step? You have to do that, and
> you presumably have to trigger some firmware call to toggle DBGEN, so it
> doesn't seem obvious why you couldn't synchronise module loading to a
> point within that process. Heck, it doesn't even need to be a module
> load, you could simply trigger a manual re-probe of a built-in (or
> already-loaded) driver. If you can parse sysfs to find a specific path
> to a device's "authstatus" attribute at the point when you think it
> should be available, I'm sure you can just as easily construct the
> relevant string to write to the relevant "probe" attribute if it is not.
>
> Really the big question here is why should upstream care about
> supporting some private product-specific internal workflow that is
> irrelevant to end users? Especially if doing so makes the end users'
> experience objectively worse, by making the driver look like it should
> work when in reality it won't.
>
> Thanks,
> Robin.
>
> >
> > Thanks,
> > Yabin
> >
> > On Wed, Aug 30, 2023 at 1:52 AM Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
> >>
> >> Hi Yabin
> >>
> >> On 29/08/2023 22:16, Yabin Cui wrote:
> >>>> How can this be enabled ? Why not enable it before probing the ETR ?
> >>> How can a user know if this has been done or not ?
> >>>
> >>> Pixel devices (like Pixel 6, 7) support enabling some debugging features
> >>> (including granting non-secure access to ETM/ETR) even on devices with
> >>> secure boot. It is only used internally and has strict requirements,
> >>> needing to connect to a server to verify identification after booting.
> >>> So it can't be established when probing ETR at device boot time.
> >>
> >> Are you not able to build the coresight drivers as modules and load
> >> them after the device has been authenticated and NS access enabled ?
> >> Running a trace session without NS access enabled on a normal device
> >> would be asking for trouble in the "normal world".
> >>
> >> Suzuki
> >>
> >>>
> >>>
> >>> On Sun, Aug 27, 2023 at 2:37 PM Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
> >>>>
> >>>> On 26/08/2023 00:39, Yabin Cui wrote:
> >>>>> Because the non-secure access can be enabled later on some devices.
> >>>>
> >>>> How can this be enabled ? Why not enable it before probing the ETR ?
> >>>> How can a user know if this has been done or not ? It is asking for
> >>>> trouble to continue without this.
> >>>>
> >>>> Suzuki
> >>>>
> >>>>>
> >>>>> Signed-off-by: Yabin Cui <yabinc@xxxxxxxxxx>
> >>>>> ---
> >>>>>     drivers/hwtracing/coresight/coresight-tmc-core.c | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> >>>>> index c106d142e632..5ebfd12b627b 100644
> >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> >>>>> @@ -370,7 +370,7 @@ static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps)
> >>>>>         struct tmc_drvdata *drvdata = dev_get_drvdata(parent);
> >>>>>
> >>>>>         if (!tmc_etr_has_non_secure_access(drvdata))
> >>>>> -             return -EACCES;
> >>>>> +             dev_warn(parent, "TMC ETR doesn't have non-secure access\n");
> >>>>>
> >>>>>         /* Set the unadvertised capabilities */
> >>>>>         tmc_etr_init_caps(drvdata, (u32)(unsigned long)dev_caps);
> >>>>
> >>
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel