diff mbox series

[v4,09/17] PCI: epf-test: Improve handling of command and status registers

Message ID 20230330085357.2653599-10-damien.lemoal@opensource.wdc.com (mailing list archive)
State Superseded
Headers show
Series PCI endpoint fixes and improvements | expand

Commit Message

Damien Le Moal March 30, 2023, 8:53 a.m. UTC 
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.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
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(-)

Comments

Bjorn Helgaas April 14, 2023, 4:11 p.m. UTC | #1 
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@kernel.org>
> 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 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
>
diff mbox series

Patch

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");