[03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly
diff mbox series

Message ID 20200706054732.99387-4-gshan@redhat.com
State New
Headers show
Series
  • Refactor SDEI client driver
Related show

Commit Message

Gavin Shan July 6, 2020, 5:47 a.m. UTC
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(-)

Comments

James Morse July 21, 2020, 8:41 p.m. UTC | #1
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
Gavin Shan July 22, 2020, 2:38 a.m. UTC | #2
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

Patch
diff mbox series

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);