Hi, # Sorry, the mail I sent some hours ago is wrong... # This is correct mail.
On 2015/07/17 0:13, Christos Zoulas wrote:> In article <[email protected]>, > Kengo NAKAHARA <[email protected]> wrote: >> >> Thank you for your comment. I fix pci_intr_alloc() uses int *counts. >> I also fix missing about man installation and some wording. Here is >> new patch, >> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api2.patch > > +int pci_intr_alloc(const struct pci_attach_args *pa, > + pci_intr_handle_t **, int *, pci_intr_type_t); > > Parameters should not have names in declarations. The man wording could > be improved, but it is a complicated explanation and what is there now > is good enough. I missed the wrong parameter name, thanks. Thank you for comment to man, but I should modify man a little more about you point out below. >> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-examp= >> le2.patch >> >>>> Could you comment this patch? > > + /* Allocation settings */ > + counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR; > > perhaps: > > + memset(counts, 0, sizeof(counts)); > > before setting the counts? The driver does not have to do memset, because pci_intr_alloc() ignores the members of counts whose index is more than "max_type" parameter and the API does memset before overwriting allocated count. > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_MSIX) != 0) > { > > That should probably be: > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, __arraycount(counts)) != > 0) { > or: > + if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_SIZE) != 0) > { The 4th parameter(max_type) must be PCI_INTR_TYPE_MSIX, PCI_INTR_TYPE_MSI, or PCI_INTR_TYPE_INTX, because the parameter lets pci_intr_alloc() decide which interrupt type to try to allocate first. Due to this, my if_wm example was incorrect, sorry. I update the exmaple. http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example2.patch I should add explanation about this specification to man. > I would keep: > - if (pci_intr_type(sc->sc_intrs[0]) == PCI_INTR_TYPE_MSIX) { > + intr_type = pci_intr_type(sc->sc_intrs[0]); > + if (intr_type == PCI_INTR_TYPE_MSIX) { OK, I modify it above patch. > So I wouldn't have to re-evaluate it. > > LGTM, ship it. Thanks! -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
