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

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

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