Re: change MSI/MSI-X APIs

[christos%astron.com@localhost (Christos Zoulas)]

In article <5553298E.5030006%iij.ad.jp@localhost>,
Kengo NAKAHARA  <k-nakahara%iij.ad.jp@localhost> wrote:
>Hi,
>
>On 2015/05/12 23:37, Christos Zoulas wrote:
>> In article <55516416.3050901%iij.ad.jp@localhost> you write:
>>> Because some device drivers must select INTx, MSI, or MSI-X depending
>>> on using devices; e.g. some devices have errata about MSI-X,
>>> so the device driver cannot use MSI-X even if the devices have
>>> MSI-X capability. In addition, some other the device drivers should
>>> use MSI-X and INTx only since some devices have errata about MSI.
>>> In a similar way, other device drivers should use MSI-X and MSI only.
>>>
>>> These selection logic codes can be done only in the device drivers
>>> codes, therefore it is required for the device drivers to be able to
>>> select interrupt types by allocation APIs.
>>>
>>>> Also can we add the disestablish/free code in the driver? What are th=
>e
>>>> steps to undo the interrupt allocation/establishment?
>>>
>>> Here is the example code of if_wm:
>>>    http://www.netbsd.org/~knakahara/unify-msi-apis/if_wm-example.diff
>>>
>>> This diff includes pci_intr_disestablish()/pci_intr_release() and
>>> pci_{intx,msi,msix}_alloc()/pci_intr_establish().
>>> How about this sample code?
>>=20
>> I don't see where it free's the sc_intrs[] it allocated in the msi* all=
>oc
>> functions. I think that a lot of the code could be merged as follows:
>>=20
>> 	http://www.netbsd.org/~christos/if_wm-example.diff
>> 	[I have not even tried to compile this, just some thoughts]
>>=20
>> What do you think?
>
>At first, in my example code of if_wm, "sc->sc_intrs" is allocated by
>pci_msix_alloc_exact(), pci_msi_alloc_exact(), or pci_intr_alloc().=20
># pci_msix_alloc_exact() and pci_msi_alloc_exact() allocate the array
># of pci_intr_handle_t (and setting other resources such as mapping
># MSI-X table).
>And then "sc->sc_intrs" is freed by pci_intr_release().
># pci_intr_release() free all of the pci_intr_handle_t array (and
># destruct other resources such as unmapping MSI-X table).
>I think above.... Well, could you tell me what do you mean
>"free's the sc_intrs[]"?

It was not obvious to me that pci_intr_release() free's all the handles.
The API certainly does not feel symmetric. Calling something with 'alloc'
int the name usually expects something to call 'free' later.

>Sorry, my example is a very simple pattern. A practical example is using
>multiqueue. In the multiqueue case, the number of TX interrupt handlers
>and RX interrupt handlers is decided dynamically by ncpu and the device's
>MSI-X vector number. So, in the multiqueue pattern, it cannot use
>such *static* struct {} wm_msix_info[].
>My example is also intended multiqueue expansion, therefore it divides
>processing to establish TX interrupt(s), to establish RX interrupt(s),
>and to establish a link-state-changing interrupt.
># "WM_NINTR" must be gone and it is required to decide dynamically
># the number of using interrupts in the future.

Still, there is room for sharing the code no matter what's the count
of allocated interrupts per group. Again, the way I see it is that
you want a function that allocates a vector of interrupts the best
way it can (in general) and have a way to specify quirks for cards
that need to do something special. How you split the interrupts in
the driver between functions depends on the driver, but the driver
does not really care in general what flavor the interrupts are (but
it might want to ask if it needs to differentiate). Having 150+ lines
of semi-duplicated code to do interrupt setup does not make a lot
of sense to me.

christos