RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()

From: "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Chun-Yi Lee <jlee@suse.com>, "Luck, Tony" <tony.luck@intel.com>,
	Will Deacon <will.deacon@arm.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Bhupesh Sharma <bhsharma@redhat.com>,
	"Neri, Ricardo" <ricardo.neri@intel.com>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
	"Matt Fleming" <matt@codeblueprint.co.uk>,
	"Zijlstra, Peter" <peter.zijlstra@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Subject: RE: [PATCH V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
Date: Thu, 8 Mar 2018 05:31:03 +0000	[thread overview]
Message-ID: <FFF73D592F13FD46B8700F0A279B802F2E5818E4@ORSMSX114.amr.corp.intel.com> (raw)
In-Reply-To: <20180307121047.GG23662@pd.tnic>

> > +({									\
> > +	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();							\
> 
> So failure to queue that work is such a critical problem that we need to BUG()
> and can't possibly continue and shoult not attempt recovery at all?
> 

I think it's not critical, we can just return error status.
I think the problem in itself is not at all critical but when I initially thought about
why the problem could have occurred, it sounded like one i.e. ideally (if the system
is running fine) we should always be able to queue work. Failure to queue means
that the previous work is already on queue and that shouldn't be the case.
So, thought, maybe something bad had happened already (just doubtful).

But, I see your point. BUG() sounds more like an over kill. Instead of fixing an existing
problem, this patch could completely take down the system.

> IOW, we should always strive to fail gracefully and not shit in pants at the first
> sign of trouble.
>

Yes, that makes sense. I will remove BUG() in V3 (in the two places that I introduced).

> Even checkpatch warns here:
> 
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code
> rather than BUG() or BUG_ON()
> #184: FILE: drivers/firmware/efi/runtime-wrappers.c:92:
> +               BUG();                                                  \
>

Sure! I will fix this

> 
> and by looking at the other output, you should run your patches through
> checkpatch. Some of the things make sense like:
> 
> WARNING: quoted string split across lines
> #97: FILE: drivers/firmware/efi/efi.c:341:
> +               pr_err("Failed to create efi_rts_workqueue, EFI runtime services "
> +                      "disabled.\n");
> 
> for example.
> 

I will fix this one too.

Another warning by checkpatch is "use of in_atomic() in drivers code"
Do you think it's OK to check if were are "in_atomic()" in drivers code.
I wasn't able to decide on other alternative, if possible, could you please suggest one?

Regards,
Sai