On Sat, Mar 25, 2023 at 8:02 AM Damien Le Moal
<damien.lemoal@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
