Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <485dbd08-7616-09e3-c9eb-de0a1f78572b@amd.com>
Date:   Wed, 17 May 2023 10:09:12 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Cc:     yazen.ghannam@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        markgross@kernel.org, hdegoede@redhat.com,
        Shyam-sundar.S-k@amd.com, linux-edac@vger.kernel.org,
        clemens@ladisch.de, jdelvare@suse.com, linux-hwmon@vger.kernel.org,
        mario.limonciello@amd.com, babu.moger@amd.com
Subject: Re: [PATCH 5/6] hwmon: (k10temp) Define helper function to read CCD
 temp
Content-Language: en-US
To:     Guenter Roeck <linux@roeck-us.net>
References: <20230516202430.4157216-1-yazen.ghannam@amd.com>
 <20230516202430.4157216-6-yazen.ghannam@amd.com>
 <f61f548b-0007-404e-9c9d-d84a2a657983@roeck-us.net>
From:   Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <f61f548b-0007-404e-9c9d-d84a2a657983@roeck-us.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WXlmRUp0OEVObFM3cFlDMGkyRmVlRU5IU2UvOEZRUkQ2TGJ1eXJxZWszVjJV?=
 =?utf-8?B?VTQ2b0xXMXJMN2U4NEJoRm9QcGRBME9PTmdGVkdiNCt6NzBVYk9nL0IzMEg1?=
 =?utf-8?B?K2RwekhUMXlZS2E1c3huTnl4a1YvSytQSllKSkw1K2c0VkhaMFFFSHR5MkZI?=
 =?utf-8?B?dGtBN2JtWXBLVGZyU3RPQW1zbXJ1bXNKcG9EZnRqUEJrLzVpZUFyUDFzS0Q0?=
 =?utf-8?B?bEJzRXc2UWhTWE4xekNYUWtSMGQ3UWdqYVVYVlFQUDNySXpNdU9pSkNSYnFB?=
 =?utf-8?B?STJyZy8rWFVPbnpBRFk3VmxMcmI4WjhETHVxazVOaE80akJHSUtkN0Z5ZzRp?=
 =?utf-8?B?bDRNczIxeklEU0dvSnlvT1EzNmlUdU5zS0pvdVIxbHBzMFliUXVrTVc2S2Q4?=
 =?utf-8?B?bGxQSWFHNXppNTkxc2trVGxnbEJUcy9FMzZWNmZmNTlaTlhsWHg2SjFTNE5R?=
 =?utf-8?B?bCt5OXJNWlBFbmgwNUFZeDhtUkNNeFVTR3VXZDlzcVlZbW9xakF5WkV4UkJ1?=
 =?utf-8?B?RTM1eWhnNUk0d3pWekxkY3JCTUVLRENDNHZpdmVPNTNEbmhGeDViYzRTdGw5?=
 =?utf-8?B?T0FaQkRsbUZVbGFxN1ZYT1VTVy9hd1pSS21xeUcxNWhxYzg5NlVJeENFMEUv?=
 =?utf-8?B?b0thdTRGUzdMVjhSd2cyQm1hRG9RSWk2RlVZanlxTnZOYkdlNzdtYVQ3NFhG?=
 =?utf-8?B?SHJZR1BRM2NCZU16djh0Zng1ektWRFZVVkFqSkZ2T0wyMnFpeXhhRnVDcG5i?=
 =?utf-8?B?ekRPLzFRRFhpMWd4N3hsTDlXc05XTEFibnRNK2IxWG90d2R3V1ZwQVFSbkRh?=
 =?utf-8?B?UHV4Yy92WHhwb2kvMHJxWGo5V2ZYUFBzSURCdjZOS2xkVUVvUURJQ1l4RHdX?=
 =?utf-8?B?eVc3QmdHWCtEVTRuVTBHWXBjU2thZS9CTG9xaXBhOXhtWHBYVWs2cUtHWXdv?=
 =?utf-8?B?WlpsRVZsU01aeStBclZxYkNKVFZ0RDVxRUZSOHVUTWROVWI5SEFOL1Z3R1FZ?=
 =?utf-8?B?RlRzR0hpQUxZckloUlkrSnY5bWgwUjNnUERYTU9PTGlNYkxFaFJFNzlIK04w?=
 =?utf-8?B?RE5jZE0rY21tbWphRjVYWDJlM216aFpNSXBrL3c3VG5FZlZJNE9WQmJTSHV1?=
 =?utf-8?B?bVY1YjBHWDgxMlBGa2hMLy9pQnhjZ3FZZ1h0WlVuZEx5dDNpVGNERzlrRFpP?=
 =?utf-8?B?a3FINFoyNTRqN2JLSnJNNDhzcEM2MEw1MUpnV29qTDQwS2R5VUNYaGxMN1NQ?=
 =?utf-8?B?UnludHBsL0Z0cGJCQW1mSStQVlhGM0dYVGdtejNNV0tIK0JYRWJ1aHgrOGFr?=
 =?utf-8?B?MjF6YUlIZ0llNjRrNWRwVW94alMwZWFUc0xZQVRDVlIyZzBLV0kxWUljQU9Y?=
 =?utf-8?B?WkpJMjFCQXRHczV1ZmNFaHpiNkhRUUxtTXFpTzhpc1ZJZkFsQTk5clBiQjMy?=
 =?utf-8?B?TXpxaEQ0UlpVWGNyRDZwYmxscDZ4Zlo1N085Vm1zQ1ZTTnhCRVR1MHhmQUQr?=
 =?utf-8?B?a2dsbkF1R0tFZVNDa05lQzUxRXNEc1RnWlozdWYrSVRJV2xwZVZDT0Q4QzJX?=
 =?utf-8?B?RlY2N011VTJBbC9XWExHL0NoVE1DUEplZXhycEhHbzRJR0NmdlJDZkZFTkQ0?=
 =?utf-8?B?RDNFSkNHMkl3Q1k1VE5CVFZQQTI0UithSk9qbm1wR1h6bC9zVDVDNDFEMkdE?=
 =?utf-8?B?T2xwVzByVlVzVGw1K3hQcWZvRkQ1bXI0dUkwcktUakxDSFBhcXRHMGlGRzMr?=
 =?utf-8?B?MUY2K3JQWXNLcnZVK1dQcE1rSDlZU0FxSUh0WnBEbEJIamxhTHBGREVlSmRo?=
 =?utf-8?B?SVczK1RwYi9iclpEbDRhT3J2ZmNLN3E1QnZlaEVtWFUvZVJMbXl1VTcxZWhQ?=
 =?utf-8?B?cFdSQ3dKUkVWNityTGRjRFhJeXVxRlQwajFseWtsbkFJQWs0RjEwSXk1QmdL?=
 =?utf-8?B?VXBSQUY5b3FLNXlkeC83S0VSYVdKSSt5ZGUyclhMS1p1NHN0cTBEUXdQSDVD?=
 =?utf-8?B?d2xZS3g4WGRaSXM0MXNJd0w3RDI4N3J0RjFYTkc4TkdsaEJIWUVOQUNBd3Fx?=
 =?utf-8?B?dmdBZGp0cXhBd0FacWxjd2lKOTdVSTk0YnBZcTIzc2pxb245djFycjd1cDRR?=
 =?utf-8?Q?OXEFWl9jaN9gkos3Kkx9L06uG?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3cceadff-0a8a-4a6f-34ef-08db56e04bea
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2023 14:09:15.1249
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IprkmYf3drcjTY9rgmXY2bO6UMzBo7nr+AFGZcspTFbMrlg9RvOTJsiXaP+NOWSaERfkeOX2EsL8+b2ExAKmoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB5486
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/17/23 8:30 AM, Guenter Roeck wrote:
> On Tue, May 16, 2023 at 03:24:29PM -0500, Yazen Ghannam wrote:
>> The CCD temperature register is read in two places. These reads are done
>> using an AMD SMN access, and a number of parameters are needed for the
>> operation.
>>
>> Move the SMN access and parameter gathering into a helper function in
>> order to simply the code flow. This also has a benefit of centralizing
>> the hardware register access in a single place in case fixes or special
>> decoding is required.
>>
>> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>> ---
>>  drivers/hwmon/k10temp.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
>> index 6ea1fa62b7c1..06af1fe38af7 100644
>> --- a/drivers/hwmon/k10temp.c
>> +++ b/drivers/hwmon/k10temp.c
>> @@ -150,6 +150,18 @@ static void read_tempreg_nb_zen(struct pci_dev *pdev, u32 *regval)
>>  		*regval = 0;
>>  }
>>  
>> +static int read_ccd_temp_reg(struct k10temp_data *data, int ccd, u32 *regval)
>> +{
>> +	u16 node_id = amd_pci_dev_to_node_id(data->pdev);
>> +	u32 tmp;
>> +
>> +	if (amd_smn_read(node_id, ZEN_CCD_TEMP(data->ccd_offset, ccd), &tmp))
>> +		return -EINVAL;
> 
> As mentioned before, this is not appropriate. The error code
> from amd_smn_read() should be returned.
>

Okay, will do.

>> +
>> +	*regval = tmp;
> 
> This seems pointless. Why not just pass regval as parameter
> to amd_smn_read() ?
> 
> This function should be reduced to
> 
> 	return amd_smn_read(node_id, ZEN_CCD_TEMP(data->ccd_offset, ccd), regval));
> 

Yes, will do. I think I got carried away with the "don't save invalid
value" idea. But this isn't necessary if the caller checks the return
code and avoids using the value.

>> +	return 0;
>> +}
>> +
>>  static long get_raw_temp(struct k10temp_data *data)
>>  {
>>  	u32 regval;
>> @@ -214,9 +226,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
>>  				*val = 0;
>>  			break;
>>  		case 2 ... 13:		/* Tccd{1-12} */
>> -			if (amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
>> -					 ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
>> -					 &regval))
>> +			if (read_ccd_temp_reg(data, channel - 2, &regval))
>>  				return -EINVAL;
> 
> Now this is really wrong, no matter what. Why bother replacing the
> error code in read_ccd_temp_reg() only to replace it again ?
>

Understood. Will fix like the other places.

Thanks,
Yazen