DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size
@ 2020-03-18 17:40 Oscar Carter
  2020-03-24  9:54 ` Quentin Deslandes
  0 siblings, 1 reply; 5+ messages in thread
From: Oscar Carter @ 2020-03-18 17:40 UTC (permalink / raw)
  To: Forest Bond, Greg Kroah-Hartman
  Cc: devel, Oscar Carter, Malcolm Priestley, linux-kernel,
	Gabriela Bittencourt, Colin Ian King, Dan Carpenter

Use ARRAY_SIZE to replace the hardcoded size so we will never have a
mismatch.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
Changelog v1 -> v2
- Use ARRAY_SIZE(priv->cck_pwr_tbl) everywhere instead of introducing a new
  variable to hold its value.

 drivers/staging/vt6656/main_usb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 5e48b3ddb94c..acfcc11c3b61 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -23,6 +23,7 @@

 #include <linux/etherdevice.h>
 #include <linux/file.h>
+#include <linux/kernel.h>
 #include "device.h"
 #include "card.h"
 #include "baseband.h"
@@ -145,7 +146,7 @@ static int vnt_init_registers(struct vnt_private *priv)

 	init_cmd->init_class = DEVICE_INIT_COLD;
 	init_cmd->exist_sw_net_addr = priv->exist_sw_net_addr;
-	for (ii = 0; ii < 6; ii++)
+	for (ii = 0; ii < ARRAY_SIZE(init_cmd->sw_net_addr); ii++)
 		init_cmd->sw_net_addr[ii] = priv->current_net_addr[ii];
 	init_cmd->short_retry_limit = priv->short_retry_limit;
 	init_cmd->long_retry_limit = priv->long_retry_limit;
@@ -184,7 +185,7 @@ static int vnt_init_registers(struct vnt_private *priv)
 	priv->cck_pwr = priv->eeprom[EEP_OFS_PWR_CCK];
 	priv->ofdm_pwr_g = priv->eeprom[EEP_OFS_PWR_OFDMG];
 	/* load power table */
-	for (ii = 0; ii < 14; ii++) {
+	for (ii = 0; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
 		priv->cck_pwr_tbl[ii] =
 			priv->eeprom[ii + EEP_OFS_CCK_PWR_TBL];
 		if (priv->cck_pwr_tbl[ii] == 0)
@@ -200,7 +201,7 @@ static int vnt_init_registers(struct vnt_private *priv)
 	 * original zonetype is USA, but custom zonetype is Europe,
 	 * then need to recover 12, 13, 14 channels with 11 channel
 	 */
-	for (ii = 11; ii < 14; ii++) {
+	for (ii = 11; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
 		priv->cck_pwr_tbl[ii] = priv->cck_pwr_tbl[10];
 		priv->ofdm_pwr_tbl[ii] = priv->ofdm_pwr_tbl[10];
 	}
--
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size
  2020-03-18 17:40 [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size Oscar Carter
@ 2020-03-24  9:54 ` Quentin Deslandes
  2020-03-24 13:18   ` Dan Carpenter
  0 siblings, 1 reply; 5+ messages in thread
From: Quentin Deslandes @ 2020-03-24  9:54 UTC (permalink / raw)
  To: Oscar Carter
  Cc: devel, Malcolm Priestley, Greg Kroah-Hartman, linux-kernel,
	Forest Bond, Gabriela Bittencourt, Colin Ian King, Dan Carpenter

On 03/18/20 18:40:15, Oscar Carter wrote:
> Use ARRAY_SIZE to replace the hardcoded size so we will never have a
> mismatch.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
> Changelog v1 -> v2
> - Use ARRAY_SIZE(priv->cck_pwr_tbl) everywhere instead of introducing a new
>   variable to hold its value.
> 
>  drivers/staging/vt6656/main_usb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 5e48b3ddb94c..acfcc11c3b61 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -23,6 +23,7 @@
> 
>  #include <linux/etherdevice.h>
>  #include <linux/file.h>
> +#include <linux/kernel.h>
>  #include "device.h"
>  #include "card.h"
>  #include "baseband.h"
> @@ -145,7 +146,7 @@ static int vnt_init_registers(struct vnt_private *priv)
> 
>  	init_cmd->init_class = DEVICE_INIT_COLD;
>  	init_cmd->exist_sw_net_addr = priv->exist_sw_net_addr;
> -	for (ii = 0; ii < 6; ii++)
> +	for (ii = 0; ii < ARRAY_SIZE(init_cmd->sw_net_addr); ii++)
>  		init_cmd->sw_net_addr[ii] = priv->current_net_addr[ii];
>  	init_cmd->short_retry_limit = priv->short_retry_limit;
>  	init_cmd->long_retry_limit = priv->long_retry_limit;
> @@ -184,7 +185,7 @@ static int vnt_init_registers(struct vnt_private *priv)
>  	priv->cck_pwr = priv->eeprom[EEP_OFS_PWR_CCK];
>  	priv->ofdm_pwr_g = priv->eeprom[EEP_OFS_PWR_OFDMG];
>  	/* load power table */
> -	for (ii = 0; ii < 14; ii++) {
> +	for (ii = 0; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
>  		priv->cck_pwr_tbl[ii] =
>  			priv->eeprom[ii + EEP_OFS_CCK_PWR_TBL];
>  		if (priv->cck_pwr_tbl[ii] == 0)
> @@ -200,7 +201,7 @@ static int vnt_init_registers(struct vnt_private *priv)
>  	 * original zonetype is USA, but custom zonetype is Europe,
>  	 * then need to recover 12, 13, 14 channels with 11 channel
>  	 */
> -	for (ii = 11; ii < 14; ii++) {
> +	for (ii = 11; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
>  		priv->cck_pwr_tbl[ii] = priv->cck_pwr_tbl[10];
>  		priv->ofdm_pwr_tbl[ii] = priv->ofdm_pwr_tbl[10];
>  	}
> --
> 2.20.1
> 

Looks good, however are we certain priv->cck_pwr_tbl and
priv->ofdm_pwr_tbl are always the same size?

What about using a macro for cck_pwr_tbl and ofdm_pwr_tbl size in
device.h? Or a BUILD_BUG() if array's sizes are different? It could be
helpful for future developers to say these arrays must be the same size.

Thanks,
Quentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size
  2020-03-24  9:54 ` Quentin Deslandes
@ 2020-03-24 13:18   ` Dan Carpenter
  2020-03-25  9:19     ` Quentin Deslandes
  0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2020-03-24 13:18 UTC (permalink / raw)
  To: Quentin Deslandes
  Cc: devel, Oscar Carter, Malcolm Priestley, Greg Kroah-Hartman,
	linux-kernel, Forest Bond, Gabriela Bittencourt, Colin Ian King

On Tue, Mar 24, 2020 at 09:54:56AM +0000, Quentin Deslandes wrote:
> On 03/18/20 18:40:15, Oscar Carter wrote:
> > Use ARRAY_SIZE to replace the hardcoded size so we will never have a
> > mismatch.
> > 
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > ---
> > Changelog v1 -> v2
> > - Use ARRAY_SIZE(priv->cck_pwr_tbl) everywhere instead of introducing a new
> >   variable to hold its value.
> > 
> >  drivers/staging/vt6656/main_usb.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index 5e48b3ddb94c..acfcc11c3b61 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -23,6 +23,7 @@
> > 
> >  #include <linux/etherdevice.h>
> >  #include <linux/file.h>
> > +#include <linux/kernel.h>
> >  #include "device.h"
> >  #include "card.h"
> >  #include "baseband.h"
> > @@ -145,7 +146,7 @@ static int vnt_init_registers(struct vnt_private *priv)
> > 
> >  	init_cmd->init_class = DEVICE_INIT_COLD;
> >  	init_cmd->exist_sw_net_addr = priv->exist_sw_net_addr;
> > -	for (ii = 0; ii < 6; ii++)
> > +	for (ii = 0; ii < ARRAY_SIZE(init_cmd->sw_net_addr); ii++)
> >  		init_cmd->sw_net_addr[ii] = priv->current_net_addr[ii];
> >  	init_cmd->short_retry_limit = priv->short_retry_limit;
> >  	init_cmd->long_retry_limit = priv->long_retry_limit;
> > @@ -184,7 +185,7 @@ static int vnt_init_registers(struct vnt_private *priv)
> >  	priv->cck_pwr = priv->eeprom[EEP_OFS_PWR_CCK];
> >  	priv->ofdm_pwr_g = priv->eeprom[EEP_OFS_PWR_OFDMG];
> >  	/* load power table */
> > -	for (ii = 0; ii < 14; ii++) {
> > +	for (ii = 0; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
> >  		priv->cck_pwr_tbl[ii] =
> >  			priv->eeprom[ii + EEP_OFS_CCK_PWR_TBL];
> >  		if (priv->cck_pwr_tbl[ii] == 0)
> > @@ -200,7 +201,7 @@ static int vnt_init_registers(struct vnt_private *priv)
> >  	 * original zonetype is USA, but custom zonetype is Europe,
> >  	 * then need to recover 12, 13, 14 channels with 11 channel
> >  	 */
> > -	for (ii = 11; ii < 14; ii++) {
> > +	for (ii = 11; ii < ARRAY_SIZE(priv->cck_pwr_tbl); ii++) {
> >  		priv->cck_pwr_tbl[ii] = priv->cck_pwr_tbl[10];
> >  		priv->ofdm_pwr_tbl[ii] = priv->ofdm_pwr_tbl[10];
> >  	}
> > --
> > 2.20.1
> > 
> 
> Looks good, however are we certain priv->cck_pwr_tbl and
> priv->ofdm_pwr_tbl are always the same size?
> 
> What about using a macro for cck_pwr_tbl and ofdm_pwr_tbl size in
> device.h? Or a BUILD_BUG() if array's sizes are different? It could be
> helpful for future developers to say these arrays must be the same size.

That's a bit over engineering something which is pretty trivial.
Normally, we would just make the size a define instead of a magic number
14.

	u8 cck_pwr_tbl[14];
	u8 ofdm_pwr_tbl[14];
	u8 ofdm_a_pwr_tbl[42];

If people change the size in the future (unlikely) and it causes a bug
then they kind of deserve it because they need to ensure all the new
stuff is initialized, right?  If they change it and it results in a
buffer overflow then static checkers would complain.  If they changed it
and it resulted in uninitialized data being used then it would be zero
so that's okay.

So, yeah.  Ideally we would figure out a reason for the magic number 14
and create a define, but it's not strictly required.  This patch makes
the code better and doesn't introduce any problems that weren't already
there.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size
  2020-03-24 13:18   ` Dan Carpenter
@ 2020-03-25  9:19     ` Quentin Deslandes
  2020-03-26 17:22       ` Oscar Carter
  0 siblings, 1 reply; 5+ messages in thread
From: Quentin Deslandes @ 2020-03-25  9:19 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, Oscar Carter, Malcolm Priestley, Greg Kroah-Hartman,
	linux-kernel, Forest Bond, Gabriela Bittencourt, Colin Ian King

On 03/24/20 16:18:30, Dan Carpenter wrote:
> That's a bit over engineering something which is pretty trivial.
> Normally, we would just make the size a define instead of a magic number
> 14.

My bad, I meant "define", not "macro".

> If people change the size in the future (unlikely) and it causes a bug
> then they kind of deserve it because they need to ensure all the new
> stuff is initialized, right?  If they change it and it results in a
> buffer overflow then static checkers would complain.  If they changed it
> and it resulted in uninitialized data being used then it would be zero
> so that's okay.

I wasn't sure where I should stand on this, that's clearer now.

Thanks,
Quentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size
  2020-03-25  9:19     ` Quentin Deslandes
@ 2020-03-26 17:22       ` Oscar Carter
  0 siblings, 0 replies; 5+ messages in thread
From: Oscar Carter @ 2020-03-26 17:22 UTC (permalink / raw)
  To: Dan Carpenter, Quentin Deslandes
  Cc: devel, Oscar Carter, Malcolm Priestley, Greg Kroah-Hartman,
	linux-kernel, Forest Bond, Gabriela Bittencourt, Colin Ian King

On Wed, Mar 25, 2020 at 09:19:24AM +0000, Quentin Deslandes wrote:
> On 03/24/20 16:18:30, Dan Carpenter wrote:
> > That's a bit over engineering something which is pretty trivial.
> > Normally, we would just make the size a define instead of a magic number
> > 14.
>
> My bad, I meant "define", not "macro".
>
> > If people change the size in the future (unlikely) and it causes a bug
> > then they kind of deserve it because they need to ensure all the new
> > stuff is initialized, right?  If they change it and it results in a
> > buffer overflow then static checkers would complain.  If they changed it
> > and it resulted in uninitialized data being used then it would be zero
> > so that's okay.
>
> I wasn't sure where I should stand on this, that's clearer now.
>
> Thanks,
> Quentin

Dan and Quentin, thanks for your time to review my work, and make comments.

oscar carter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 17:40 [PATCH v2] staging: vt6656: Use ARRAY_SIZE instead of hardcoded size Oscar Carter
2020-03-24  9:54 ` Quentin Deslandes
2020-03-24 13:18   ` Dan Carpenter
2020-03-25  9:19     ` Quentin Deslandes
2020-03-26 17:22       ` Oscar Carter

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git