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.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
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.");
+
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);
-
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) {
+ 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:
--
2.38.1
On Mon, Jul 17, 2023 at 9:54 PM Srinivas Pandruvada
<[email protected]> 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 <[email protected]>
> ---
> 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:
> --