Re: [PATCH] iwl3945: added channel sysfs entry

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2851 bytes --]

On Thu, 2008-09-25 at 15:34 +0800, Zhu Yi wrote:
> On Wed, 2008-09-24 at 02:44 -0600, Johannes Berg wrote:
> > On Wed, 2008-09-24 at 13:57 +0800, Zhu Yi wrote:
> > > From: Abhijeet Kolekar <abhijeet.kolekar@intel.com>
> > > 
> > > The patch adds channels sysfs entry for iwl3945. Make it consistent
> > > with iwlagn.
> > 
> > I advocate just make it consistent by removing it entirely.
> 
> We need a way to show the channel info the card EEPROM advocated, not
> the final one intersected with the regdomain. Are you suggesting a
> debugfs entry?

Not that I understand why you need that since iw shows all channels you
are registering (albeit with possibly a few more flags like "disabled"),
but yeah, if anything all this stuff should be in debugfs. There's also
this little fact that this channel list shouldn't ever get used, you
should be (and afaik are since I fixed it) using what the regdomain is
enforcing, and you're already printing the channel list when enough
debugging flags are set on driver load.

Therefore, I don't understand why you need to clutter the driver with
this code. Malicious gossip has it you're trying to obfuscate the code
to a point where everybody else refuses to work on it.

Anyhow, I see none of the sysfs files you're exporting as being
userspace API that is actually needed in that form.

Also, looking into more details, your sysfs files seem to fall into
three categories:

 1) things that mac80211 provides but you haven't hooked up
 2) things that are generally useful, but you've added to your driver
    instead of helping build the generic framework
 3) things that are truly only useful for your internal debugging

1) notably TX power;

2) it looks like retry_rate should be settable through a generic rate
   control API if it is truly useful (otherwise category 3), power_level
   also belongs here, sometimes one can't avoid the impression that
   you're adding things like that to avoid having to help define and
   implement the generic functionality;

3) Neither flags nor filter flags seem useful (and filter flags you can
   actually influence via nl80211) to set outside test environments,
   statistics falls here too, same probably with antenna, status, error
   and events logs and "measurement".

Similar issues surround the module parameters, I won't go into them
other than saying that they constitute API as well and at least having
different versions for 4965 and 5000 hw seems entirely pointless. Many
of them seem rather pointless in general, and something not only you
have been doing and that I've asked for before is that I'd like to see a
generic option to disable hardware crypto in _mac80211_ rather than each
driver because this is actually genuinely useful if you want to monitor
frames at the same time.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]