patch idea for wfx

patch idea for wfx

[Jaehee Park]

I was reading the wfx codebase yesterday and it seems like there are a lot of instances of "container_of()" structure which is specific to the linux kernel. I saw a "fixme" comment in the code that says that the container_of() structure is preferred and I wanted to tackle it. Here's the line with the fixme comment:
https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/sta.c#L744

Maybe it's too big of a task though. What do you think?

(Side question: The wfx project is also in the staging/driver but I'm not seeing the linux/kernel/git/gregkh/staging repository anywhere on github -- I'm probably not understanding something basic here but shouldn't the staging directory be a subrepo or submodule within the linux kernel repo? ) 

Now I'm trying to understand the wvif structure they are using in the functions. In their previous examples container_of() took in 3 things: [1] work, [2] struct wfx_dev, and [3] the name. I'm refering to the man page for the container_of inputs. 
I'm not entirely sure why container_of structure is helpful. But this article seems to say it makes runtime faster: http://www.kroah.com/log/linux/container_of.html

Another question I had has to do with testing. I understand we're building the kernel to check if the code-change compiles correctly (I've enabled the silicon labs driver in the menuconfig and saw that it builds successfully for basic changes). But for more complex changes, would it be a good idea to test it on the hardware -- after build, install the kernel package on an apu2 for example? I have a apu2 and I can buy a siliconlab wifi card to test it on there. 

I'm brain-dumping ideas messily (sorry! please let me know what you'd recommend) and I'm mostly just conveying my thought process. I am having trouble with IRC at the moment so I'm sending my thoughts here. 

Thanks,
Jaehee

Re: patch idea for wfx

[Fabio M. De Francesco]

On gioved? 31 marzo 2022 16:14:34 CEST Jaehee Park wrote:
>
> [...]
>
> I'm not entirely sure why container_of structure is helpful. But this article seems to say it makes runtime faster: http://www.kroah.com/log/linux/container_of.html

Hi Jaehee,

container_of() is widely used in Linux kernel code. It's a macro whose purpose
is to return the address of the struct that contains a given field. Users pass 
a pointer to the field, the name of the struct, and the name of the field and 
it returns a pointer to the embedding struct.

It's faster than other means because it's a macro that makes subtractions on 
pointers.

Many in-kernel data structures, like the doubly linked lists API, use it for
convenience and type safety.

Regards,

Fabio M. De Francesco

Re: patch idea for wfx

[Stefano Brivio]

Hi Jaehee,

On Thu, 31 Mar 2022 10:14:34 -0400
Jaehee Park <jhpark1013@gmail.com> wrote:

> I was reading the wfx codebase yesterday and it seems like there are
> a lot of instances of "container_of()" structure which is specific to
> the linux kernel. I saw a "fixme" comment in the code that says that
> the container_of() structure is preferred and I wanted to tackle it.
> Here's the line with the fixme comment:
> https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/sta.c#L744
> 
> Maybe it's too big of a task though. What do you think?

At a glance, it doesn't look *that* big:

	~/net-next/drivers/staging/wfx$ grep "\->vif" * | wc -l
	49

...maybe spend a couple of hours on it and decide if it turns out to be
too complicated? I also can't evaluate so easily. :)

> (Side question: The wfx project is also in the staging/driver but I'm
> not seeing the linux/kernel/git/gregkh/staging repository anywhere on
> github -- I'm probably not understanding something basic here but
> shouldn't the staging directory be a subrepo or submodule within the
> linux kernel repo? ) 

Kernel development doesn't happen on Github: you see some repositories
on Github because they are provided as mirrors of the original
repositories, but the staging tree is here:
	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/

By the way, staging is not a sub-repository of any sort, it's just
another tree, and merges (following pull requests) happen "between"
trees, this is an example:
	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=dfdc1de64248b5e1024d8188aeaf0e59ec6cecd5

Note: the staging tree contains the whole kernel, not just staging
drivers. However, it's the place where development happens for staging
drivers. Let me know if this is clear enough ;)

Now, for the wfx driver itself: the Github repository hosts an
out-of-tree version of the driver, the reason is explained here:
	https://github.com/SiliconLabs/wfx-linux-driver#upstream-status

that is, provide users/hardware vendors with a simplified way to do
backports, meaning applying new changes to older kernel versions.

By the way, for some reason, wfx driver development actually seems to
happen here:
	https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/drivers/staging/wfx

(check the date of the latest change) rather than here:

	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/staging/wfx

so I would suggest that you use the net-next tree in this case. Or,
even better, ask the maintainer about it.

> Now I'm trying to understand the wvif structure they are using in the
> functions. In their previous examples container_of() took in 3
> things: [1] work, [2] struct wfx_dev, and [3] the name. I'm refering
> to the man page for the container_of inputs. I'm not entirely sure
> why container_of structure is helpful. But this article seems to say
> it makes runtime faster:
> http://www.kroah.com/log/linux/container_of.html

Let's consider the example you found. You have, at the beginning of
that function:

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

which means: get a pointer to the private data of the driver.

Here we have "vif", a representation of a generic WiFi interface
(struct ieee80211_vif), which contains a structure including
driver-specific data.

That is, the driver needs to associate to this interface some data that
is specific to this hardware, it can store and access it using drv_priv.

Look at the structure definition:
	https://elixir.bootlin.com/linux/latest/C/ident/ieee80211_vif

the drv_priv pointer is _contained_ in (not referenced by) that, more
generic, struct (see below).

Then:

        /* FIXME: prefer use of container_of() to get vif */
        wvif->vif = vif;

the vif in struct wfx_vif points "back" to the generic interface data.
Once this is set, whenever the driver needs to access that interface
data, it uses wvif->vif:

.-----------------------------------.
|  .------------------------------. |
'->| struct ieee80211_vif         | |
   |------------------------------| |
   | 1                            | |
   | 2                            | |
   |  .--------------------------.| |
   | 3| struct wfx_vif drv_priv   | |
   |  |---------------------------| |
   |  | ...                       | |
   |  |                           | |
   |  | struct ieee80211_vif *vif---'
   '------------------------------'

...but there's no need to have a pointer there! The compiler knows the
offset of drv_priv within a struct ieee80211_vif. In the example that's
"3", but it's actually 888 in my build:

	$ pahole drivers/staging/wfx/wfx.o | grep -A50 "^struct ieee80211_vif " | grep drv_priv
	u8                         drv_priv[] __attribute__((__aligned__(8))); /*   888     0 */

so, by substracting "3" (or, actually, 888) from the address of your
drv_priv, you already have a pointer to struct ieee80211_vif.

So far, this is just what Greg explained in that article. Why is it
"faster", then?

Well, in the current version with the pointer, the CPU will need to
1. load what's written at that location, and then 2. load the result.

Instead, container_of tells the CPU: you have the data at this point,
minus 888. That value is already in the code, you don't need step 1.

Well, step 1. itself isn't that slow, but skipping it probably helps
with prefetching:
	https://en.wikipedia.org/wiki/Cache_prefetching

> Another question I had has to do with testing. I understand we're
> building the kernel to check if the code-change compiles correctly
> (I've enabled the silicon labs driver in the menuconfig and saw that
> it builds successfully for basic changes). But for more complex
> changes, would it be a good idea to test it on the hardware -- after
> build, install the kernel package on an apu2 for example? I have a
> apu2 and I can buy a siliconlab wifi card to test it on there. 

Another possibility can also be to ask the maintainer, or on the
project mailing list, if somebody can test your changes.

If you switch to container_of(), and it builds properly, that's a very
good indication of correctness, but I would personally feel a bit more
confident if an actual functionality test was done.

> I'm brain-dumping ideas messily (sorry! please let me know what you'd
> recommend)

No no, it's appreciated!

> and I'm mostly just conveying my thought process. I am
> having trouble with IRC at the moment so I'm sending my thoughts
> here. 

Let us know if you need assistance with IRC troubles, by the way.

-- 
Stefano

Re: patch idea for wfx

[Jaehee Park]

On Thu, Mar 31, 2022 at 07:52:12PM +0200, Stefano Brivio wrote:
> Hi Jaehee,
> 
> On Thu, 31 Mar 2022 10:14:34 -0400
> Jaehee Park <jhpark1013@gmail.com> wrote:
> 
> > I was reading the wfx codebase yesterday and it seems like there are
> > a lot of instances of "container_of()" structure which is specific to
> > the linux kernel. I saw a "fixme" comment in the code that says that
> > the container_of() structure is preferred and I wanted to tackle it.
> > Here's the line with the fixme comment:
> > https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/sta.c#L744
> > 
> > Maybe it's too big of a task though. What do you think?
> 
> At a glance, it doesn't look *that* big:
> 
> 	~/net-next/drivers/staging/wfx$ grep "\->vif" * | wc -l
> 	49
> 
> ...maybe spend a couple of hours on it and decide if it turns out to be
> too complicated? I also can't evaluate so easily. :)
> 
> > (Side question: The wfx project is also in the staging/driver but I'm
> > not seeing the linux/kernel/git/gregkh/staging repository anywhere on
> > github -- I'm probably not understanding something basic here but
> > shouldn't the staging directory be a subrepo or submodule within the
> > linux kernel repo? ) 
> 
> Kernel development doesn't happen on Github: you see some repositories
> on Github because they are provided as mirrors of the original
> repositories, but the staging tree is here:
> 	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/
> 
> By the way, staging is not a sub-repository of any sort, it's just
> another tree, and merges (following pull requests) happen "between"
> trees, this is an example:
> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=dfdc1de64248b5e1024d8188aeaf0e59ec6cecd5
> 
> Note: the staging tree contains the whole kernel, not just staging
> drivers. However, it's the place where development happens for staging
> drivers. Let me know if this is clear enough ;)
> 

This makes a lot of sense. Thank you!

> Now, for the wfx driver itself: the Github repository hosts an
> out-of-tree version of the driver, the reason is explained here:
> 	https://github.com/SiliconLabs/wfx-linux-driver#upstream-status
> 
> that is, provide users/hardware vendors with a simplified way to do
> backports, meaning applying new changes to older kernel versions.
> 
> By the way, for some reason, wfx driver development actually seems to
> happen here:
> 	https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/drivers/staging/wfx
> 

Good to know!

> (check the date of the latest change) rather than here:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/staging/wfx
> 
> so I would suggest that you use the net-next tree in this case. Or,
> even better, ask the maintainer about it.
> 
> > Now I'm trying to understand the wvif structure they are using in the
> > functions. In their previous examples container_of() took in 3
> > things: [1] work, [2] struct wfx_dev, and [3] the name. I'm refering
> > to the man page for the container_of inputs. I'm not entirely sure
> > why container_of structure is helpful. But this article seems to say
> > it makes runtime faster:
> > http://www.kroah.com/log/linux/container_of.html
> 
> Let's consider the example you found. You have, at the beginning of
> that function:
> 
>         struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv;
> 
> which means: get a pointer to the private data of the driver.
> 
> Here we have "vif", a representation of a generic WiFi interface
> (struct ieee80211_vif), which contains a structure including
> driver-specific data.
> 
> That is, the driver needs to associate to this interface some data that
> is specific to this hardware, it can store and access it using drv_priv.
> 
> Look at the structure definition:
> 	https://elixir.bootlin.com/linux/latest/C/ident/ieee80211_vif
> 
> the drv_priv pointer is _contained_ in (not referenced by) that, more
> generic, struct (see below).
> 
> Then:
> 
>         /* FIXME: prefer use of container_of() to get vif */
>         wvif->vif = vif;
> 
> the vif in struct wfx_vif points "back" to the generic interface data.
> Once this is set, whenever the driver needs to access that interface
> data, it uses wvif->vif:
> 
> .-----------------------------------.
> |  .------------------------------. |
> '->| struct ieee80211_vif         | |
>    |------------------------------| |
>    | 1                            | |
>    | 2                            | |
>    |  .--------------------------.| |
>    | 3| struct wfx_vif drv_priv   | |
>    |  |---------------------------| |
>    |  | ...                       | |
>    |  |                           | |
>    |  | struct ieee80211_vif *vif---'
>    '------------------------------'
> 
> ...but there's no need to have a pointer there! The compiler knows the
> offset of drv_priv within a struct ieee80211_vif. In the example that's
> "3", but it's actually 888 in my build:
> 
> 	$ pahole drivers/staging/wfx/wfx.o | grep -A50 "^struct ieee80211_vif " | grep drv_priv
> 	u8                         drv_priv[] __attribute__((__aligned__(8))); /*   888     0 */
> 
> so, by substracting "3" (or, actually, 888) from the address of your
> drv_priv, you already have a pointer to struct ieee80211_vif.

Ok so my understanding is: vif is an instance of the struct ieee80211_vif which is from the /net/mac80211.h in the linux kernel code and so the compiler knows the offset of drv_priv because it's defined within this generic struct.

This makes is so much clearer in my head; Thank you so much!! I didn't know about pahole. Such a neat tool!

> 
> So far, this is just what Greg explained in that article. Why is it
> "faster", then?
> 
> Well, in the current version with the pointer, the CPU will need to
> 1. load what's written at that location, and then 2. load the result.
> 
> Instead, container_of tells the CPU: you have the data at this point,
> minus 888. That value is already in the code, you don't need step 1.
> 
> Well, step 1. itself isn't that slow, but skipping it probably helps
> with prefetching:
> 	https://en.wikipedia.org/wiki/Cache_prefetching
> 
> > Another question I had has to do with testing. I understand we're
> > building the kernel to check if the code-change compiles correctly
> > (I've enabled the silicon labs driver in the menuconfig and saw that
> > it builds successfully for basic changes). But for more complex
> > changes, would it be a good idea to test it on the hardware -- after
> > build, install the kernel package on an apu2 for example? I have a
> > apu2 and I can buy a siliconlab wifi card to test it on there. 
> 
> Another possibility can also be to ask the maintainer, or on the
> project mailing list, if somebody can test your changes.
> 
> If you switch to container_of(), and it builds properly, that's a very
> good indication of correctness, but I would personally feel a bit more
> confident if an actual functionality test was done.
> 
> > I'm brain-dumping ideas messily (sorry! please let me know what you'd
> > recommend)
> 
> No no, it's appreciated!
> 
> > and I'm mostly just conveying my thought process. I am
> > having trouble with IRC at the moment so I'm sending my thoughts
> > here. 
> 
> Let us know if you need assistance with IRC troubles, by the way.
> 
> -- 
> Stefano
> 

Thank you Stefano for explanining contain_of() so clearly! To implement it, I think I need to implement work_struct from linux/workqueue.h because I see it's being used in container_of() defined in functions: wfx_cooling_timeout_work, wfx_beacon_loss_work, wfx_update_tim_work.
I'm not totally sure of my questions / what to ask here - maybe how does the kernel know that there is work and what work to queue? My questions aren't clear even to me so I need to do more research into mac80211 subsystem's hardware handling. I'll try to ask again in a few hours :)

Thanks!
Jaehee ("Jay-hee")

Re: patch idea for wfx

[Stefano Brivio]

On Thu, 31 Mar 2022 17:47:33 -0400
Jaehee Park <jhpark1013@gmail.com> wrote:

> [...]
>
> Ok so my understanding is: vif is an instance of the struct
> ieee80211_vif which is from the /net/mac80211.h in the linux kernel
> code and so the compiler knows the offset of drv_priv because it's
> defined within this generic struct.

Correct! Well, it's not an instance, it's just a reference to an
instance -- note that it's a pointer. Usually by "instance" one refers
to something more concrete (such as the storage for a given struct).

	struct ieee80211_vif vif <- instance!

	struct ieee80211_vif *vif -> hmm, you could call it an
				     instance, but might cause confusion

> This makes is so much clearer in my head; Thank you so much!! I
> didn't know about pahole. Such a neat tool!

You're welcome, I'm glad it's clear now!

> [...]
>
> Thank you Stefano for explanining contain_of() so clearly! To
> implement it, I think I need to implement work_struct from
> linux/workqueue.h because I see it's being used in container_of()
> defined in functions: wfx_cooling_timeout_work, wfx_beacon_loss_work,
> wfx_update_tim_work.

Hah, no, forget about that! It's just the very same construct but it
has little to do with what you want to change.

Take wfx_update_tim_work(): struct wfx_vif here is like struct
ieee80211_vif because it carries a struct work_struct, here, which is
like your struct wfx_wif:

	struct wfx_vif *wvif = container_of(work, struct wfx_vif, update_tim_work);

meaning:

- struct wfx_vif contains a struct work_struct, and you have a pointer
  to the inner struct ("work")

- "update_tim_work" is the name of the instance of struct work_struct
  inside struct wfx_vif

- from "work", take a pointer to the struct wfx_vif which contains it

> I'm not totally sure of my questions / what to ask here - maybe how
> does the kernel know that there is work and what work to queue?

http://www.makelinux.net/ldd3/chp-7-sect-6.shtml

...but it's unrelated with the issue at hand. :)

-- 
Stefano

Re: patch idea for wfx

[Jaehee Park]

On Fri, Apr 01, 2022 at 12:12:16AM +0200, Stefano Brivio wrote:
> On Thu, 31 Mar 2022 17:47:33 -0400
> Jaehee Park <jhpark1013@gmail.com> wrote:
> 
> > [...]
> >
> > Ok so my understanding is: vif is an instance of the struct
> > ieee80211_vif which is from the /net/mac80211.h in the linux kernel
> > code and so the compiler knows the offset of drv_priv because it's
> > defined within this generic struct.
> 
> Correct! Well, it's not an instance, it's just a reference to an
> instance -- note that it's a pointer. Usually by "instance" one refers
> to something more concrete (such as the storage for a given struct).
> 
> 	struct ieee80211_vif vif <- instance!
> 
> 	struct ieee80211_vif *vif -> hmm, you could call it an
> 				     instance, but might cause confusion
> 
> > This makes is so much clearer in my head; Thank you so much!! I
> > didn't know about pahole. Such a neat tool!
> 
> You're welcome, I'm glad it's clear now!
> 
> > [...]
> >
> > Thank you Stefano for explanining contain_of() so clearly! To
> > implement it, I think I need to implement work_struct from
> > linux/workqueue.h because I see it's being used in container_of()
> > defined in functions: wfx_cooling_timeout_work, wfx_beacon_loss_work,
> > wfx_update_tim_work.
> 
> Hah, no, forget about that! It's just the very same construct but it
> has little to do with what you want to change.
> 
> Take wfx_update_tim_work(): struct wfx_vif here is like struct
> ieee80211_vif because it carries a struct work_struct, here, which is
> like your struct wfx_wif:
> 
> 	struct wfx_vif *wvif = container_of(work, struct wfx_vif, update_tim_work);
> 
> meaning:
> 
> - struct wfx_vif contains a struct work_struct, and you have a pointer
>   to the inner struct ("work")
> 
> - "update_tim_work" is the name of the instance of struct work_struct
>   inside struct wfx_vif
> 
> - from "work", take a pointer to the struct wfx_vif which contains it
> 
> > I'm not totally sure of my questions / what to ask here - maybe how
> > does the kernel know that there is work and what work to queue?
> 
> http://www.makelinux.net/ldd3/chp-7-sect-6.shtml
> 
> ...but it's unrelated with the issue at hand. :)
> 
> -- 
> Stefano
>
Hi Stefano,

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.


-----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)`

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]


-----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?**

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?

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.
- (struct wfx_vif *): The pointer seems to be of unsigned bytes (https://github.com/SiliconLabs/wfx-linux-driver/blob/63952592e64d2ca5b4613ab5559ba40b73ef47e8/wfx.h#L115)
- They are taking those raw bytes into the struct. (or the other way around?)

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? 

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.

Thanks!
Jaehee

-----container_of() implementation attempt-----
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 03025ef7f1be..0aeb848f4164 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -689,7 +689,7 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 {
 	int i;
 	struct wfx_dev *wdev = hw->priv;
-	struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv;
+	struct wfx_vif *wvif = container_of(vif, struct ieee80211_vif, drv_priv);

 	vif->driver_flags |= IEEE80211_VIF_BEACON_FILTER |
 			     IEEE80211_VIF_SUPPORTS_UAPSD |
@@ -708,7 +708,7 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	}

 	/* FIXME: prefer use of container_of() to get vif */
-	wvif->vif = vif;
+	// wvif->vif = vif;
 	wvif->wdev = wdev;

 	wvif->link_id_map = 1; /* link-id 0 is reserved for multicast */

Re: patch idea for wfx

[Fabio M. De Francesco]

On mercoledì 6 aprile 2022 06:28:21 CEST Jaehee Park wrote:
> -----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)`
> 
> 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]

Hi Jaehee,

Probably, before changing any code which could be improved with the use of 
container_of(), you'd better see how this macro is used in other code that 
works properly.

Look at the following example. In drivers/dma/tegra20-apb-dma.c, we can look 
at a one line function called "to_tegra_dma_chan()":

static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
{
        return container_of(dc, struct tegra_dma_channel, dma_chan);
}

"to_tegra_dma_chan()" takes a pointer to a "struct dma_chan" that is called 
"dc" and it returns a pointer to the "struct tegra_dma_channel" which embeds 
the above-mentioned "struct dma_chan". 

Please take a look at the definition of "struct tegra_dma_channel". You'll 
notice that it has a field of type "dma_chan" that is called "dma_chan" 
(coincidentally the names of field and type are the same; this could confuse 
you but you easily see that they are different entities):

struct tegra_dma_channel {
        struct dma_chan dma_chan;
        ...
}

How can "to_tegra_dma_chan()" return a pointer to the embedding "struct 
tegra_dma_channel"?

It uses cointainer_of(). The first argument of that macro is the pointer 
passed to the function ("struct dma_chan *dc"). The second argument is 
the embedding "struct tegra_dma_channel". The third is the name of the 
field (not the type) pointed by "dc".

I hope it helps.

Fabio

Re: patch idea for wfx

[Jaehee Park]

On Wed, Apr 06, 2022 at 03:54:35PM +0200, Fabio M. De Francesco wrote:
> On mercoledì 6 aprile 2022 06:28:21 CEST Jaehee Park wrote:
> > -----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)`
> > 
> > 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]
> 
> Hi Jaehee,
> 
> Probably, before changing any code which could be improved with the use of 
> container_of(), you'd better see how this macro is used in other code that 
> works properly.
> 
> Look at the following example. In drivers/dma/tegra20-apb-dma.c, we can look 
> at a one line function called "to_tegra_dma_chan()":
> 
> static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
> {
>         return container_of(dc, struct tegra_dma_channel, dma_chan);
> }
> 
> "to_tegra_dma_chan()" takes a pointer to a "struct dma_chan" that is called 
> "dc" and it returns a pointer to the "struct tegra_dma_channel" which embeds 
> the above-mentioned "struct dma_chan". 
> 
> Please take a look at the definition of "struct tegra_dma_channel". You'll 
> notice that it has a field of type "dma_chan" that is called "dma_chan" 
> (coincidentally the names of field and type are the same; this could confuse 
> you but you easily see that they are different entities):
> 
> struct tegra_dma_channel {
>         struct dma_chan dma_chan;
>         ...
> }
> 
> How can "to_tegra_dma_chan()" return a pointer to the embedding "struct 
> tegra_dma_channel"?
> 
> It uses cointainer_of(). The first argument of that macro is the pointer 
> passed to the function ("struct dma_chan *dc"). The second argument is 
> the embedding "struct tegra_dma_channel". The third is the name of the 
> field (not the type) pointed by "dc".
> 
> I hope it helps.
> 
> Fabio
> 
>
Hi Fabio, Thank you for your explanation! I wanted to go over how I defined container_of() -- I think I am following the macros input correctly (see my explanation below). Let me know if there's anything odd:

As you've mentioned, the container_of() function contains first the pointer, container struct that embeds the member, and the name of the member within the struct.

In my case, the first argument of the macro is the pointer vif which is from struct ieee80211_vif *vif.
The second argument is the embedding structure struct ieee80211_vif.
The third is the name of the field pointed by vif which is drv_priv (this is a member within the struct ieee80211_vif.)

I also found this example:
https://github.com/open-sdr/openwifi/blob/b9b3abd35319afbe6475a268730b06772cef23a5/driver/sdr.c#L1742
Their implmentation seems similar. But they are setting the output of container of to struct ieee80211_vif -- this makes total sense because the pointer types of the container_of output and whatever we're setting it to should match.

I'm setting it to struct wfx_vif which doesn't match. The reason I did this is so that I can follow the original code which seems to be type punning ieee80211_vif into wfx_vif.

(Attempt 1)
So, to match, it should actually be:
struct wfx_vif *wvif = (struct wfx_vif *)container_of(vif, struct ieee80211_vif, drv_priv);
But this gives an error:
 error: static assertion failed: "pointer type mismatch in container_of()"

(Attempt 2)
Another thing I tried was
struct ieee80211_vif *vif_container = container_of(vif, struct ieee80211_vif, drv_priv);
struct wfx_vif *wvif = (struct wfx_vif *)vif_container;
Here, I was trying to set it to struct ieee80211_vif first before type punning it to struct wfx_vif.. but got the same error.
error: static assertion failed: "pointer type mismatch in container_of()

(Attempt 3):
Then I tried:
struct ieee80211_vif *vif_container = container_of((void *)vif, struct ieee80211_vif, drv_priv);
struct wfx_vif *wvif = (struct wfx_vif *)vif_container;

and now it's building without errors.

But I'm still not sure if this is correct. I'm trying to understand it right now.
I wanted to convey my thought process but maybe I made it more confusing. Let me know your thoughts!

Thanks,
Jaehee

Re: patch idea for wfx

[Stefano Brivio]

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

Re: patch idea for wfx

[Fabio M. De Francesco]

On mercoledì 6 aprile 2022 16:24:32 CEST Jaehee Park wrote:
> On Wed, Apr 06, 2022 at 03:54:35PM +0200, Fabio M. De Francesco wrote:
> > On mercoledì 6 aprile 2022 06:28:21 CEST Jaehee Park wrote:
> > > -----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)`
> > > 
> > > 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]
> > 
> > Hi Jaehee,
> > 
> > Probably, before changing any code which could be improved with the use of 
> > container_of(), you'd better see how this macro is used in other code that 
> > works properly.
> > 
> > Look at the following example. In drivers/dma/tegra20-apb-dma.c, we can look 
> > at a one line function called "to_tegra_dma_chan()":
> > 
> > static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
> > {
> >         return container_of(dc, struct tegra_dma_channel, dma_chan);
> > }
> > 
> > "to_tegra_dma_chan()" takes a pointer to a "struct dma_chan" that is called 
> > "dc" and it returns a pointer to the "struct tegra_dma_channel" which embeds 
> > the above-mentioned "struct dma_chan". 
> > 
> > Please take a look at the definition of "struct tegra_dma_channel". You'll 
> > notice that it has a field of type "dma_chan" that is called "dma_chan" 
> > (coincidentally the names of field and type are the same; this could confuse 
> > you but you easily see that they are different entities):
> > 
> > struct tegra_dma_channel {
> >         struct dma_chan dma_chan;
> >         ...
> > }
> > 
> > How can "to_tegra_dma_chan()" return a pointer to the embedding "struct 
> > tegra_dma_channel"?
> > 
> > It uses cointainer_of(). The first argument of that macro is the pointer 
> > passed to the function ("struct dma_chan *dc"). The second argument is 
> > the embedding "struct tegra_dma_channel". The third is the name of the 
> > field (not the type) pointed by "dc".
> > 
> > I hope it helps.
> > 
> > Fabio
> > 
> >
> Hi Fabio, Thank you for your explanation! I wanted to go over how I defined container_of() -- I think I am following the macros input correctly (see my explanation below). Let me know if there's anything odd:
> 
> As you've mentioned, the container_of() function contains first the pointer, container struct that embeds the member, and the name of the member within the struct.
> 
> In my case, the first argument of the macro is the pointer vif which is from struct ieee80211_vif *vif.
> The second argument is the embedding structure struct ieee80211_vif.
> The third is the name of the field pointed by vif which is drv_priv (this is a member within the struct ieee80211_vif.)
> 
> I also found this example:
> https://github.com/open-sdr/openwifi/blob/b9b3abd35319afbe6475a268730b06772cef23a5/driver/sdr.c#L1742
> Their implementation seems similar. But they are setting the output of container of to struct ieee80211_vif -- this makes total sense because the pointer types of the container_of output and whatever we're setting it to should match.
> 
> I'm setting it to struct wfx_vif which doesn't match. The reason I did this is so that I can follow the original code which seems to be type punning ieee80211_vif into wfx_vif.
> 
> (Attempt 1)
> So, to match, it should actually be:
> struct wfx_vif *wvif = (struct wfx_vif *)container_of(vif, struct ieee80211_vif, drv_priv);
> But this gives an error:
>  error: static assertion failed: "pointer type mismatch in container_of()"
> 
> (Attempt 2)
> Another thing I tried was
> struct ieee80211_vif *vif_container = container_of(vif, struct ieee80211_vif, drv_priv);
> struct wfx_vif *wvif = (struct wfx_vif *)vif_container;
> Here, I was trying to set it to struct ieee80211_vif first before type punning it to struct wfx_vif.. but got the same error.
> error: static assertion failed: "pointer type mismatch in container_of()
> 
> (Attempt 3):
> Then I tried:
> struct ieee80211_vif *vif_container = container_of((void *)vif, struct ieee80211_vif, drv_priv);
> struct wfx_vif *wvif = (struct wfx_vif *)vif_container;
> 
> and now it's building without errors.
> 
> But I'm still not sure if this is correct. I'm trying to understand it right now.
> I wanted to convey my thought process but maybe I made it more confusing. Let me know your thoughts!
> 
> Thanks,
> Jaehee
> 
Hi Jaehee,

A little premise: I haven't neither looked at the code you are trying to 
change, nor at the struct ieee80211_vif and its members. 

Instead I've only taken a look at the prototype of the function 
wfx_add_interface(), which is:

int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif);

It is clear that "vif" is a pointer to a "struct ieee80211_vif".

You wrote the following:

"In my case, the first argument of the macro is the pointer vif which is 
from struct ieee80211_vif *vif.".

Nit, pointers are addresses _to_ memory locations, not _from_ memory 
locations. Besides. if vif is a pointer to a "struct ieee80211_vif" (as
the prototype says) how can it be a pointer to a member field of the
same "struct ieee80211_vif"?

And the you went on with this:

"The second argument is the embedding structure struct ieee80211_vif.
The third is the name of the field pointed by vif which is drv_priv 
(this is a member within the struct ieee80211_vif.)"

^^^^^ here you write that "vif" is a pointer to a member within the struct ieeee80211_vif ^^^^^

Having said this, now I'm looking at "Attempt 3":

"struct ieee80211_vif *vif_container = container_of((void *)vif, struct ieee80211_vif, drv_priv);"

You are still using container_of() incorrectly. As you see, the function's 
prototype says that "vif" is a pointer to a "struct ieee80211_vif".

Either "vif" is a pointer to the embedding structure (struct ieee80211_vif)
or it is a pointer to a member contained in the above struct :) 

Instead you're still confusing members (fields, if you prefer) and embedding 
structures. 

The first parameter of container_of() must be a pointer to the third parameter 
(a member of the struct whose name is in the second parameter of that macro), 
but you are still using a pointer to the second parameter (i.e., the struct that contains "drv_priv" (I take your words for true when you say that "drv_priv" is a member of 
"struct ieee80211_vif" because I haven't checked).

As I said I don't want to read the code of wfx_add_interface() and I don't
want to say to you what has to be fixed. But you are still using container_of()
incorrectly and for the wrong purpose: the purpose of container_of() is to 
return a pointer to the second parameter, while you have only the name of a
member and a pointer to that same member.

The fact that the code compiles only after casting the first argument to 
"void *" is another strong hint that something is still wrong.

Please take your time to re-read the emails that Stefano and I sent to you.
I'm sure that you now have enough information to figure it out when and
how to use container_of()

Thanks,

Fabio

Re: patch idea for wfx

[Jaehee Park]

On Wed, Apr 06, 2022 at 06:04:12PM +0200, Fabio M. De Francesco wrote:
> On mercoledì 6 aprile 2022 16:24:32 CEST Jaehee Park wrote:
> > On Wed, Apr 06, 2022 at 03:54:35PM +0200, Fabio M. De Francesco wrote:
> > > On mercoledì 6 aprile 2022 06:28:21 CEST Jaehee Park wrote:
> > > > -----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)`
> > > > 
> > > > 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]
> > > 
> > > Hi Jaehee,
> > > 
> > > Probably, before changing any code which could be improved with the use of 
> > > container_of(), you'd better see how this macro is used in other code that 
> > > works properly.
> > > 
> > > Look at the following example. In drivers/dma/tegra20-apb-dma.c, we can look 
> > > at a one line function called "to_tegra_dma_chan()":
> > > 
> > > static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
> > > {
> > >         return container_of(dc, struct tegra_dma_channel, dma_chan);
> > > }
> > > 
> > > "to_tegra_dma_chan()" takes a pointer to a "struct dma_chan" that is called 
> > > "dc" and it returns a pointer to the "struct tegra_dma_channel" which embeds 
> > > the above-mentioned "struct dma_chan". 
> > > 
> > > Please take a look at the definition of "struct tegra_dma_channel". You'll 
> > > notice that it has a field of type "dma_chan" that is called "dma_chan" 
> > > (coincidentally the names of field and type are the same; this could confuse 
> > > you but you easily see that they are different entities):
> > > 
> > > struct tegra_dma_channel {
> > >         struct dma_chan dma_chan;
> > >         ...
> > > }
> > > 
> > > How can "to_tegra_dma_chan()" return a pointer to the embedding "struct 
> > > tegra_dma_channel"?
> > > 
> > > It uses cointainer_of(). The first argument of that macro is the pointer 
> > > passed to the function ("struct dma_chan *dc"). The second argument is 
> > > the embedding "struct tegra_dma_channel". The third is the name of the 
> > > field (not the type) pointed by "dc".
> > > 
> > > I hope it helps.
> > > 
> > > Fabio
> > > 
> > >
> > Hi Fabio, Thank you for your explanation! I wanted to go over how I defined container_of() -- I think I am following the macros input correctly (see my explanation below). Let me know if there's anything odd:
> > 
> > As you've mentioned, the container_of() function contains first the pointer, container struct that embeds the member, and the name of the member within the struct.
> > 
> > In my case, the first argument of the macro is the pointer vif which is from struct ieee80211_vif *vif.
> > The second argument is the embedding structure struct ieee80211_vif.
> > The third is the name of the field pointed by vif which is drv_priv (this is a member within the struct ieee80211_vif.)
> > 
> > I also found this example:
> > https://github.com/open-sdr/openwifi/blob/b9b3abd35319afbe6475a268730b06772cef23a5/driver/sdr.c#L1742
> > Their implementation seems similar. But they are setting the output of container of to struct ieee80211_vif -- this makes total sense because the pointer types of the container_of output and whatever we're setting it to should match.
> > 
> > I'm setting it to struct wfx_vif which doesn't match. The reason I did this is so that I can follow the original code which seems to be type punning ieee80211_vif into wfx_vif.
> > 
> > (Attempt 1)
> > So, to match, it should actually be:
> > struct wfx_vif *wvif = (struct wfx_vif *)container_of(vif, struct ieee80211_vif, drv_priv);
> > But this gives an error:
> >  error: static assertion failed: "pointer type mismatch in container_of()"
> > 
> > (Attempt 2)
> > Another thing I tried was
> > struct ieee80211_vif *vif_container = container_of(vif, struct ieee80211_vif, drv_priv);
> > struct wfx_vif *wvif = (struct wfx_vif *)vif_container;
> > Here, I was trying to set it to struct ieee80211_vif first before type punning it to struct wfx_vif.. but got the same error.
> > error: static assertion failed: "pointer type mismatch in container_of()
> > 
> > (Attempt 3):
> > Then I tried:
> > struct ieee80211_vif *vif_container = container_of((void *)vif, struct ieee80211_vif, drv_priv);
> > struct wfx_vif *wvif = (struct wfx_vif *)vif_container;
> > 
> > and now it's building without errors.
> > 
> > But I'm still not sure if this is correct. I'm trying to understand it right now.
> > I wanted to convey my thought process but maybe I made it more confusing. Let me know your thoughts!
> > 
> > Thanks,
> > Jaehee
> > 
> Hi Jaehee,
> 
> A little premise: I haven't neither looked at the code you are trying to 
> change, nor at the struct ieee80211_vif and its members. 
> 
> Instead I've only taken a look at the prototype of the function 
> wfx_add_interface(), which is:
> 
> int wfx_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif);
> 
> It is clear that "vif" is a pointer to a "struct ieee80211_vif".
> 
> You wrote the following:
> 
> "In my case, the first argument of the macro is the pointer vif which is 
> from struct ieee80211_vif *vif.".
> 
> Nit, pointers are addresses _to_ memory locations, not _from_ memory 
> locations. Besides. if vif is a pointer to a "struct ieee80211_vif" (as
> the prototype says) how can it be a pointer to a member field of the
> same "struct ieee80211_vif"?
> 
> And the you went on with this:
> 
> "The second argument is the embedding structure struct ieee80211_vif.
> The third is the name of the field pointed by vif which is drv_priv 
> (this is a member within the struct ieee80211_vif.)"
> 
> ^^^^^ here you write that "vif" is a pointer to a member within the struct ieeee80211_vif ^^^^^
> 
> Having said this, now I'm looking at "Attempt 3":
> 
> "struct ieee80211_vif *vif_container = container_of((void *)vif, struct ieee80211_vif, drv_priv);"
> 
> You are still using container_of() incorrectly. As you see, the function's 
> prototype says that "vif" is a pointer to a "struct ieee80211_vif".
> 
> Either "vif" is a pointer to the embedding structure (struct ieee80211_vif)
> or it is a pointer to a member contained in the above struct :) 
> 
> Instead you're still confusing members (fields, if you prefer) and embedding 
> structures. 
> 
> The first parameter of container_of() must be a pointer to the third parameter 
> (a member of the struct whose name is in the second parameter of that macro), 
> but you are still using a pointer to the second parameter (i.e., the struct that contains "drv_priv" (I take your words for true when you say that "drv_priv" is a member of 
> "struct ieee80211_vif" because I haven't checked).
> 
> As I said I don't want to read the code of wfx_add_interface() and I don't
> want to say to you what has to be fixed. But you are still using container_of()
> incorrectly and for the wrong purpose: the purpose of container_of() is to 
> return a pointer to the second parameter, while you have only the name of a
> member and a pointer to that same member.
> 
> The fact that the code compiles only after casting the first argument to 
> "void *" is another strong hint that something is still wrong.
> 
> Please take your time to re-read the emails that Stefano and I sent to you.
> I'm sure that you now have enough information to figure it out when and
> how to use container_of()
> 
> Thanks,
> 
> Fabio
> 
>
Thanks for helping! I sorted it out through on irc; the container_of() inputs have been corrected.