On Thu, Mar 30, 2023 at 05:53:49PM +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.
s/bar/BAR/
s/a casts/a cast/ (I suspect this is still a lookup, not a cast)
s/achive/achieve/
s/mmio/MMIO/
I don't think "cast" is the right way to describe what's going on
here.
> Reviewed-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> ---
> 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 ee90ba3a957b..fa48e9b3c393 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -613,9 +613,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:
> @@ -659,12 +664,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
>
