Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <03e8a254-51ff-9eb9-78a5-10ac76c3b3da@intel.com>
Date:   Thu, 14 Apr 2022 16:41:54 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [RESEND PATCH v1 2/8] firmware_loader: Check fw_state_is_done in
 loading_store
Content-Language: en-US
To:     Tom Rix <trix@redhat.com>, <mcgrof@kernel.org>,
        <gregkh@linuxfoundation.org>, <rafael@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     <lgoncalv@redhat.com>, <yilun.xu@intel.com>, <hao.wu@intel.com>,
        <matthew.gerlach@intel.com>, <basheer.ahmed.muddebihal@intel.com>,
        <tianfei.zhang@intel.com>
References: <20220323233331.155121-1-russell.h.weight@intel.com>
 <20220323233331.155121-3-russell.h.weight@intel.com>
 <0dc2d770-176f-9c58-e875-cb65ff38509c@redhat.com>
From:   Russ Weight <russell.h.weight@intel.com>
In-Reply-To: <0dc2d770-176f-9c58-e875-cb65ff38509c@redhat.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?UzB1cnJZTjVmREpZRTFwWUFRU25jQ2ZXeGVSUTJuclpxWUQ1MWtqekRyZWdF?=
 =?utf-8?B?M0NBMUJGajhTZ1l2VEhESGt0RDlLS3lSeVFLUEdxdGRLcTFVVXJ3N1lJK3p2?=
 =?utf-8?B?b3pRQm84NG4zdjRlOGNDUHlvWm00eGZNRGtVMTlWUjhwMzV3UGdubHlDT2FD?=
 =?utf-8?B?VHNqbmtDQUJQMmJBczdrV1FmZXlTWWVQdG9pZ1ExZElpWW5FbXZkdllBZ3Fv?=
 =?utf-8?B?UDVRQWl5NU5VcVdiQXlHbTdQQ1R0by80ZnFSQzBOaW1MSFVQUUZmT3RNalVs?=
 =?utf-8?B?eE0wbzVaYXJyYnZUYWpPeVp4TS9LUEEvUzhrd1BJU1lWVzBPdVVvaE1xZEU1?=
 =?utf-8?B?dzJ0ejNIRG5pM3Zzay9zaE5pV0V3OTh1a25mb0hUU2hhUllhQSt1M2dJWmNU?=
 =?utf-8?B?RWtYSHdYYjg0RXRzSUtoRGhEd055RkozaGkwQkY5UkNNUTZLUkVKWXZHL1k2?=
 =?utf-8?B?R1JUWWZ1eWQ1MWhjZE9BdWdVOERnYk5iZ0U5TE9STlZ3ejZFQUROREJvTksr?=
 =?utf-8?B?emNRSEM3TmJsTkJkblYwalo1bVdBUzhWb3lkZ0dYUG12cmFhYzM1Y1FRSFAz?=
 =?utf-8?B?OUVkUmtpOVR3L05BQzIvN252eGxCaWNNVGFIdGRyTnBJclNaSDJMNzlUS2gy?=
 =?utf-8?B?cmgwQ0VsMldtMGhZUjl6N0ZQZEhqL2RlZmNOMXR0dWdqbTlOb2l5WTFKZ2lG?=
 =?utf-8?B?cWFzQVcyYmRUbmd6eWZTSks2SGlaYS81UFliVzZUTUloNm8rWk9QK0hPSmFD?=
 =?utf-8?B?dTZQRXpUUmZVZENMY29VMm83NWk2b3VxQnZBa0ptKzRkMjRZWlR2WWRWK2du?=
 =?utf-8?B?QXdmM2ozeUYzU2h1Ui83R1NmRWo3Ty9aalFDaFpJaytyMFNvRndyeWxXWWtP?=
 =?utf-8?B?SEd1Y3NybUtoOXFSaG12SW9qT2J0N2lHT210ZXlVS214L0d5TnVaUVZzUkRN?=
 =?utf-8?B?RTJUNksxcU5RME1RN3lGN3hOTk1hellxWEI0RkdPVElRZFF2UEVpYU5uQURC?=
 =?utf-8?B?cVU2NEVKUkhLZmRYVmNUMHdRNWw2NTdWSzkyckV4SE9iNG9qSjMwV3R6OFZt?=
 =?utf-8?B?aWNYeUVCSi9sTUxNQmRlUjZKS0VGS1Zob2VGYjVSTTc4cVVNdXRPUEh4UDk1?=
 =?utf-8?B?djBRZllCR1pReUlZcUNFSFFyK0ZrbDN5dGtIbnp5dFB6ZDdLQ1pzMnRiZmxr?=
 =?utf-8?B?RkdobDVUTlhtVU9FL1gvd3VHYVNTQm9pQ2NUdDFUaHcvTVd2RHhUNU5JZFZu?=
 =?utf-8?B?eFg5MlVTUXVJbitYZUZvem9ndC9mYis5aHpDa3NOMnVQMnk1Q1RlVjNOd1JV?=
 =?utf-8?B?SE5uTVBtdmNUMlV3U3BmWERZdTBVTjVWMHQ2Y0tqZS9xMVdKeUdXNkJ5c01v?=
 =?utf-8?B?a1RxMVhLUGR1WmhQa1N2UDU1a0J1OUE5eGR6U2daWTJHSVFCTm5nTUlKaDFL?=
 =?utf-8?B?cWU1dHhCV01VcS9UalNrUW5mZ1JBQm5FL24vMFgzdTNsZEFaRjRaRnpHN1h2?=
 =?utf-8?B?YTB2RlErdFJEcFVzQUpuSGpWK0VwREN5aEVPaE5PYmRRb3lwekJRejNIc0RW?=
 =?utf-8?B?bUxSckVLSXgwaWZFSXFNcG5vZ2pQMS9pOStqZ0xnQjR2VUJHR05CaUVNVGhP?=
 =?utf-8?B?bERoK0NmMzVHYnE2cmlpRmk3akJaYTgrL21jZHA5MGtXWC92RWJkSGQrOGs2?=
 =?utf-8?B?VCt2VHZiTEU4clBZejR1Q1p3aWx1ZSthSDJPcC9tYnA0OUtMNXFiNmtQdWtP?=
 =?utf-8?B?c2xGSmp6YUhvWDVXMGQvMFNZejczTFhzcHBGUEYzOFBCaDgrZG81QnFNTW45?=
 =?utf-8?B?Y1kwUFV2aERZQ1FLWDl1VWwvZzhiMGRwZDRBUzhXVEd2clFpSCszdTlTTkky?=
 =?utf-8?B?NER6NnNLU0hLalQvVDRlcEtzbjFXTVpmbWErL2dLRXVkYWcxMU01T3dRejFJ?=
 =?utf-8?B?ZlluNm1LYUNUU1BqbzdDMksrYThqV1RCZUYvcU43RVVWbTRGZzVFTWg2ZEln?=
 =?utf-8?B?RElqekgvaDhwRS94aHFKNjBjWGdCQVdPMmJDTGU4VElUUXYvenVCbnUwakF0?=
 =?utf-8?B?TWVVMk8rcytzclV3bXhKYk9ENS9yNUJ6RUYzejZ4ZVBqZXQ5TlJ4QXQyZ0Mv?=
 =?utf-8?B?VXNTZnRDOTc3UVFrdVh5dUJiZGtlSWlDUk4vSExFcUQ1ZkRHZlVualJMZHhQ?=
 =?utf-8?B?c1dFZDlHdWNrRkxQY0JiN0hQaGVHOUJCcFBPQWQ5NWJVV21aVnFISkxQMUtE?=
 =?utf-8?B?VmY1VTVzeGx0emorNWR2QmErT2VYN1VqVy8ybC9oMmRJbWM3c2gzbCtyVzhj?=
 =?utf-8?B?WjZZYWpRK0RvaFRoQ3NTZWV1RlNUYk01bW1mS08xZVg4aGVzRHBLS2RJbXNW?=
 =?utf-8?Q?aEPS5Z1VBPS5sIkg=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 642b651d-4447-4178-30f4-08da1e705d86
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1899.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 23:41:58.3248
 (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: bTv0rIBRS7Thgp5irCw7ADaMCs8m0Z4kVVDloZ5sUIdLHzscBgObphMi5lx1o0G5aqHquynAtgB3OAdnjgsz7F5PbqFW5Hfny20xvm2usxY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5439
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 4/2/22 07:47, Tom Rix wrote:
>
> On 3/23/22 4:33 PM, Russ Weight wrote:
>> Add the fw_state_is_done() function and exit early from
>> firmware_loading_store() if the state is already "done". This is being done
>> in preparation for supporting persistent sysfs nodes to allow userspace to
>> upload firmware to a device, potentially reusing the sysfs loading and data
>> files multiple times.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v1:
>>    - No change from RFC patch
>> ---
>>   drivers/base/firmware_loader/fallback.c | 2 +-
>>   drivers/base/firmware_loader/firmware.h | 5 +++++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
>> index 4afb0e9312c0..d82e055a4297 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -250,7 +250,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>>         mutex_lock(&fw_lock);
>>       fw_priv = fw_sysfs->fw_priv;
>> -    if (fw_state_is_aborted(fw_priv))
>> +    if (fw_state_is_aborted(fw_priv) || fw_state_is_done(fw_priv))
>>           goto out;
>>         switch (loading) {
>> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
>> index 2889f446ad41..58768d16f8df 100644
>> --- a/drivers/base/firmware_loader/firmware.h
>> +++ b/drivers/base/firmware_loader/firmware.h
>> @@ -149,6 +149,11 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
>>       __fw_state_set(fw_priv, FW_STATUS_DONE);
>>   }
>>   +static inline bool fw_state_is_done(struct fw_priv *fw_priv)
>> +{
>> +    return __fw_state_check(fw_priv, FW_STATUS_DONE);
>
> This looks like the fw_sysfs_done() in fallback.c
>
> IMO this one and similar wrappers should move to firmware.h and use the *_is_* naming.

Thanks for catching that Tom. Yes, the new fw_state_is_done() function is exactly
the same as fw_sysfs_done(). I'll follow your suggestion and move fw_sysfs_done()
and fw_sysfs_loading() to firmware.h as fw_state_is_done() and fw_state_is_loading().

- Russ
>
> Tom
>
>> +}
>> +
>>   int assign_fw(struct firmware *fw, struct device *device);
>>     #ifdef CONFIG_FW_LOADER
>