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

[Prakhya, Sai Praneeth]

+Cc Miguel Ojeda

> > > +({									\
> > > +	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