Re: change MSI/MSI-X APIs

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

Hi,

On 2015/07/17 0:13, Christos Zoulas wrote:
> In article <55A72C01.2050401%iij.ad.jp@localhost>,
> Kengo NAKAHARA  <k-nakahara%iij.ad.jp@localhost> 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?
> 
> +	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) {

Mmm, as a side of the API implementation, below 2 code are the same behavior.
[1] Current implementation
====================
        counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
        counts[PCI_INTR_TYPE_MSI] = 1;
        counts[PCI_INTR_TYPE_INTX] = 1;

        if (pci_intr_alloc(pa, &sc->sc_intrs, counts, PCI_INTR_TYPE_MSIX) != 0) {
====================

[2] Your implementation
====================
        memset(counts, 0, sizeof(counts));
        counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
        counts[PCI_INTR_TYPE_MSI] = 1;
        counts[PCI_INTR_TYPE_INTX] = 1;

        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) {
====================

I thought [1] was better specification, however I think again your
[2] is better. So, I should modify man and example code as match it.

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

> 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 <k-nakahara%iij.ad.jp@localhost>