> Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > Hi, > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@xxxxxxxxxxx wrote: > > From: Yu-Yen Ting <steventing@xxxxxxxxxxx> > > > > MSI interrupt should be enabled on certain platform. > > > > Add a module parameter disable_msi to disable MSI interrupt, > > driver will then use legacy interrupt instead. > > And the interrupt mode is not able to change at run-time, so > > the module parameter is read only. > > Well, if we unbind/rebind the device, probe() will pick up the new > value. e.g.: > > echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/unbind > echo '0000:01:00.0' > /sys/bus/pci/drivers/rtw_pci/bind > > So is it really necessary to mark read-only? I think there's a general > understanding that module parameters are not always "immediately > effective." If there's a general understanding of not always effective immediately, I think I can change the file mode to 0644. > > > Tested-by: Ján Veselý <jano.vesely@xxxxxxxxx> > > Signed-off-by: Yu-Yen Ting <steventing@xxxxxxxxxxx> > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 51 > ++++++++++++++++++++++++++++++-- > > drivers/net/wireless/realtek/rtw88/pci.h | 1 + > > 2 files changed, 49 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > > index 23dd06a..25410f6 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, > void *dev) > > if (!rtwpci->irq_enabled) > > goto out; > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > Why exactly do you have to mask interrupts during the ISR? Is there a > race in rtw_pci_irq_recognized() or something? I think there is a race between SW and HW, if we do not stop the IRQ first, write 1 clear will make the interrupt to be lost. > > > rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); > > > > if (irq_status[0] & IMR_MGNTDOK) > > ... > > > Otherwise, looks fine: > > Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx> > Yan-Hsuan
