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-...@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



_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to