Re: [PATCH v2 26/29] media: mc: Maintain a list of open file handles in a media device

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent, Hans,

On Mon, Feb 05, 2024 at 05:41:36PM +0200, Laurent Pinchart wrote:
> On Mon, Feb 05, 2024 at 04:32:44PM +0100, Hans Verkuil wrote:
> > On 05/02/2024 16:16, Laurent Pinchart wrote:
> > > On Mon, Feb 05, 2024 at 04:11:43PM +0100, Hans Verkuil wrote:
> > >> On 20/12/2023 11:37, Sakari Ailus wrote:
> > >>> The list of file handles is needed to deliver media events as well as for
> > >>> other purposes in the future.
> > >>>
> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >>> ---
> > >>>  drivers/media/mc/mc-device.c  | 23 ++++++++++++++++++++++-
> > >>>  drivers/media/mc/mc-devnode.c |  2 +-
> > >>>  include/media/media-devnode.h |  4 +++-
> > >>>  3 files changed, 26 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > >>> index 67a39cb63f89..9cc055deeec9 100644
> > >>> --- a/drivers/media/mc/mc-device.c
> > >>> +++ b/drivers/media/mc/mc-device.c
> > >>> @@ -45,9 +45,11 @@ static inline void __user *media_get_uptr(__u64 arg)
> > >>>  	return (void __user *)(uintptr_t)arg;
> > >>>  }
> > >>>  
> > >>> -static int media_device_open(struct file *filp)
> > >>> +static int media_device_open(struct media_devnode *devnode, struct file *filp)
> > >>>  {
> > >>> +	struct media_device *mdev = to_media_device(devnode);
> > >>>  	struct media_device_fh *fh;
> > >>> +	unsigned long flags;
> > >>>  
> > >>>  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> > >>>  	if (!fh)
> > >>> @@ -55,12 +57,23 @@ static int media_device_open(struct file *filp)
> > >>>  
> > >>>  	filp->private_data = &fh->fh;
> > >>>  
> > >>> +	spin_lock_irqsave(&mdev->fh_list_lock, flags);
> > >>
> > >> The only reason for using the irqsave variant is because we want
> > >> to support events in the future, and those can be sent in irq context.
> > > 
> > > Even in that case, would media_device_open() ever be called from
> > > interrupt context ? spin_lock_irqsave() is only needed if you don't know
> > > which context the function can be called from. If we know we'll be
> > > called from interruptible context only, you can use spin_lock_irq()
> > > instead.
> > 
> > Someone can call open() while at the same time the kernel sends a
> > media event from interrupt context. Such an event function will walk
> > over the fh_list. The irqsave here is meant to ensure that no event
> > interrupt can run while we add our fh to the fh list.
> 
> You don't need spin_lock_irqsave() for that, spin_lock_irq() is enough.
> In your interrupt handler, you need spin_lock() only.
> spin_lock_irqsave() is for places that can be called both from
> interruptible and non-interruptible contexts.

I'll address this in v3.

-- 
Sakari Ailus