staging/wlan-ng query: convert to flexible array member

staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

Hello,

First, my apologies for the long email.
I am requesting guidance on how to approach resolving the zero element flexible
VLO struct implementation in this driver in file drivers/staging/waln-ng/hfa384x.f

The struct hfa384x_pdrec contains nested structs with zero element arrays.  These
zero element structs are part of a union 'data' inside the struct container. This
union 'data' is the last element of this container. Please see the code snip below:

<snip>

	1068 struct hfa384x_pdrec {
	   1         __le16 len;             /* in words */
	   2         __le16 code;
	   3         union pdr {
	   4                 struct hfa384x_pdr_pcb_partnum pcb_partnum;
	  11                 struct hfa384x_pdr_nicid nicid;
	  12                 struct hfa384x_pdr_refdac_measurements refdac_measurements;
	  13                 struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
	  14                 struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
	  15                 struct hfa384x_pdr_mac_address mac_address;
	  39         } data;
	  40 } __packed;

</snip>

The three structures in question are declared as follows in the same file:

<snip>
	962  struct hfa384x_pdr_refdac_measurements {
	   1         u16 value[0];
	   2 } __packed;
	   3
	   4 struct hfa384x_pdr_vgdac_measurements {
	   5         u16 value[0];
	   6 } __packed;
	   7
	   8 struct hfa384x_pdr_level_comp_measurements {
	   9         u16 value[0];
	  10 } __packed;
</snip>

As per the C99 specifications, the flexible array struct should have at least
one member other than the true flexible array member. So converting these from
[0] to [] is not feasible in the current form.

I did not find these struct variables being used for memory allocation in the
code directly. My find may be short since the implementation appears to get very
complex as I tried to get deeper.

Can you please suggest how should I approach correcting the zero element flex
array implementation here? Can these structs be removed if they are unused?

Thank you.
./drv

Re: staging/wlan-ng query: convert to flexible array member

[Greg Kroah-Hartman]

On Tue, Nov 08, 2022 at 08:42:59PM +0530, Deepak R Varma wrote:
> Hello,
> 
> First, my apologies for the long email.
> I am requesting guidance on how to approach resolving the zero element flexible
> VLO struct implementation in this driver in file drivers/staging/waln-ng/hfa384x.f
> 
> The struct hfa384x_pdrec contains nested structs with zero element arrays.  These
> zero element structs are part of a union 'data' inside the struct container. This
> union 'data' is the last element of this container. Please see the code snip below:
> 
> <snip>
> 
> 	1068 struct hfa384x_pdrec {
> 	   1         __le16 len;             /* in words */
> 	   2         __le16 code;
> 	   3         union pdr {
> 	   4                 struct hfa384x_pdr_pcb_partnum pcb_partnum;
> 	  11                 struct hfa384x_pdr_nicid nicid;
> 	  12                 struct hfa384x_pdr_refdac_measurements refdac_measurements;
> 	  13                 struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
> 	  14                 struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
> 	  15                 struct hfa384x_pdr_mac_address mac_address;
> 	  39         } data;
> 	  40 } __packed;
> 
> </snip>
> 
> The three structures in question are declared as follows in the same file:
> 
> <snip>
> 	962  struct hfa384x_pdr_refdac_measurements {
> 	   1         u16 value[0];
> 	   2 } __packed;
> 	   3
> 	   4 struct hfa384x_pdr_vgdac_measurements {
> 	   5         u16 value[0];
> 	   6 } __packed;
> 	   7
> 	   8 struct hfa384x_pdr_level_comp_measurements {
> 	   9         u16 value[0];
> 	  10 } __packed;
> </snip>
> 
> As per the C99 specifications, the flexible array struct should have at least
> one member other than the true flexible array member. So converting these from
> [0] to [] is not feasible in the current form.
> 
> I did not find these struct variables being used for memory allocation in the
> code directly. My find may be short since the implementation appears to get very
> complex as I tried to get deeper.
> 
> Can you please suggest how should I approach correcting the zero element flex
> array implementation here? Can these structs be removed if they are unused?

Are you sure they are unused?

They look like structures that are read from the memory of a device,
right?  Try removing the structures from the union and see what happens
:)

thanks,

greg k-h

Re: staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2022 at 08:42:59PM +0530, Deepak R Varma wrote:
> > Hello,
> >
> > First, my apologies for the long email.
> > I am requesting guidance on how to approach resolving the zero element flexible
> > VLO struct implementation in this driver in file drivers/staging/waln-ng/hfa384x.f
> >
> > The struct hfa384x_pdrec contains nested structs with zero element arrays.  These
> > zero element structs are part of a union 'data' inside the struct container. This
> > union 'data' is the last element of this container. Please see the code snip below:
> >
> > <snip>
> >
> > 	1068 struct hfa384x_pdrec {
> > 	   1         __le16 len;             /* in words */
> > 	   2         __le16 code;
> > 	   3         union pdr {
> > 	   4                 struct hfa384x_pdr_pcb_partnum pcb_partnum;
> > 	  11                 struct hfa384x_pdr_nicid nicid;
> > 	  12                 struct hfa384x_pdr_refdac_measurements refdac_measurements;
> > 	  13                 struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
> > 	  14                 struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
> > 	  15                 struct hfa384x_pdr_mac_address mac_address;
> > 	  39         } data;
> > 	  40 } __packed;
> >
> > </snip>
> >
> > The three structures in question are declared as follows in the same file:
> >
> > <snip>
> > 	962  struct hfa384x_pdr_refdac_measurements {
> > 	   1         u16 value[0];
> > 	   2 } __packed;
> > 	   3
> > 	   4 struct hfa384x_pdr_vgdac_measurements {
> > 	   5         u16 value[0];
> > 	   6 } __packed;
> > 	   7
> > 	   8 struct hfa384x_pdr_level_comp_measurements {
> > 	   9         u16 value[0];
> > 	  10 } __packed;
> > </snip>
> >
> > As per the C99 specifications, the flexible array struct should have at least
> > one member other than the true flexible array member. So converting these from
> > [0] to [] is not feasible in the current form.
> >
> > I did not find these struct variables being used for memory allocation in the
> > code directly. My find may be short since the implementation appears to get very
> > complex as I tried to get deeper.
> >
> > Can you please suggest how should I approach correcting the zero element flex
> > array implementation here? Can these structs be removed if they are unused?
>
> Are you sure they are unused?
>
> They look like structures that are read from the memory of a device,
> right?  Try removing the structures from the union and see what happens
> :)

I did remove the structs from the union and it built fine. Is there anything else I
can check/test to verify the impact?

<snip>
	drv@qemulion:~/git/kernels/staging$ git diff
	diff --git a/drivers/staging/wlan-ng/hfa384x.h b/drivers/staging/wlan-ng/hfa384x.h
	index 0611e37df6ac..8fe10aa93dfb 100644
	--- a/drivers/staging/wlan-ng/hfa384x.h
	+++ b/drivers/staging/wlan-ng/hfa384x.h
	@@ -1077,9 +1077,6 @@ struct hfa384x_pdrec {
			struct hfa384x_pdr_mfisuprange mfisuprange;
			struct hfa384x_pdr_cfisuprange cfisuprange;
			struct hfa384x_pdr_nicid nicid;
	-               struct hfa384x_pdr_refdac_measurements refdac_measurements;
	-               struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
	-               struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
			struct hfa384x_pdr_mac_address mac_address;
			struct hfa384x_pdr_mkk_callname mkk_callname;
			struct hfa384x_pdr_regdomain regdomain;
	drv@qemulion:~/git/kernels/staging$ make M=drivers/staging/wlan-ng/
	  CC [M]  drivers/staging/wlan-ng/prism2usb.o
	  CC [M]  drivers/staging/wlan-ng/p80211netdev.o
	  LD [M]  drivers/staging/wlan-ng/prism2_usb.o
	  MODPOST drivers/staging/wlan-ng/Module.symvers
	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
	drv@qemulion:~/git/kernels/staging$
</snip>

>
> thanks,
>
> greg k-h

Re: staging/wlan-ng query: convert to flexible array member

[Greg Kroah-Hartman]

On Tue, Nov 08, 2022 at 09:15:10PM +0530, Deepak R Varma wrote:
> On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 08, 2022 at 08:42:59PM +0530, Deepak R Varma wrote:
> > > Hello,
> > >
> > > First, my apologies for the long email.
> > > I am requesting guidance on how to approach resolving the zero element flexible
> > > VLO struct implementation in this driver in file drivers/staging/waln-ng/hfa384x.f
> > >
> > > The struct hfa384x_pdrec contains nested structs with zero element arrays.  These
> > > zero element structs are part of a union 'data' inside the struct container. This
> > > union 'data' is the last element of this container. Please see the code snip below:
> > >
> > > <snip>
> > >
> > > 	1068 struct hfa384x_pdrec {
> > > 	   1         __le16 len;             /* in words */
> > > 	   2         __le16 code;
> > > 	   3         union pdr {
> > > 	   4                 struct hfa384x_pdr_pcb_partnum pcb_partnum;
> > > 	  11                 struct hfa384x_pdr_nicid nicid;
> > > 	  12                 struct hfa384x_pdr_refdac_measurements refdac_measurements;
> > > 	  13                 struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
> > > 	  14                 struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
> > > 	  15                 struct hfa384x_pdr_mac_address mac_address;
> > > 	  39         } data;
> > > 	  40 } __packed;
> > >
> > > </snip>
> > >
> > > The three structures in question are declared as follows in the same file:
> > >
> > > <snip>
> > > 	962  struct hfa384x_pdr_refdac_measurements {
> > > 	   1         u16 value[0];
> > > 	   2 } __packed;
> > > 	   3
> > > 	   4 struct hfa384x_pdr_vgdac_measurements {
> > > 	   5         u16 value[0];
> > > 	   6 } __packed;
> > > 	   7
> > > 	   8 struct hfa384x_pdr_level_comp_measurements {
> > > 	   9         u16 value[0];
> > > 	  10 } __packed;
> > > </snip>
> > >
> > > As per the C99 specifications, the flexible array struct should have at least
> > > one member other than the true flexible array member. So converting these from
> > > [0] to [] is not feasible in the current form.
> > >
> > > I did not find these struct variables being used for memory allocation in the
> > > code directly. My find may be short since the implementation appears to get very
> > > complex as I tried to get deeper.
> > >
> > > Can you please suggest how should I approach correcting the zero element flex
> > > array implementation here? Can these structs be removed if they are unused?
> >
> > Are you sure they are unused?
> >
> > They look like structures that are read from the memory of a device,
> > right?  Try removing the structures from the union and see what happens
> > :)
> 
> I did remove the structs from the union and it built fine. Is there anything else I
> can check/test to verify the impact?
> 
> <snip>
> 	drv@qemulion:~/git/kernels/staging$ git diff
> 	diff --git a/drivers/staging/wlan-ng/hfa384x.h b/drivers/staging/wlan-ng/hfa384x.h
> 	index 0611e37df6ac..8fe10aa93dfb 100644
> 	--- a/drivers/staging/wlan-ng/hfa384x.h
> 	+++ b/drivers/staging/wlan-ng/hfa384x.h
> 	@@ -1077,9 +1077,6 @@ struct hfa384x_pdrec {
> 			struct hfa384x_pdr_mfisuprange mfisuprange;
> 			struct hfa384x_pdr_cfisuprange cfisuprange;
> 			struct hfa384x_pdr_nicid nicid;
> 	-               struct hfa384x_pdr_refdac_measurements refdac_measurements;
> 	-               struct hfa384x_pdr_vgdac_measurements vgdac_measurements;
> 	-               struct hfa384x_pdr_level_comp_measurements level_compc_measurements;
> 			struct hfa384x_pdr_mac_address mac_address;
> 			struct hfa384x_pdr_mkk_callname mkk_callname;
> 			struct hfa384x_pdr_regdomain regdomain;
> 	drv@qemulion:~/git/kernels/staging$ make M=drivers/staging/wlan-ng/
> 	  CC [M]  drivers/staging/wlan-ng/prism2usb.o
> 	  CC [M]  drivers/staging/wlan-ng/p80211netdev.o
> 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.o
> 	  MODPOST drivers/staging/wlan-ng/Module.symvers
> 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
> 	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
> 	drv@qemulion:~/git/kernels/staging$
> </snip>
> 

Test the device to make sure it still works?

Re: staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

On Tue, Nov 08, 2022 at 04:52:44PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2022 at 09:15:10PM +0530, Deepak R Varma wrote:
> > On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> > 	  MODPOST drivers/staging/wlan-ng/Module.symvers
> > 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
> > 	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
> > 	drv@qemulion:~/git/kernels/staging$
> > </snip>
> >
>
> Test the device to make sure it still works?

I was able to build and load the driver on my machine. I do not have p54 device
to test. Is there another way to test it? Some sort of a udev program???

<snip>
	 static struct usb_driver prism2_usb_driver = {
	-       .name = "prism2_usb",
	+       .name = "prism2_usb_dvk",
		.probe = prism2sta_probe_usb,
		.disconnect = prism2sta_disconnect_usb,
		.id_table = usb_prism_tbl,
	drv@qemulion:~/git/kernels/staging$ sudo dmesg
	[  948.476144] prism2_usb: module is from the staging directory, the quality is unknown, you have been warned.
	[  948.478631] usbcore: registered new interface driver prism2_usb_dvk
</snip>

Thank you,
./drv

>

Re: staging/wlan-ng query: convert to flexible array member

[Greg Kroah-Hartman]

On Tue, Nov 08, 2022 at 11:07:23PM +0530, Deepak R Varma wrote:
> On Tue, Nov 08, 2022 at 04:52:44PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 08, 2022 at 09:15:10PM +0530, Deepak R Varma wrote:
> > > On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> > > 	  MODPOST drivers/staging/wlan-ng/Module.symvers
> > > 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
> > > 	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
> > > 	drv@qemulion:~/git/kernels/staging$
> > > </snip>
> > >
> >
> > Test the device to make sure it still works?
> 
> I was able to build and load the driver on my machine. I do not have p54 device
> to test. Is there another way to test it? Some sort of a udev program???

You need the real hardware to test it properly.

Re: staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

On Tue, Nov 08, 2022 at 07:32:19PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2022 at 11:07:23PM +0530, Deepak R Varma wrote:
> > On Tue, Nov 08, 2022 at 04:52:44PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 08, 2022 at 09:15:10PM +0530, Deepak R Varma wrote:
> > > > On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> > > > 	  MODPOST drivers/staging/wlan-ng/Module.symvers
> > > > 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
> > > > 	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
> > > > 	drv@qemulion:~/git/kernels/staging$
> > > > </snip>
> > > >
> > >
> > > Test the device to make sure it still works?
> >
> > I was able to build and load the driver on my machine. I do not have p54 device
> > to test. Is there another way to test it? Some sort of a udev program???
>
> You need the real hardware to test it properly.

Do you know if I work with someone to test the change locally rather than
sending in a untested change?


Thank you,
./drv
>

Re: staging/wlan-ng query: convert to flexible array member

[Greg Kroah-Hartman]

On Wed, Nov 09, 2022 at 12:08:41AM +0530, Deepak R Varma wrote:
> On Tue, Nov 08, 2022 at 07:32:19PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 08, 2022 at 11:07:23PM +0530, Deepak R Varma wrote:
> > > On Tue, Nov 08, 2022 at 04:52:44PM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Nov 08, 2022 at 09:15:10PM +0530, Deepak R Varma wrote:
> > > > > On Tue, Nov 08, 2022 at 04:34:15PM +0100, Greg Kroah-Hartman wrote:
> > > > > 	  MODPOST drivers/staging/wlan-ng/Module.symvers
> > > > > 	  LD [M]  drivers/staging/wlan-ng/prism2_usb.ko
> > > > > 	  BTF [M] drivers/staging/wlan-ng/prism2_usb.ko
> > > > > 	drv@qemulion:~/git/kernels/staging$
> > > > > </snip>
> > > > >
> > > >
> > > > Test the device to make sure it still works?
> > >
> > > I was able to build and load the driver on my machine. I do not have p54 device
> > > to test. Is there another way to test it? Some sort of a udev program???
> >
> > You need the real hardware to test it properly.
> 
> Do you know if I work with someone to test the change locally rather than
> sending in a untested change?

If you can find someone, sure!

Re: staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

On Tue, Nov 08, 2022 at 08:43:52PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 09, 2022 at 12:08:41AM +0530, Deepak R Varma wrote:
> > On Tue, Nov 08, 2022 at 07:32:19PM +0100, Greg Kroah-Hartman wrote:
> > > > > Test the device to make sure it still works?
> > > >
> > > > I was able to build and load the driver on my machine. I do not have p54 device
> > > > to test. Is there another way to test it? Some sort of a udev program???
> > >
> > > You need the real hardware to test it properly.
> >
> > Do you know if I work with someone to test the change locally rather than
> > sending in a untested change?
>
> If you can find someone, sure!

Hello Greg,
I reached out to couple of recent patch authors to check if they are able to
help test the change.

Hello all,
Does anyone have a hardware that can test prism2_usb [wlan-ng] driver change? I
assume it would be quick to test the change I am proposing. Let me know if you
are able to help.

Thank you!
./drv


>

Re: staging/wlan-ng query: convert to flexible array member

[Gustavo A. R. Silva]

On Tue, Nov 08, 2022 at 08:42:59PM +0530, Deepak R Varma wrote:
> 
> Can you please suggest how should I approach correcting the zero element flex
> array implementation here? Can these structs be removed if they are unused?

You can try using DECLARE_FLEX_ARRAY(). See this[1] patch.

--
Gustavo

[1] https://git.kernel.org/linus/6e4a53ee7989c8a2b9fc3b14cd90f6e2d613ca76

Re: staging/wlan-ng query: convert to flexible array member

[Deepak R Varma]

On Sun, Nov 13, 2022 at 12:33:43AM -0600, Gustavo A. R. Silva wrote:
> On Tue, Nov 08, 2022 at 08:42:59PM +0530, Deepak R Varma wrote:
> >
> > Can you please suggest how should I approach correcting the zero element flex
> > array implementation here? Can these structs be removed if they are unused?
>
> You can try using DECLARE_FLEX_ARRAY(). See this[1] patch.

Thank you very much for the quick reply. The link you provided is very helpful.
I will review it in detail and correct my patch proposal accordingly.

ps: Also. thank you for including the cc list. I has already made a lot of noise
with on the lists, so did not copy them intentionally in my email to you.

./drv

>
> --
> Gustavo
>
> [1] https://git.kernel.org/linus/6e4a53ee7989c8a2b9fc3b14cd90f6e2d613ca76
>