linux-kernel - Re: [PATCH v2 3/7] thermal: int340x: processor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0jo3iDUpc9-jWuqGhWHpM0VYctXmkEdo4_s45_NSuw92w@mail.gmail.com>
Date:   Thu, 17 Aug 2023 22:08:16 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     daniel.lezcano@...aro.org, rafael@...nel.org, rui.zhang@...el.com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/7] thermal: int340x: processor_thermal: Use non MSI interrupts

On Mon, Jul 17, 2023 at 9:54 PM Srinivas Pandruvada
<srinivas.pandruvada@...ux.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@...ux.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:
> --