[PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Pavel Roskin]

hw.priv is set twice, and the second time it's set incorrectly to an
area relative to the master device, which wasn't allocated for private
data.

Signed-off-by: Pavel Roskin <proski@gnu.org>
---

 net/d80211/ieee80211.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index 456beb3..4f77e27 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -4550,9 +4550,6 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	mdev->ieee80211_ptr = &sdata->wdev;
 	sdata->wdev.wiphy = wiphy;
 
-	local->hw.priv = (char *)mdev->priv +
-			 ((sizeof(struct ieee80211_sub_if_data) +
-			   NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST);
 	local->hw.queues = 1; /* default */
 
 	local->mdev = mdev;

Re: [PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Johannes Berg]

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

On Sat, 2007-02-17 at 02:42 -0500, Pavel Roskin wrote:
> hw.priv is set twice, and the second time it's set incorrectly to an
> area relative to the master device, which wasn't allocated for private
> data.

Yeah, this is obviously correct. Guess I was drinking when I did these
patches. I really wonder why I never saw any of your problems though, I
do have bcm43xx running here. Not that the driver actually does anything
for me, it always refuses to associate...

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> Signed-off-by: Pavel Roskin <proski@gnu.org>
> ---
> 
>  net/d80211/ieee80211.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
> index 456beb3..4f77e27 100644
> --- a/net/d80211/ieee80211.c
> +++ b/net/d80211/ieee80211.c
> @@ -4550,9 +4550,6 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
>  	mdev->ieee80211_ptr = &sdata->wdev;
>  	sdata->wdev.wiphy = wiphy;
>  
> -	local->hw.priv = (char *)mdev->priv +
> -			 ((sizeof(struct ieee80211_sub_if_data) +
> -			   NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST);
>  	local->hw.queues = 1; /* default */
>  
>  	local->mdev = mdev;
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Pavel Roskin]

On Sat, 2007-02-17 at 11:16 +0100, Johannes Berg wrote:
> On Sat, 2007-02-17 at 02:42 -0500, Pavel Roskin wrote:
> > hw.priv is set twice, and the second time it's set incorrectly to an
> > area relative to the master device, which wasn't allocated for private
> > data.
> 
> Yeah, this is obviously correct. Guess I was drinking when I did these
> patches. I really wonder why I never saw any of your problems though, I
> do have bcm43xx running here. Not that the driver actually does anything
> for me, it always refuses to associate...

Oh, and by the way, wouldn't it be reasonable to have an inline function
to calculate the pointer to the priv area instead of having an actual
field for the pointer?

Also, it looks like we need a generic macro for aligning sizes.

-- 
Regards,
Pavel Roskin

Re: [PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Johannes Berg]

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

On Sun, 2007-02-18 at 16:54 -0500, Pavel Roskin wrote:
> On Sat, 2007-02-17 at 11:16 +0100, Johannes Berg wrote:
> > On Sat, 2007-02-17 at 02:42 -0500, Pavel Roskin wrote:
> > > hw.priv is set twice, and the second time it's set incorrectly to an
> > > area relative to the master device, which wasn't allocated for private
> > > data.
> > 
> > Yeah, this is obviously correct. Guess I was drinking when I did these
> > patches. I really wonder why I never saw any of your problems though, I
> > do have bcm43xx running here. Not that the driver actually does anything
> > for me, it always refuses to associate...
> 
> Oh, and by the way, wouldn't it be reasonable to have an inline function
> to calculate the pointer to the priv area instead of having an actual
> field for the pointer?

Yeah, I guess that isn't too hard to do. But if you think about what the
current code will compile to that's also just an addition based on the
struct size that is known at compile time.

> Also, it looks like we need a generic macro for aligning sizes.

I hated wrapping my head around it so I just made the padding explicit
in the struct ;)

johannes

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

Re: [PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Pavel Roskin]

On Mon, 2007-02-19 at 07:33 +0100, Johannes Berg wrote:
> On Sun, 2007-02-18 at 16:54 -0500, Pavel Roskin wrote:
> > Oh, and by the way, wouldn't it be reasonable to have an inline function
> > to calculate the pointer to the priv area instead of having an actual
> > field for the pointer?
> 
> Yeah, I guess that isn't too hard to do. But if you think about what the
> current code will compile to that's also just an addition based on the
> struct size that is known at compile time.

Perhaps you misunderstood my idea.  There is an field called priv in
struct ieee80211_hw.  That field will be written to, no matter how good
the compiler is at optimization.  And most drivers will access that
field by reading from memory.

Having an inline function brings following benefits:

1) Memory access is replaced with adding a constant (marginal speed-up)
2) Removing priv from struct ieee80211_hw (marginal memory saving)
3) priv cannot be written to (marginal safety improvement)

I see that the list doesn't look impressive, but I want to correct
possible misunderstanding anyway.

-- 
Regards,
Pavel Roskin

Re: [PATCH] d80211: fix incorrect hw.priv setting in ieee80211_alloc_hw()

[Johannes Berg]

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

On Mon, 2007-02-19 at 08:43 -0500, Pavel Roskin wrote:

> Perhaps you misunderstood my idea.  There is an field called priv in
> struct ieee80211_hw.  That field will be written to, no matter how good
> the compiler is at optimization.  And most drivers will access that
> field by reading from memory.

My mistake. I was thinking of struct wiphy.priv and not
ieee80211_hw.priv.

johannes


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