On Tue, Sep 3, 2024 at 10:51 PM Keith Busch <kbusch@xxxxxxxxxx> wrote: > > On Tue, Sep 03, 2024 at 03:07:45PM +0800, Kai-Heng Feng wrote: > > On Tue, Sep 3, 2024 at 12:29 PM Manivannan Sadhasivam > > <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > > > > > On Tue, Sep 03, 2024 at 10:55:44AM +0800, Kai-Heng Feng wrote: > > > > Meteor Lake VMD has a bug that the IRQ raises before the DMA region is > > > > ready, so the requested IO is considered never completed: > > > > [ 97.343423] nvme nvme0: I/O 259 QID 2 timeout, completion polled > > > > [ 97.343446] nvme nvme0: I/O 384 QID 3 timeout, completion polled > > > > [ 97.343459] nvme nvme0: I/O 320 QID 4 timeout, completion polled > > > > [ 97.343470] nvme nvme0: I/O 707 QID 5 timeout, completion polled > > > > > > > > The is documented as erratum MTL016 [0]. The suggested workaround is to > > > > "The VMD MSI interrupt-handler should initially perform a dummy register > > > > read to the MSI initiator device prior to any writes to ensure proper > > > > PCIe ordering." which essentially is adding a delay before the interrupt > > > > handling. > > > > > > > > > > Why can't you add a dummy register read instead? Adding a delay for PCIe > > > ordering is not going to work always. > > > > This can be done too. But it can take longer than 4us delay, so I'd > > like to keep it a bit faster here. > > An added delay is just a side effect of the read. The read flushes > pending device-to-host writes, which is most likely what the errata > really requires. I think Mani is right, you need to pay that register > read penalty to truly fix this. OK, will change the quirk to perform dummy register read. But I am not sure which is the "MSI initiator device", is it VMD controller or NVMe devices? Kai-Heng > > > > > + /* Erratum MTL016 */ > > > > + VMD_FEAT_INTERRUPT_QUIRK = (1 << 6), > > > > }; > > > > > > > > #define VMD_BIOS_PM_QUIRK_LTR 0x1003 /* 3145728 ns */ > > > > @@ -90,6 +94,8 @@ static DEFINE_IDA(vmd_instance_ida); > > > > */ > > > > static DEFINE_RAW_SPINLOCK(list_lock); > > > > > > > > +static bool interrupt_delay; > > > > + > > > > /** > > > > * struct vmd_irq - private data to map driver IRQ to the VMD shared vector > > > > * @node: list item for parent traversal. > > > > @@ -105,6 +111,7 @@ struct vmd_irq { > > > > struct vmd_irq_list *irq; > > > > bool enabled; > > > > unsigned int virq; > > > > + bool delay_irq; > > > > > > This is unused. Perhaps you wanted to use this instead of interrupt_delay? > > > > This is leftover, will scratch this. > > Maybe you should actually use it instead of making a global? The quirk > says it is device specific, so no need to punish every device if it > doesn't need it (unlikely as it is to see such a thing).