Reviewed-by: Steve Wahl <steve.w...@hpe.com>
On Thu, Nov 19, 2020 at 11:31:51AM +0100, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <t...@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 <t...@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: Cliff Whickman <c...@sgi.com>
> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Robin Holt <robinmh...@gmail.com>
> Cc: Steve Wahl <steve.w...@hpe.com>
> Cc: Dimitri Sivanich <dimitri.sivan...@hpe.com>
> Cc: Russ Anderson <russ.ander...@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
