| Message ID | 20200730014531.310465-9-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
Hi Gavin, On 30/07/2020 02:45, Gavin Shan wrote: > This removes the unnecessary while loop in sdei_event_register(). Did you notice how this code doesn't need to unwind anything to clean up? It only unlocks the mutex, and the indentation tells you it always does that. > This shouldn't cause any functional changes. Why is it better? This complicates the flow by jumping around with goto. If the unwinding were necessary, I agree this would be clearer, but as its not ... why do we need to make this harder to read? Thanks, James > diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > index 03b1179da9b4..d04329f98922 100644 > --- a/drivers/firmware/arm_sdei.c > +++ b/drivers/firmware/arm_sdei.c > @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) > WARN_ON(in_nmi()); > > mutex_lock(&sdei_events_lock); > - do { > - if (sdei_event_find(event_num)) { > - pr_warn("Event %u already registered\n", event_num); > - err = -EBUSY; > - break; > - } > + if (sdei_event_find(event_num)) { > + pr_warn("Event %u already registered\n", event_num); > + err = -EBUSY; > + goto unlock; > + } > > - event = sdei_event_create(event_num, cb, arg); > - if (IS_ERR(event)) { > - err = PTR_ERR(event); > - pr_warn("Failed to create event %u: %d\n", event_num, > - err); > - break; > - } > + event = sdei_event_create(event_num, cb, arg); > + if (IS_ERR(event)) { > + err = PTR_ERR(event); > + pr_warn("Failed to create event %u: %d\n", event_num, err); > + goto unlock; > + } > > - cpus_read_lock(); > - err = _sdei_event_register(event); > - if (err) { > - sdei_event_destroy(event); > - pr_warn("Failed to register event %u: %d\n", event_num, > - err); > - } else { > - spin_lock(&sdei_list_lock); > - event->reregister = true; > - spin_unlock(&sdei_list_lock); > - } > - cpus_read_unlock(); > - } while (0); > - mutex_unlock(&sdei_events_lock); > + cpus_read_lock(); > + err = _sdei_event_register(event); > + if (err) { > + sdei_event_destroy(event); > + pr_warn("Failed to register event %u: %d\n", event_num, err); > + goto cpu_unlock; > + } > > + spin_lock(&sdei_list_lock); > + event->reregister = true; > + spin_unlock(&sdei_list_lock); > +cpu_unlock: > + cpus_read_unlock(); > +unlock: > + mutex_unlock(&sdei_events_lock); > return err; > } > >
Hi James, On 9/19/20 2:13 AM, James Morse wrote: > On 30/07/2020 02:45, Gavin Shan wrote: >> This removes the unnecessary while loop in sdei_event_register(). > > Did you notice how this code doesn't need to unwind anything to clean up? > It only unlocks the mutex, and the indentation tells you it always does that. > Yes, I noticed it. > >> This shouldn't cause any functional changes. > > Why is it better? This complicates the flow by jumping around with goto. > > If the unwinding were necessary, I agree this would be clearer, but as its not ... why do > we need to make this harder to read? > I intended to avoid the unnecessary nested statements, caused by the "do { ... } while (0)". With that, the code looks a bit cleaner. It might make the code a bit harder to read, but it's fine to me. Besides, the unnecessary "do { ... } while (0)" looks a bit strange to me because the block is executed for once. So I think it'd better to keep the changes if you don't object strongly. Otherwise, I can drop this one. Cheers, Gavin > >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >> index 03b1179da9b4..d04329f98922 100644 >> --- a/drivers/firmware/arm_sdei.c >> +++ b/drivers/firmware/arm_sdei.c >> @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) >> WARN_ON(in_nmi()); >> >> mutex_lock(&sdei_events_lock); >> - do { >> - if (sdei_event_find(event_num)) { >> - pr_warn("Event %u already registered\n", event_num); >> - err = -EBUSY; >> - break; >> - } >> + if (sdei_event_find(event_num)) { >> + pr_warn("Event %u already registered\n", event_num); >> + err = -EBUSY; >> + goto unlock; >> + } >> >> - event = sdei_event_create(event_num, cb, arg); >> - if (IS_ERR(event)) { >> - err = PTR_ERR(event); >> - pr_warn("Failed to create event %u: %d\n", event_num, >> - err); >> - break; >> - } >> + event = sdei_event_create(event_num, cb, arg); >> + if (IS_ERR(event)) { >> + err = PTR_ERR(event); >> + pr_warn("Failed to create event %u: %d\n", event_num, err); >> + goto unlock; >> + } >> >> - cpus_read_lock(); >> - err = _sdei_event_register(event); >> - if (err) { >> - sdei_event_destroy(event); >> - pr_warn("Failed to register event %u: %d\n", event_num, >> - err); >> - } else { >> - spin_lock(&sdei_list_lock); >> - event->reregister = true; >> - spin_unlock(&sdei_list_lock); >> - } >> - cpus_read_unlock(); >> - } while (0); >> - mutex_unlock(&sdei_events_lock); >> + cpus_read_lock(); >> + err = _sdei_event_register(event); >> + if (err) { >> + sdei_event_destroy(event); >> + pr_warn("Failed to register event %u: %d\n", event_num, err); >> + goto cpu_unlock; >> + } >> >> + spin_lock(&sdei_list_lock); >> + event->reregister = true; >> + spin_unlock(&sdei_list_lock); >> +cpu_unlock: >> + cpus_read_unlock(); >> +unlock: >> + mutex_unlock(&sdei_events_lock); >> return err; >> } >> >> >
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index 03b1179da9b4..d04329f98922 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) WARN_ON(in_nmi()); mutex_lock(&sdei_events_lock); - do { - if (sdei_event_find(event_num)) { - pr_warn("Event %u already registered\n", event_num); - err = -EBUSY; - break; - } + if (sdei_event_find(event_num)) { + pr_warn("Event %u already registered\n", event_num); + err = -EBUSY; + goto unlock; + } - event = sdei_event_create(event_num, cb, arg); - if (IS_ERR(event)) { - err = PTR_ERR(event); - pr_warn("Failed to create event %u: %d\n", event_num, - err); - break; - } + event = sdei_event_create(event_num, cb, arg); + if (IS_ERR(event)) { + err = PTR_ERR(event); + pr_warn("Failed to create event %u: %d\n", event_num, err); + goto unlock; + } - cpus_read_lock(); - err = _sdei_event_register(event); - if (err) { - sdei_event_destroy(event); - pr_warn("Failed to register event %u: %d\n", event_num, - err); - } else { - spin_lock(&sdei_list_lock); - event->reregister = true; - spin_unlock(&sdei_list_lock); - } - cpus_read_unlock(); - } while (0); - mutex_unlock(&sdei_events_lock); + cpus_read_lock(); + err = _sdei_event_register(event); + if (err) { + sdei_event_destroy(event); + pr_warn("Failed to register event %u: %d\n", event_num, err); + goto cpu_unlock; + } + spin_lock(&sdei_list_lock); + event->reregister = true; + spin_unlock(&sdei_list_lock); +cpu_unlock: + cpus_read_unlock(); +unlock: + mutex_unlock(&sdei_events_lock); return err; }