On 7/19/07, Johannes Berg <[email protected]> wrote:
> Hi,
>
> It might be useful for you to put all your current code into a git tree
> and publish that to [email protected] so other people can
> take a look too :)
>
First of all, the code is far away be finished and full of bugs, We
can't success control the RX and TX yet.
For I just have a git tree in local machine, and it can't be access
from Internet, So, is there any place that can be used to put the code
or I have to apply a domain and build a server for it?
> That said,
>
> > Jeff write the RX&TX packet Description as Be order at
> > http://airgo.wdwconsulting.net/mymoin/PacketStructures,
>
> Don't have that right now, am offline.
It is online now.
>
> Well, if the packet was actually sent with your code then the hardware
> is most likely operating in little endian mode. It would, incidentally,
> be extremely confusing if the hardware worked in big endian mode, the
> 802.11 specs are quite suited for processing with a little-endian
> processor.
>
> In that case, you should write the struct as:
> struct agnx_desc {
> __le32 frag;
> __le32 address;
> } __attribute__((__packed__));
>
> and use, following my example above:
>
> desc->frag = cpu_to_le32(le32_to_cpu(desc->frag) | FRAG_OWNER))
>
Thanks for all your suggestions and fixes, I'll fix it later.
Because the code is big , so I attach it in this mail. (Be attention,
it is far from finished and has tons of bugs). And thank Jeff William
for his hard work of reverse engineer of this card. You can find more
info about the SPCS from his website
http://airgo.wdwconsulting.net/mymoin
BTW, I find a problem in the file ieee80211.c it locals in the
function "static int __ieee80211_tx()", I think the if (i ==
tx->u.tx.num_extra_frag) is ineffective, because 'i' always lower
than tx->u.tx.num_extra_frag. I guess maybe it should be if (i ==
(tx->u.tx.num_extra_frag - 1)), below is the patch, CMIIW.
BR
LiYanBo
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index fcad801..48c51bf 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -1406,7 +1406,7 @@ static int __ieee80211_tx(struct ieee80211_local
*local, struct sk_buff *
continue;
if (__ieee80211_queue_stopped(local, control->queue))
return IEEE80211_TX_FRAG_AGAIN;
- if (i == tx->u.tx.num_extra_frag) {
+ if (i == (tx->u.tx.num_extra_frag - 1)) {
control->tx_rate = tx->u.tx.last_frag_hwrate;
control->rate = tx->u.tx.last_frag_rate;
if (tx->u.tx.probe_last_frag)
On Thu, 2007-07-19 at 23:20 +0800, Li YanBo wrote:
> On 7/19/07, Johannes Berg <[email protected]> wrote:
> > Hi,
> >
>
> > It might be useful for you to put all your current code into a git tree
> > and publish that to [email protected] so other people can
> > take a look too :)
> >
>
> First of all, the code is far away be finished and full of bugs, We
> can't success control the RX and TX yet.
> For I just have a git tree in local machine, and it can't be access
> from Internet, So, is there any place that can be used to put the code
> or I have to apply a domain and build a server for it?
I guess you could, with some work, get an account on kernel.org, or I
can give you a tree on git.sipsolutions.net, or I'm sure there are other
people around as well...
johannes
On 7/19/07, Jiri Benc <[email protected]> wrote:
> On Thu, 19 Jul 2007 23:20:35 +0800, Li YanBo wrote:
> > BTW, I find a problem in the file ieee80211.c it locals in the
> > function "static int __ieee80211_tx()", I think the if (i ==
> > tx->u.tx.num_extra_frag) is ineffective, because 'i' always lower
> > than tx->u.tx.num_extra_frag. I guess maybe it should be if (i ==
> > (tx->u.tx.num_extra_frag - 1)), below is the patch, CMIIW.
>
> You're right. Could you add your Signed-off-by?
>
> Thanks,
>
> Jiri
>
Sorry for late response.
commit 800bbaa941213e8a7297d7fb9265f77f5a7b76af
Author: Li YanBo <[email protected]>
Date: Fri Jul 20 10:53:25 2007 +0800
[PATCH] mac80211: Fix change (i == tx->u.tx.num_extra_frag) to
(i == (tx->u.tx.num_extra_frag - 1)) then it can deal
the last frag correctly.
Signed-off-by: Li YanBo <[email protected]>
diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index fcad801..48c51bf 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -1406,7 +1406,7 @@ static int __ieee80211_tx(struct ieee80211_local
*local, struct sk_buff
continue;
if (__ieee80211_queue_stopped(local, control->queue))
return IEEE80211_TX_FRAG_AGAIN;
- if (i == tx->u.tx.num_extra_frag) {
+ if (i == (tx->u.tx.num_extra_frag - 1)) {
control->tx_rate = tx->u.tx.last_frag_hwrate;
control->rate = tx->u.tx.last_frag_rate;
if (tx->u.tx.probe_last_frag)
On 7/20/07, Johannes Berg <[email protected]> wrote:
> On Thu, 2007-07-19 at 23:20 +0800, Li YanBo wrote:
> > On 7/19/07, Johannes Berg <[email protected]> wrote:
> > > Hi,
> > >
> >
> > > It might be useful for you to put all your current code into a git tree
> > > and publish that to [email protected] so other people can
> > > take a look too :)
> > >
> >
> > First of all, the code is far away be finished and full of bugs, We
> > can't success control the RX and TX yet.
> > For I just have a git tree in local machine, and it can't be access
> > from Internet, So, is there any place that can be used to put the code
> > or I have to apply a domain and build a server for it?
>
> I guess you could, with some work, get an account on kernel.org, or I
Following your guider, I have send a apply to get a kernel.org
account. If all is ok, I plan build a tree at it, or else I will have
to disturb you again. anywhere, Thank you! :)
> can give you a tree on git.sipsolutions.net, or I'm sure there are other
> people around as well...
>
...hopefully I got the quoting level mess fixed... :-)
On Fri, 20 Jul 2007, Li YanBo wrote:
> On 7/20/07, Ilpo J?rvinen <[email protected]> wrote:
> >
> > - agnx_read32 & agnx_write32
> > * change to static inline, solves also the use of implicit
> > variable (reg) as you have return & assign instead (implicit
> > variables confuse readers, see Documentation/CodingStyle)
>
> It will be fixed later.
>
> > * You seem to be also doing direct access using ioread/write32
> > here and there?!?
>
> Yes, do you mean using __raw_readl and __raw_writel are better?
...I was just asking whether you're _not_ using agnx_read32/write32 but
ioread32/iowrite32 directly on purpose or not...
> > - I'm not sure if the comments per reg in phy_init() are that useful if
> > the same information is found in the header file already (just a matter
> > of taste though, shouldn't prevent merging, just IMHO)
>
> That just used to sync with the SPECS, for the SPECS always change, so it help
> me to change the code, it will be removed when the SPECS be more stable.
Like I indicated already you can as well keep them and nobody is
hurt... :-)
--
i.
On Thu, 19 Jul 2007 23:20:35 +0800, Li YanBo wrote:
> BTW, I find a problem in the file ieee80211.c it locals in the
> function "static int __ieee80211_tx()", I think the if (i ==
> tx->u.tx.num_extra_frag) is ineffective, because 'i' always lower
> than tx->u.tx.num_extra_frag. I guess maybe it should be if (i ==
> (tx->u.tx.num_extra_frag - 1)), below is the patch, CMIIW.
You're right. Could you add your Signed-off-by?
Thanks,
Jiri
>
> BR
> LiYanBo
>
> diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
> index fcad801..48c51bf 100644
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> @@ -1406,7 +1406,7 @@ static int __ieee80211_tx(struct ieee80211_local
> *local, struct sk_buff *
> continue;
> if (__ieee80211_queue_stopped(local, control->queue))
> return IEEE80211_TX_FRAG_AGAIN;
> - if (i == tx->u.tx.num_extra_frag) {
> + if (i == (tx->u.tx.num_extra_frag - 1)) {
> control->tx_rate = tx->u.tx.last_frag_hwrate;
> control->rate = tx->u.tx.last_frag_rate;
> if (tx->u.tx.probe_last_frag)
--
Jiri Benc
SUSE Labs
On Thu, 19 Jul 2007, Li YanBo wrote:
> On 7/19/07, Johannes Berg <[email protected]> wrote:
>
> > It might be useful for you to put all your current code into a git tree
> > and publish that to [email protected] so other people can
> > take a look too :)
> >
>
> First of all, the code is far away be finished and full of bugs, We
> can't success control the RX and TX yet.
It's important to have an early reviews too though not all issues
are not yet resolved.
> Thanks for all your suggestions and fixes, I'll fix it later.
> Because the code is big , so I attach it in this mail. (Be attention,
> it is far from finished and has tons of bugs).
Here are some things which you'll probably encounter when you try to get
this stuff merged, so you'll have to adjust the code sooner or later
(these can easily add many unnecessary post-comment round-trips when you
would like to get it merged asap):
- You seem to define assert and never use it, besides you could probably
use either WARN_ON or BUG_ON which belongs to kernel's generic machinery
already...
- agnx_read32 & agnx_write32
* change to static inline, solves also the use of implicit
variable (reg) as you have return & assign instead (implicit
variables confuse readers, see Documentation/CodingStyle)
* You seem to be also doing direct access using ioread/write32
here and there?!?
- no // comments allowed, use /* */ instead, however, most of them are
for dead code anyway which just makes things messy looking and should be
removed (I'd say that removing them speeds up review too)... Besides,
if you use repository, there's no need to keep dead code lying around.
> +
> +#ifdef CONFIG_PM
> +static int agnx_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> [...snip...]
> +
> + return 0;
> +}
Usually they ask you to add these...:
#else
#define agnx_pci_suspend NULL
#define agnx_pci_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct pci_driver agnx_pci_driver = {
> + .name = "agnx-pci",
> + .id_table = agnx_pci_id_tbl,
> + .probe = agnx_pci_probe,
> + .remove = __devexit_p(agnx_pci_remove),
> +#ifdef CONFIG_PM
> + .suspend = agnx_pci_suspend,
> + .resume = agnx_pci_resume,
> +#endif /* CONFIG_PM */
> +};
...and then to remove this #ifdefing.
> + /* FIXME */
> + reg = (bssid[0] << 24) | (bssid[1] << 16) | (bssid[2] << 8) | bssid[3];
> + agnx_write32(ctl_mem, AGNX_RXMC_BSSIDHI, reg);
> +
> + agnx_read32(ctl_mem, AGNX_RXMC_BSSIDLO);
> + reg &= 0xffff0000;
Define this BSSIDLO_MASK and use it here with ~ operator.
> + reg |= (bssid[4] << 8) | bssid[5];
> + agnx_write32(ctl_mem, AGNX_RXMC_BSSIDLO, reg);
...Maybe the "FIXME" already indicates the questionability of that
bssid endianess stuff...? The same comments apply to MACHI/LO stuff...
- I'm not sure if the comments per reg in phy_init() are that useful if
the same information is found in the header file already (just a matter
of taste though, shouldn't prevent merging, just IMHO)
> + /* Clear the Disable Tx interrupt bit in Interrupt Mask */
> + agnx_read32(ctl_mem, AGNX_INT_MASK);
> + reg &= ~0x20000;
> + agnx_write32(ctl_mem, AGNX_INT_MASK, reg);
You should not use numeric constant then (besides, IRQ_TX_DISABLE is
already defined!), if the purpose of this is known (there are more this
kind of things I suppose)... Then the comment would not be necessary any
more either as the code itself would explain the same to the reader... :-)
Obviously some/many values (in other places) will remain unknown as
reverse-engineered and that's something we just have to live with but
that's no excuse for known bits...
> +/* Recieve Management Control Registers */
typo
> +#define calibra_delay(priv) \
> + do { \
> + udelay(40); \
> + } while (0)
static inline
- ...other endianess issues should be dealt with...
> +#define FIR_TABLE_NUM 24
> +static const u32
> +tx_fir_table[FIR_TABLE_NUM] = { 0x19, 0x5d, 0xce, 0x151, 0x1c3, 0x1ff, 0x1ea,
[...snip...]
> + for (i = 0; i < FIR_TABLE_NUM; i++)
Drop FIR_TABLE_NUM and use ARRAY_SIZE macro in the for construct (and
elsewhere if there are other users). It's available in linux/kernel.h,
also check if your code implements something that is already given in
linux/kernel.h and replace them with the generic machinery. Same applies
to #define GAIN_TABLE_NUM 32 (and others?)
...those people specialized to wireless stuff have probably some / many
additional notes too...
--
i.
On 7/20/07, Li YanBo <[email protected]> wrote:
> On 7/20/07, Johannes Berg <[email protected]> wrote:
> > On Thu, 2007-07-19 at 23:20 +0800, Li YanBo wrote:
> > > On 7/19/07, Johannes Berg <[email protected]> wrote:
> > > > Hi,
> > > >
> > >
> > > > It might be useful for you to put all your current code into a git tree
> > > > and publish that to [email protected] so other people can
> > > > take a look too :)
> > > >
> > >
> > > First of all, the code is far away be finished and full of bugs, We
> > > can't success control the RX and TX yet.
> > > For I just have a git tree in local machine, and it can't be access
> > > from Internet, So, is there any place that can be used to put the code
> > > or I have to apply a domain and build a server for it?
> >
> > I guess you could, with some work, get an account on kernel.org, or I
>
> Following your guider, I have send a apply to get a kernel.org
> account. If all is ok, I plan build a tree at it, or else I will have
> to disturb you again. anywhere, Thank you! :)
>
> > can give you a tree on git.sipsolutions.net, or I'm sure there are other
> > people around as well...
> >
Hi,
After waiting two weeks, I haven't get the response from kernel.org,
so I guess they refuse my apply, for I have no the condition to set up
a website now, I have to ask you help to get a tree from you website
git.sipsolutions.net, if you willing, please tell me the first step I
need to do, thank you very much!
BR
Li YanBo
On 8/7/07, Johannes Berg <[email protected]> wrote:
> On Tue, 2007-08-07 at 18:18 +0800, Li YanBo wrote:
>
> > Ok, it works now. really thanks for your warmhearted help.
>
> Great. You're welcome. You might want to announce it to linux-wireless :)
>
Hi,
Under the great help of Johannes, the currently code of Airgo MIMO
wireless driver can be download from
http://git.sipsolutions.net/agnx.git/ now. Welcome any comment and
suggestion from you. The TX part of the driver can't work correctly so
far. we will try our best to let it work correctly ASAP.
BR
Li YanBo