Re: [RESEND PATCH v2] firmware/psci: Add debugfs support to ease debugging

[Ulf Hansson <ulf.hansson@linaro.org>]

On Mon, 26 Sept 2022 at 14:04, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 14:39, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Mon, 26 Sept 2022 at 13:08, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > To ease debugging of PSCI supported features, add debugfs file called
> > > 'psci' describing PSCI and SMC CC versions, enabled features and
> > > options.
> > >
> > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > I sketched this while trying to narrow down the particular issue on
> > > the Qualcomm platform (which started as an attempt to implement PSCI
> > > domains, but then led to understanding that while the platform claims
> > > supports OSI, it doesn't support SET_SUSPEND). We were going to use
> > > this to help to narrow down issues with the PSCI support on other
> > > platforms as well.
> > >
> > > Changes since v1:
> > > - Extended the table to include MEM_PROTECT functions (noted by Mark
> > >   Brown)
> > > - Switched to seq_puts where possible
> > > - Changed S_IRUGO to 0444
> > >
> > > The patch is resent to include soc@kernel.org
> > > ---
> > >  drivers/firmware/psci/psci.c | 116 ++++++++++++++++++++++++++++++++++-
> > >  include/uapi/linux/psci.h    |  14 +++++
> > >  2 files changed, 129 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > index cfb448eabdaa..9fdcb6bff403 100644
> > > --- a/drivers/firmware/psci/psci.c
> > > +++ b/drivers/firmware/psci/psci.c
> > > @@ -9,6 +9,7 @@
> > >  #include <linux/acpi.h>
> > >  #include <linux/arm-smccc.h>
> > >  #include <linux/cpuidle.h>
> > > +#include <linux/debugfs.h>
> > >  #include <linux/errno.h>
> > >  #include <linux/linkage.h>
> > >  #include <linux/of.h>
> > > @@ -324,12 +325,125 @@ static void psci_sys_poweroff(void)
> > >         invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> > >  }
> > >
> > > -static int __init psci_features(u32 psci_func_id)
> > > +static int psci_features(u32 psci_func_id)
> > >  {
> > >         return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> > >                               psci_func_id, 0, 0);
> > >  }
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > +
> > > +#define PSCI_ID(ver, _name) \
> > > +       { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
> > > +#define PSCI_ID_NATIVE(ver, _name) \
> > > +       { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
> > > +
> > > +/* A table of all optional functions */
> > > +static const struct {
> > > +       u32 fn;
> > > +       const char *name;
> > > +} psci_fn_ids[] = {
> > > +       PSCI_ID_NATIVE(0_2, MIGRATE),
> > > +       PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> > > +       PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> > > +       PSCI_ID(1_0, CPU_FREEZE),
> > > +       PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> > > +       PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> > > +       PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> > > +       PSCI_ID(1_0, SET_SUSPEND_MODE),
> > > +       PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> > > +       PSCI_ID_NATIVE(1_0, STAT_COUNT),
> > > +       PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> > > +       PSCI_ID(1_1, MEM_PROTECT),
> > > +       PSCI_ID_NATIVE(1_1, MEM_PROTECT_CHECK_RANGE),
> > > +};
> > > +
> > > +static int psci_debugfs_read(struct seq_file *s, void *data)
> > > +{
> > > +       int feature, type, i;
> > > +       u32 ver;
> > > +
> > > +       ver = psci_ops.get_version();
> >
> > This call and some other calls below, triggers calls into the PSCI FW
> > (via smc calls). Rather than having to do that, each time user space
> > wants to read the debugfs data, we could cache/store that data during
> > the "probe" sequence of the psci fw driver. In this way, we don't need
> > to run the smc calls each time, but just once when probing.
> >
> > Did you consider that option? Or perhaps there are other problems of doing that?
>
> Basically I did not want to complicate this. While caching firmware
> information sounds appealing, it would complicate adding support for
> the additional debugging features, like checking sleep state stats.
>
> So, I'd say, it's not worth doing that.

Alright, no strong opinion from my side. Feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

Kind regards
Uffe