Received-SPF: pass (google.com: domain of linux-kernel+bounces-25044-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <920e6d40-78d8-4e98-8f85-6d015681887f@intel.com>
Date: Fri, 12 Jan 2024 13:24:07 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 2/2] x86/resctrl: Read supported bandwidth sources
 using CPUID command
Content-Language: en-US
To: <babu.moger@amd.com>, <corbet@lwn.net>, <fenghua.yu@intel.com>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<dave.hansen@linux.intel.com>
CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<seanjc@google.com>, <kim.phillips@amd.com>, <jmattson@google.com>,
	<ilpo.jarvinen@linux.intel.com>, <jithu.joseph@intel.com>,
	<kan.liang@linux.intel.com>, <nikunj@amd.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<rick.p.edgecombe@intel.com>, <rppt@kernel.org>,
	<maciej.wieczor-retman@intel.com>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <eranian@google.com>,
	<peternewman@google.com>, <dhagiani@amd.com>
References: <20231201005720.235639-1-babu.moger@amd.com>
 <ba1305bf2a29ce2667d01cf997718126a7ea607b.1705009003.git.babu.moger@amd.com>
 <4c5bb5ac-1e84-4021-845d-cafea5a1fbde@intel.com>
 <3bd2a660-82fd-51fe-f7cd-492908d90ca7@amd.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <3bd2a660-82fd-51fe-f7cd-492908d90ca7@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K1VrQ1pJN09IZGRSTlVycTc0Zy8yZmRmQ0o1N0NNa0tUaXduOWhTNHNRUVRk?=
 =?utf-8?B?SkhkZ2RRbTVBNTZ0THBvNDEvTWJsZHlGMFQvUFBzbklPUGNLckFreHZPSTY4?=
 =?utf-8?B?QjhLTCtzODVLN0FCb0ZURjMwSU03Vkk4MmFCNG1ZMk5JWFBDcmE5M2NjdVBP?=
 =?utf-8?B?STg2UXlqc2I4QTVsMXFCUklBWE04a21jamZzWW9YYkhsYVU4SUlYN2M2bldI?=
 =?utf-8?B?VDBXRzBBamlodm9wQWRIZ0g3Vi9DTTQ0SU9CMHdqSFdLT0hUK3F6NkJWMCt0?=
 =?utf-8?B?MHkwTmgzMGJCaXFNdis4dDNEUmVwYndXendnNlFiclFUUmlsNFM0bm5VaHMz?=
 =?utf-8?B?NkpvYmhmRjNhNWIwTnF4eDlkUFJHWGpHTUlNOUw5WHlHQmtkeUlXbU9vSGFO?=
 =?utf-8?B?SWUwWTd6SjRiYmF6MStqaFZuZjd6MGF6djhjT0ovcTgvT0ZuS2R6bFNsNGFO?=
 =?utf-8?B?WXhCRWhITlRaazVJdDk2a2toS2ZHdnJnUzd0ZnphVmJvSnhHTWN1QzNNalpj?=
 =?utf-8?B?by9QRWFwM09wazJuaFVpNkczYXVwcGY0TUFGM0VOV3ZacklKNE1yVEptb1Ju?=
 =?utf-8?B?M2p5cnp4a2hxOUpGNU1QOVV0VVo5OVVid29qNUc0b2dGWDlDVVlZTTNRcVVT?=
 =?utf-8?B?d2t6enNKUHBhNXZqZGo2N01RTC93WWM1NDJmeUZldjlTZFc2NFg2TWpqK1di?=
 =?utf-8?B?TEwrUkNTajJoVW1wbm9sV0VwS0Z5WHlGM1Y5dmp6aGczZWFQRW1va2kvVE9r?=
 =?utf-8?B?OE85cERNbHNJZ05LVy9mb2UrN2w4d3V1VkZLazFYZ3BuQVZncS9wZ3ZnbEJu?=
 =?utf-8?B?ekFySHRxdHU5NU5IZkNTalZuS2dOKzhQRSttTENVN1M5NnN4bjZoUk1BcUdq?=
 =?utf-8?B?TFE5NnRXMDB6TnIrSURKSVBDS0FXZUZPZXFhc3R4M2dPNnBtMm5XZDNGL01E?=
 =?utf-8?B?MENGdlRWYWthZ3dCeW5iZkQ0VG14N244Z29GSTl5ekVEU1dxQ3VCZlVHdjdX?=
 =?utf-8?B?NWU0aStiNHhFdzhlTUMvT2lVc3Z3eEFYdmErLzhIRGJaV2ZPWllTYmxXeURh?=
 =?utf-8?B?blZhdU80TGJEMlBNbzN4WFJ1NE5nM1JPVzZES0VtcjlxVXcwQ0JxWG52L0ZV?=
 =?utf-8?B?YzU4UlVtZDJCN1BGRFZpUS9semNsQU1icFNuQ3lYRUhJTVhIbWtvbDNZRzJi?=
 =?utf-8?B?NFRJSEV4L1Rmd3lxZTVuOEk0SUc0eUo2emJvZGJLd1J3WkdHYzNsYjFoRWJz?=
 =?utf-8?B?R2M5TFlYeHhMeUllSHAyaENXNXR0WlRFbm9qRTh4NlNBR2N4c1FtSDZyb1N3?=
 =?utf-8?B?UFNYam1DTW1MVlpNU2lVS0g0MXRtbmZsVVJkU0IzeFRPRnZkQmM4UVoybkVs?=
 =?utf-8?B?MWx6MmtoVjFPdFR2dGtHT3RrRGE2eGJ6bjlFWnRUVnZhYlVSd2tQcmlYa1FC?=
 =?utf-8?B?ZnVrdjhFQmRiVUx2MkhWYVU5TVFBY1RsZTd6a29VbjhIY214dXRBU3RTYjNP?=
 =?utf-8?B?bmtvQXR0cW42NHdmMytlV1VoQ2xQV3l2K3Q4Ry8rRDVucVBXdS9pTjd3ZUdr?=
 =?utf-8?B?QVJuV1hDTDB3RERFTXVpTHFoNG1lM1NybVA1ek90dlJjNVU0b0ppTEcxeFFp?=
 =?utf-8?B?MllRZk84bmtsZUg4ekYxb2psU2hITnZzYWZQVnYzR0J3bm02UEJBcEpWeDAy?=
 =?utf-8?B?MVVHajJHZHhTUktoYW9hY1FPdzRISlpJVnNlSlp0eVZwaVlORFBOOThDaU1H?=
 =?utf-8?B?LzMyekVGaXJFQUN0TC9wcG1kSk82ZEtNUy9lTzF4SkpvTHJDbHJuQ0NxTnFx?=
 =?utf-8?B?YTRKS1RHdzAxZzlHZkdZSnZIRHlyN0w4MVVGSTdrd2ZGZG5CLzA0WlQrb1c3?=
 =?utf-8?B?VTI2QktGVXlQaGFTVUZ3TkFxYld4MEU5MG9BazJiMU9oV1VxYVdCNUlRUmVo?=
 =?utf-8?B?ZCtvZWRpTmwzWlhheCtEbkd3eXl3UktHN0pWOHdzaGRHWFlEY1d0ZnR6QTRa?=
 =?utf-8?B?dlFCS0FsbmdYY0dKQ3VzdUE3eEZZMUJya01MSlhnOTBVaktzMUxxL1I2Vy9W?=
 =?utf-8?B?ZHEzRnJuWFVaZmsrMHdjZEhhM1JZMmxiK1JnTU5WUHJYaEVINUtWQzdDSSsx?=
 =?utf-8?B?TkJObzRZcmpmUDk0ZHdhT1UxUWZKZy9aZ0RhVElzUGJqYS8yS1BUdTFpQjZX?=
 =?utf-8?B?S3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ccf7920d-1c93-4364-8ff3-08dc13b4d162
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2024 21:24:10.9199
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4Iv/nQvTuCrKk3PhB03STnvLfnO9/Vu7/ILocjUDT98DmNJ5ONLQL8Uevcd3TGvKEBGlAnF/rNK2SrY9fsJH59zZ+yV1dhqSoyv+adjJH9g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6563
X-OriginatorOrg: intel.com

Hi Babu,

On 1/12/2024 12:38 PM, Moger, Babu wrote:
> Hi  Reinette,
> 
> On 1/12/2024 1:02 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 1/11/2024 1:36 PM, Babu Moger wrote:
>>
>>> @@ -1686,6 +1681,13 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>>>           return -EINVAL;
>>>       }
>>>   +    /* mon_config cannot be more than the supported set of events */
>> copy&paste error? There is no mon_config in this function.
> Yea. it should be mbm_cfg_mask.  Will fix it.

I do not think it is correct to replace mon_config with mbm_cfg_mask. Is this comment
not referring to the user provided value (that is checked against mbm_cfg_mask)? So
perhaps something like:

	 /* Check value from user against supported events. */
	or
	 /* Value from user cannot be more than the supported set of events. */

Please feel free to improve.

>>
>> (copy&paste difficulties reminds me of [1])
>>
>>> +    if ((val & hw_res->mbm_cfg_mask) != val) {
>>> +        rdt_last_cmd_printf("Invalid event configuration: The maximum valid "
>>> +                    "bitmask is 0x%02x\n", hw_res->mbm_cfg_mask);
>> checkpatch.pl should have warned about this split of text across two lines.
>> Logging functions and single strings are allowed to exceed the max line length.
>> If you just merge the two lines then checkpatch.pl may still warn for resctrl strings
>> but that is because it does not recognize rdt_last_cmd_printf() as a logging function.
>>
>> You can also just shorten the string so this patch passes the checkpatch.pl check.
>> For example,
>> "Invalid event configuration: maximum valid mask is 0x%02x\n"
>> or
>> "Invalid event configuration: maximum is 0x%02x\n"
>> or ?
> 
> Yes. Checkpatch reported error when I split the text.
> 
> How about this?. Checkpatch is happy.
> 
> rdt_last_cmd_printf("Invalid event configuration: max valid mask is 0x%02x\n",
> +                                   hw_res->mbm_cfg_mask);
> 

Looks good.

Reinette