On Fri, 2010-03-19 at 10:15 -0500, Kumar Gala wrote: > On Mar 18, 2010, at 10:06 PM, Michael Ellerman wrote: > > > On Thu, 2010-03-18 at 09:53 -0500, Kumar Gala wrote: > >> From: Lan Chunhe-B25806 <b25...@freescale.com> > >> > >> Freescale QorIQ P4080 has three MSI banks and the original code > >> can not work well. This patch adds multiple MSI banks support for > >> Freescale processor. > >> > >> Signed-off-by: Lan Chunhe-B25806 <b25...@freescale.com> > >> Signed-off-by: Roy Zang <tie-fei.z...@freescale.com> > > > >> @@ -146,9 +149,13 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev, > >> int nvec, int type) > >> unsigned int virq; > >> struct msi_desc *entry; > >> struct msi_msg msg; > >> - struct fsl_msi *msi_data = fsl_msi; > >> + struct fsl_msi *msi_data; > >> > >> list_for_each_entry(entry, &pdev->msi_list, list) { > >> + if (entry->irq == NO_IRQ) > >> + continue; > > > > This looks wrong, entry->irq should always be 0 here because it was just > > kzalloc'ed - you should only be doing this check in teardown.
This was the important comment, the rest was nit-picking :) > >> - WARN_ON(ppc_md.setup_msi_irqs); > >> - ppc_md.setup_msi_irqs = fsl_setup_msi_irqs; > >> - ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs; > >> - ppc_md.msi_check_device = fsl_msi_check_device; > >> + /* The multiple setting ppc_md.setup_msi_irqs will not harm things */ > >> + if (!ppc_md.setup_msi_irqs) { > >> + ppc_md.setup_msi_irqs = fsl_setup_msi_irqs; > >> + ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs; > >> + ppc_md.msi_check_device = fsl_msi_check_device; > >> + } else if (ppc_md.setup_msi_irqs != fsl_setup_msi_irqs) { > >> + dev_err(&dev->dev, "Different MSI driver already installed!\n"); > >> + err = -EBUSY; /* or some other error code */ > >> + goto error_out; > >> + } > > > > I liked it the way it was, because having two competing MSI backends > > means something's probably not going to work. But it's your driver so > > whatever you like. > > The previous WARN_ON() is problematic when we have multiple (of the > same type) MSI blocks. The check was intended to do exactly what you > are suggesting. If you think its doing something else let us know. Right, yeah I see what you mean - dev_err() is a bit harder to spot than a WARN() but it's probably sufficient. cheers
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev