Re: [PATCH 06/10] mei: vsc: Event notifier fixes

[Hans de Goede <hansg@kernel.org>]

Hi,

On 25-Jun-25 11:12 AM, Usyskin, Alexander wrote:
>> Subject: [PATCH 06/10] mei: vsc: Event notifier fixes
>>
>> vsc_tp_register_event_cb() can race with vsc_tp_thread_isr(), add a mutex
>> to protect against this.
>>
>> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/misc/mei/vsc-tp.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
>> index 0feebffabdb3..76a6aa606a26 100644
>> --- a/drivers/misc/mei/vsc-tp.c
>> +++ b/drivers/misc/mei/vsc-tp.c
>> @@ -79,9 +79,8 @@ struct vsc_tp {
>>
>>  	vsc_tp_event_cb_t event_notify;
>>  	void *event_notify_context;
>> -
>> -	/* used to protect command download */
>> -	struct mutex mutex;
>> +	struct mutex event_notify_mutex;	/* protects event_notify +
>> context */
>> +	struct mutex mutex;			/* protects command
>> download */
>>  };
>>
>>  /* GPIO resources */
>> @@ -113,6 +112,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
>> *data)
>>  {
>>  	struct vsc_tp *tp = data;
>>
> 
> The mutex overhead looks out of place here in the interrupt handler.
> Maybe it can be replaced with something lighter?

Using mutexes in *threaded* isr handlers is quite normal, e.g.
both the SPI core (used as transport here) and the I2C cor will
take + release a mutex for each data transfer over the bus and
a threaded ISR handler may do more then 1 data transfer for a single
interrupt.

As to using something lighter I could not come up with any lighter
solution then this.

Also note that this is moved to a workqueue later in the series,
since the threaded ISR actually waits for the wake_up() call
done by the hard part of the ISR and an ISR waiting for
the interrupt to trigger a second/third/... time inside the ISR
handler is just plain wrong.

> BTW is it possible to have interrupt before call to vsc_tp_register_event_c

The interrupt gets triggered by a GPIO connected to the VSC,
so if the VSC is well behaved then the interrupt should not
trigger. But we cannot really count on that.

Regards,

Hans



>> +	guard(mutex)(&tp->event_notify_mutex);
>> +
>>  	if (tp->event_notify)
>>  		tp->event_notify(tp->event_notify_context);
>>
>> @@ -399,6 +400,8 @@ EXPORT_SYMBOL_NS_GPL(vsc_tp_need_read,
>> "VSC_TP");
>>  int vsc_tp_register_event_cb(struct vsc_tp *tp, vsc_tp_event_cb_t event_cb,
>>  			    void *context)
>>  {
>> +	guard(mutex)(&tp->event_notify_mutex);
>> +
>>  	tp->event_notify = event_cb;
>>  	tp->event_notify_context = context;
>>
>> @@ -499,6 +502,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>  		return ret;
>>
>>  	mutex_init(&tp->mutex);
>> +	mutex_init(&tp->event_notify_mutex);
>>
>>  	/* only one child acpi device */
>>  	ret = acpi_dev_for_each_child(ACPI_COMPANION(dev),
>> @@ -523,6 +527,7 @@ static int vsc_tp_probe(struct spi_device *spi)
>>  err_destroy_lock:
>>  	free_irq(spi->irq, tp);
>>
>> +	mutex_destroy(&tp->event_notify_mutex);
>>  	mutex_destroy(&tp->mutex);
>>
>>  	return ret;
>> @@ -537,6 +542,7 @@ static void vsc_tp_remove(struct spi_device *spi)
>>
>>  	free_irq(spi->irq, tp);
>>
>> +	mutex_destroy(&tp->event_notify_mutex);
>>  	mutex_destroy(&tp->mutex);
>>  }
>>
>> --
>> 2.49.0
>