| Message ID | 20200706054732.99387-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
Hi Gavin, On 06/07/2020 06:47, Gavin Shan wrote: > The SDEI parameter is dereferenced by @sdei_event->registered or > @sdei_event->private_registered, depending on the event type. > They > can be dereferenced directly so that the intermediate variable > (@regs) can be removed. Unless its there to make it obvious that the shared/private blocks of this are doing the same work. > It makes the code looks a bit simplified. I disagree. > Also, @event->event_num instead of @event_num is used for retrieving > the event number for the shared event, which makes the code similar > to the case of private event. Sure, this is an an inconsistency that could be changed. But I don't think its worth the effort! > Besides, the local scoped variables > are reordered according to their importance by the way. Please: never do this. Thanks, James
Hi James, On 7/22/20 6:41 AM, James Morse wrote: > On 06/07/2020 06:47, Gavin Shan wrote: >> The SDEI parameter is dereferenced by @sdei_event->registered or >> @sdei_event->private_registered, depending on the event type. > >> They >> can be dereferenced directly so that the intermediate variable >> (@regs) can be removed. > > Unless its there to make it obvious that the shared/private blocks of this are doing the > same work. > > >> It makes the code looks a bit simplified. > > I disagree. > Well, @regs can be removed and the scope of @reg is narrowed. However, it might be not worthy for the effort. I will drop this part of changes in v2. > >> Also, @event->event_num instead of @event_num is used for retrieving >> the event number for the shared event, which makes the code similar >> to the case of private event. > > Sure, this is an an inconsistency that could be changed. But I don't think its worth the > effort! > Yes, but I think it's still worthy to make the code consistent. Lets keep this change in v2 only. All other changes will be dropped. > >> Besides, the local scoped variables >> are reordered according to their importance by the way. > > Please: never do this. > Sure. Thanks, Gavin
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index a75212a743d3..35a319e7e1e6 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -211,38 +211,35 @@ static struct sdei_event *sdei_event_create(u32 event_num, event->type = result; if (event->type == SDEI_EVENT_TYPE_SHARED) { - reg = kzalloc(sizeof(*reg), GFP_KERNEL); - if (!reg) { + event->registered = kzalloc(sizeof(*reg), GFP_KERNEL); + if (!event->registered) { err = -ENOMEM; goto fail; } - reg->event_num = event_num; - reg->priority = event->priority; - - reg->callback = cb; - reg->callback_arg = cb_arg; - event->registered = reg; + event->registered->event_num = event->event_num; + event->registered->priority = event->priority; + event->registered->callback = cb; + event->registered->callback_arg = cb_arg; } else { int cpu; - struct sdei_registered_event __percpu *regs; + struct sdei_registered_event *reg; - regs = alloc_percpu(struct sdei_registered_event); - if (!regs) { + event->private_registered = + alloc_percpu(struct sdei_registered_event); + if (!event->private_registered) { err = -ENOMEM; goto fail; } for_each_possible_cpu(cpu) { - reg = per_cpu_ptr(regs, cpu); + reg = per_cpu_ptr(event->private_registered, cpu); reg->event_num = event->event_num; reg->priority = event->priority; reg->callback = cb; reg->callback_arg = cb_arg; } - - event->private_registered = regs; } spin_lock(&sdei_list_lock);
The SDEI parameter is dereferenced by @sdei_event->registered or @sdei_event->private_registered, depending on the event type. They can be dereferenced directly so that the intermediate variable (@regs) can be removed. It makes the code looks a bit simplified. Also, @event->event_num instead of @event_num is used for retrieving the event number for the shared event, which makes the code similar to the case of private event. Besides, the local scoped variables are reordered according to their importance by the way. Signed-off-by: Gavin Shan <gshan@redhat.com> --- drivers/firmware/arm_sdei.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)