Re: [tip:x86/cache 9/13] arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used

[Philip Li]

On Tue, Jan 24, 2023 at 02:05:37PM +0100, Borislav Petkov wrote:
> On Tue, Jan 24, 2023 at 08:37:14PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
> > head:   0a363fb23ee2f7beb08437ad7db86d195878d79f
> > commit: dc2a3e857981f859889933cf66ded117d74edff1 [9/13] x86/resctrl: Add interface to read mbm_total_bytes_config
> 
> These patches have been around for a long while now. How come you test them just
> now, after I take them?

Sorry Boris for the late reply. We will look into this to understand what happened
for the patches that why we didn't report it out earlier before they landed on tip.

> 
> > >> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1456:6: warning: variable 'h' set but not used [-Wunused-but-set-variable]
> >            u32 h;
> >                ^
> 
> The fix is simple - use rsmsrl(), see below.
> 
> If it weren't that simple I'd simply ignore this warning altogether. Yes, it is
> unused because the damn high MSR value needs to go somewhere, for chrissakes.
> And if it didn't need that masking with MAX_EVT_CONFIG_BITS but the low 32-bit
> MSR value would fit right into mon_info->mon_config, then I would've ignored
> this one even faster.
> 
> And besides, I'm wondering if you're spending your 0day resources properly if
> you're testing patches one-by-one for W=1 warnings?!
> 
> If it were me, I'd prefer if you build-test patches on lkml for build *errors*,
> *before* they get picked up in some tree instead of testing the applied branches
> already for some questionable warnings...

Thanks a lot for the suggestions, we will think of this to continuously optimize the
service. Right now, we try to build-test the patches that we can find a suitable base
to apply the patches successfully, some of effort could fail. Then we only test them
when they appears on repo. We will keep monitoring the patch testing status to see
anything we can fix as well.

> 
> Thx.
> 
> ---
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index cd4e668e5019..9bd0eb050e7a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1453,17 +1453,17 @@ static void mon_event_config_read(void *info)
>  {
>  	struct mon_config_info *mon_info = info;
>  	unsigned int index;
> -	u32 h;
> +	u64 msrval;
>  
>  	index = mon_event_config_index_get(mon_info->evtid);
>  	if (index == INVALID_CONFIG_INDEX) {
>  		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>  		return;
>  	}
> -	rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
> +	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>  
>  	/* Report only the valid event configuration bits */
> -	mon_info->mon_config &= MAX_EVT_CONFIG_BITS;
> +	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>  }
>  
>  static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette