diff mbox

[1/4] fsl_msi: fix the conflict of virt_msir's chip_data

Message ID 1271403278-30091-1-git-send-email-leoli@freescale.com (mailing list archive)
State Superseded
Delegated to: Kumar Gala
Headers show

Commit Message

Yang Li April 16, 2010, 7:34 a.m. UTC 
From: Zhao Chenhui <b26998@freescale.com>

In fsl_of_msi_probe(), the virt_msir's chip_data have been stored
the pointer to struct mpic. We add a struct fsl_msi_cascade_data
to store the pointer to struct fsl_msi and msir_index.  Otherwise,
the pointer to struct mpic will be over-written, and will cause
problem when calling eoi() of the irq.

Signed-off-by: Zhao Chenhui <b26998@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/sysdev/fsl_msi.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

Comments

Michael Ellerman April 19, 2010, 2:40 a.m. UTC | #1 
On Fri, 2010-04-16 at 15:34 +0800, Li Yang wrote:
> From: Zhao Chenhui <b26998@freescale.com>
> 
> In fsl_of_msi_probe(), the virt_msir's chip_data have been stored
> the pointer to struct mpic. We add a struct fsl_msi_cascade_data
> to store the pointer to struct fsl_msi and msir_index.  Otherwise,
> the pointer to struct mpic will be over-written, and will cause
> problem when calling eoi() of the irq.

I don't quite understand. Do you mean someone was overwriting
handler_data somewhere?
 
> @@ -309,9 +319,19 @@ static int __devinit fsl_of_msi_probe(struct of_device *dev,
>  			break;
>  		virt_msir = irq_of_parse_and_map(dev->node, i);
>  		if (virt_msir != NO_IRQ) {
> -			set_irq_data(virt_msir, (void *)i);
> +			cascade_data = kzalloc(
> +					sizeof(struct fsl_msi_cascade_data),
> +					GFP_KERNEL);
> +			if (!cascade_data) {
> +				dev_err(&dev->dev,
> +					"No memory for MSI cascade data\n");
> +				err = -ENOMEM;
> +				goto error_out;

The error handling in this routine is not great, most of the setup is
not torn down properly in the error paths AFAICS, this adds another.

> +			}
> +			cascade_data->index = i;
> +			cascade_data->data = msi;
> +			set_irq_data(virt_msir, (void *)cascade_data);
>  			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
> -			set_irq_chip_data(virt_msir, msi);
>  		}
>  	}
>  

cheers
Yang Li April 19, 2010, 4:50 a.m. UTC | #2 
On 4/19/2010 10:40 AM, Michael Ellerman wrote:
> On Fri, 2010-04-16 at 15:34 +0800, Li Yang wrote:
>    
>> From: Zhao Chenhui<b26998@freescale.com>
>>
>> In fsl_of_msi_probe(), the virt_msir's chip_data have been stored
>> the pointer to struct mpic. We add a struct fsl_msi_cascade_data
>> to store the pointer to struct fsl_msi and msir_index.  Otherwise,
>> the pointer to struct mpic will be over-written, and will cause
>> problem when calling eoi() of the irq.
>>      
> I don't quite understand. Do you mean someone was overwriting
> handler_data somewhere?
>    

The patch at http://patchwork.ozlabs.org/patch/48794/ was overwriting 
the chip_data.  We move the newly added pointer to fsl_msi structure to 
the handler data.

>
>    
>> @@ -309,9 +319,19 @@ static int __devinit fsl_of_msi_probe(struct of_device *dev,
>>   			break;
>>   		virt_msir = irq_of_parse_and_map(dev->node, i);
>>   		if (virt_msir != NO_IRQ) {
>> -			set_irq_data(virt_msir, (void *)i);
>> +			cascade_data = kzalloc(
>> +					sizeof(struct fsl_msi_cascade_data),
>> +					GFP_KERNEL);
>> +			if (!cascade_data) {
>> +				dev_err(&dev->dev,
>> +					"No memory for MSI cascade data\n");
>> +				err = -ENOMEM;
>> +				goto error_out;
>>      
> The error handling in this routine is not great, most of the setup is
> not torn down properly in the error paths AFAICS, this adds another.
>    

You are right.  Need to add a separate patch to fix all these.

Thanks.
- Leo
Kumar Gala April 19, 2010, 12:19 p.m. UTC | #3 
On Apr 18, 2010, at 11:50 PM, Li Yang wrote:

> On 4/19/2010 10:40 AM, Michael Ellerman wrote:
>> On Fri, 2010-04-16 at 15:34 +0800, Li Yang wrote:
>>   
>>> From: Zhao Chenhui<b26998@freescale.com>
>>> 
>>> In fsl_of_msi_probe(), the virt_msir's chip_data have been stored
>>> the pointer to struct mpic. We add a struct fsl_msi_cascade_data
>>> to store the pointer to struct fsl_msi and msir_index.  Otherwise,
>>> the pointer to struct mpic will be over-written, and will cause
>>> problem when calling eoi() of the irq.
>>>     
>> I don't quite understand. Do you mean someone was overwriting
>> handler_data somewhere?
>>   
> 
> The patch at http://patchwork.ozlabs.org/patch/48794/ was overwriting the chip_data.  We move the newly added pointer to fsl_msi structure to the handler data.

Let's fix that patch.

- k
Yang Li April 20, 2010, 3:10 a.m. UTC | #4 
On Mon, Apr 19, 2010 at 8:19 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Apr 18, 2010, at 11:50 PM, Li Yang wrote:
>
>> On 4/19/2010 10:40 AM, Michael Ellerman wrote:
>>> On Fri, 2010-04-16 at 15:34 +0800, Li Yang wrote:
>>>
>>>> From: Zhao Chenhui<b26998@freescale.com>
>>>>
>>>> In fsl_of_msi_probe(), the virt_msir's chip_data have been stored
>>>> the pointer to struct mpic. We add a struct fsl_msi_cascade_data
>>>> to store the pointer to struct fsl_msi and msir_index.  Otherwise,
>>>> the pointer to struct mpic will be over-written, and will cause
>>>> problem when calling eoi() of the irq.
>>>>
>>> I don't quite understand. Do you mean someone was overwriting
>>> handler_data somewhere?
>>>
>>
>> The patch at http://patchwork.ozlabs.org/patch/48794/ was overwriting the chip_data.  We move the newly added pointer to fsl_msi structure to the handler data.
>
> Let's fix that patch.

All right.  You can merge the two patches if you like it that way.

Thanks.
Leo
diff mbox

Patch

diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index ad453ca..716862f 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -21,6 +21,7 @@ 
 #include <asm/prom.h>
 #include <asm/hw_irq.h>
 #include <asm/ppc-pci.h>
+#include <asm/mpic.h>
 #include "fsl_msi.h"
 
 struct fsl_msi_feature {
@@ -28,6 +29,10 @@  struct fsl_msi_feature {
 	u32 msiir_offset;
 };
 
+struct fsl_msi_cascade_data {
+	struct fsl_msi *data;
+	int index;
+};
 
 static inline u32 fsl_msi_read(u32 __iomem *base, unsigned int reg)
 {
@@ -132,7 +137,7 @@  static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
 
 static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 {
-	int rc, hwirq;
+	int rc, hwirq = NO_IRQ;
 	unsigned int virq;
 	struct msi_desc *entry;
 	struct msi_msg msg;
@@ -172,11 +177,15 @@  out_free:
 static void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
 {
 	unsigned int cascade_irq;
-	struct fsl_msi *msi_data = get_irq_chip_data(irq);
+	struct fsl_msi *msi_data;
 	int msir_index = -1;
 	u32 msir_value = 0;
 	u32 intr_index;
 	u32 have_shift = 0;
+	struct fsl_msi_cascade_data *cascade_data;
+
+	cascade_data = (struct fsl_msi_cascade_data *)get_irq_data(irq);
+	msi_data = cascade_data->data;
 
 	spin_lock(&desc->lock);
 	if ((msi_data->feature &  FSL_PIC_IP_MASK) == FSL_PIC_IP_IPIC) {
@@ -191,7 +200,7 @@  static void fsl_msi_cascade(unsigned int irq, struct irq_desc *desc)
 	if (unlikely(desc->status & IRQ_INPROGRESS))
 		goto unlock;
 
-	msir_index = (int)desc->handler_data;
+	msir_index = cascade_data->index;
 
 	if (msir_index >= NR_MSI_REG)
 		cascade_irq = NO_IRQ;
@@ -243,6 +252,7 @@  static int __devinit fsl_of_msi_probe(struct of_device *dev,
 	int virt_msir;
 	const u32 *p;
 	struct fsl_msi_feature *features = match->data;
+	struct fsl_msi_cascade_data *cascade_data = NULL;
 
 	printk(KERN_DEBUG "Setting up Freescale MSI support\n");
 
@@ -309,9 +319,19 @@  static int __devinit fsl_of_msi_probe(struct of_device *dev,
 			break;
 		virt_msir = irq_of_parse_and_map(dev->node, i);
 		if (virt_msir != NO_IRQ) {
-			set_irq_data(virt_msir, (void *)i);
+			cascade_data = kzalloc(
+					sizeof(struct fsl_msi_cascade_data),
+					GFP_KERNEL);
+			if (!cascade_data) {
+				dev_err(&dev->dev,
+					"No memory for MSI cascade data\n");
+				err = -ENOMEM;
+				goto error_out;
+			}
+			cascade_data->index = i;
+			cascade_data->data = msi;
+			set_irq_data(virt_msir, (void *)cascade_data);
 			set_irq_chained_handler(virt_msir, fsl_msi_cascade);
-			set_irq_chip_data(virt_msir, msi);
 		}
 	}