+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
N�����r��y����b�X��ǧv�^�)޺{.n�+����{�y����^n�r���z���h�����&���G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�

Reply via email to