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;
 
+	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

> 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?

BTW is it possible to have interrupt before call to vsc_tp_register_event_cb?

- - 
Thanks,
Sasha

> +	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

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
>

On 25-Jun-25 11:23 AM, Hans de Goede wrote:
> 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.

p.s.

I forgot to mention that this interrupt also does not trigger
that frequently that we really need to worry about overhead.

The VSC sits between the user-facing camera and the Intel
CPU/SoC's CSI2 receiver. So we only need to talk to it
(generating interrupts) on probe() and when starting/stopping
streaming video from the camera. Each start/stop we'll get
a bunch of interrupts but outside of that the interrupt never
triggers. So overhead is not really a big worry here.

Regards,

Hans





> 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
>>
>

> Subject: Re: [PATCH 06/10] mei: vsc: Event notifier fixes
> 
> On 25-Jun-25 11:23 AM, Hans de Goede wrote:
> > 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.
> 
> p.s.
> 
> I forgot to mention that this interrupt also does not trigger
> that frequently that we really need to worry about overhead.
> 
> The VSC sits between the user-facing camera and the Intel
> CPU/SoC's CSI2 receiver. So we only need to talk to it
> (generating interrupts) on probe() and when starting/stopping
> streaming video from the camera. Each start/stop we'll get
> a bunch of interrupts but outside of that the interrupt never
> triggers. So overhead is not really a big worry here.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> > 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
> >

Look like better be safe then sorry here, let's add mutex.

Reviewed-by: Alexander Usyskin <alexander.usyskin@intel.com>

> >
> >
> >>> +	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
> >>
> >