| Message ID | 20230308090313.1653-10-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI endpoint fixes and improvements | expand |
On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote: > The pci-epf-test driver uses the test register bar memory directly > to get and execute a test registers set by the RC side and defined > using a struct pci_epf_test_reg. This direct use relies on a casts of > the register bar to get a pointer to a struct pci_epf_test_reg to > execute the test case and sending back the test result through the > status field of struct pci_epf_test_reg. In practice, the status field > is always updated before an interrupt is raised in > pci_epf_test_raise_irq(), to ensure that the RC side sees the updated > status when receiving the interrupts. > > However, such cast-based direct access does not ensure that changes to > the status register make it to memory, and so visible to the host, > before an interrupt is raised, thus potentially resulting in the RC host > not seeing the correct status result for a test. > > Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when > accessing the command and status fields of a pci_epf_test_reg structure. > This ensure that a test start (pci_epf_test_cmd_handler() function) and > completion (with the function pci_epf_test_raise_irq()) achive a correct > synchronization with the host side mmio register accesses. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 43d623682850..e0cf8c2bf6db 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > struct pci_epf *epf = epf_test->epf; > struct device *dev = &epf->dev; > struct pci_epc *epc = epf->epc; > + u32 status = reg->status | STATUS_IRQ_RAISED; > int count; > > - reg->status |= STATUS_IRQ_RAISED; > + /* > + * Set the status before raising the IRQ to ensure that the host sees > + * the updated value when it gets the IRQ. > + */ > + WRITE_ONCE(reg->status, status); For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write has reached the memory (it could be stored in a write buffer). If you really care about synchronization, then you should do a read back of the variable. Thanks, Mani > > switch (reg->irq_type) { > case IRQ_TYPE_LEGACY: > @@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) > enum pci_barno test_reg_bar = epf_test->test_reg_bar; > struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > - command = reg->command; > + command = READ_ONCE(reg->command); > if (!command) > goto reset_handler; > > - reg->command = 0; > - reg->status = 0; > + WRITE_ONCE(reg->command, 0); > + WRITE_ONCE(reg->status, 0); > > if (reg->irq_type > IRQ_TYPE_MSIX) { > dev_err(dev, "Failed to detect IRQ type\n"); > -- > 2.39.2 >
On 2023/03/16 0:51, Manivannan Sadhasivam wrote: > On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote: >> The pci-epf-test driver uses the test register bar memory directly >> to get and execute a test registers set by the RC side and defined >> using a struct pci_epf_test_reg. This direct use relies on a casts of >> the register bar to get a pointer to a struct pci_epf_test_reg to >> execute the test case and sending back the test result through the >> status field of struct pci_epf_test_reg. In practice, the status field >> is always updated before an interrupt is raised in >> pci_epf_test_raise_irq(), to ensure that the RC side sees the updated >> status when receiving the interrupts. >> >> However, such cast-based direct access does not ensure that changes to >> the status register make it to memory, and so visible to the host, >> before an interrupt is raised, thus potentially resulting in the RC host >> not seeing the correct status result for a test. >> >> Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when >> accessing the command and status fields of a pci_epf_test_reg structure. >> This ensure that a test start (pci_epf_test_cmd_handler() function) and >> completion (with the function pci_epf_test_raise_irq()) achive a correct >> synchronization with the host side mmio register accesses. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c >> index 43d623682850..e0cf8c2bf6db 100644 >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c >> @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, >> struct pci_epf *epf = epf_test->epf; >> struct device *dev = &epf->dev; >> struct pci_epc *epc = epf->epc; >> + u32 status = reg->status | STATUS_IRQ_RAISED; >> int count; >> >> - reg->status |= STATUS_IRQ_RAISED; >> + /* >> + * Set the status before raising the IRQ to ensure that the host sees >> + * the updated value when it gets the IRQ. >> + */ >> + WRITE_ONCE(reg->status, status); > > For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write > has reached the memory (it could be stored in a write buffer). If you really > care about synchronization, then you should do a read back of the variable. WRITE_ONCE() does an explicit cast to volatile. So unless the compiler is seriously buggy, the value should make it to memory. Sure it could be in the CPU cache, but given that this is memory allocated from dma coherent, I am not sure that can happen. Or am I missing something ?
On Wed, Mar 15, 2023, at 16:51, Manivannan Sadhasivam wrote: > On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote: >> + /* >> + * Set the status before raising the IRQ to ensure that the host sees >> + * the updated value when it gets the IRQ. >> + */ >> + WRITE_ONCE(reg->status, status); > > For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write > has reached the memory (it could be stored in a write buffer). If you really > care about synchronization, then you should do a read back of the variable. This is not MMIO, this is the local access to a variable that is accessed through MMIO from the remote host. Reading it back would not change anything here as far as I can tell, Arnd
On Thu, Mar 16, 2023 at 04:25:58PM +0100, Arnd Bergmann wrote: > On Wed, Mar 15, 2023, at 16:51, Manivannan Sadhasivam wrote: > > On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote: > >> + /* > >> + * Set the status before raising the IRQ to ensure that the host sees > >> + * the updated value when it gets the IRQ. > >> + */ > >> + WRITE_ONCE(reg->status, status); > > > > For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write > > has reached the memory (it could be stored in a write buffer). If you really > > care about synchronization, then you should do a read back of the variable. > > This is not MMIO, this is the local access to a variable that is accessed > through MMIO from the remote host. Reading it back would not change anything > here as far as I can tell, > Ah, sorry I got confused this with my EPF driver... Yeah for a variable WRITE_ONCE is sufficient. Thanks, Mani > Arnd
On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote: > The pci-epf-test driver uses the test register bar memory directly > to get and execute a test registers set by the RC side and defined > using a struct pci_epf_test_reg. This direct use relies on a casts of > the register bar to get a pointer to a struct pci_epf_test_reg to > execute the test case and sending back the test result through the > status field of struct pci_epf_test_reg. In practice, the status field > is always updated before an interrupt is raised in > pci_epf_test_raise_irq(), to ensure that the RC side sees the updated > status when receiving the interrupts. > > However, such cast-based direct access does not ensure that changes to > the status register make it to memory, and so visible to the host, > before an interrupt is raised, thus potentially resulting in the RC host > not seeing the correct status result for a test. > > Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when > accessing the command and status fields of a pci_epf_test_reg structure. > This ensure that a test start (pci_epf_test_cmd_handler() function) and > completion (with the function pci_epf_test_raise_irq()) achive a correct > synchronization with the host side mmio register accesses. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> Thanks, Mani > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 43d623682850..e0cf8c2bf6db 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > struct pci_epf *epf = epf_test->epf; > struct device *dev = &epf->dev; > struct pci_epc *epc = epf->epc; > + u32 status = reg->status | STATUS_IRQ_RAISED; > int count; > > - reg->status |= STATUS_IRQ_RAISED; > + /* > + * Set the status before raising the IRQ to ensure that the host sees > + * the updated value when it gets the IRQ. > + */ > + WRITE_ONCE(reg->status, status); > > switch (reg->irq_type) { > case IRQ_TYPE_LEGACY: > @@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) > enum pci_barno test_reg_bar = epf_test->test_reg_bar; > struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > - command = reg->command; > + command = READ_ONCE(reg->command); > if (!command) > goto reset_handler; > > - reg->command = 0; > - reg->status = 0; > + WRITE_ONCE(reg->command, 0); > + WRITE_ONCE(reg->status, 0); > > if (reg->irq_type > IRQ_TYPE_MSIX) { > dev_err(dev, "Failed to detect IRQ type\n"); > -- > 2.39.2 >
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index 43d623682850..e0cf8c2bf6db 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, struct pci_epf *epf = epf_test->epf; struct device *dev = &epf->dev; struct pci_epc *epc = epf->epc; + u32 status = reg->status | STATUS_IRQ_RAISED; int count; - reg->status |= STATUS_IRQ_RAISED; + /* + * Set the status before raising the IRQ to ensure that the host sees + * the updated value when it gets the IRQ. + */ + WRITE_ONCE(reg->status, status); switch (reg->irq_type) { case IRQ_TYPE_LEGACY: @@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) enum pci_barno test_reg_bar = epf_test->test_reg_bar; struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; - command = reg->command; + command = READ_ONCE(reg->command); if (!command) goto reset_handler; - reg->command = 0; - reg->status = 0; + WRITE_ONCE(reg->command, 0); + WRITE_ONCE(reg->status, 0); if (reg->irq_type > IRQ_TYPE_MSIX) { dev_err(dev, "Failed to detect IRQ type\n");
The pci-epf-test driver uses the test register bar memory directly to get and execute a test registers set by the RC side and defined using a struct pci_epf_test_reg. This direct use relies on a casts of the register bar to get a pointer to a struct pci_epf_test_reg to execute the test case and sending back the test result through the status field of struct pci_epf_test_reg. In practice, the status field is always updated before an interrupt is raised in pci_epf_test_raise_irq(), to ensure that the RC side sees the updated status when receiving the interrupts. However, such cast-based direct access does not ensure that changes to the status register make it to memory, and so visible to the host, before an interrupt is raised, thus potentially resulting in the RC host not seeing the correct status result for a test. Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when accessing the command and status fields of a pci_epf_test_reg structure. This ensure that a test start (pci_epf_test_cmd_handler() function) and completion (with the function pci_epf_test_raise_irq()) achive a correct synchronization with the host side mmio register accesses. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)