Re: [PATCH v15 00/13] mux controller abstraction and iio/i2c muxes

[Peter Rosin]

On 2017-06-03 22:26, Luc Van Oostenryck wrote:
> On Sat, Jun 3, 2017 at 9:34 PM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> On Sat, Jun 03, 2017 at 08:37:21PM +0200, Luc Van Oostenryck wrote:
>>> On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
>>> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>>>> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>>>>> From: Peter Rosin <peda@xxxxxxxxxx>
>>>>>
>>>>> Hi Greg,
>>>>>
>>>>> Philipp found problems in v14 with using a mutex for locking that was
>>>>> the outcome of the review for v13, so I'm now using a semaphore instead
>>>>> of the rwsem that was in v13. That at least got rid of the scary call
>>>>> to downgrade_write. However, I'm still unsure about what you actually
>>>>> meant with your comment about lack of sparse markings [1]. I did add
>>>>> __must_check to the funcs that selects the mux, but I've got this
>>>>> feeling that this is not what you meant?
>>>>
>>>> I thought there was a way to mark a function as requiring a lock be held
>>>> when it is being called.  Does sparse not support that anymore?
>>>
>>> sparse still support these annotations, of course.
>>> In this case, I suppose you're talking about '__must_hold()' which
>>> *must* be used instead of a pair of '__releases()' + '__acquires()'
>>> when the lock is help on function entry and exit.
>>
>> Ah, yes, that's what I was thinking of.  I don't know if sparse can
>> track things like this across an exported symbol, so I doubt it really
>> will help here.  Sorry for the noise.
> 
> No problem, I'm glad to help for sparse related things.
> 
> I didn't saw the code in question because the lkml.org link Peter
> gave didn't work for me and I don't know much about exported symbols
> (but I think the sole effect is to add some data in some symbol table).
> But these annotations just work based on the declarations, very much
> like type checking. So if you have something in scope like the following:
> 
> void do_stuff_locked(struct s *ptr) __must_hold(*ptr);
> 
> ...
> 
> void do_stuff_unlocked(struct s *ptr)
> {
>         ...
>         do_stuff_locked(ptr);        // will warn
>         ...
> }
> 
> You will have a warning from sparse unless the code preceding and following
> the call to do_stuff_locked() lock & then unlock 'ptr', generaly
> indirectly by a pair
> of functions, the one before with an '__acquires()' in its declaration
> the one after
> with a '__releases()' in its declaration:
> 
> void lock_stuff(struct s *ptr) __acquires(*ptr);
> void unlock_stuff(struct s *ptr) __releases(*ptr);
> 
> void do_stuff_unlocked(struct s *ptr)
> {
>         lock_stuff(ptr);
>         do_stuff_locked(ptr);        // won't warn
>         unlock_stuff(ptr);
> }

Ok, thanks for the explanation! The above was what I gathered when I
looked around, and since it didn't really fit the usage pattern of the
mux api I was stomped. When comparing the mux code with the above,
mux_control_select would be an __acquires (albeit a conditional one,
but let's not muddy the waters unnecessarily) and mux_control_deselect
would be a __releases.

But for long time mux consumers, like the video mux, it must be OK to
only acquire the mux, and not release it right away in the same context,
which I assume will be very hard for sparse to handle sanely? E.g. I
think sparse also complains if there are unbalanced __acquires and
__releases in some context, no?

Cheers,
peda

BTW, the core mux code is at the below link if the lkml link continues
to fail:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=a3b02a9c6591ce154cd44e2383406390a45b530c