| Message ID | 20230325070226.511323-11-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI endpoint fixes and improvements | expand |
On Sat, Mar 25, 2023 at 8:02 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > > Command codes are never combined together as flags into a single value. > Thus we can replace the series of "if" tests in > pci_epf_test_cmd_handler() with a cleaner switch-case statement. > This also allows checking that we got a valid command and print an error > message if we did not. > > 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 | 30 +++++++++---------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index fa48e9b3c393..c2a14f828bdf 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -676,41 +676,39 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) > goto reset_handler; > } > > - if ((command & COMMAND_RAISE_LEGACY_IRQ) || > - (command & COMMAND_RAISE_MSI_IRQ) || > - (command & COMMAND_RAISE_MSIX_IRQ)) { > + switch (command) { > + case COMMAND_RAISE_LEGACY_IRQ: > + case COMMAND_RAISE_MSI_IRQ: > + case COMMAND_RAISE_MSIX_IRQ: > pci_epf_test_raise_irq(epf_test, reg); > - goto reset_handler; > - } > - > - if (command & COMMAND_WRITE) { > + break; > + case COMMAND_WRITE: > ret = pci_epf_test_write(epf_test, reg); > if (ret) > reg->status |= STATUS_WRITE_FAIL; > else > reg->status |= STATUS_WRITE_SUCCESS; > pci_epf_test_raise_irq(epf_test, reg); > - goto reset_handler; > - } As a minor improvement on this cleanup I would suggest either switching the if / else condition above or the two below, the inverted logic makes it confusing. (one test case is if (ret) and the two others are if (!ret) with inverted results, all could share the same code (same logic)). > - > - if (command & COMMAND_READ) { > + break; > + case COMMAND_READ: > ret = pci_epf_test_read(epf_test, reg); > if (!ret) > reg->status |= STATUS_READ_SUCCESS; > else > reg->status |= STATUS_READ_FAIL; > pci_epf_test_raise_irq(epf_test, reg); > - goto reset_handler; > - } > - > - if (command & COMMAND_COPY) { > + break; > + case COMMAND_COPY: > ret = pci_epf_test_copy(epf_test, reg); > if (!ret) > reg->status |= STATUS_COPY_SUCCESS; > else > reg->status |= STATUS_COPY_FAIL; > pci_epf_test_raise_irq(epf_test, reg); > - goto reset_handler; > + break; > + default: > + dev_err(dev, "Invalid command\n"); > + break; > } > > reset_handler: > -- > 2.39.2 >
On 3/28/23 15:56, Rick Wertenbroek wrote: > On Sat, Mar 25, 2023 at 8:02 AM Damien Le Moal > <damien.lemoal@opensource.wdc.com> wrote: >> >> Command codes are never combined together as flags into a single value. >> Thus we can replace the series of "if" tests in >> pci_epf_test_cmd_handler() with a cleaner switch-case statement. >> This also allows checking that we got a valid command and print an error >> message if we did not. >> >> 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 | 30 +++++++++---------- >> 1 file changed, 14 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c >> index fa48e9b3c393..c2a14f828bdf 100644 >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c >> @@ -676,41 +676,39 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) >> goto reset_handler; >> } >> >> - if ((command & COMMAND_RAISE_LEGACY_IRQ) || >> - (command & COMMAND_RAISE_MSI_IRQ) || >> - (command & COMMAND_RAISE_MSIX_IRQ)) { >> + switch (command) { >> + case COMMAND_RAISE_LEGACY_IRQ: >> + case COMMAND_RAISE_MSI_IRQ: >> + case COMMAND_RAISE_MSIX_IRQ: >> pci_epf_test_raise_irq(epf_test, reg); >> - goto reset_handler; >> - } >> - >> - if (command & COMMAND_WRITE) { >> + break; >> + case COMMAND_WRITE: >> ret = pci_epf_test_write(epf_test, reg); >> if (ret) >> reg->status |= STATUS_WRITE_FAIL; >> else >> reg->status |= STATUS_WRITE_SUCCESS; >> pci_epf_test_raise_irq(epf_test, reg); >> - goto reset_handler; >> - } > > As a minor improvement on this cleanup I would suggest either switching > the if / else condition above or the two below, the inverted logic makes it > confusing. (one test case is if (ret) and the two others are if (!ret) with > inverted results, all could share the same code (same logic)). Indeed, good idea. I will add one more patch to do that. > >> - >> - if (command & COMMAND_READ) { >> + break; >> + case COMMAND_READ: >> ret = pci_epf_test_read(epf_test, reg); >> if (!ret) >> reg->status |= STATUS_READ_SUCCESS; >> else >> reg->status |= STATUS_READ_FAIL; >> pci_epf_test_raise_irq(epf_test, reg); >> - goto reset_handler; >> - } >> - >> - if (command & COMMAND_COPY) { >> + break; >> + case COMMAND_COPY: >> ret = pci_epf_test_copy(epf_test, reg); >> if (!ret) >> reg->status |= STATUS_COPY_SUCCESS; >> else >> reg->status |= STATUS_COPY_FAIL; >> pci_epf_test_raise_irq(epf_test, reg); >> - goto reset_handler; >> + break; >> + default: >> + dev_err(dev, "Invalid command\n"); >> + break; >> } >> >> reset_handler: >> -- >> 2.39.2 >>
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index fa48e9b3c393..c2a14f828bdf 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -676,41 +676,39 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) goto reset_handler; } - if ((command & COMMAND_RAISE_LEGACY_IRQ) || - (command & COMMAND_RAISE_MSI_IRQ) || - (command & COMMAND_RAISE_MSIX_IRQ)) { + switch (command) { + case COMMAND_RAISE_LEGACY_IRQ: + case COMMAND_RAISE_MSI_IRQ: + case COMMAND_RAISE_MSIX_IRQ: pci_epf_test_raise_irq(epf_test, reg); - goto reset_handler; - } - - if (command & COMMAND_WRITE) { + break; + case COMMAND_WRITE: ret = pci_epf_test_write(epf_test, reg); if (ret) reg->status |= STATUS_WRITE_FAIL; else reg->status |= STATUS_WRITE_SUCCESS; pci_epf_test_raise_irq(epf_test, reg); - goto reset_handler; - } - - if (command & COMMAND_READ) { + break; + case COMMAND_READ: ret = pci_epf_test_read(epf_test, reg); if (!ret) reg->status |= STATUS_READ_SUCCESS; else reg->status |= STATUS_READ_FAIL; pci_epf_test_raise_irq(epf_test, reg); - goto reset_handler; - } - - if (command & COMMAND_COPY) { + break; + case COMMAND_COPY: ret = pci_epf_test_copy(epf_test, reg); if (!ret) reg->status |= STATUS_COPY_SUCCESS; else reg->status |= STATUS_COPY_FAIL; pci_epf_test_raise_irq(epf_test, reg); - goto reset_handler; + break; + default: + dev_err(dev, "Invalid command\n"); + break; } reset_handler: