diff mbox

[v2,03/22] MSI: Remove the redundant irq_set_chip_data()

Message ID 1411614872-4009-4-git-send-email-wangyijing@huawei.com
State New
Headers show

Commit Message

wangyijing Sept. 25, 2014, 3:14 a.m. UTC
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(-)

Comments

Thierry Reding Sept. 25, 2014, 7:19 a.m. UTC | #1
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
wangyijing Sept. 26, 2014, 2:04 a.m. UTC | #2
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
>
Thierry Reding Sept. 26, 2014, 8:09 a.m. UTC | #3
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
Thierry Reding Sept. 26, 2014, 8:09 a.m. UTC | #4
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 mbox

Patch

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;