| Message ID | 20200722095740.28560-10-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
On Wed, 22 Jul 2020 19:57:32 +1000 Gavin Shan <gshan@redhat.com> wrote: > This removes the unnecessary while loop in sdei_event_unregister(). > This shouldn't cause any functional changes. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop" > --- > drivers/firmware/arm_sdei.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > index ab1088fbdd37..d03371161aaf 100644 > --- a/drivers/firmware/arm_sdei.c > +++ b/drivers/firmware/arm_sdei.c > @@ -491,26 +491,24 @@ int sdei_event_unregister(u32 event_num) > > mutex_lock(&sdei_events_lock); > event = sdei_event_find(event_num); > - do { > - if (!event) { > - pr_warn("Event %u not registered\n", event_num); > - err = -ENOENT; > - break; > - } > + if (!event) { > + pr_warn("Event %u not registered\n", event_num); > + err = -ENOENT; > + goto out; > + } > > - spin_lock(&sdei_list_lock); > - event->reregister = false; > - event->reenable = false; > - spin_unlock(&sdei_list_lock); > + spin_lock(&sdei_list_lock); > + event->reregister = false; > + event->reenable = false; > + spin_unlock(&sdei_list_lock); > > - err = _sdei_event_unregister(event); > - if (err) > - break; > + err = _sdei_event_unregister(event); > + if (err) > + goto out; > > - sdei_event_destroy(event); > - } while (0); > + sdei_event_destroy(event); > +out: As in previous I'd give the label a name to indicate there is still work to be done. > mutex_unlock(&sdei_events_lock); > - That whitespace is arguably slightly useful from a readability point of view. I would leave it here. > return err; > } >
Hi Jonathan, On 7/24/20 1:51 AM, Jonathan Cameron wrote: > On Wed, 22 Jul 2020 19:57:32 +1000 > Gavin Shan <gshan@redhat.com> wrote: > >> This removes the unnecessary while loop in sdei_event_unregister(). >> This shouldn't cause any functional changes. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop" >> --- >> drivers/firmware/arm_sdei.c | 30 ++++++++++++++---------------- >> 1 file changed, 14 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >> index ab1088fbdd37..d03371161aaf 100644 >> --- a/drivers/firmware/arm_sdei.c >> +++ b/drivers/firmware/arm_sdei.c >> @@ -491,26 +491,24 @@ int sdei_event_unregister(u32 event_num) >> >> mutex_lock(&sdei_events_lock); >> event = sdei_event_find(event_num); >> - do { >> - if (!event) { >> - pr_warn("Event %u not registered\n", event_num); >> - err = -ENOENT; >> - break; >> - } >> + if (!event) { >> + pr_warn("Event %u not registered\n", event_num); >> + err = -ENOENT; >> + goto out; >> + } >> >> - spin_lock(&sdei_list_lock); >> - event->reregister = false; >> - event->reenable = false; >> - spin_unlock(&sdei_list_lock); >> + spin_lock(&sdei_list_lock); >> + event->reregister = false; >> + event->reenable = false; >> + spin_unlock(&sdei_list_lock); >> >> - err = _sdei_event_unregister(event); >> - if (err) >> - break; >> + err = _sdei_event_unregister(event); >> + if (err) >> + goto out; >> >> - sdei_event_destroy(event); >> - } while (0); >> + sdei_event_destroy(event); >> +out: > As in previous I'd give the label a name to indicate there is still work > to be done. > Sure, I will rename this to "unlock" in v3. However, I would hold v3 for a while until v2 is fully reviewed. Please take a look on the left patches if you have more time, thanks for your time on this series. >> mutex_unlock(&sdei_events_lock); >> - > That whitespace is arguably slightly useful from a readability point of view. > I would leave it here. > Ok. I will keep this in v3 then. >> return err; >> } >> Thanks, Gavin
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c index ab1088fbdd37..d03371161aaf 100644 --- a/drivers/firmware/arm_sdei.c +++ b/drivers/firmware/arm_sdei.c @@ -491,26 +491,24 @@ int sdei_event_unregister(u32 event_num) mutex_lock(&sdei_events_lock); event = sdei_event_find(event_num); - do { - if (!event) { - pr_warn("Event %u not registered\n", event_num); - err = -ENOENT; - break; - } + if (!event) { + pr_warn("Event %u not registered\n", event_num); + err = -ENOENT; + goto out; + } - spin_lock(&sdei_list_lock); - event->reregister = false; - event->reenable = false; - spin_unlock(&sdei_list_lock); + spin_lock(&sdei_list_lock); + event->reregister = false; + event->reenable = false; + spin_unlock(&sdei_list_lock); - err = _sdei_event_unregister(event); - if (err) - break; + err = _sdei_event_unregister(event); + if (err) + goto out; - sdei_event_destroy(event); - } while (0); + sdei_event_destroy(event); +out: mutex_unlock(&sdei_events_lock); - return err; }
This removes the unnecessary while loop in sdei_event_unregister(). This shouldn't cause any functional changes. Signed-off-by: Gavin Shan <gshan@redhat.com> --- v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop" --- drivers/firmware/arm_sdei.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)