On Fri, 11 Dec 2020 18:20:50 +0100, Jaroslav Kysela wrote: > > Dne 11. 12. 20 v 17:59 Takashi Iwai napsal(a): > > On Fri, 11 Dec 2020 17:45:45 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 11. 12. 20 v 9:38 Takashi Iwai napsal(a): > >>> Currently alsactl-restore tries to initialize the device when an error > >>> is found for restore action. But this isn't the right behavior in the > >>> case where the lock is held; it implies that another alsactl is > >>> running concurrently, hence you shouldn't initialize the card at the > >>> same time. The situation is found easily when two alsactls get > >>> started by both udev and systemd (note that those two invocations are > >>> the designed behavior, see /usr/lib/udev/rules.d/78-sound-cards.rules > >>> for details). > >>> > >>> This patch changes load_state() not to handle the initialization if > >>> the locking fails. > >> > >> The operation should serialize in this case (there's limit of 10 seconds which > >> should be enough to finish the initialization). The state_lock() function > >> should return -EBUSY when the file is locked (and I'm fine to change the > >> behaviour from 'init' to 'skip' for this lock state). > >> > >> It seems that -EEXIST is returned when the lock file exists, but the > >> open(file, O_CREAT|O_EXCL, 0644) caller has not enough priviledges to access > >> this file when another user owns the file. > >> > >> But alsactl from /lib/udev/rules.d/90-alsa-restore.rules and > >> /lib/systemd/system/alsa-restore.service should be run as root, right? > > > > Yes, it should be root. > > > > I also wondered how EEXIST comes, too. Maybe it's also the race > > between the first open(O_RDWR) and the second > > open(O_RDWR|O_CREAT|O_EXCL)? If so, it'd be better to go back again > > to the normal open(O_RDWR)? > > Maybe. It seems enough to add EEXIST errno check to the "if (errno == EBUSY || > errno == EAGAIN)" condition to repeat the open sequence. The -EBUSY will be > returned correctly then. The one second delay is harmless in my eyes for the > second task. I'm afraid that a significant delay can be confusing. And this should be the race only once, so no need to add the artificial delay, IMO. (BTW, now I noticed that we decrease timeout twice :) Takashi
