Re: [PATCH v2] misc/sgi-xp: Replace in_interrupt() usage

[Steve Wahl]

Reviewed-by: Steve Wahl <steve.wahl@xxxxxxx>

On Thu, Nov 19, 2020 at 11:31:51AM +0100, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Cliff Whickman <cpw@xxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Robin Holt <robinmholt@xxxxxxxxx>
> Cc: Steve Wahl <steve.wahl@xxxxxxx>
> Cc: Dimitri Sivanich <dimitri.sivanich@xxxxxxx>
> Cc: Russ Anderson <russ.anderson@xxxxxxx>
> ---
> 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