Re: [PATCH 09/15] ALSA: line6: Add hwdep interface to access the POD control messages

From: "Andrej Kruták" <dev@andree.sk>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH 09/15] ALSA: line6: Add hwdep interface to access the POD control messages
Date: Sun, 14 Aug 2016 09:59:07 +0200	[thread overview]
Message-ID: <CAFnc+wVSarqT5E529mnshV882OJV8+nui9BJ9amiN66oZ++OwA@mail.gmail.com> (raw)
In-Reply-To: <s5h37m9g3iu.wl-tiwai@suse.de>

On Fri, Aug 12, 2016 at 10:01 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 12 Aug 2016 18:43:30 +0200,
> Andrej Kruták wrote:
>>
>> On Fri, Aug 12, 2016 at 2:30 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > On Fri, 12 Aug 2016 14:15:16 +0200,
>> >>
>> >> >> > Also, the blocking read/write control isn't usually done by a
>> >> >> > semaphore.  Then you can handle the interrupt there.
>> >> >> >
>> >> >> >
>> >> >>
>> >> >> I actually wonder why, semaphores seemed perfect for this... Do you
>> >> >> have some hints?
>> >> >
>> >> > Assume you want to interrupt the user-space app while it's being
>> >> > blocked by the semaphore.  With your code, you can't.
>> >> >
>> >> >
>> >>
>> >> You can - down_interruptible() is there for this exact reason.
>> >
>> > There is another blocking path: you keep semaphore down after
>> > line6_hwdep_read() until line6_hwdep_push_message().  What happens if
>> > user-space interrupts during that, and line6_hwdep_push_message() is
>> > delayed or stall by some reason?
>> >
>>
>> Actually, I think I don't see what's the "another path" here, could
>> you please elaborate one more bit? I just want to make sure to not
>> reimplement the same race using waitqueue...
>>
>> What's the point then? line6_hwdep_push_message() could get not
>> scheduled for some while. So until it calls up(), line6_hwdep_read()
>> will block on down_interruptible(), or until signal (in which case
>> user gets -ERESTARTSYS). After up() is called, there are data in
>> buffer...
>
> Well, what happens if user aborts before up() is called in
> line6_hwdep_push_message()?  Now the driver calls close, and it frees
> the memory.  What if, at the very same time,
> line6_hwdep_push_message() is triggered?
>

Right, the 'active' flag is cleary not a good lock...

>> If line6_hwdep_read() returns after interrupt, no problem -
>> the buffer will just continue to be filled and semaphore will be
>> up()'d while there's free buffer space. Or until the device is
>> closed...
>
> Or, what if line6_hwdep_push_message() is triggered twice or more
> before line6_hwdep_read() is called?  It will call up() twice or
> more.  Then at this point, you call line6_hwdep_read() concurrently
> from two threads...  How do they protect against each other?
>

There's read_lock, unfortunately after "fixing" the sleep-in-atomic in
line6_hwdep_read(), it's broken now...

>> If I do the same via waitqueue, I will have the same problems, no?
>> Maybe if you could post the steps where you see the race...
>
> In your code, it's not clear that you're protecting from what.
> A simple lock+wait loop shows it more easily, at least.
>

Okay, I'll try to rethink/rework it, simplify the code, and get back.

Thanks!

-- 
Andrej
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel