Re: patch idea for wfx

[Stefano Brivio <sbrivio@redhat.com>]

Hi Jaehee,

On Wed, 6 Apr 2022 00:28:21 -0400
Jaehee Park <jhpark1013@gmail.com> wrote:

> [...]
>
> Sorry about the late reply! It's taking me a while to wrap my head around this! I think I'm slowly getting it, but I have a few questions. This email is long so here's an outline to make it a bit more readable:
> Section 1: my current understanding (reiteration of what you had already explained)
> Section 2: my implementation of container_of() and the error message
> Section 3: my confusion about the different types (struct wfx_vif vs u8 drv_priv[])
> 
> -----Section 1-----
> - We're implementing container_of() to get vif. we are doing so with container_of() which does two things: [1] it speeds up compilation because the container_of() saves steps that the cpu would have to take to retrieve data, [2] we don't have to define multipe pointers by setting it in code:
>     - the code before: got a pointer to the drv_priv, then point back to the generic interface data (wvif->vif = vif;). So, another pointer is defind by the vif in struct wfx_vif to point to the struct ieee80211_vif interface.
>     - after implementing container_of: we don't have to define another pointer. the compiler already has the pointer to the struct ieee80211_vif via offset of drv_priv! So all it needs to do is subtract the offset and you have the pointer.

This looks correct to me.

> -----Section 2-----
> Here's my diff (below). I've implemented container_of() in function wfx_add_interface (to start).
> - If I'm understanding this correctly, we're replacing the line
> `struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv`
> and replacing it with
> `struct wfx_vif *wvif = container_of(vif, struct ieee80211_vif, drv_priv)`

Not really: wvif (the driver-specific structure) is included in vif
(the generic structure), not the other way around. This:

	struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv;

should stay as it is -- you don't have another way to fetch wvif.

> This is the error message I get:
> ./include/linux/container_of.h:17:41: error: initialization of ‘struct wfx_vif *’ from incompatible pointer type ‘struct ieee80211_vif *’ [-Werror=incompatible-pointer-types]

Right, because you're getting a pointer to struct ieee80211_vif,
correctly, but for the wrong purpose. It's fetching "vif" that you want
to simplify, not fetching "wvif".

> -----Section 3-----
> So there is an incompatible pointer type error. I think to solve this error, I need to clarify this line:
> struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv; link below
> Can you help me understand this line of code again?
> https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/sta.c#L724
> 
> **Question: Are they punning raw bytes from drv_priv into the struct wfx_vif?**

Yes.

> So there's already data in the drv_priv, and this line of code is taking those raw bytes (u8 drv_priv[]) into the wfx_vif struct?

Yes. It's not really "taking" anything, it's just a reference
assignment.

> This is a very new concept to me and I wanted to ask if this is a correct thought:
> 
> - drv_priv: the bytes are in u8 array so whatever is in this array will be ordered in contiguous memory (https://elixir.bootlin.com/linux/latest/source/include/net/mac80211.h#L1756) where the __aligned is a compiler builtin that guarantees alignment.

Alignment doesn't specifically matter here, but yes, that's correct.

> - (struct wfx_vif *): The pointer seems to be of unsigned bytes (https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/wfx.h#L115)

...what do you mean exactly?

> - They are taking those raw bytes into the struct. (or the other way around?)

Nothing is copied, they're taking a reference to this data.

> So maybe the solution would be to set the container_of() to struct ieee80211_vif to match the pointer type. And then afterwards pack that into struct wfx_vif? 

Not really, you just need to turn things around. :)

> Let me know if my question doesn't make sense and I'm happy to get on a chat/call if it makes it easier.

Does IRC work for you now?

-- 
Stefano