Tony Chuang <yhchuang@xxxxxxxxxxx> 於 2019年8月27日 週二 下午8:12寫道： > > Hi Daniel and Ján, > > > > From: Ján Veselý > Sent: Friday, August 23, 2019 3:22 PM > > On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@xxxxxxxxxxxx> wrote: > > > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > > > I checked the discussion on the v1 patch thread but I still don't follow > > > this. > > > > > > You're worried about the case where we're inside the interrupt handler and: > > > 1. We read the interrupt status to note what needs to be done > > > 2. <another interrupt arrives here, requiring other work to be done> > > > 3. We clear the interrupt status bits > > > 4. We proceed to handle the interrupt but missing any work requested by > > > the interrupt in step 2. > > > > > > Is that right? > > > > > > I'm not an expert here, but I don't think this is something that drivers > > > have to worry about. Surely the interrupt controller can be expected to > > > have a mechanism to "queue up" any interrupt that arrives while an > > > interrupt is being handled? Otherwise handling of all types of > > > edge-triggered interrupts (not just MSI) would be overly painful across the > > > board. > > > > That's my understanding as well. > > entering the interrupt vector clears the IFLAG, so any interrupt will > > wait until the IFLAG is restored, or delivered to a different CPU. > > wouldn't it be safer to enable interrupts only _after_ registering the > > handler in the "rtw_pci_request_irq" function? > > > > regards, > > Jan > > > Yes that's not something drivers need to care about. But I think it is > Because there's a race condition between SW/HW when clearing the ISR. > If interrupt comes after reading ISR and before write-1-clear, the interrupt > controller would have interrupt status raised, and never issue interrupt > signal to host when other new interrupts status are raised. > > To avoid this, driver requires to protect the ISR write-1-clear process by > disabling the IMR. > > > > > > > > > > > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for > > > what correct interrupt controller behaviour should look like. > > > > > > > + ret = pci_enable_msi(pdev); > > > > > > pci_enable_msi() is "deprecated, don't use" > > Do you mean I should remove this? > But I cannot find another proper way to enable the MSI. > Maybe pci_alloc_irq_vectors() could work but I am not sure if > It is recommended. According to the kernel documentation "The MSI Driver Guide HOWTO", pci_alloc_irq_vectors, pci_irq_vector and pci_free_irq_vectors are the functions. https://elixir.bootlin.com/linux/v5.3-rc6/source/Documentation/PCI/msi-howto.rst Here is an example in r8169 module. https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/net/ethernet/realtek/r8169_main.c#L6603 Jian-Hong Pan
