Re: [PATCH] Fix effect aborting in ff-memless

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sun, Jun 19, 2016 at 02:44:35PM +0200, Manuel Reimer wrote:
> Hello,
> 
> while debugging a problem with hid-sony I got stuck with a problem
> actually caused by ff-memless. In some situations effect aborting is
> delayed, so it may be triggered seconds after all devices have been
> destroyed, which causes the kernel to panic.
> 
> The aborting request actually gets received here:
> https://github.com/torvalds/linux/blob/master/drivers/input/ff-memless.c#L467
> This "aborting" flag is then handled here:
> https://github.com/torvalds/linux/blob/master/drivers/input/ff-memless.c#L376
> But before this line is reached, there is a time check to check if
> the effect actually is due to be started:
> https://github.com/torvalds/linux/blob/master/drivers/input/ff-memless.c#L359
> 
> This time check now causes a problem if the effect, which is meant
> to be *aborted* was scheduled to be *started* some time in future
> and the device is destroyed before this time is reached.

I am not clear how this can happen. If effect hasn't actually started
playing (i.e. we have FF_EFFECT_STARTED bit set, but FF_EFFECT_PLAYING
is not yet set), then when stopping effect we do not need to do anything
except clear FF_EFFECT_STARTED (since we did not touch the hardware
yet).

Now, if FF_EFFECT_PLAYING is set, that means that play_at time is in
the past and we won't be skipping this effect in ml_get_combo_effect().

Could you please post a stack trace of the crash you observed?

> 
> My patch fixes this by setting the trigger times to "now" after
> setting the aborting flag. This way the following code deals with
> aborting the effect immediately without setting a timer for it.
> 
> There may be other ways to fix this, so I would be happy to get some
> feedback.
> 
> 
> Signed-off-by: Manuel Reimer <mail@m-reimer.de>
> 
> --- a/drivers/input/ff-memless.c	2016-05-13 16:06:29.722685021 +0200
> +++ b/drivers/input/ff-memless.c	2016-06-19 14:25:39.790375270 +0200
> @@ -463,9 +463,11 @@ static int ml_ff_playback(struct input_d
>  	} else {
>  		pr_debug("initiated stop\n");
> 
> -		if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> +		if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
>  			__set_bit(FF_EFFECT_ABORTING, &state->flags);
> -		else
> +			state->play_at = jiffies;
> +			state->adj_at = jiffies;
> +		} else
>  			__clear_bit(FF_EFFECT_STARTED, &state->flags);
>  	}
> 

Thanks.

-- 
Dmitry