Re: [PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel owned devices

[Scott Wood <scottwood@freescale.com>]

On Thu, 2015-03-12 at 10:46 -0500, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, March 12, 2015 4:53 AM
> > To: Bhushan Bharat-R65777
> > Cc: linuxppc-dev@ozlabs.org
> > Subject: Re: [PATCH 3/4 RFC] fsl/msi: Add MSI bank allocation for kernel
> > owned devices
> > 
> > With the patchset as is, how would one indicate whether kernel devices
> > should get a bank?
> 
> For kernel owned device, in fsl_setup_msi_irqs() we check if there is
> a reserved MSI bank, if not then reserve a msi bank. If reserve fails
> then setup_msi_irq() fails. I think there is no fallback to Legacy
> interrupt.

If enabling MSI fails, it's up to the driver to fall back to legacy
interrupts (grep drivers for pci_enable_msi for examples).

> >  Specifically, when the kernel does have an MSI-
> > capable device but we'd prefer to use legacy interrupts to keep MSIs
> > available to VFIO.
> 
> Do we want this?

You'd previously raised concern about wanting to give all MSI banks to
VFIO.  The kernel might still have PCIe devices that are not
performance-critical.  That said, I'm not going to nack the patchset if
it requires the kernel to allocate a bank for itself.

> > > @@ -231,15 +304,12 @@ static int fsl_setup_msi_irqs(struct pci_dev
> > *pdev, int nvec, int type)
> > >  		if (specific_msi_bank) {
> > >  			hwirq = msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
> > >  		} else {
> > > -			/*
> > > -			 * Loop over all the MSI devices until we find one that
> > has an
> > > -			 * available interrupt.
> > > -			 */
> > > -			list_for_each_entry(msi_data, &msi_head, list) {
> > > -				hwirq = msi_bitmap_alloc_hwirqs(&msi_data-
> > >bitmap, 1);
> > > -				if (hwirq >= 0)
> > > -					break;
> > > +			msi_data = fsl_msi_get_reserved_msi_bank(pdev);
> > > +			if (!msi_data) {
> > > +				dev_err(&pdev->dev, "No MSI Bank allocated\n");
> > > +				goto out_free;
> > 
> > Is this really an error?  Seems like dev_info() would be more appropriate
> > to indicate the absence of a resource where you can fall back to another
> > option.
> 
> There is no fallback in fsl_setup_msi_irqs(), We have to error out from fsl_setup_msi_irqs(), no?

I meant as far as the user is concerned, not whether you return an error from the function.

-Scott