Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230717195405.730869-1-srinivas.pandruvada@linux.intel.com> <20230717195405.730869-4-srinivas.pandruvada@linux.intel.com>
In-Reply-To: <20230717195405.730869-4-srinivas.pandruvada@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 17 Aug 2023 22:08:16 +0200
Message-ID: <CAJZ5v0jo3iDUpc9-jWuqGhWHpM0VYctXmkEdo4_s45_NSuw92w@mail.gmail.com>
Subject: Re: [PATCH v2 3/7] thermal: int340x: processor_thermal: Use non MSI interrupts
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     daniel.lezcano@linaro.org, rafael@kernel.org, rui.zhang@intel.com,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Jul 17, 2023 at 9:54 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> There are issues in using MSI interrupts for processor thermal device.
> The support is not consistent across generations. But the legacy PCI
> interrupts work on all current generations.
>
> Hence always use legacy PCI interrupts by default, instead of MSI.
> Add a module param to use of MSI, so that MSI can be still used.

So I would prefer the subject of this patch to say "Use non-MSI
interrupts by default".  Otherwise it suggests that it won't be
possible to use MSIs at all.

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2:
> Changed msi_enabled to type bool
>
>  .../processor_thermal_device_pci.c            | 33 ++++++++++++-------
>  1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> index 5a2bcfff0a68..2be9b7f660d1 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> @@ -15,6 +15,11 @@
>
>  #define DRV_NAME "proc_thermal_pci"
>
> +static bool msi_enabled;
> +module_param(msi_enabled, bool, 0644);
> +MODULE_PARM_DESC(msi_enabled,
> +       "Use PCI MSI based interrupts for processor thermal device.");

I think that "use_msi" would be a better name for this switch.

> +
>  struct proc_thermal_pci {
>         struct pci_dev *pdev;
>         struct proc_thermal_device *proc_priv;
> @@ -219,8 +224,6 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
>                 return ret;
>         }
>
> -       pci_set_master(pdev);
> -

How is this change related to the rest of the patch?

>         INIT_DELAYED_WORK(&pci_info->work, proc_thermal_threshold_work_fn);
>
>         ret = proc_thermal_add(&pdev->dev, proc_priv);
> @@ -248,16 +251,23 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
>                 goto err_ret_mmio;
>         }
>
> -       /* request and enable interrupt */
> -       ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> -       if (ret < 0) {
> -               dev_err(&pdev->dev, "Failed to allocate vectors!\n");
> -               goto err_ret_tzone;
> -       }
> -       if (!pdev->msi_enabled && !pdev->msix_enabled)
> +       if (msi_enabled) {

Shouldn't this still check the pdev MSI flags?

> +               pci_set_master(pdev);
> +               /* request and enable interrupt */
> +               ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "Failed to allocate vectors!\n");
> +                       goto err_ret_tzone;
> +               }
> +               if (!pdev->msi_enabled && !pdev->msix_enabled)
> +                       irq_flag = IRQF_SHARED;
> +
> +               irq =  pci_irq_vector(pdev, 0);
> +       } else {
>                 irq_flag = IRQF_SHARED;
> +               irq = pdev->irq;
> +       }
>
> -       irq =  pci_irq_vector(pdev, 0);
>         ret = devm_request_threaded_irq(&pdev->dev, irq,
>                                         proc_thermal_irq_handler, NULL,
>                                         irq_flag, KBUILD_MODNAME, pci_info);
> @@ -273,7 +283,8 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
>         return 0;
>
>  err_free_vectors:
> -       pci_free_irq_vectors(pdev);
> +       if (msi_enabled)
> +               pci_free_irq_vectors(pdev);
>  err_ret_tzone:
>         thermal_zone_device_unregister(pci_info->tzone);
>  err_ret_mmio:
> --