Re: change MSI/MSI-X APIs

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for many suggestions!

On 2015/05/15 0:17, Christos Zoulas wrote:
> On May 14,  7:40pm, k-nakahara%iij.ad.jp@localhost (Kengo NAKAHARA) wrote:
> -- Subject: Re: change MSI/MSI-X APIs
> Here's a slightly modified version that gets rid of the flags and simplifies
> the more common case.
> 
> The following could be an enum:
> 
> typedef int pci_intr_type_t;
> 
> #define PCI_INTR_TYPE_INTX 0
> #define PCI_INTR_TYPE_MSI  1
> #define PCI_INTR_TYPE_MSIX 2
> #define PCI_INTR_TYPE_MAX  3

I agree it with obsoleting flag argument.

> int
> pci_intr_alloc(const struct pci_attach_args *, pci_intr_handle_t **ihps,
>     int *counts, size_t ncounts);
> 
> If the counts[i] == 0, then you don't allocate, if it is -1, you allocate max,
> otherwise you allocate up to count.

"count[i] == 0" case and "count[i] > 0" case seem good to me.

But, in the "count[i] == -1" case, what do you mean "max"? I think the needed
counts are decided only by device drivers, because there may be a limit by
hardware or software.
# One example of hardware is the numbers of NIC TX/RX queues. The numbers are
# different from MSI-X vector number. The numbers are decided from the model
# number as far as I know....
So, I think the inside of API does not know what value is appropriate.

Thus, I rethink "u_int *" type may be appropriate for "counts" argument.
I consider the type while implementing.

> NULL in the counts means do whatever you think is best.

Mmm..., it is difficult to decide "best".... Maybe, I think the way wanted
by many device drivers is:
    (1) allocate one MSI (not MSI-X) if the device can use MSI
    (2) allocate INTx if MSI cannot be allocated

Is there better way? If there is, I should use the way :)

> (I am passing the "ncounts" so that the API does not need to be versioned
> if there are more interrupt flavors added)

I agree completely. I did not think so much detail about when more interrupt
flavors is added.

> pci_intr_type_t pci_intr_type(pci_intr_handle_t *ihp);
> 
> xxxx_attach()
> {
> 
> We return a negative errno and a positive nintrs, so the common case where
> the driver does not care:
> 
> 	if  ((nintrs = pci_intr_alloc(*pa, &sc->sc_intrs, NULL, 0)) < 0) {
> 		error("foo %d\n", -nintrs);
> 		return;
> 	}
> 
> 	for (i = 0; i < nintrs; i++)
> 		sc->sc_ihs[i] = pci_intr_establish(pc, sc->sc_intrs[i], level,
> 		    func, arg);
> 
> Otherwise if we want to be selective:
> 
> 	int counts[PCI_INTR_TYPE_MAX];
> 	memset(counts, 0, sizeof(counts);
> 
> 	// Don't want msi, leave it 0
> 	counts[PCI_INTR_TYPE_MSIX] = -1;
> 	counts[PCI_INTR_TYPE_INTX] = -1;
> 
> 	if  ((nintrs = pci_intr_alloc(*pa, &sc->sc_intrs, counts,
> 	    PCI_INTR_TYPE_MAX)) < 0)
> 	    ....
> 
> What do you think?

It seems good to me.
I implement the API code and the practical usage example.


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>