Re: [PATCH v1 05/15] platform/x86/amd/pmf: Add debugfs information

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 7/12/22 16:58, Shyam Sundar S K wrote:
> Add debugfs support to the PMF driver so that using this interface the
> live counters from the PMFW can be queried to see if the power parameters
> are getting set properly when a certain power mode change happens.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/core.c | 46 +++++++++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/pmf.h  |  3 ++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index b6006e8ee1a1..3d2af6a8e5e4 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -8,6 +8,7 @@
>   * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> @@ -46,6 +47,49 @@
>  #define DELAY_MIN_US	2000
>  #define DELAY_MAX_US	3000
>  
> +#ifdef CONFIG_DEBUG_FS

linux/debugfs.h defines stubs for the used debugfs functions when
debugfs is not enabled, which will cause all the referred code
to also get optimized away. So the #ifdef here is not necessary
please drop it.

> +static int current_power_limits_show(struct seq_file *seq, void *unused)
> +{
> +	struct amd_pmf_dev *dev = seq->private;
> +	struct amd_pmf_static_slider_granular table;
> +	int mode, src = 0;
> +
> +	mode = amd_pmf_get_pprof_modes(dev);
> +	src = amd_pmf_get_power_source();
> +	amd_pmf_update_slider(dev, SLIDER_OP_GET, mode, &table);
> +	seq_printf(seq, "spl:%u fppt:%u sppt:%u sppt_apu_only:%u stt_min:%u stt[APU]:%u stt[HS2]: %u\n",
> +		   table.prop[src][mode].spl,
> +		   table.prop[src][mode].fppt,
> +		   table.prop[src][mode].sppt,
> +		   table.prop[src][mode].sppt_apu_only,
> +		   table.prop[src][mode].stt_min,
> +		   table.prop[src][mode].stt_skin_temp[STT_TEMP_APU],
> +		   table.prop[src][mode].stt_skin_temp[STT_TEMP_HS2]);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(current_power_limits);
> +
> +static void amd_pmf_dbgfs_unregister(struct amd_pmf_dev *dev)
> +{
> +	debugfs_remove_recursive(dev->dbgfs_dir);
> +}
> +
> +static void amd_pmf_dbgfs_register(struct amd_pmf_dev *dev)
> +{
> +	dev->dbgfs_dir = debugfs_create_dir("amd_pmf", NULL);
> +	debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
> +			    &current_power_limits_fops);
> +}
> +#else
> +static inline void amd_pmf_dbgfs_register(struct amd_pmf_dev *dev)
> +{
> +}
> +
> +static inline void amd_pmf_dbgfs_unregister(struct amd_pmf_dev *dev)
> +{
> +}

and also drop the entire #else block where you define your own stubs.

> +#endif /* CONFIG_DEBUG_FS */
> +
>  int amd_pmf_get_power_source(void)
>  {
>  	if (power_supply_is_system_supplied() > 0)
> @@ -231,6 +275,7 @@ static int amd_pmf_probe(struct platform_device *pdev)
>  	apmf_acpi_init(dev);
>  	platform_set_drvdata(pdev, dev);
>  	amd_pmf_init_features(dev);
> +	amd_pmf_dbgfs_register(dev);
>  
>  	mutex_init(&dev->lock);
>  	dev_info(dev->dev, "registered PMF device successfully\n");
> @@ -244,6 +289,7 @@ static int amd_pmf_remove(struct platform_device *pdev)
>  
>  	mutex_destroy(&dev->lock);
>  	amd_pmf_deinit_features(dev);
> +	amd_pmf_dbgfs_unregister(dev);
>  	kfree(dev->buf);
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index a405987ae653..77021ef4bfde 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -102,6 +102,9 @@ struct amd_pmf_dev {
>  	enum platform_profile_option current_profile;
>  	struct platform_profile_handler pprof;
>  	struct mutex lock; /* protects the PMF interface */
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +	struct dentry *dbgfs_dir;
> +#endif /* CONFIG_DEBUG_FS */

And don't forget to drop the #if here. Also note your #if-s are not consistent
you are using:

#ifdef CONFIG_DEBUG_FS

vs:

#if IS_ENABLED(CONFIG_DEBUG_FS)

But since both should be dropped anyways this is not a problem :)

Regards,

Hans