| Message ID | 1411614872-4009-4-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote: > Currently, pcie-designware, pcie-rcar, pci-tegra drivers > use irq chip_data to save the msi_chip pointer. They > already call irq_set_chip_data() in their own MSI irq map > functions. So irq_set_chip_data() in arch_setup_msi_irq() > is useless. Again, I think this should be the other way around. If drivers do something that's already handled by the core, then the duplicate code should be dropped from the drivers. Thierry
On 2014/9/25 15:19, Thierry Reding wrote: > On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote: >> Currently, pcie-designware, pcie-rcar, pci-tegra drivers >> use irq chip_data to save the msi_chip pointer. They >> already call irq_set_chip_data() in their own MSI irq map >> functions. So irq_set_chip_data() in arch_setup_msi_irq() >> is useless. > > Again, I think this should be the other way around. If drivers do > something that's already handled by the core, then the duplicate code > should be dropped from the drivers. Hi Thierry, this is different thing, because chip_data is specific to IRQ controller, and in other platform, like in x86, chip_data is used to save irq_cfg. So we can not call irq_set_chip_data() in core code. x86 irq piece code int arch_setup_hwirq(unsigned int irq, int node) { struct irq_cfg *cfg; unsigned long flags; int ret; cfg = alloc_irq_cfg(irq, node); if (!cfg) return -ENOMEM; raw_spin_lock_irqsave(&vector_lock, flags); ret = __assign_irq_vector(irq, cfg, apic->target_cpus()); raw_spin_unlock_irqrestore(&vector_lock, flags); if (!ret) irq_set_chip_data(irq, cfg); ------------->Save irq_cfg else free_irq_cfg(irq, cfg); return ret; } Thanks! Yijing. > > Thierry >
On Fri, Sep 26, 2014 at 10:04:45AM +0800, Yijing Wang wrote: > On 2014/9/25 15:19, Thierry Reding wrote: > > On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote: > >> Currently, pcie-designware, pcie-rcar, pci-tegra drivers > >> use irq chip_data to save the msi_chip pointer. They > >> already call irq_set_chip_data() in their own MSI irq map > >> functions. So irq_set_chip_data() in arch_setup_msi_irq() > >> is useless. > > > > Again, I think this should be the other way around. If drivers do > > something that's already handled by the core, then the duplicate code > > should be dropped from the drivers. > > Hi Thierry, this is different thing, because chip_data is specific to IRQ > controller, and in other platform, like in x86, chip_data is used to save irq_cfg. > So we can not call irq_set_chip_data() in core code. > > x86 irq piece code > > int arch_setup_hwirq(unsigned int irq, int node) > { > struct irq_cfg *cfg; > unsigned long flags; > int ret; > > cfg = alloc_irq_cfg(irq, node); > if (!cfg) > return -ENOMEM; > > raw_spin_lock_irqsave(&vector_lock, flags); > ret = __assign_irq_vector(irq, cfg, apic->target_cpus()); > raw_spin_unlock_irqrestore(&vector_lock, flags); > > if (!ret) > irq_set_chip_data(irq, cfg); ------------->Save irq_cfg > else > free_irq_cfg(irq, cfg); > return ret; > } Okay, makes sense to keep irq_set_chip_data() for driver-specific data then. Thierry
On Thu, Sep 25, 2014 at 11:14:13AM +0800, Yijing Wang wrote: > Currently, pcie-designware, pcie-rcar, pci-tegra drivers > use irq chip_data to save the msi_chip pointer. They > already call irq_set_chip_data() in their own MSI irq map > functions. So irq_set_chip_data() in arch_setup_msi_irq() > is useless. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > --- > drivers/pci/msi.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Thierry Reding <treding@nvidia.com> > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index 51d7e62..50f67a3 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -41,14 +41,13 @@ int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > if (err < 0) > return err; > > - irq_set_chip_data(desc->irq, chip); > - > return 0; > } > > void __weak arch_teardown_msi_irq(unsigned int irq) > { > - struct msi_chip *chip = irq_get_chip_data(irq); > + struct msi_desc *entry = irq_get_msi_desc(irq); > + struct msi_chip *chip = entry->dev->bus->msi; > > if (!chip || !chip->teardown_irq) > return; > -- > 1.7.1 >
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 51d7e62..50f67a3 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -41,14 +41,13 @@ int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) if (err < 0) return err; - irq_set_chip_data(desc->irq, chip); - return 0; } void __weak arch_teardown_msi_irq(unsigned int irq) { - struct msi_chip *chip = irq_get_chip_data(irq); + struct msi_desc *entry = irq_get_msi_desc(irq); + struct msi_chip *chip = entry->dev->bus->msi; if (!chip || !chip->teardown_irq) return;
Currently, pcie-designware, pcie-rcar, pci-tegra drivers use irq chip_data to save the msi_chip pointer. They already call irq_set_chip_data() in their own MSI irq map functions. So irq_set_chip_data() in arch_setup_msi_irq() is useless. Signed-off-by: Yijing Wang <wangyijing@huawei.com> --- drivers/pci/msi.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)