From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935261AbeCHJMw (ORCPT ); Thu, 8 Mar 2018 04:12:52 -0500 Received: from mail-qk0-f172.google.com ([209.85.220.172]:41038 "EHLO mail-qk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751355AbeCHJMt (ORCPT ); Thu, 8 Mar 2018 04:12:49 -0500 X-Google-Smtp-Source: AG47ELuoanWHOLlk/yLzN5hsMhMYGT1hgBOIwYpiW5U2f+gcimCrUnj6lvCLs5xQLJohJm2wdQjPNrdT+I7ff7d2cbs= MIME-Version: 1.0 In-Reply-To: References: <1520292190-5027-1-git-send-email-sai.praneeth.prakhya@intel.com> <1520292190-5027-3-git-send-email-sai.praneeth.prakhya@intel.com> From: Miguel Ojeda Date: Thu, 8 Mar 2018 10:12:27 +0100 Message-ID: Subject: Re: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services() To: "Prakhya, Sai Praneeth" Cc: "linux-efi@vger.kernel.org" , linux-kernel , "Lee@vger.kernel.org" , Chun-Yi , Borislav Petkov , "Luck, Tony" , Will Deacon , "Hansen, Dave" , Mark Rutland , Bhupesh Sharma , "Neri, Ricardo" , "Shankar, Ravi V" , Matt Fleming , "Zijlstra, Peter" , Ard Biesheuvel , "Williams, Dan J" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 8, 2018 at 5:22 AM, Prakhya, Sai Praneeth wrote: >> > +struct workqueue_struct *efi_rts_wq; >> > + >> > static bool disable_runtime; >> > static int __init setup_noefi(char *arg) { @@ -329,6 +331,19 @@ >> > static int __init efisubsys_init(void) >> > return 0; >> > >> > /* >> > + * Since we process only one efi_runtime_service() at a time, an >> > + * ordered workqueue (which creates only one execution context) >> > + * should suffice all our needs. >> > + */ >> > + efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0); >> >> efi_rts_wq or efi_rts_workqueue? >> >> > + if (!efi_rts_wq) { >> > + pr_err("Failed to create efi_rts_workqueue, EFI runtime services " >> >> Same here. > > Sure! I will make it consistent with "efi_rts_wq". Just tried to be more verbose > with names :) > It is not a big deal, but using the exact same name is better for the purposes of grepping and things like that :-) By the way, check the commit title/message, there are some others there too. > [...] > >> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5) \ >> > +({ \ >> > + struct efi_runtime_work efi_rts_work; \ >> > + \ >> > + INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts); \ >> > + efi_rts_work.func = _rts; \ >> > + efi_rts_work.arg1 = _arg1; \ >> > + efi_rts_work.arg2 = _arg2; \ >> > + efi_rts_work.arg3 = _arg3; \ >> > + efi_rts_work.arg4 = _arg4; \ >> > + efi_rts_work.arg5 = _arg5; \ >> > + /* \ >> > + * queue_work() returns 0 if work was already on queue, \ >> > + * _ideally_ this should never happen. \ >> > + */ \ >> > + if (queue_work(efi_rts_wq, &efi_rts_work.work)) \ >> > + flush_work(&efi_rts_work.work); \ >> > + else \ >> > + BUG(); \ >> >> Thanks for the change! One remark, I would just do: > > Sorry! but I am planning to remove BUG(). Looks like it could defeat the purpose > of patch. Please see Boris comments on the other thread. No problem. Let's see how it looks in v3 :-) > > [...] > >> > +/* >> > + * efi_runtime_work: Details of EFI Runtime Service work >> > + * @func: EFI Runtime Service function identifier >> > + * @arg<1-5>: EFI Runtime Service function arguments >> > + * @status: Status of executing EFI Runtime Service >> > + */ >> > +struct efi_runtime_work { >> > + u8 func; >> > + void *arg1; >> > + void *arg2; >> > + void *arg3; >> > + void *arg4; >> > + void *arg5; >> > + efi_status_t status; >> > + struct work_struct work; >> > +}; >> >> Why is efi_runtime_work in the .h at all? >> > > Thanks for the catch. I will move it to runtime-wrappers.c file and will make it > static too. It isn't being used in any other place. > >> Please CC me for the next version! :-) > > Sure! Sorry for that. I should have done in V2. Thanks! Cheers, Miguel > > Regards, > Sai