Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Thu, 17 Aug 2023 22:08:16 +0200 | 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@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: > --
| |