Re: [PATCH 0/6] rework d80211 cookie pointer

Re: [PATCH 0/6] rework d80211 cookie pointer

[Michael Wu]

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

On Thursday 02 November 2006 17:38, Johannes Berg wrote:
> Now, I tried to update adm8211 and rt2x00 as well, but faced some
> problems. adm8211 actually accesses the struct net_device and
> calls it's open() call! This is broken.
>
Good catch. Looks like I never updated the suspend/resume code properly when I 
ported to d80211. The fix is easy - the code just needs to call the 
adm8211_open/stop functions directly. I'll send a patch later tonight.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

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

On Fri, 2006-11-03 at 00:09 +0100, Johannes Berg wrote:

>  /* Return a pointer to network statistics data area for the given device. */
> -void * ieee80211_dev_stats(struct net_device *dev);
> +void *ieee80211_dev_stats(struct ieee80211_local *ieee80211_ptr);

Ok, that one was wrong. But what is it doing in the public API? We need
to remove it from the public API and leave struct net_device *dev as the
parameter. adm8211 actually uses it and increases the tx_fifo_error
counter, but that's a bit strange.

johannes

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

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

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


> Ok, that one was wrong. But what is it doing in the public API? We need
> to remove it from the public API and leave struct net_device *dev as the
> parameter. adm8211 actually uses it and increases the tx_fifo_error
> counter, but that's a bit strange.

Fixed, and since, as always, netdev doesn't like my patches, they're
here instead:

http://johannes.sipsolutions.net/files/d80211-mdev-cleanup/

I also added d80211-reduce-mdev.patch which reduces stupid mdev usage,
but see the description in the patch itself.

johannes

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

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[John W. Linville]

On Fri, Nov 03, 2006 at 01:46:31AM +0100, Johannes Berg wrote:
> 
> > Ok, that one was wrong. But what is it doing in the public API? We need
> > to remove it from the public API and leave struct net_device *dev as the
> > parameter. adm8211 actually uses it and increases the tx_fifo_error
> > counter, but that's a bit strange.
> 
> Fixed, and since, as always, netdev doesn't like my patches, they're
> here instead:
> 
> http://johannes.sipsolutions.net/files/d80211-mdev-cleanup/
> 
> I also added d80211-reduce-mdev.patch which reduces stupid mdev usage,
> but see the description in the patch itself.

403 Forbidden (shouldn't that be Verboten? :-)

FWIW, I have the other 6 patches on the 'pending' branch of
wireless-dev.  Driver authors, please proceed accordingly.

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

On Thu, 2006-11-02 at 23:15 -0500, John W. Linville wrote:

> 403 Forbidden (shouldn't that be Verboten? :-)

Nah, that's ok, that particular Apache instance is running in London,
UK :)

Fixed, sorry about that, I don't usually allow directory listing in that
hierarchy and forgot to enable it (so you could have gotten
d80211-reduce-mdev.patch by appending it to that URL :)

johannes

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Ivo van Doorn]

On Friday 03 November 2006 05:15, John W. Linville wrote:
> On Fri, Nov 03, 2006 at 01:46:31AM +0100, Johannes Berg wrote:
> > 
> > > Ok, that one was wrong. But what is it doing in the public API? We need
> > > to remove it from the public API and leave struct net_device *dev as the
> > > parameter. adm8211 actually uses it and increases the tx_fifo_error
> > > counter, but that's a bit strange.
> > 
> > Fixed, and since, as always, netdev doesn't like my patches, they're
> > here instead:
> > 
> > http://johannes.sipsolutions.net/files/d80211-mdev-cleanup/
> > 
> > I also added d80211-reduce-mdev.patch which reduces stupid mdev usage,
> > but see the description in the patch itself.
> 
> 403 Forbidden (shouldn't that be Verboten? :-)
> 
> FWIW, I have the other 6 patches on the 'pending' branch of
> wireless-dev.  Driver authors, please proceed accordingly.

I am currently working on the fixes for rt2x00, the above patches look very good,
and also provide me with a chance to cleanup some garbage in rt2x00. :)
I'll make the patches for this part of my patch series which I will send to the netdev
list in 1 or 2 weeks. (I cannot do it any sooner due to school and work).

Ivo

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

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

On Sun, 2006-11-05 at 16:20 +0100, Ivo van Doorn wrote:

> I am currently working on the fixes for rt2x00, the above patches look very good,
> and also provide me with a chance to cleanup some garbage in rt2x00. :)
> I'll make the patches for this part of my patch series which I will send to the netdev
> list in 1 or 2 weeks. (I cannot do it any sooner due to school and work).

Actually, disregard these patches. I have totally different ones I'm
about to post.

johannes

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

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

On Fri, 2006-11-03 at 01:46 +0100, Johannes Berg wrote:

> d80211-reduce-mdev.patch
> d80211-ethtool.patch
> d80211-cookie.patch

GRRRR, whitespace damaged. I swear I'm going to submit a s/ {8}/\t/
patch some of these days. wiggle should handle that just fine, and
personally, I have reached a point where I'm far more annoyed by the
damaged whitespace all over than I would be if a patch failed to apply
(which actually happens all the time, so it's sort of a regular thing)

In the meantime, I've fixed the damage.

johannes

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Michael Wu]

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

On Thursday 02 November 2006 19:22, Johannes Berg wrote:
> Ok, that one was wrong. But what is it doing in the public API? We need
> to remove it from the public API and leave struct net_device *dev as the
> parameter. adm8211 actually uses it and increases the tx_fifo_error
> counter, but that's a bit strange.
>
That's because TX might fail for reasons other than not getting an ACK. I 
can't say I've actually seen this happen, so it might just be something left 
over from tulip that doesn't need to be there now. (or perhaps it only 
happens when there's something really bad going on) However, what's so bad 
about letting drivers update some statistics if it is possible? If you remove 
ieee80211_dev_stats, please provide some other way for drivers to access 
struct net_device_stats.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/6] d80211: change the cookie to be opaque

[Johannes Berg]

On Thu, 2006-11-02 at 21:28 -0500, Michael Wu wrote:

> That's because TX might fail for reasons other than not getting an ACK. I 
> can't say I've actually seen this happen, so it might just be something left 
> over from tulip that doesn't need to be there now. (or perhaps it only 
> happens when there's something really bad going on) However, what's so bad 
> about letting drivers update some statistics if it is possible? If you remove 
> ieee80211_dev_stats, please provide some other way for drivers to access 
> struct net_device_stats.

Well, you were only changing the master netdev's stats, which isn't
really useful because you want to change the device that transmitted the
frame. But you cannot, because you don't have access to it. In general,
the stack should be doing this, so if some other tx errors are possible
maybe we should add a flag to the tx status.

johannes

Re: [PATCH 0/6] rework d80211 cookie pointer

[Michael Wu]

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

On Thursday 02 November 2006 17:38, Johannes Berg wrote:
> I'm sorry I didn't manage to update these drivers. As for out-of-tree
> drivers, I obviously haven't updated them either. It *shouldn't*
> be too hard as the net_dev shouldn't be used in many places.

So uh, what am I going to prefix all my printk messages with now that I can't 
access dev->name? It seems like the other d80211 drivers have a habit of 
prefixing their messages with the driver name (wrong thing to do, IMHO), 
which is different from pretty much all other network drivers.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/6] rework d80211 cookie pointer

[Johannes Berg]

Michael Wu wrote:

> So uh, what am I going to prefix all my printk messages with now that I
> can't
> access dev->name? It seems like the other d80211 drivers have a habit of
> prefixing their messages with the driver name (wrong thing to do, IMHO),
> which is different from pretty much all other network drivers.

Yeah, I thought about that and added that function to get the wiphy index.
cfg80211 makes the wiphy index an actual identifier that you can use to
enumerate all devices it has attached etc, so that makes sense. and d80211
also puts it into sysfs.

Is that good enough?

johannes

Re: [PATCH 0/6] rework d80211 cookie pointer

[Michael Wu]

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

On Friday 03 November 2006 14:08, Johannes Berg wrote:
> Yeah, I thought about that and added that function to get the wiphy index.
> cfg80211 makes the wiphy index an actual identifier that you can use to
> enumerate all devices it has attached etc, so that makes sense. and d80211
> also puts it into sysfs.
>
> Is that good enough?
>
I still prefer obtaining the name of the master interface because well.. all 
other network drivers prefix their messages with the name of their interface.

The master interface name should be as good as the wiphy index to enumerate 
attached devices.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/6] rework d80211 cookie pointer

[Johannes Berg]

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

On Fri, 2006-11-03 at 17:14 -0500, Michael Wu wrote:

> I still prefer obtaining the name of the master interface because well.. all 
> other network drivers prefix their messages with the name of their interface.
> 
> The master interface name should be as good as the wiphy index to enumerate 
> attached devices.

I'm trying to get rid of the master interface mostly. I can surely add a
way to get it if we finally decide that we want/need to keep it.

johannes

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

Re: [PATCH 3/6] d80211: add a perm_addr hardware property

[Michael Wu]

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

On Thursday 02 November 2006 17:38, Johannes Berg wrote:
> After removing knowledge of the master net_dev from drivers,
> they'll still need a way to tell us which MAC address they have.
> This is that way, the perm_addr is initially used for all devices.
>
Shouldn't you add something to ieee80211_set_mac_address so the driver/d80211 
can find out what MAC address the user actually wants?

Might be easier to just point perm_addr to dev_addr, but that would make it 
obvious that perm_addr is a bit messy. A wrapper for drivers that returns a 
pointer to dev_addr might be a better idea.

Also, do we need to update interfaces associated with the master interface 
when the master MAC address changes?

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/6] d80211: add a perm_addr hardware property

[Johannes Berg]

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

On Fri, 2006-11-03 at 16:49 -0500, Michael Wu wrote:

> Shouldn't you add something to ieee80211_set_mac_address so the driver/d80211 
> can find out what MAC address the user actually wants?

Hmm? d80211 already handles the case of the user changing a net_dev's
mac address fine. And it'll tell your driver about it whenever it
matters, at the time the user tries to UP that interface. And other than
that, the driver doesn't care.

> Might be easier to just point perm_addr to dev_addr, but that would make it 
> obvious that perm_addr is a bit messy. A wrapper for drivers that returns a 
> pointer to dev_addr might be a better idea.

I don't understand this. perm_addr is a field in the ieee80211_hw struct
reflecting the permanent mac address of the card. What's wrong with
that?

> Also, do we need to update interfaces associated with the master interface 
> when the master MAC address changes?

No. The interfaces are allowed to have differing mac addresses. But I
think they should all start out with the permanent mac address which is
why I put that field there. I guess you could argue for always using the
master's mac address. I would prefer new devices to start out with the
permanent mac address, but we can change that.

johannes

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

Re: [PATCH 4/6] d80211: add a struct device* hardware property

[Michael Wu]

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

On Thursday 02 November 2006 17:38, Johannes Berg wrote:
> +	/* hardware device */
> +	struct device *dev;
> +
Can we just pass this in as an argument instead? No one is gonna look at it 
ever again after ieee80211_register_hw, so I don't think it's worth putting 
in struct ieee80211_hw.

> +	local->mdev->class_dev.dev = hw->dev;
Why not use SET_NETDEV_DEV?

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/6] d80211: add a struct device* hardware property

[Johannes Berg]

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

On Fri, 2006-11-03 at 17:27 -0500, Michael Wu wrote:
> On Thursday 02 November 2006 17:38, Johannes Berg wrote:
> > +	/* hardware device */
> > +	struct device *dev;
> > +
> Can we just pass this in as an argument instead? No one is gonna look at it 
> ever again after ieee80211_register_hw, so I don't think it's worth putting 
> in struct ieee80211_hw.

Actually, it is used for all new devices as well. Yeah, we could pull it
out of the mdev again, but it feels stupid to go to so many
indirections. The code is already barely understandable. I had a hard
time cleaning up the places where struct net_device * is passed, but the
only thing it's ever used for is deref'ing ->ieee80211_ptr to get local.

> > +	local->mdev->class_dev.dev = hw->dev;
> Why not use SET_NETDEV_DEV?

You're right, it should use that.

johannes

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

Re: [PATCH 4/6] d80211: add a struct device* hardware property

[Johannes Berg]

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

On Sat, 2006-11-04 at 00:21 +0100, Johannes Berg wrote:

> Actually, it is used for all new devices as well. Yeah, we could pull it
> out of the mdev again, 

Oh, in fact, we do. I still think it feels stupid and will change it.

johannes

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

Re: [PATCH 5/6] d80211: add a ethtool_ops hardware property

[Michael Wu]

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

On Thursday 02 November 2006 17:39, Johannes Berg wrote:
> After removing knowledge of the master net_dev from drivers,
> some will still want to have ethtool ops assigned (rt2x00 uses
> it). This is that way.
But bcm43xx sets ethtool ops through bcm43xx_netdev_setup. Why not have rt2x00 
do the same? Then we won't have to go through all the trouble of adding 
things to ieee80211_hw and ieee80211_register_hw to set things in net_device. 
This would work out pretty well for SET_NETDEV_DEV too.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 5/6] d80211: add a ethtool_ops hardware property

[Johannes Berg]

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

On Fri, 2006-11-03 at 17:43 -0500, Michael Wu wrote:

> But bcm43xx sets ethtool ops through bcm43xx_netdev_setup. Why not have rt2x00 
> do the same? 

Actually, look closer. I removed bcm43xx_netdev_setup as well as the
setup() callback for the hw as well as the bcm43xx ethtool ops which
were useless anyway.

> Then we won't have to go through all the trouble of adding 
> things to ieee80211_hw and ieee80211_register_hw to set things in net_device. 
> This would work out pretty well for SET_NETDEV_DEV too.

No. We have to add these things anyway if we want them copied to the
slave netdevs. Besides, the driver has no business mucking with the
netdevs.

johannes

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