On Wednesday 21 November 2007 10:57:51 Luis R. Rodriguez wrote:
> thanks for this work BTW. CC'ing other maintainers, you probably just
> forgot. I know this is a pain but this is a pretty big patch, can you
> split it into a few for better review? I also know this is just debug
> stuff but it would help. One for each main task you mention below
> might do it, or something like that.
ok. i did that in my last 2 posts, but you are right - i missed your comments
here. i will address them now, but please be aware, that the last 2 patches
are slightly different, i.e. i renamed AR5K_... to ATH5K_... and a few
smaller things to keep scripts/checkpatch.pl happy.
> > > +#define AR5K_PRINTK(_sc, _level, _fmt, ...) \
> > > + printk(_level "ath5k %s: " _fmt, \
> > > + ((_sc) && (_sc)->hw) ? wiphy_name((_sc)->hw->wiphy) : "",
> > > \ + ##__VA_ARGS__)
> > > +
> > > +#define AR5K_PRINTK_LIMIT(_sc, _level, _fmt, ...) do { \
> > > + if (net_ratelimit()) \
> > > + AR5K_PRINTK(_sc, _level, _fmt, ##__VA_ARGS__); \
> > > + } while (0)
> > > +
> > > +#define AR5K_INFO(_sc, _fmt, ...) \
> > > + AR5K_PRINTK(_sc, KERN_INFO, _fmt, ##__VA_ARGS__)
> > > +
> > > +#define AR5K_WARN(_sc, _fmt, ...) \
> > > + AR5K_PRINTK_LIMIT(_sc, KERN_WARNING, _fmt, ##__VA_ARGS__)
> > > +
> > > +#define AR5K_ERR(_sc, _fmt, ...) \
> > > + AR5K_PRINTK_LIMIT(_sc, KERN_ERR, _fmt, ##__VA_ARGS__)
>
> I was kind of hoping we could stick to dev_[info|warn|err] for these.
> IIRC you had mentioned you can't get the device name printed? Is that
> right?
no. i print the name of the phyX device which i get with "wiphy_name()". i
think this is more descriptive than the PCI bus id, especially if you want to
match output from mac80211 to messages within ath5k. i do print a message
like "ath5k_pci 0000:00:0e.0: registered as 'phy0'" via dev_info and from
that point on the phyX device name is used.
anyways using these defines, and giving them sc gives us the possibility to
switch them to use dev_info as well, we'd just have to change
#define ATH5K_PRINTK(_sc, _level, _fmt, ...) \
dev_info(&(_sc)->pdev->dev, _fmt, ##__VA_ARGS__)
but i think, as i said above, using the phy name makes things clearer. also
note that this is more or less the same as used in other mac80211 based
drivers.
> > > diff --git a/drivers/net/wireless/ath5k/base.c
> > > b/drivers/net/wireless/ath5k/base.c index 77e3855..6ca7ebf 100644
> > > --- a/drivers/net/wireless/ath5k/base.c
> > > +++ b/drivers/net/wireless/ath5k/base.c
>
> Notice the license on base.[ch]. It's a "Dual GPL/BSD" license.
> Although all of our recent changes have been licensed the 3-clause-BSD
> for these files we did not verify the same for previous changes on
> MadWifi's files (stuff in base.[ch]) so its best to just keep the Dual
> license there for that code in case previous authors did intend on
> licesning their changes under the GPL (hence the problem with using
> vague "alternatively" language for Dual licensing). This presents a
> problem in trying to shift code on base.[ch] onto what you would think
> is an OK ISC-only licensed code. This is also the problem we knew we'd
> face down the road with trying to help OpenBSD -- in the end we may
> want to just shuffle around GPL code into ISC licensed code and well,
> then it becomes a pretty heavy nightmware for us to keep track of
> which part of the code is licensed under what for them.
>
> As we have it right now base.[ch] contains code which *may* have been
> patched in for GPL-only intentions (prior to kernel inclusion). In
> your patch series I'd like to see careful use of this shifting. If you
> are just deleting code, great, but if you are reusing code please
> either go through the git-log to see what license the changes were
> made (haha, good one huh!) or well lets just GPL those new files you
> are creating. The debug print stuff is pretty useless to other kernels
> too as its very specific to Linux.
ugh, that's getting complicated...
ok then. let's split it up into the 2 patches i've sent. i think the first one
is pretty clear, as it just renames stuff, especially in base.c (i doubt that
it's even worth a copyright claim). so would that be fine with a BSD license?
next: patch 2, debugging, is the complicated one, because i move stuff from
base.c and hc.c to a different file, debugging.c (ath_printrxbuf(),
ath_printtxbuf(), ath_dump_skb() and various other debugging parts). most of
this code comes into git from Jiri Slaby's initial commit 88c37f32... "Net:
add ath5k wireless driver" (git blame and git-gui is great!)
tracking things further than that is difficult but i found:
ath5k_printrxbuf() and ath5k_printtxbuf() also exist in a very similar way in
madwifi, and it's pretty obvious they come from there, hence they are dual
GPL/BSD licensed, right?
the debugging code i took out of ath_stoprecv() into a new function
ath5k_debug_printrxbuffs() also seems to have it's origin in madwifi too,
although modified for different list handling. so probably dual BSD/GPL too.
as far as i can tell ath_dump_skb and ath_dump_modes first appeared in Jiri
Slaby's initial commit 88c37f32 so i'm not sure about their license. i found
some messages which indicate that he intended GPL, but that did not hold or
something?
ath5k_debug_dump_hwstate() clearly comes from openbsd
ar5k_ar5212/11_dump_state, so it's ISC license (afaik, or BSD???).
so as a summary these are the functions i moved into debug.c:
function source license
-----------------------------------------------
ath5k_debug_printrxbuf() madwifi BSD/GPL
ath5k_debug_printrxbuffs() madwifi BSD/GPL
ath5k_debug_printtxbuf() madwifi BSD/GPL
ath5k_debug_dump_hwstate() openbsd ISC
ath5k_debug_dump_modes() jiri ???
ath5k_debug_dump_skb() jiri ???
+ it contains additional stuff (debugfs) which i made, therefore can license
however i like.
which makes it license-able under what? i personally don't really care, and as
you said it's only debugging stuff, so probably it won't go back into BSD
systems anyways. at the same time as far as i understand we can take BSD
stuff and re-license it under GPL, right, it's just not considered to be a
nice thing to do. so would it be o.k. in this case to make debug.c licensed
as GPL only?
anyhow i don't care about which license, and i don't want to get into license
politics, but it is a real pain if we have to spend the same amount of time
doing that kind of history searching as we spend coding - for every change we
make. especially since most of this is probably irrelevant as nobody is going
to port it back to BSD or it came from the ultra-permissive dual BSD/GPL
anyways.
so please advise me which license is the most practical to use.
> > > 0x%x)\n", + AR5K_INFO(sc, "Atheros AR%s chip found (MAC: 0x%x, PHY:
> > > 0x%x)\n", ath5k_chip_name(AR5K_VERSION_VER,sc->ah->ah_mac_srev),
> > > sc->ah->ah_mac_srev,
> > > sc->ah->ah_phy_revision);
> > > @@ -580,27 +490,28 @@ ath5k_pci_probe(struct pci_dev *pdev,
> > > if(sc->ah->ah_radio_5ghz_revision &&
> > > !sc->ah->ah_radio_2ghz_revision) { /* No 5GHz support -> report 2GHz
> > > radio */ if(!test_bit(MODE_IEEE80211A,
> > > sc->ah->ah_capabilities.cap_mode)){ -
> > > dev_info(&pdev->dev, "RF%s 2GHz radio found (0x%x)\n", +
> > > AR5K_INFO(sc, "RF%s 2GHz radio found (0x%x)\n",
> > > ath5k_chip_name(AR5K_VERSION_RAD,sc->ah->ah_radio_5ghz_revision),
> > > sc->ah->ah_radio_5ghz_revision); /* No 2GHz support (5110 and some 5Ghz
> > > only cards) -> report 5Ghz radio */ } else
> > > if(!test_bit(MODE_IEEE80211B, sc->ah->ah_capabilities.cap_mode)){ -
> > > dev_info(&pdev->dev, "RF%s 5GHz radio found
> > > (0x%x)\n", + AR5K_INFO(sc, "RF%s 5GHz radio
> > > found (0x%x)\n",
>
> See for example, here I wouldn't mind leaving dev_info, etc, with our
> own macros.
well i can change these lines back to use dev_info, but don't you think this
output is o.k. too? an example:
ath5k_pci 0000:00:0e.0: registered as 'phy4'
ath5k phy4: Device only partially supported.
ath5k phy4: Atheros AR5414 chip found (MAC: 0xa5, PHY: 0x61)
ath5k_pci 0000:00:0f.0: registered as 'phy5'
ath5k phy5: Atheros AR5213 chip found (MAC: 0x56, PHY: 0x41)
ath5k phy5: RF5111 5GHz radio found (0x17)
ath5k phy5: RF2111 2GHz radio found (0x23)
> > > +++ b/drivers/net/wireless/ath5k/debug.c
> > > @@ -0,0 +1,256 @@
> > > +/*
> > > + * Copyright (c) 2004-2007 Reyk Floeter <reyk@xxxxxxxxxxx>
> > > + * Copyright (c) 2006-2007 Nick Kossifidis <mickflemm@xxxxxxxxx>
>
> You're adding unique code (debugfs stuff), you should add your
> Copyright entry here.
o.k.
> But note that this will look very different if
> this is GPL'd. Please refer to SFLC's guidelines if you are to GPL
> these files (probably going to be needed).
> > > + for (m = 0; m < NUM_DRIVER_MODES; m++) {
> > > + printk(KERN_DEBUG "Mode %u: channels %d, rates %d\n", m,
> > > + modes[m].num_channels,
> > > modes[m].num_rates); + printk(KERN_DEBUG " channels:\n");
>
> How about dev_dbg instead?
i think in general we should stick with one method, so that would be
ATH5K_DBG. which in turn can be made to use dev_dbg if we choose to. but i
made ATH5K_DBG rate limited (net_ratelimit()) so we can't use it to print
several lines like this. i think it's ok, now, as all printk(KERN_DEBUG,...
are within debug.[ch], and we can change it easily to have the same output
like ATH5K_DGB. or we add another ATH5k_DBG_NONLIMITED or something?
> > > + ATH_DEBUG_RESET = 0x00000001, /* reset processing */
> > > + ATH_DEBUG_INTR = 0x00000002, /* ISR */
> > > + ATH_DEBUG_MODE = 0x00000004, /* mode init/setup */
> > > + ATH_DEBUG_XMIT = 0x00000008, /* basic xmit operation
> > > */ + ATH_DEBUG_BEACON = 0x00000010, /* beacon handling */
> > > + ATH_DEBUG_BEACON_PROC = 0x00000020, /* beacon ISR proc */ +
> > > ATH_DEBUG_CALIBRATE = 0x00000100, /* periodic calibration */ +
> > > ATH_DEBUG_TXPOWER = 0x00000200, /* transmit power */ +
> > > ATH_DEBUG_LED = 0x00000400, /* led management */ +
> > > ATH_DEBUG_DUMP_RX = 0x00001000, /* print received skb content
> > > */ + ATH_DEBUG_DUMP_TX = 0x00002000, /* print transmit skb
> > > content */ + ATH_DEBUG_DUMPSTATE = 0x00004000, /* dump
> > > register state */ + ATH_DEBUG_DUMPMODES = 0x00008000, /* dump
> > > modes */ + ATH_DEBUG_TRACE = 0x00010000, /* trace
> > > function calls */ + ATH_DEBUG_FATAL = 0x80000000, /*
> > > fatal errors */ + ATH_DEBUG_ANY = 0xffffffff
> > > +};
>
> While you're at it can you move these to use kernel-doc? You'll have
> to name the enum, perhaps ath5k_debug ?
o.k.
thank you for your comments, luis!
i would appreciate it if you could give me more advice concerning the
licensing issues as discussed above, so i can send in a correctly licensed
patch.
bruno
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
