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: <c4351aaf-d04c-6b13-6dbb-d75509b5ad4f@intel.com>
Date:   Mon, 24 Oct 2022 16:17:01 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH 02/14] platform/x86/intel/ifs: Propagate load failure
 error code
Content-Language: en-US
To:     Sohil Mehta <sohil.mehta@intel.com>
CC:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <gregkh@linuxfoundation.org>, <ashok.raj@intel.com>,
        <tony.luck@intel.com>, <linux-kernel@vger.kernel.org>,
        <platform-driver-x86@vger.kernel.org>, <patches@lists.linux.dev>,
        <ravi.v.shankar@intel.com>, <thiago.macieira@intel.com>,
        <athenas.jimenez.gonzalez@intel.com>, <hdegoede@redhat.com>,
        <markgross@kernel.org>
References: <20221021203413.1220137-1-jithu.joseph@intel.com>
 <20221021203413.1220137-3-jithu.joseph@intel.com>
 <4028a1c7-ed84-37a8-1d94-178b5aa201a9@intel.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <4028a1c7-ed84-37a8-1d94-178b5aa201a9@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Z0Npb0IwUjNUc3A2Y1BTa280ZjliN1pTdnczb3lSVGE4cFI1TUNnWXJZckN5?=
 =?utf-8?B?eUx2bS9IaGw3c0ozcHlqZExZMkJJUG5QMlgyMU5JTWRDM0pDVy9uMmlsT0Y4?=
 =?utf-8?B?NWVvc2dsemFhcHFsdzNndWVvSFlhb0phMFpaVDhHTjNaSVdEUVBLeVFRS3VM?=
 =?utf-8?B?Vkdua2NmeVNOSnVqd3dOYkxaUUhNbDJheUxxT2p3UkNpcGR2Q2xzWXZ1cWpT?=
 =?utf-8?B?K2FQSW16emlnOG5OeG42bXNnS2U1eitlb2QwRzlZTWFCaktTTm9XcG9SNjJy?=
 =?utf-8?B?K3VKZ0FyemxpbFh0MTZaODFZUnJLSXNiUHRUSVBFanNyM2FzNXRPeHRmSlBZ?=
 =?utf-8?B?SUtoR1kzSFNaRDk5ak5UQ09nQTNydWN6cHdJbVNLdkYyS1FZZHlJOFFRc202?=
 =?utf-8?B?VFlubkp2MW94Z3ZkREI3NkFFdHM2VUxoWjFIWG1nNEdDSW1MNGlnQUlaT2F5?=
 =?utf-8?B?K3FuYnJMTlhYMzRDRHN3SHU4dVZkUjhVUG5KcldINE1kYjF0NU4vNllEYWY5?=
 =?utf-8?B?TnQ5bVQra29sbk5IckdRMit1Mkc2NDVJK2xTeWgxU0ZMUmk4c3RhcVE5ajli?=
 =?utf-8?B?N3JjSHZCTFZ5R2Vha2lQeXNrT0dEVVJJK2Zsd3ZLM3VqVXJWZjZ2Nkd2ZCtR?=
 =?utf-8?B?VXpYNXdRZXBpbXpRbENlSHpNWWZwaGs0VDdWWGhOMFJDb0U4MTAxUGpWdWh6?=
 =?utf-8?B?elM1eFVlY3JuNEVJdDhjM0twYXhPSDdYbU5WOU5lanYramkrRTlFdE04UlhV?=
 =?utf-8?B?VUtiZ3VtUUFMNGpCc0U2bGRGUzQ3Mjl3M2R0eURuR2d5dHJ3WlRIREFqQzJT?=
 =?utf-8?B?WUFQRG41NVpUMmlDcE9rQzI5MXFhRTV5TXpJK0s0Rk1qK2kzWlJMT1dFL2d2?=
 =?utf-8?B?RXRwMTZOZjd1THpVUVZNbk1hNVQvbms5RnNTN09nREdhajdPV25rVHFaSm0w?=
 =?utf-8?B?OUpJUFFjbEdVMVRENU9xbnUycHNQTGlXSnZVRXhqYTlhSDh0NDVXdGRralQr?=
 =?utf-8?B?eUZUMzhKZFRNVm9NTmYxdTVjQlRzQkZvdnpZQVVPeUVXb2dtWnZRcUJPaTlq?=
 =?utf-8?B?b1BDT0FWMmVwRXc5ZnVJWG4yeWZUbThhSUxxSGFNY3BybXpia0lWQjZ4WDhW?=
 =?utf-8?B?V1Q1SW90UWZQWURib0IreVBuRzEvYjVyd3VGZ0tXZnFUY1MrMDVjZWN0NWEr?=
 =?utf-8?B?d3IyMUxGdlA0SlF2ZnJEZVRhMGx4MmRYUEhDR0xtUWtNK1NqV2lOYmhodVpl?=
 =?utf-8?B?Ti9VT0lOVWlLK0F0T3UzQ2tNaFlCZTR1c1RXdVlnNTBIK3hDRDdBU2JVbm9Q?=
 =?utf-8?B?QzVyZTM1MHpPMkE3MEtxMnB0QWczOUVqNWo4eHFDY25JaGxFcVg1SDdEaity?=
 =?utf-8?B?bktzeDFjL09OQVFDaURpQ2tLS29CRnV5Q2lUMEtqUVRMSi91aGxjNFMvS2V4?=
 =?utf-8?B?bHdDdzhITGIvc0JUdld2aXNPOEZkS25HSXlVRk1BeS9MaUpEVVE5aktDUGJL?=
 =?utf-8?B?dHh6bUFIaHphZ2dRQ2NEYUhCQVlLdmk0WW1KRHJrOGx2d0phbGE1ZVFiRmtE?=
 =?utf-8?B?R21IaVIrMG9MWUZlbnFWVTRGOWY4RmcyL2QzL1c2MS9QenFnUWZyM3F0RXI2?=
 =?utf-8?B?c0ZQTTRja1p6YzRjWWZTZ2FUUlovZWxLYTdKSm5LWDhMeTFka0ZDNEdrQXZN?=
 =?utf-8?B?R0RMVHo0WWdTdVFFWVo5UTQrUE1mU0diN1lVT1JUdVFoZHZzaTJMU1hqaVpK?=
 =?utf-8?B?N2lGRFlCOFJNMGc5L1gvM3g1emIrU2RBdzZIbGNSWkNyZi94bHBTdG0vbDA1?=
 =?utf-8?B?OThFNXFwTXJ6c2VCVnR1WklDa1VhSzloNDVRajdmNUtyZ2pETFRhU0JOcEFa?=
 =?utf-8?B?dmpVRUh6RGdna0xiN2s5SFNmY0J5Q3hzTmNCTVBaa3gycng0MWxNa1VTcnRW?=
 =?utf-8?B?L0xualB3Q3hGTnUzQTZEbFJnZEs1V1h4OW9sR0E1ekhuMzc0bW9LNUNmcWdv?=
 =?utf-8?B?eGgwTFZ2TjFWTnd1MjVsa09sMm9zbUQzWVVqZzVwb1Q4NndmaWZoMUFnUW4w?=
 =?utf-8?B?TWxsQTE0Y29FdzFzSVZpd1JCbTV1RU4zNEljWldaVWJmOFBiNGVBU295Tmhz?=
 =?utf-8?B?Ty80NWRHU3VOVGlBL3J2b2F6eVNHUVFMMTR3bm12WXNBR2xrVkhJZFFoOVRZ?=
 =?utf-8?B?Wmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c4b88ee4-4792-4c93-d89d-08dab615dcdd
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2022 23:17:04.4472
 (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: nP/AG9A2TWTcbGMuFIZmIfBCq3jbPkWnIBIQXu617oKFp6merWtaxF/yR56cuQwd0XzFIiDt+2PfvuTTo6agTA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5541
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE 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 10/24/2022 3:52 PM, Sohil Mehta wrote:
> Hi Jithu,
> 
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Existing implementation was returning fixed error code to user space
>> regardless of the load failure encountered.
>>
> 
> The tense is a bit confusing here. Also, 'Existing implementation' is typically implied and unnecessary. Would something like this be better?
> 
> The reload operation returns a fixed error code to user space regardless of the load failure encountered.

Thanks Sohil for the review, will reword as you suggest above

> 
> Modify..
> 
>> Modify this to propagate the actual error code to user space.
>>
> 
> ...
> 
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index d056617ddc85..ebaa1d6a2b18 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -234,7 +234,7 @@ static bool ifs_image_sanity_check(struct device *dev, const struct microcode_he
>>    * Load ifs image. Before loading ifs module, the ifs image must be located
>>    * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
>>    */
>> -void ifs_load_firmware(struct device *dev)
>> +int ifs_load_firmware(struct device *dev)
>>   {
>>       struct ifs_data *ifsd = ifs_get_data(dev);
>>       const struct firmware *fw;
>> @@ -263,4 +263,6 @@ void ifs_load_firmware(struct device *dev)
>>       release_firmware(fw);
>>   done:
>>       ifsd->loaded = (ret == 0);
>> +
>> +    return ret;
>>   }

This change is still needed by the new code, more below

>> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
>> index 37d8380d6fa8..4af4e1bea98d 100644
>> --- a/drivers/platform/x86/intel/ifs/sysfs.c
>> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
>> @@ -94,9 +94,8 @@ static ssize_t reload_store(struct device *dev,
>>                   struct device_attribute *attr,
>>                   const char *buf, size_t count)
>>   {
>> -    struct ifs_data *ifsd = ifs_get_data(dev);
>>       bool res;
>> -
>> +    int rc;
>>   
> 
> Does rc refer to return code? The other IFS functions like above use the commonly used 'ret' variable. Any specific reason for the inconsistency?
> 
> Also, patch 11 completely removes the reload_store() function. Should we avoid a separate patch to fix something that is going to be removed soon? Would re-ordering the patches help in that regard?

You are right that reload is removed subsequently, only the ifs_load_firmware() part above is needed for the new code . Will move the above to a new patch between current patches 11 and 12 (and drop this patch)


Jithu