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