Re: [IWN] Reviw split 2
On 3 August 2013 04:23, Cedric GROSS c...@cgross.info wrote: Can you please post an updated diff against what's in -HEAD now? As requested here is full patch. Thanks! It should. 4965 part was not impacted. But Don't you said that full patch break your 5100 ? Yup, it is breaking it very quickly. I'll try this patch against -HEAD and see what happens. But, there's ~ 4000 lines of patch to review. Some bits are easy to merge, some bits aren't easy to merge. :) Thanks! What would you like to merge next? -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part de Adrian Chadd Envoyé : samedi 3 août 2013 20:20 À : Cedric GROSS Cc : freebsd-wireless@freebsd.org Objet : Re: [IWN] Reviw split 2 On 3 August 2013 04:23, Cedric GROSS c...@cgross.info wrote: Can you please post an updated diff against what's in -HEAD now? As requested here is full patch. Thanks! It should. 4965 part was not impacted. But Don't you said that full patch break your 5100 ? Yup, it is breaking it very quickly. I'll try this patch against -HEAD and see what happens. Ok. But, there's ~ 4000 lines of patch to review. Some bits are easy to merge, some bits aren't easy to merge. :) It's surely in parameters part that there's a fail. May be will end by that. Thanks! What would you like to merge next? Prepare for context switching (the sc-rxon modification), it's still modification without adding functionality. And after that, adding context switching with PAN support, should not break your NIC. Next, may be parameter by parameter, so we will see where is break. With bapt help, I'm also pointing a problem with AMRR. With time, rate is still decreasing because of cumulative ackfailcnt transmit to ieee80211_ratectl_tx_complete. What kind of value does this function wait ? Absolute number or relative to the previous call ? Also, why do you send it by ref in iwn_tx_done ? -adrian Cedric ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Ok, why'd you change the debug print macro to check if the debug flags match the check, rather than if the debug flags are set in the check? ie (f) (v) versus ( (f) (v) == (v) ) ? I'd like to tidy up the debugging changes that are left in your source file before we move onto the next bits. -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part de Adrian Chadd Envoyé : samedi 3 août 2013 21:37 À : Cedric GROSS Cc : freebsd-wireless@freebsd.org Objet : Re: [IWN] Reviw split 2 Ok, why'd you change the debug print macro to check if the debug flags match the check, rather than if the debug flags are set in the check? ie (f) (v) versus ( (f) (v) == (v) ) ? It's for reducing tracing verbosity and just do trace when associate with another IWN_DEBUG_* So if you wish to debug only XMIT, trace also print only associate with that level (ie IWN_DEBUG_TRACE | IWN_DEBUG_XMIT) I'd like to tidy up the debugging changes that are left in your source file before we move onto the next bits. -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
On 3 August 2013 12:43, Cedric GROSS c...@cgross.info wrote: Ok, why'd you change the debug print macro to check if the debug flags match the check, rather than if the debug flags are set in the check? ie (f) (v) versus ( (f) (v) == (v) ) ? It's for reducing tracing verbosity and just do trace when associate with another IWN_DEBUG_* So if you wish to debug only XMIT, trace also print only associate with that level (ie IWN_DEBUG_TRACE | IWN_DEBUG_XMIT) Ok. I like the general idea, but I think overloading that for the general case is against POLA. Eg, ath(4), ath_hal(4), net80211(4) all have the mask idea, rather than the exact match idea. So there are cases where multiple bits are set in a debug mask (eg some INPUT and 11N flags in net80211) since they're relevant for both. So I'd like to come up with an alternative way to do trace debugging like you ask. Maybe what we should do is add a DPRINTF_TRACE() macro for things that are specifically _trace_ events, then have a separate trace bitmap for trace debugging. -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
-Message d'origine- De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part de Adrian Chadd Envoyé : samedi 3 août 2013 21:50 À : Cedric GROSS Cc : freebsd-wireless@freebsd.org Objet : Re: [IWN] Reviw split 2 On 3 August 2013 12:43, Cedric GROSS c...@cgross.info wrote: Ok, why'd you change the debug print macro to check if the debug flags match the check, rather than if the debug flags are set in the check? ie (f) (v) versus ( (f) (v) == (v) ) ? It's for reducing tracing verbosity and just do trace when associate with another IWN_DEBUG_* So if you wish to debug only XMIT, trace also print only associate with that level (ie IWN_DEBUG_TRACE | IWN_DEBUG_XMIT) Ok. I like the general idea, but I think overloading that for the general case is against POLA. Eg, ath(4), ath_hal(4), net80211(4) all have the mask idea, rather than the exact match idea. So there are cases where multiple bits are set in a debug mask (eg some INPUT and 11N flags in net80211) since they're relevant for both. So I'd like to come up with an alternative way to do trace debugging like you ask. Maybe what we should do is add a DPRINTF_TRACE() macro for things that are specifically _trace_ events, then have a separate trace bitmap for trace debugging. Ok, I'll do that. -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Nah, let's take it easier for now. Just leave the trace flags where they are in your diff, remove the flags check so the debug mask works the way it is, and submit a diff with _just_ the remaining debug flags line changes. That way we know what the required changes are, it will only bug people doing active testing (ie, you, me and -wireless) and we have the flags there for when we do sit down and figure out the right way to do trace debugging. Let's get these debug flags for trace in the tree, shorten the diff, and move onto the next set of structural changes. -adrian ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
-Message d'origine- De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part de Adrian Chadd Envoyé : jeudi 1 août 2013 23:49 À : Cedric GROSS Cc : freebsd-wireless@freebsd.org Objet : Re: [IWN] Reviw split 2 Committed! I'm about to commit the config options change so you can add IWN_DEBUG to your kernel config. Next! Want to send out an updated diff? You already have the split 3 (previously sent). I test it, it's still apply well against -HEAD. Cedric ___ freebsd-wireless@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-wireless To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Committed!
I'm about to commit the config options change so you can add IWN_DEBUG
to your kernel config.
Next! Want to send out an updated diff?
-adrian
On 31 July 2013 08:36, Cedric GROSS c...@cgross.info wrote:
-Message d'origine-
De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part
de Adrian Chadd
Envoyé : mercredi 31 juillet 2013 17:08
À : Cedric GROSS
Cc : freebsd-wireless@freebsd.org
Objet : Re: [IWN] Reviw split 2
Hi,
There's some more whitespace things to fix in your diff.
-
-bus_dmamap_sync(sc-rxq.data_dmat, data-map,
-BUS_DMASYNC_POSTREAD);
-DPRINTF(sc, IWN_DEBUG_ANY,
+...DPRINTF(sc, IWN_DEBUG_ANY,
%s: scanning channel %d status %x\n,
__func__, scan-chan, le32toh(scan-
status));
.. notice how you've indented DPRINTF there? You should fix that. :)
Fixed.
+#ifdefIWN_DEBUG
+#define IWN_DESC(x) case x:...return #x #define COUNTOF(array)
+(sizeof(array) / sizeof(array[0]))
There should be a tab between the #define and the thing you're
defining, rather than a space.
Done.
+ * This function print firmawre register
.. typo, that should be firmware :)
Yep fixed.
+..};
+..DPRINTF(sc, IWN_DEBUG_REGISTER,
+CSR values: (2nd byte of IWN_INT_COALESCING is
IWN_INT_PERIODIC)%s,
+\n);
+..for (i = 0; i COUNTOF(csr_tbl); i++){
.. there needs to be a tab in front of the two lines after the
DPRINTF(). Well, strictly speaking, there should be a tab (to bring it
to the same indent level) and then four spaces (as it's a continuation
of the line above it.)
Fixed
Now, you're making IWN_DEBUG an option, right? Once you've done this,
I'll go make sure you can put it in the kernel config file as a build
option (and I'll enable it by default on i386/amd64.)
Yes, I think should be a good thing.
Thanks!
-adrian
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Hi,
There's some more whitespace things to fix in your diff.
-
-bus_dmamap_sync(sc-rxq.data_dmat, data-map,
-BUS_DMASYNC_POSTREAD);
-DPRINTF(sc, IWN_DEBUG_ANY,
+...DPRINTF(sc, IWN_DEBUG_ANY,
%s: scanning channel %d status %x\n,
__func__, scan-chan, le32toh(scan-status));
.. notice how you've indented DPRINTF there? You should fix that. :)
+#ifdefIWN_DEBUG
+#define IWN_DESC(x) case x:...return #x
+#define COUNTOF(array) (sizeof(array) / sizeof(array[0]))
There should be a tab between the #define and the thing you're
defining, rather than a space.
+ * This function print firmawre register
.. typo, that should be firmware :)
+..};
+..DPRINTF(sc, IWN_DEBUG_REGISTER,
+CSR values: (2nd byte of IWN_INT_COALESCING is IWN_INT_PERIODIC)%s,
+\n);
+..for (i = 0; i COUNTOF(csr_tbl); i++){
.. there needs to be a tab in front of the two lines after the
DPRINTF(). Well, strictly speaking, there should be a tab (to bring it
to the same indent level) and then four spaces (as it's a continuation
of the line above it.)
Now, you're making IWN_DEBUG an option, right? Once you've done this,
I'll go make sure you can put it in the kernel config file as a build
option (and I'll enable it by default on i386/amd64.)
Thanks!
-adrian
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
-Message d'origine-
De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part
de Adrian Chadd
Envoyé : mercredi 31 juillet 2013 17:08
À : Cedric GROSS
Cc : freebsd-wireless@freebsd.org
Objet : Re: [IWN] Reviw split 2
Hi,
There's some more whitespace things to fix in your diff.
-
-bus_dmamap_sync(sc-rxq.data_dmat, data-map,
-BUS_DMASYNC_POSTREAD);
-DPRINTF(sc, IWN_DEBUG_ANY,
+...DPRINTF(sc, IWN_DEBUG_ANY,
%s: scanning channel %d status %x\n,
__func__, scan-chan, le32toh(scan-
status));
.. notice how you've indented DPRINTF there? You should fix that. :)
Fixed.
+#ifdefIWN_DEBUG
+#define IWN_DESC(x) case x:...return #x #define COUNTOF(array)
+(sizeof(array) / sizeof(array[0]))
There should be a tab between the #define and the thing you're
defining, rather than a space.
Done.
+ * This function print firmawre register
.. typo, that should be firmware :)
Yep fixed.
+..};
+..DPRINTF(sc, IWN_DEBUG_REGISTER,
+CSR values: (2nd byte of IWN_INT_COALESCING is
IWN_INT_PERIODIC)%s,
+\n);
+..for (i = 0; i COUNTOF(csr_tbl); i++){
.. there needs to be a tab in front of the two lines after the
DPRINTF(). Well, strictly speaking, there should be a tab (to bring it
to the same indent level) and then four spaces (as it's a continuation
of the line above it.)
Fixed
Now, you're making IWN_DEBUG an option, right? Once you've done this,
I'll go make sure you can put it in the kernel config file as a build
option (and I'll enable it by default on i386/amd64.)
Yes, I think should be a good thing.
Thanks!
-adrian
register3.patch
Description: Binary data
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Looks good! I'll commit this soon.
Thanks!
-adrian
On 31 July 2013 08:36, Cedric GROSS c...@cgross.info wrote:
-Message d'origine-
De : adrian.ch...@gmail.com [mailto:adrian.ch...@gmail.com] De la part
de Adrian Chadd
Envoyé : mercredi 31 juillet 2013 17:08
À : Cedric GROSS
Cc : freebsd-wireless@freebsd.org
Objet : Re: [IWN] Reviw split 2
Hi,
There's some more whitespace things to fix in your diff.
-
-bus_dmamap_sync(sc-rxq.data_dmat, data-map,
-BUS_DMASYNC_POSTREAD);
-DPRINTF(sc, IWN_DEBUG_ANY,
+...DPRINTF(sc, IWN_DEBUG_ANY,
%s: scanning channel %d status %x\n,
__func__, scan-chan, le32toh(scan-
status));
.. notice how you've indented DPRINTF there? You should fix that. :)
Fixed.
+#ifdefIWN_DEBUG
+#define IWN_DESC(x) case x:...return #x #define COUNTOF(array)
+(sizeof(array) / sizeof(array[0]))
There should be a tab between the #define and the thing you're
defining, rather than a space.
Done.
+ * This function print firmawre register
.. typo, that should be firmware :)
Yep fixed.
+..};
+..DPRINTF(sc, IWN_DEBUG_REGISTER,
+CSR values: (2nd byte of IWN_INT_COALESCING is
IWN_INT_PERIODIC)%s,
+\n);
+..for (i = 0; i COUNTOF(csr_tbl); i++){
.. there needs to be a tab in front of the two lines after the
DPRINTF(). Well, strictly speaking, there should be a tab (to bring it
to the same indent level) and then four spaces (as it's a continuation
of the line above it.)
Fixed
Now, you're making IWN_DEBUG an option, right? Once you've done this,
I'll go make sure you can put it in the kernel config file as a build
option (and I'll enable it by default on i386/amd64.)
Yes, I think should be a good thing.
Thanks!
-adrian
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
RE: [IWN] Reviw split 2
-Message d'origine-
De : owner-freebsd-wirel...@freebsd.org [mailto:owner-freebsd-
wirel...@freebsd.org] De la part de Adrian Chadd
Envoyé : dimanche 28 juillet 2013 21:25
À : Cedric GROSS
Cc : freebsd-wireless@freebsd.org
Objet : Re: [IWN] Reviw split 2
Hi!
Feedback time!
- DPRINTF(sc, IWN_DEBUG_TRACE, -%s done\n, __func__);
+ DPRINTF(sc, IWN_DEBUG_TRACE, -%s: end\n,__func__);
+
.. all that did was delete a space and add a new line. Just add the
space back in there, delete the new line; then that part of the diff
will disappear.
Replace also done by end to be coherent with other trace
- if (!(sc-sc_flags IWN_FLAG_HAS_11N)) {
+ if (!(sc-sc_flags IWN_FLAG_HAS_11N)){
Again, you've just deleted a space. You don't need to do that. :-) Just
put the space back in, that part of the diff will disappear.
- break;
+#endif
+ break;
.. the #endif is fine, but you reformatted the break line. tsk.
There's another break, and another DPRINTF that you did that to.
Ok
Then in the include file:
-#define IWN_INT0x008
+#define IWN_INT0x008
#define IWN_INT_MASK 0x00c
-#define IWN_FH_INT 0x010
-#define IWN_RESET 0x020
+#define IWN_FH_INT 0x010
+#define IWN_GPIO_IN0x018 /* read external chip
pins */
+#define IWN_RESET 0x020
#define IWN_GP_CNTRL 0x024
-#define IWN_HW_REV 0x028
-#define IWN_EEPROM 0x02c
+#define IWN_HW_REV 0x028
+#define IWN_EEPROM 0x02c
.. most of those are just tab spaces. You can eliminate those, and all
you'd be adding in is the GPIO_IN line.
I like to see align values but anyway.
Here's my .vimrc file. It includes colour marking for tabs/spaces,
indenting, and lines longer than 78 characters. Thank Bernhard for it.
It's great for inspecting code and diffs for things like whitespace
changes, trailing whitespace at the end of a line, tabs which should be
spaces (and vice versa), etc.
adrian@lucy:~] cat ~/.vimrc
LCD-friendly!
:set bg=dark
Do general syntax highlighting
:syntax enable
Highlight tab characters as . (with . being tab spaces) Highlight
extra spaces (ie, before the end of line) with C
:set list
:set listchars=tab:.,trail:C
:highlight NearLength ctermbg=magenta ctermfg=white guibg=#592959
:highlight OverLength ctermbg=red ctermfg=white guibg=#592929 Match
characters after position 78; set highlight to be magenta Second
match characters after position 80; set highlight to be red :match
NearLength /\%78v.\+/ :2match OverLength /\%81v.\+/
Enable auto-indenting
:set ai
Thanks :)
On 27 July 2013 09:09, Cedric GROSS gros...@free.fr wrote:
Hello,
A new patch.
This one add :
- a debugging function printing uCode registery.
- IWN_DEBUG make option
A review has be done also which can safely undefined IWN_DEBUG.
Previously do so lead to compile error.
Cedric
Joined, corrected patch.
Cedric
register2.patch
Description: Binary data
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
Re: [IWN] Reviw split 2
Hi!
Feedback time!
- DPRINTF(sc, IWN_DEBUG_TRACE, -%s done\n, __func__);
+ DPRINTF(sc, IWN_DEBUG_TRACE, -%s: end\n,__func__);
+
.. all that did was delete a space and add a new line. Just add the
space back in there, delete the new line; then that part of the diff
will disappear.
- if (!(sc-sc_flags IWN_FLAG_HAS_11N)) {
+ if (!(sc-sc_flags IWN_FLAG_HAS_11N)){
Again, you've just deleted a space. You don't need to do that. :-)
Just put the space back in, that part of the diff will disappear.
- break;
+#endif
+ break;
.. the #endif is fine, but you reformatted the break line. tsk.
There's another break, and another DPRINTF that you did that to.
Then in the include file:
-#define IWN_INT0x008
+#define IWN_INT0x008
#define IWN_INT_MASK 0x00c
-#define IWN_FH_INT 0x010
-#define IWN_RESET 0x020
+#define IWN_FH_INT 0x010
+#define IWN_GPIO_IN0x018 /* read external chip pins */
+#define IWN_RESET 0x020
#define IWN_GP_CNTRL 0x024
-#define IWN_HW_REV 0x028
-#define IWN_EEPROM 0x02c
+#define IWN_HW_REV 0x028
+#define IWN_EEPROM 0x02c
.. most of those are just tab spaces. You can eliminate those, and all
you'd be adding in is the GPIO_IN line.
Here's my .vimrc file. It includes colour marking for tabs/spaces,
indenting, and lines longer than 78 characters. Thank Bernhard for it.
It's great for inspecting code and diffs for things like whitespace
changes, trailing whitespace at the end of a line, tabs which should
be spaces (and vice versa), etc.
adrian@lucy:~] cat ~/.vimrc
LCD-friendly!
:set bg=dark
Do general syntax highlighting
:syntax enable
Highlight tab characters as . (with . being tab spaces)
Highlight extra spaces (ie, before the end of line) with C
:set list
:set listchars=tab:.,trail:C
:highlight NearLength ctermbg=magenta ctermfg=white guibg=#592959
:highlight OverLength ctermbg=red ctermfg=white guibg=#592929
Match characters after position 78; set highlight to be magenta
Second match characters after position 80; set highlight to be red
:match NearLength /\%78v.\+/
:2match OverLength /\%81v.\+/
Enable auto-indenting
:set ai
On 27 July 2013 09:09, Cedric GROSS gros...@free.fr wrote:
Hello,
A new patch.
This one add :
- a debugging function printing uCode registery.
- IWN_DEBUG make option
A review has be done also which can safely undefined IWN_DEBUG. Previously
do so lead to compile error.
Cedric
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
___
freebsd-wireless@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-wireless
To unsubscribe, send any mail to freebsd-wireless-unsubscr...@freebsd.org
