On Mon, Jul 29, 2024 at 10:16:04AM +0000, edmund.raile wrote:

...

$ git revert -s b5b519965c4c

Yes, 5b5 can be removed via revert, but what is the difference in effect? Just time saving?

The title of commit generated by git-revert could be helpful when reading history of changes later. The code change is itself important, while the history of changes often assists developers to work between several trees.

...

$ git revert -s 7ba5ca32fe6e

This one I'd like to ask you about: The original inline comment in amdtp-stream.c amdtp_domain_stream_pcm_pointer()

// This function is called in software IRQ context of
// period_work or process context.
//
// When the software IRQ context was scheduled by software IRQ
// context of IT contexts, queued packets were already handled.
// Therefore, no need to flush the queue in buffer furthermore.
//
// When the process context reach here, some packets will be
// already queued in the buffer. These packets should be handled
// immediately to keep better granularity of PCM pointer.
//
// Later, the process context will sometimes schedules software
// IRQ context of the period_work. Then, no need to flush the
// queue by the same reason as described in the above

(let's call the above v1) was replaced with

// In software IRQ context, the call causes dead-lock to disable the tasklet
// synchronously.

on occasion of 7ba5ca32fe6e (let's call this v2).

I sought to replace it with

// use wq to prevent deadlock between process context spin_lock
// of snd_pcm_stream_lock_irq() in snd_pcm_status64() and
// softIRQ context spin_lock of snd_pcm_stream_lock_irqsave()
// in snd_pcm_period_elapsed()

to prevent this issue from occurring again (let's call this v3).

Should I include v1, v3 or a combination of v1 and v3 in my next patch?

I think we pay enough attention to regenerate the deadlock, thus v3 is a good choice. But the cause of deadlock in the above description is a bit wrong. It is typical AB/BA deadlock, like:

Thread 0: 1. Acquire stream lock by calling 'snd_pcm_stream_lock_irq()' or so 2. Wait until running tasklet finishes by calling 'tasklet_disable_in_atomic()' (actually at tasklet_unlock_spin_wait())

Thread 1: 1. Launch tasklet 2. Wait until the stream lock released

The softIRQ context does not related to any type of lock in sound subsystem essentially.

Thanks

Takashi Sakamoto