Re: [PATCH 1/1] platform/x86: intel/pmc: Remove unused param idx from pmc_for_each_mode()

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

Hi,

On 8/6/24 10:30 AM, Ilpo Järvinen wrote:
> pmc_for_each_mode() takes i (index) variable name as a parameter but
> the loop index is not used by any of the callers. Make the index
> variable internal to pmc_for_each_mode().
> 
> This also changes the loop logic such that ->lpm_en_modes[] is never
> read beyond num_lpm_modes.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/intel/pmc/core.c       | 18 +++++++-----------
>  drivers/platform/x86/intel/pmc/core.h       | 10 ++++++----
>  drivers/platform/x86/intel/pmc/core_ssram.c |  4 ++--
>  3 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 01ae71c6df59..0e88a89a236a 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -728,12 +728,11 @@ static int pmc_core_substate_res_show(struct seq_file *s, void *unused)
>  	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
>  	const int lpm_adj_x2 = pmc->map->lpm_res_counter_step_x2;
>  	u32 offset = pmc->map->lpm_residency_offset;
> -	unsigned int i;
>  	int mode;
>  
>  	seq_printf(s, "%-10s %-15s\n", "Substate", "Residency");
>  
> -	pmc_for_each_mode(i, mode, pmcdev) {
> +	pmc_for_each_mode(mode, pmcdev) {
>  		seq_printf(s, "%-10s %-15llu\n", pmc_lpm_modes[mode],
>  			   adjust_lpm_residency(pmc, offset + (4 * mode), lpm_adj_x2));
>  	}
> @@ -787,11 +786,10 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
>  static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index)
>  {
>  	struct pmc_dev *pmcdev = s->private;
> -	unsigned int i;
>  	int mode;
>  
>  	seq_printf(s, "%30s |", "Element");
> -	pmc_for_each_mode(i, mode, pmcdev)
> +	pmc_for_each_mode(mode, pmcdev)
>  		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
>  
>  	seq_printf(s, " %9s |\n", "Status");
> @@ -833,14 +831,14 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>  			u32 req_mask = 0;
>  			u32 lpm_status;
>  			const struct pmc_bit_map *map;
> -			int mode, idx, i, len = 32;
> +			int mode, i, len = 32;
>  
>  			/*
>  			 * Capture the requirements and create a mask so that we only
>  			 * show an element if it's required for at least one of the
>  			 * enabled low power modes
>  			 */
> -			pmc_for_each_mode(idx, mode, pmcdev)
> +			pmc_for_each_mode(mode, pmcdev)
>  				req_mask |= lpm_req_regs[mp + (mode * num_maps)];
>  
>  			/* Get the last latched status for this map */
> @@ -863,7 +861,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>  				seq_printf(s, "pmc%d: %26s |", pmc_index, map[i].name);
>  
>  				/* Loop over the enabled states and display if required */
> -				pmc_for_each_mode(idx, mode, pmcdev) {
> +				pmc_for_each_mode(mode, pmcdev) {
>  					bool required = lpm_req_regs[mp + (mode * num_maps)] &
>  							bit_mask;
>  					seq_printf(s, " %9s |", required ? "Required" : " ");
> @@ -925,7 +923,6 @@ static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
>  {
>  	struct pmc_dev *pmcdev = s->private;
>  	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
> -	unsigned int idx;
>  	bool c10;
>  	u32 reg;
>  	int mode;
> @@ -939,7 +936,7 @@ static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
>  		c10 = true;
>  	}
>  
> -	pmc_for_each_mode(idx, mode, pmcdev) {
> +	pmc_for_each_mode(mode, pmcdev) {
>  		if ((BIT(mode) & reg) && !c10)
>  			seq_printf(s, " [%s]", pmc_lpm_modes[mode]);
>  		else
> @@ -960,7 +957,6 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
>  	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
>  	bool clear = false, c10 = false;
>  	unsigned char buf[8];
> -	unsigned int idx;
>  	int m, mode;
>  	u32 reg;
>  
> @@ -979,7 +975,7 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
>  	mode = sysfs_match_string(pmc_lpm_modes, buf);
>  
>  	/* Check string matches enabled mode */
> -	pmc_for_each_mode(idx, m, pmcdev)
> +	pmc_for_each_mode(m, pmcdev)
>  		if (mode == m)
>  			break;
>  
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index ea04de7eb9e8..c8851f128adc 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -604,10 +604,12 @@ int lnl_core_init(struct pmc_dev *pmcdev);
>  void cnl_suspend(struct pmc_dev *pmcdev);
>  int cnl_resume(struct pmc_dev *pmcdev);
>  
> -#define pmc_for_each_mode(i, mode, pmcdev)		\
> -	for (i = 0, mode = pmcdev->lpm_en_modes[i];	\
> -	     i < pmcdev->num_lpm_modes;			\
> -	     i++, mode = pmcdev->lpm_en_modes[i])
> +#define pmc_for_each_mode(mode, pmcdev)						\
> +	for (unsigned int __i = 0, __cond;					\
> +	     __cond = __i < (pmcdev)->num_lpm_modes,				\
> +	     __cond && ((mode) = (pmcdev)->lpm_en_modes[__i]),			\
> +	     __cond;								\
> +	     __i++)
>  
>  #define DEFINE_PMC_CORE_ATTR_WRITE(__name)				\
>  static int __name ## _open(struct inode *inode, struct file *file)	\
> diff --git a/drivers/platform/x86/intel/pmc/core_ssram.c b/drivers/platform/x86/intel/pmc/core_ssram.c
> index 1bde86c54eb9..9eea5118653b 100644
> --- a/drivers/platform/x86/intel/pmc/core_ssram.c
> +++ b/drivers/platform/x86/intel/pmc/core_ssram.c
> @@ -45,7 +45,7 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
>  	struct telem_endpoint *ep;
>  	const u8 *lpm_indices;
>  	int num_maps, mode_offset = 0;
> -	int ret, mode, i;
> +	int ret, mode;
>  	int lpm_size;
>  	u32 guid;
>  
> @@ -116,7 +116,7 @@ static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
>  	 *
>  	 */
>  	mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
> -	pmc_for_each_mode(i, mode, pmcdev) {
> +	pmc_for_each_mode(mode, pmcdev) {
>  		u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
>  		int m;
>