Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
Re: change MSI/MSI-X APIs
2 views
Skip to first unread message
Kengo NAKAHARA
Jul 15, 2015, 2:55:01 AM7/15/15
to
Hi,
I'm sorry for late reply.
On 2015/05/31 6:45, David Laight wrote:
> On Mon, May 11, 2015 at 03:15:25PM +0900, Kengo NAKAHARA wrote:
>> Hi,
>>
>> I received feedback from some device driver authors. They point out
>> establish, disestablish and release APIs should be unified for INTx,
>> MSI and MSI-X. So, I would change the APIs as below:
>
> Some more feedback...
>
> PCIe devices that only support MSI-X could have support for very
> large numbers of interrupts.
>
> Some might only be needed if a specific function is used,
> or might only be used to load sharing (eg multi-q ethernet).
> In either case you might want to allocate some of the MSI-X
> vectors at driver load time, and allocate others at a much later
> time.
> IIRC nothing in the hardware spec stops you doing this.
>
> Theoretically you could allocate the MSI-X vector (etc)
> when the interrupt is enabled, and dealloacate on disable
> (apart from timing problems on the hardware).
> I'm not suggesting you go that far!
>
> This would also make it less likely that interrupts won't
> be available for drivers that initialise later on.
Theoretically yes, but I think it is less benefit in spite of the
complex implementation.
# e.g. error handling code and fallback code to MSI or INTx
Since MSI-X vectors are device specific resources, the other devices
which are initialized later can allocate MSI-X vectors independently
from the devices initialized before.
Of course, the device allocates a bit of memory when it call
pci_msi{,x}_alloc APIs. However, the amount of memory is little, so
I think there is only few cases which the later initialized devices
can be available by allocating MSI-X vectors at enabling interrupt
time.
Thanks,
--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.
Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit
Kengo NAKAHARA <k-nak...@iij.ad.jp>
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de
I'm sorry for late reply.
On 2015/05/31 6:45, David Laight wrote:
> On Mon, May 11, 2015 at 03:15:25PM +0900, Kengo NAKAHARA wrote:
>> Hi,
>>
>> I received feedback from some device driver authors. They point out
>> establish, disestablish and release APIs should be unified for INTx,
>> MSI and MSI-X. So, I would change the APIs as below:
>
> Some more feedback...
>
> PCIe devices that only support MSI-X could have support for very
> large numbers of interrupts.
>
> Some might only be needed if a specific function is used,
> or might only be used to load sharing (eg multi-q ethernet).
> In either case you might want to allocate some of the MSI-X
> vectors at driver load time, and allocate others at a much later
> time.
> IIRC nothing in the hardware spec stops you doing this.
>
> Theoretically you could allocate the MSI-X vector (etc)
> when the interrupt is enabled, and dealloacate on disable
> (apart from timing problems on the hardware).
> I'm not suggesting you go that far!
>
> This would also make it less likely that interrupts won't
> be available for drivers that initialise later on.
Theoretically yes, but I think it is less benefit in spite of the
complex implementation.
# e.g. error handling code and fallback code to MSI or INTx
Since MSI-X vectors are device specific resources, the other devices
which are initialized later can allocate MSI-X vectors independently
from the devices initialized before.
Of course, the device allocates a bit of memory when it call
pci_msi{,x}_alloc APIs. However, the amount of memory is little, so
I think there is only few cases which the later initialized devices
can be available by allocating MSI-X vectors at enabling interrupt
time.
Thanks,
--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.
Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit
Kengo NAKAHARA <k-nak...@iij.ad.jp>
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de
Kengo NAKAHARA
Jul 15, 2015, 3:00:51 AM7/15/15
to
Hi,
On 2015/05/19 2:39, Terry Moore wrote:
>> -----Original Message-----
>> From: tech-ke...@NetBSD.org [mailto:tech-ke...@NetBSD.org] On
>> Behalf Of Kengo NAKAHARA
>> Sent: Sunday, May 17, 2015 23:58
>> To: tech...@netbsd.org; t...@mcci.com
>> Cc: chri...@astron.com
>> Subject: Re: change MSI/MSI-X APIs
>>
>>> I'm sure there are even better ways to handle this variation; just
> making a
>>> concrete example of how it might be done.
>>
>> What do you think about below pci_intr_alloc() API?
>> http://mail-index.netbsd.org/tech-kern/2015/05/18/msg018725.html
>>
>> I think this API simplify for many device drivers to use, however I am
>> unsure this API meets your pointing out.
>> Could you comment?
>
> Hi Nakahara-san,
>
> I think you and Christos are converging on something that will be quite
> suitable. "Too many cooks spoil the broth", as the cliché goes, so I'll stay
> out of the kitchen.
I am going to propose fixed API patch soon. If you think the patch is not
suitable, I welcome your return. :)
On 2015/05/19 2:39, Terry Moore wrote:
>> -----Original Message-----
>> From: tech-ke...@NetBSD.org [mailto:tech-ke...@NetBSD.org] On
>> Behalf Of Kengo NAKAHARA
>> Sent: Sunday, May 17, 2015 23:58
>> To: tech...@netbsd.org; t...@mcci.com
>> Cc: chri...@astron.com
>> Subject: Re: change MSI/MSI-X APIs
>>
>>> I'm sure there are even better ways to handle this variation; just
> making a
>>> concrete example of how it might be done.
>>
>> What do you think about below pci_intr_alloc() API?
>> http://mail-index.netbsd.org/tech-kern/2015/05/18/msg018725.html
>>
>> I think this API simplify for many device drivers to use, however I am
>> unsure this API meets your pointing out.
>> Could you comment?
>
> Hi Nakahara-san,
>
> I think you and Christos are converging on something that will be quite
> suitable. "Too many cooks spoil the broth", as the cliché goes, so I'll stay
> out of the kitchen.
I am going to propose fixed API patch soon. If you think the patch is not
suitable, I welcome your return. :)
Kengo NAKAHARA
Jul 15, 2015, 6:45:00 AM7/15/15
to
Hi,
I'm sorry for late reply, too.
On 2015/05/18 22:05, Christos Zoulas wrote:
> On May 18, 3:57pm, k-nak...@iij.ad.jp (Kengo NAKAHARA) wrote:
> -- Subject: Re: change MSI/MSI-X APIs
>
> - by max (-1) I meant the maximum number that the bus will allow. The driver
> would then decide how to split the interrupts amongst functions.
OK, I implemented such "max" number.
> - I considered using unsigned int/size_t in the counts argument and then
> I thought that it is best if it agreed with the return type, since we
> overload the return type with (negative error/positive number of interrupts).
> Another way to do this would be to return the number of interrupts allocated
> in the 0'th member of the counts array.
I implemented like latter way, but I think 0'th member may cause
confusion. So, I implemented that the function sets really allocated
counts to the same member of the counts array.
e.g. if the driver call pci_intr_alloc() in following way
====================
int counts[PCI_INTR_TYPE_SIZE];
counts[PCI_INTR_TYPE_MSIX] = 5;
counts[PCI_INTR_TYPE_MSI] = 1;
counts[PCI_INTR_TYPE_INTX] = 1;
error = pci_intr_alloc(pa, ihps, counts, PCI_INTR_TYPE_MSIX);
====================
on success of MSI-X allocation, "counts" is overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 5;
counts[PCI_INTR_TYPE_MSI] == 0;
counts[PCI_INTR_TYPE_INTX] == 0;
====================
on failed MSI-X but success of MSI allocation, "counts" is
overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 0;
counts[PCI_INTR_TYPE_MSI] == 1;
counts[PCI_INTR_TYPE_INTX] == 0;
====================
on failed MSI-X and MSI, but success of INTx allocation,
"counts" is overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 0;
counts[PCI_INTR_TYPE_MSI] == 0;
counts[PCI_INTR_TYPE_INTX] == 1;
====================
on failed (of all allocation functions), pci_intr_alloc() returns
non-zero value, and the all members of "counts" are 0.
On success, the driver can also use below simple way
====================
allocated_count = counts[pci_intr_type(intr_handlers[0]);
====================
Thus, here is the implementation of above specification (include man)
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
furthermore, here is if_wm usage example by msaitoh@n.o
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
Could you comment this patch?
I'm sorry for late reply, too.
On 2015/05/18 22:05, Christos Zoulas wrote:
> On May 18, 3:57pm, k-nak...@iij.ad.jp (Kengo NAKAHARA) wrote:
> -- Subject: Re: change MSI/MSI-X APIs
>
> | What do you think about below pci_intr_alloc() API?
> | http://mail-index.netbsd.org/tech-kern/2015/05/18/msg018725.html
> |
> | I think this API simplify for many device drivers to use, however I am
> | unsure this API meets your pointing out.
> | Could you comment?
>
> I am fine with it; just some clarifications:
> | What do you think about below pci_intr_alloc() API?
> | http://mail-index.netbsd.org/tech-kern/2015/05/18/msg018725.html
> |
> | I think this API simplify for many device drivers to use, however I am
> | unsure this API meets your pointing out.
> | Could you comment?
>
>
> - by max (-1) I meant the maximum number that the bus will allow. The driver
> would then decide how to split the interrupts amongst functions.
OK, I implemented such "max" number.
> - I considered using unsigned int/size_t in the counts argument and then
> I thought that it is best if it agreed with the return type, since we
> overload the return type with (negative error/positive number of interrupts).
> Another way to do this would be to return the number of interrupts allocated
> in the 0'th member of the counts array.
I implemented like latter way, but I think 0'th member may cause
confusion. So, I implemented that the function sets really allocated
counts to the same member of the counts array.
e.g. if the driver call pci_intr_alloc() in following way
====================
int counts[PCI_INTR_TYPE_SIZE];
counts[PCI_INTR_TYPE_MSIX] = 5;
counts[PCI_INTR_TYPE_MSI] = 1;
counts[PCI_INTR_TYPE_INTX] = 1;
error = pci_intr_alloc(pa, ihps, counts, PCI_INTR_TYPE_MSIX);
====================
on success of MSI-X allocation, "counts" is overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 5;
counts[PCI_INTR_TYPE_MSI] == 0;
counts[PCI_INTR_TYPE_INTX] == 0;
====================
on failed MSI-X but success of MSI allocation, "counts" is
overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 0;
counts[PCI_INTR_TYPE_MSI] == 1;
counts[PCI_INTR_TYPE_INTX] == 0;
====================
on failed MSI-X and MSI, but success of INTx allocation,
"counts" is overwritten below
====================
counts[PCI_INTR_TYPE_MSIX] == 0;
counts[PCI_INTR_TYPE_MSI] == 0;
counts[PCI_INTR_TYPE_INTX] == 1;
====================
on failed (of all allocation functions), pci_intr_alloc() returns
non-zero value, and the all members of "counts" are 0.
On success, the driver can also use below simple way
====================
allocated_count = counts[pci_intr_type(intr_handlers[0]);
====================
Thus, here is the implementation of above specification (include man)
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
furthermore, here is if_wm usage example by msaitoh@n.o
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
Could you comment this patch?
Christos Zoulas
Jul 15, 2015, 10:27:35 AM7/15/15
to
In article <55A63935...@iij.ad.jp>,
+.Ft int
+.Fn pci_intr_alloc "struct pci_attach_args *pa" \
+"pci_intr_handle_t **ihp" "pci_intr_type_t *counts" \
+"pci_intr_type_t max_type"
And then in the examples you are passing it "int counts[x];":
+ int counts[PCI_INTR_TYPE_SIZE];
+ counts[PCI_INTR_TYPE_MSIX] = 5;
+ counts[PCI_INTR_TYPE_MSI] = 1;
+ counts[PCI_INTR_TYPE_INTX] = 1;
+ error = pci_intr_alloc(pa, ihps, counts,
I think you want int *counts in the declaration; this also avoids the problem
of the enum being possibly unsigned (depending on the implementation).
>http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
I like the patch. There is a typo "interrput".
>Could you comment this patch?
I like it! Thanks for working on this...
christos
Kengo NAKAHARA <k-nak...@iij.ad.jp> wrote:
>Thus, here is the implementation of above specification (include man)
> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
>furthermore, here is if_wm usage example by msaitoh@n.o
This has a problem; in the man and code page, you declare pci_intr_alloc as:
>Thus, here is the implementation of above specification (include man)
> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
>furthermore, here is if_wm usage example by msaitoh@n.o
+.Ft int
+.Fn pci_intr_alloc "struct pci_attach_args *pa" \
+"pci_intr_handle_t **ihp" "pci_intr_type_t *counts" \
+"pci_intr_type_t max_type"
And then in the examples you are passing it "int counts[x];":
+ int counts[PCI_INTR_TYPE_SIZE];
+ counts[PCI_INTR_TYPE_MSIX] = 5;
+ counts[PCI_INTR_TYPE_MSI] = 1;
+ counts[PCI_INTR_TYPE_INTX] = 1;
+ error = pci_intr_alloc(pa, ihps, counts,
I think you want int *counts in the declaration; this also avoids the problem
of the enum being possibly unsigned (depending on the implementation).
>http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
I like the patch. There is a typo "interrput".
>Could you comment this patch?
christos
Kengo NAKAHARA
Jul 16, 2015, 2:20:40 AM7/16/15
to
Hi,
On 2015/07/15 23:27, Christos Zoulas wrote:
> In article <55A63935...@iij.ad.jp>,
> Kengo NAKAHARA <k-nak...@iij.ad.jp> wrote:
>
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
>> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
>
> I like the patch. There is a typo "interrput".
I fix typo and update counts of pci_intr_alloc() to use int * type.
Here is new patch,
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example2.patch
>> Could you comment this patch?
>
patches after a few days or weeks.
On 2015/07/15 23:27, Christos Zoulas wrote:
> In article <55A63935...@iij.ad.jp>,
> Kengo NAKAHARA <k-nak...@iij.ad.jp> wrote:
>
>> Thus, here is the implementation of above specification (include man)
>> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
>> furthermore, here is if_wm usage example by msaitoh@n.o
>
>> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api.patch
>> furthermore, here is if_wm usage example by msaitoh@n.o
>
> This has a problem; in the man and code page, you declare pci_intr_alloc as:
>
> +.Ft int
> +.Fn pci_intr_alloc "struct pci_attach_args *pa" \
> +"pci_intr_handle_t **ihp" "pci_intr_type_t *counts" \
> +"pci_intr_type_t max_type"
>
> And then in the examples you are passing it "int counts[x];":
>
> + int counts[PCI_INTR_TYPE_SIZE];
> + counts[PCI_INTR_TYPE_MSIX] = 5;
> + counts[PCI_INTR_TYPE_MSI] = 1;
> + counts[PCI_INTR_TYPE_INTX] = 1;
> + error = pci_intr_alloc(pa, ihps, counts,
>
> I think you want int *counts in the declaration; this also avoids the problem
> of the enum being possibly unsigned (depending on the implementation).
Thank you for your comment. I fix pci_intr_alloc() uses int *counts.
>
> +.Ft int
> +.Fn pci_intr_alloc "struct pci_attach_args *pa" \
> +"pci_intr_handle_t **ihp" "pci_intr_type_t *counts" \
> +"pci_intr_type_t max_type"
>
> And then in the examples you are passing it "int counts[x];":
>
> + int counts[PCI_INTR_TYPE_SIZE];
> + counts[PCI_INTR_TYPE_MSIX] = 5;
> + counts[PCI_INTR_TYPE_MSI] = 1;
> + counts[PCI_INTR_TYPE_INTX] = 1;
> + error = pci_intr_alloc(pa, ihps, counts,
>
> I think you want int *counts in the declaration; this also avoids the problem
> of the enum being possibly unsigned (depending on the implementation).
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
>> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example.patch
>
> I like the patch. There is a typo "interrput".
Here is new patch,
http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-example2.patch
>> Could you comment this patch?
>
> I like it! Thanks for working on this...
Could you comment? If there is no issue, I'm going to commit these
patches after a few days or weeks.
Christos Zoulas
Jul 16, 2015, 11:14:17 AM7/16/15
to
In article <55A72C01...@iij.ad.jp>,
+ 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.
> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-examp=
+ 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) {
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) {
So I wouldn't have to re-evaluate it.
LGTM, ship it.
christos
Kengo NAKAHARA <k-nak...@iij.ad.jp> 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,
>
>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
+ 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.
> http://netbsd.org/~knakahara/unify-alloc-api/unify-alloc-api-wm-examp=
>le2.patch
>
>>> Could you comment this patch?
+ /* Allocation settings */
>
>>> Could you comment this patch?
+ 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) {
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) {
So I wouldn't have to re-evaluate it.
LGTM, ship it.
christos
Kengo NAKAHARA
Jul 17, 2015, 3:00:31 AM7/17/15
to
Hi,
On 2015/07/17 0:13, Christos Zoulas wrote:
> In article <55A72C01...@iij.ad.jp>,
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=
[1] Current implementation
====================
counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
[2] Your implementation
====================
memset(counts, 0, sizeof(counts));
counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
====================
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!
On 2015/07/17 0:13, Christos Zoulas wrote:
> In article <55A72C01...@iij.ad.jp>,
> Kengo NAKAHARA <k-nak...@iij.ad.jp> 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
>
>>
>> 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.
> + 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.
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?
>
>>
>>>> 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.
> + 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) {
[1] Current implementation
====================
counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
counts[PCI_INTR_TYPE_MSI] = 1;
counts[PCI_INTR_TYPE_INTX] = 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;
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) {
> So I wouldn't have to re-evaluate it.
>
> LGTM, ship it.
Kengo NAKAHARA
Jul 17, 2015, 3:01:46 AM7/17/15
to
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...@iij.ad.jp>,
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!
# 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...@iij.ad.jp>,
> Kengo NAKAHARA <k-nak...@iij.ad.jp> 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
>
>>
>> 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.
> + 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
>>
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()
> + counts[PCI_INTR_TYPE_MSIX] = WM_MAX_NINTR;
>
> perhaps:
>
> + memset(counts, 0, sizeof(counts));
>
> before setting the counts?
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) {
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) {
> So I wouldn't have to re-evaluate it.
>
> LGTM, ship it.
Christos Zoulas
Jul 17, 2015, 4:31:04 AM7/17/15
to
| 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.
Yes, thanks I was very confused about it.
| 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.
christos
0 new messages