Re: [PATCH 2/2] PCI: designware: implement multiple MSI irq setup

[Lucas Stach <l.stach@pengutronix.de>]

Am Freitag, den 20.03.2015, 17:07 -0500 schrieb Bjorn Helgaas:
> On Mon, Mar 09, 2015 at 06:33:08PM +0100, Lucas Stach wrote:
> > Allows to properly set up multiple MSI irqs per device.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > The ifdef is needed to avoid a compile error on
> > !CONFIG_PCI_MSI, as msi_list is only part of pci_dev
> > when the kernel is compiled with MSI support.
> > ---
> >  drivers/pci/host/pcie-designware.c | 46 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 1f4ea6f2d910..8a15ea2e8eab 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -269,6 +269,9 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
> >  	}
> >  
> >  	*pos = pos0;
> > +	desc->nvec_used = no_irqs;
> > +	desc->msi_attrib.multiple = order_base_2(no_irqs);
> > +
> >  	return irq;
> >  
> >  no_valid_irq:
> > @@ -305,6 +308,48 @@ static int dw_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
> >  
> >  	return 0;
> >  }
> > +#ifdef CONFIG_PCI_MSI
> > +static int dw_msi_setup_irqs(struct msi_controller *chip, struct pci_dev *pdev,
> > +			     int nvec, int type)
> > +{
> > +	int irq, pos;
> > +	struct msi_desc *desc;
> > +	struct msi_msg msg;
> > +	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
> > +
> > +	/* MSI-X interrupts are not supported */
> > +	if (type == PCI_CAP_ID_MSIX)
> > +		return -EINVAL;
> > +
> > +	WARN_ON(!list_is_singular(&pdev->msi_list));
> > +	desc = list_entry(pdev->msi_list.next, struct msi_desc, list);
> > +
> > +	irq = assign_irq(nvec, desc, &pos);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	if (pp->ops->get_msi_addr)
> > +		msg.address_lo = pp->ops->get_msi_addr(pp);
> > +	else
> > +		msg.address_lo = virt_to_phys((void *)pp->msi_data);
> > +	msg.address_hi = 0x0;
> > +
> > +	if (pp->ops->get_msi_data)
> > +		msg.data = pp->ops->get_msi_data(pp, pos);
> > +	else
> > +		msg.data = pos;
> > +
> > +	pci_write_msi_msg(irq, &msg);
> > +
> > +	return 0;
> > +}
> > +#else
> > +static int dw_msi_setup_irqs(struct msi_chip *chip, struct pci_dev *pdev,
> > +			     int nvec, int type)
> > +{
> > +	return -ENOSYS;
> > +}
> 
> Seems strange that we need even this stub function when we don't have
> CONFIG_PCI_MSI.  Is there some way we can fix this so dw_pcie_msi_chip is
> only defined when we have CONFIG_PCI_MSI?
> 
I would rather not do this. We try to keep as much code as possible to
at least see some compile coverage, even if the options are not set by
using the IS_ENABLED() macro instead of ifdefs. All other MSI related
code compiles even without CONFIG_PCI_MSI being set.

The problem with the above function is that the msi_list member of
struct pci_device is only declared if CONFIG_PCI_MSI is set. So of you
really want to get rid of the above stub, my preferred solution would be
to remove the ifdef in struct pci_device, so we can compile the code
regardless of the config options.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |