Re: change MSI/MSI-X APIs

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

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

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