Re: [RFC PATCH 1/9] media: add request API core and UAPI

[Tomasz Figa]

On Fri, Feb 2, 2018 at 4:33 PM, Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>> >> +/**
>> >> + * struct media_request_queue - queue of requests
>> >> + *
>> >> + * @mdev:    media_device that manages this queue
>> >> + * @ops:     implementation of the queue
>> >> + * @mutex:   protects requests, active_request, req_id, and all members of
>> >> + *           struct media_request
>> >> + * @active_request: request being currently run by this queue
>> >> + * @requests:        list of requests (not in any particular order) that this
>> >> + *           queue owns.
>> >> + * @req_id:  counter used to identify requests for debugging purposes
>> >> + */
>> >> +struct media_request_queue {
>> >> +     struct media_device *mdev;
>> >> +     const struct media_request_queue_ops *ops;
>> >> +
>> >> +     struct mutex mutex;
>> >
>> > Any particular reason for using a mutex? The request queue lock will need
>> > to be acquired from interrupts, too, so this should be changed to a
>> > spinlock.
>>
>> Will it be acquired from interrupts? In any case it should be possible
>> to change this to a spinlock.
>
> Using mutexes will effectively make this impossible, and I don't think we
> can safely say there's not going to be a need for that. So spinlocks,
> please.
>

IMHO whether a mutex or spinlock is the right thing depends on what
kind of critical section it is used for. If it only protects data (and
according to the comment, this one seems to do so), spinlock might
actually have better properties, e.g. not introducing the need to
reschedule, if another CPU is accessing the data at the moment. It
might also depend on how heavy the data accesses are, though. We
shouldn't need to spin for too long time.

Best regards,
Tomasz