Messages in this thread | | | Date | Thu, 19 Nov 2020 10:11:37 -0600 | From | Steve Wahl <> | Subject | Re: [PATCH v2] misc/sgi-xp: Replace in_interrupt() usage |
| |
Reviewed-by: Steve Wahl <steve.wahl@hpe.com>
On Thu, Nov 19, 2020 at 11:31:51AM +0100, Sebastian Andrzej Siewior wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > The usage of in_interrupt() in xpc_partition_disengaged() is clearly > intended to avoid canceling the timeout timer when the function is invoked > from the timer callback. > > While in_interrupt() is deprecated and ill defined as it does not provide > what the name suggests it catches the intended case. > > Add an argument to xpc_partition_disengaged() which is true if called > from timer and otherwise false. > Use del_timer_sync() instead of del_singleshot_timer_sync() which is the > same thing. > > Note: This does not prevent reentrancy into the function as the function > has no concurrency control and timer callback and regular task context > callers can happen concurrently on different CPUs or the timer can > interrupt the task context before it is able to cancel it. > > While the only driver which is providing the arch_xpc_ops callbacks > (xpc_uv) seems not to have a reentrancy problem and the only negative > effect would be a double dev_info() entry in dmesg, the whole mechanism is > conceptually broken. > > But that's not subject of this cleanup endeavour and left as an exercise to > the folks who might have interest to make that code fully correct. > > [bigeasy: Add the argument, use del_timer_sync().] > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Cliff Whickman <cpw@sgi.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Robin Holt <robinmholt@gmail.com> > Cc: Steve Wahl <steve.wahl@hpe.com> > Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com> > Cc: Russ Anderson <russ.anderson@hpe.com> > --- > v1…v2: Use xpc_partition_disengaged_from_timer() and don't export the > argument. Suggested by Greg. > > drivers/misc/sgi-xp/xpc.h | 1 + > drivers/misc/sgi-xp/xpc_main.c | 2 +- > drivers/misc/sgi-xp/xpc_partition.c | 20 +++++++++++++++----- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/misc/sgi-xp/xpc.h b/drivers/misc/sgi-xp/xpc.h > index 71db60edff655..225f2bb84e39b 100644 > --- a/drivers/misc/sgi-xp/xpc.h > +++ b/drivers/misc/sgi-xp/xpc.h > @@ -634,6 +634,7 @@ extern int xpc_setup_rsvd_page(void); > extern void xpc_teardown_rsvd_page(void); > extern int xpc_identify_activate_IRQ_sender(void); > extern int xpc_partition_disengaged(struct xpc_partition *); > +extern int xpc_partition_disengaged_from_timer(struct xpc_partition *part); > extern enum xp_retval xpc_mark_partition_active(struct xpc_partition *); > extern void xpc_mark_partition_inactive(struct xpc_partition *); > extern void xpc_discovery(void); > diff --git a/drivers/misc/sgi-xp/xpc_main.c b/drivers/misc/sgi-xp/xpc_main.c > index e5244fc1dab30..84610bbcc1314 100644 > --- a/drivers/misc/sgi-xp/xpc_main.c > +++ b/drivers/misc/sgi-xp/xpc_main.c > @@ -179,7 +179,7 @@ xpc_timeout_partition_disengage(struct timer_list *t) > > DBUG_ON(time_is_after_jiffies(part->disengage_timeout)); > > - (void)xpc_partition_disengaged(part); > + xpc_partition_disengaged_from_timer(part); > > DBUG_ON(part->disengage_timeout != 0); > DBUG_ON(xpc_arch_ops.partition_engaged(XPC_PARTID(part))); > diff --git a/drivers/misc/sgi-xp/xpc_partition.c b/drivers/misc/sgi-xp/xpc_partition.c > index 57df06820bae2..1999d02923dee 100644 > --- a/drivers/misc/sgi-xp/xpc_partition.c > +++ b/drivers/misc/sgi-xp/xpc_partition.c > @@ -262,8 +262,8 @@ xpc_get_remote_rp(int nasid, unsigned long *discovered_nasids, > * from us. Though we requested the remote partition to deactivate with regard > * to us, we really only need to wait for the other side to disengage from us. > */ > -int > -xpc_partition_disengaged(struct xpc_partition *part) > +static int __xpc_partition_disengaged(struct xpc_partition *part, > + bool from_timer) > { > short partid = XPC_PARTID(part); > int disengaged; > @@ -289,9 +289,9 @@ xpc_partition_disengaged(struct xpc_partition *part) > } > part->disengage_timeout = 0; > > - /* cancel the timer function, provided it's not us */ > - if (!in_interrupt()) > - del_singleshot_timer_sync(&part->disengage_timer); > + /* Cancel the timer function if not called from it */ > + if (!from_timer) > + del_timer_sync(&part->disengage_timer); > > DBUG_ON(part->act_state != XPC_P_AS_DEACTIVATING && > part->act_state != XPC_P_AS_INACTIVE); > @@ -303,6 +303,16 @@ xpc_partition_disengaged(struct xpc_partition *part) > return disengaged; > } > > +int xpc_partition_disengaged(struct xpc_partition *part) > +{ > + return __xpc_partition_disengaged(part, false); > +} > + > +int xpc_partition_disengaged_from_timer(struct xpc_partition *part) > +{ > + return __xpc_partition_disengaged(part, true); > +} > + > /* > * Mark specified partition as active. > */ > -- > 2.29.2 >
-- Steve Wahl, Hewlett Packard Enterprise
| |