Hi, Thank you for many suggestions!
On 2015/05/15 0:17, Christos Zoulas wrote: > On May 14, 7:40pm, [email protected] (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 <[email protected]>
