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

[Bhupesh Sharma <bhupesh.sharma@linaro.org>]

On 7/28/22 2:26 AM, Dmitry Baryshkov wrote:
> On Wed, 27 Jul 2022 at 23:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>
>>
>>
>> On 7/28/22 2:23 AM, Dmitry Baryshkov wrote:
>>> On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
>>>>> To ease debugging of PSCI supported features, add debugfs file called
>>>>> 'psci' describing PSCI and SMC CC versions, enabled features and
>>>>> options.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
>>>>>     include/uapi/linux/psci.h    |   9 +++
>>>>>     2 files changed, 120 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>>>> index b907768eea01..6595cc964635 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,121 @@ 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)
>>>>
>>>> This change doesn't seem related to the patch $SUBJECT.
>>>> Also is it really needed? If yes, probably this should be a separate patch.
>>>
>>> It is related and I don't think it should be moved to a separate
>>> patch. Removing the __init annotation from psci_features() is
>>> necessary to allow using psci_features() from psci_debufs_read(),
>>> which is definitely not an __init code. Otherwise reading from
>>> debugfs/psci would cause null pointer exceptions.
>>
>> Ok, and what is the behavior with CONFIG_DEBUG_FS = n?
>> Does your patch work well in that case?
> 
> Yes. Any particular reasons for the question?

Your debugfs changes in this patch are protected with CONFIG_DEBUG_FS,
while the  __init code change is not.

So, IMO its not really needed if CONFIG_DEBUG_FS is set to =n (hence
probably needs to be a separate patch).

Thanks.

>>>>>     {
>>>>>         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),
>>>>> +};
>>>>> +
>>>>> +static int psci_debugfs_read(struct seq_file *s, void *data)
>>>>> +{
>>>>> +     int feature, type, i;
>>>>> +     u32 ver;
>>>>> +
>>>>> +     ver = psci_ops.get_version();
>>>>> +     seq_printf(s, "PSCIv%d.%d\n",
>>>>> +                PSCI_VERSION_MAJOR(ver),
>>>>> +                PSCI_VERSION_MINOR(ver));
>>>>> +
>>>>> +     /* PSCI_FEATURES is available only starting from 1.0 */
>>>>> +     if (PSCI_VERSION_MAJOR(ver) < 1)
>>>>> +             return 0;
>>>>> +
>>>>> +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
>>>>> +     if (feature != PSCI_RET_NOT_SUPPORTED) {
>>>>> +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
>>>>> +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
>>>>> +                        PSCI_VERSION_MAJOR(ver),
>>>>> +                        PSCI_VERSION_MINOR(ver));
>>>>> +     } else {
>>>>> +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
>>>>> +     }
>>>>> +
>>>>> +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
>>>>> +     if (feature < 0) {
>>>>> +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
>>>>> +     } else {
>>>>> +             seq_printf(s, "OSI is %ssupported\n",
>>>>> +                        (feature & BIT(0)) ? "" : "not ");
>>>>> +             seq_printf(s, "%s StateID format is used\n",
>>>>> +                        (feature & BIT(1)) ? "Extended" : "Original");
>>>>> +     }
>>>>> +
>>>>> +     type = psci_ops.migrate_info_type();
>>>>> +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
>>>>> +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
>>>>> +             unsigned long cpuid;
>>>>> +
>>>>> +             seq_printf(s, "Trusted OS %smigrate capable\n",
>>>>> +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
>>>>> +             cpuid = psci_migrate_info_up_cpu();
>>>>> +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
>>>>> +     } else if (type == PSCI_0_2_TOS_MP) {
>>>>> +             seq_printf(s, "Trusted OS migration not required\n");
>>>>> +     } else {
>>>>> +             if (type != PSCI_RET_NOT_SUPPORTED)
>>>>> +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
>>>>> +     }
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
>>>>> +             feature = psci_features(psci_fn_ids[i].fn);
>>>>> +             if (feature == PSCI_RET_NOT_SUPPORTED)
>>>>> +                     continue;
>>>>> +             if (feature < 0)
>>>>> +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
>>>>> +             else
>>>>> +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int psci_debugfs_open(struct inode *inode, struct file *f)
>>>>> +{
>>>>> +     return single_open(f, psci_debugfs_read, NULL);
>>>>> +}
>>>>> +
>>>>> +static const struct file_operations psci_debugfs_ops = {
>>>>> +     .owner = THIS_MODULE,
>>>>> +     .open = psci_debugfs_open,
>>>>> +     .release = single_release,
>>>>> +     .read = seq_read,
>>>>> +     .llseek = seq_lseek
>>>>> +};
>>>>> +
>>>>> +static int __init psci_debugfs_init(void)
>>>>> +{
>>>>> +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
>>>>> +                                                &psci_debugfs_ops));
>>>>> +}
>>>>> +late_initcall(psci_debugfs_init)
>>>>> +#endif
>>>>> +
>>>>>     #ifdef CONFIG_CPU_IDLE
>>>>>     static int psci_suspend_finisher(unsigned long state)
>>>>>     {
>>>>> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
>>>>> index 2bf93c0d6354..f6f0bad5858b 100644
>>>>> --- a/include/uapi/linux/psci.h
>>>>> +++ b/include/uapi/linux/psci.h
>>>>> @@ -48,11 +48,20 @@
>>>>>     #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
>>>>>
>>>>>     #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
>>>>> +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
>>>>> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
>>>>> +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
>>>>>     #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
>>>>>     #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
>>>>> +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
>>>>> +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
>>>>>     #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
>>>>>
>>>>> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
>>>>> +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
>>>>>     #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
>>>>> +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
>>>>> +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
>>>>>     #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
>>>>>
>>>>>     /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
>>>
>>>
>>>
> 
> 
>