Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

* Michael S. Tsirkin | 2011-08-05 00:04:13 [+0300]:

>> Looking at it further here is only one spot where the lock is hold. So
>> giving the fact that an ISR is not reentrant and is not executed on two
>> cpus at the same time why do we need a lock here?
>
>I'm not sure anymore. I think the idea was to use
>it for synchronization down the road somehow,
>but it never materialized. Let's drop that lock completely.
Okay. So I post antoher patch with this lock removed and cc stable.

>> So there is _no_ protection
>> against read-modify-write of user vs kernel so even that
>> pci_block_user_cfg_access() is kinda pointless.
>
>I didn't get that. pci_block_user_cfg_access is to prevent
>sysfs access while we read modify-write the command register.
>Isn't it effective for that?
It probably works well enough for you because you only care the one bit
and don't change anything else in the kernel driver.

Lets assume user land changes another bit in this register:

    user                                kernel
 read config()                             |
   add a bit                               |
     |                                  interrupt
     |                                block user land
     |                              read + clear + write
     |                               unblock user land
 write config back                         | 


You did not *re-read* the config field after the interrupt so kernel's
modifications are lost. So you get two interrupts accounted while only
one happend. It seems to me that you could drop this "user block" thing
since you never change anything outside of this command register and it
does not stop the race.

>> pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.
>
>Why block in open? We don't access the device there, do we?
Yeah. That might not work for you since you need change other values.

Sebastian