[PATCH 0/3] staging: vt6656: Cleanup of the vnt_get_frame_time function

[PATCH 0/3] staging: vt6656: Cleanup of the vnt_get_frame_time function

[Oscar Carter]

This patch series makes a cleanup of the vnt_get_frame_time function.

The first patch removes the define RATE_54M and changes it for the
ARRAY_SIZE macro. This way avoid possibles issues if the size of the
vnt_frame_time array change in the future but not change accordingly the
RATE_54M constant.

The second patch makes use of the define RATE_11M instead of a magic
number.

The third patch remove unnecessary local variable initialization.

Oscar Carter (3):
  staging: vt6656: Use ARRAY_SIZE instead of define RATE_54M
  staging: vt6656: Use define instead of magic number for tx_rate
  staging: vt6656: Remove unnecessary local variable initialization

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

--
2.20.1

[PATCH 1/3] staging: vt6656: Use ARRAY_SIZE instead of define RATE_54M

[Oscar Carter]

Use ARRAY_SIZE to replace the define RATE_54M so we will never have a
mismatch. In this way, avoid the possibility of a buffer overflow if
this define is changed in the future to a greater value.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/baseband.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
index a19a563d8bcc..3e4bd637849a 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -136,7 +136,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
 	unsigned int preamble;
 	unsigned int rate = 0;

-	if (tx_rate > RATE_54M)
+	if (tx_rate >= ARRAY_SIZE(vnt_frame_time))
 		return 0;

 	rate = (unsigned int)vnt_frame_time[tx_rate];
--
2.20.1

Re: [PATCH 1/3] staging: vt6656: Use ARRAY_SIZE instead of define RATE_54M

[Dan Carpenter]

On Sat, Apr 04, 2020 at 04:13:58PM +0200, Oscar Carter wrote:
> Use ARRAY_SIZE to replace the define RATE_54M so we will never have a
> mismatch. In this way, avoid the possibility of a buffer overflow if
> this define is changed in the future to a greater value.
> 

Future proofing is not really a valid reason to change this.  We have to
assume that future programmers are not idiots.

The only valid reason to do this is readability, but I'm not convinced
the new version is more readable.

regards,
dan carpenter

Re: [PATCH 1/3] staging: vt6656: Use ARRAY_SIZE instead of define RATE_54M

[Oscar Carter]

On Mon, Apr 06, 2020 at 02:13:23PM +0300, Dan Carpenter wrote:
> On Sat, Apr 04, 2020 at 04:13:58PM +0200, Oscar Carter wrote:
> > Use ARRAY_SIZE to replace the define RATE_54M so we will never have a
> > mismatch. In this way, avoid the possibility of a buffer overflow if
> > this define is changed in the future to a greater value.
> >
>
> Future proofing is not really a valid reason to change this.

Ok, then I leave it as is.

> We have to assume that future programmers are not idiots.
>
That was not my intention. I'm sorry.

> The only valid reason to do this is readability, but I'm not convinced
> the new version is more readable.
>
Ok.

> regards,
> dan carpenter
>
Thanks,
oscar carter

[PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Oscar Carter]

Use the define RATE_11M present in the file "device.h" instead of the
magic number 3. So the code is more clear.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/baseband.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
index 3e4bd637849a..a785f91c1566 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -24,6 +24,7 @@

 #include <linux/bits.h>
 #include <linux/kernel.h>
+#include "device.h"
 #include "mac.h"
 #include "baseband.h"
 #include "rf.h"
@@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,

 	rate = (unsigned int)vnt_frame_time[tx_rate];

-	if (tx_rate <= 3) {
+	if (tx_rate <= RATE_11M) {
 		if (preamble_type == 1)
 			preamble = 96;
 		else
--
2.20.1

Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Dan Carpenter]

On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> Use the define RATE_11M present in the file "device.h" instead of the
> magic number 3. So the code is more clear.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  drivers/staging/vt6656/baseband.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> index 3e4bd637849a..a785f91c1566 100644
> --- a/drivers/staging/vt6656/baseband.c
> +++ b/drivers/staging/vt6656/baseband.c
> @@ -24,6 +24,7 @@
> 
>  #include <linux/bits.h>
>  #include <linux/kernel.h>
> +#include "device.h"
>  #include "mac.h"
>  #include "baseband.h"
>  #include "rf.h"
> @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> 
>  	rate = (unsigned int)vnt_frame_time[tx_rate];
> 
> -	if (tx_rate <= 3) {
> +	if (tx_rate <= RATE_11M) {

This is nice.  And if we don't apply patch 1 then it's even nicer
because then "tx_rate" is treated consistently.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Greg Kroah-Hartman]

On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> Use the define RATE_11M present in the file "device.h" instead of the
> magic number 3. So the code is more clear.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/staging/vt6656/baseband.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> index 3e4bd637849a..a785f91c1566 100644
> --- a/drivers/staging/vt6656/baseband.c
> +++ b/drivers/staging/vt6656/baseband.c
> @@ -24,6 +24,7 @@
> 
>  #include <linux/bits.h>
>  #include <linux/kernel.h>
> +#include "device.h"
>  #include "mac.h"
>  #include "baseband.h"
>  #include "rf.h"
> @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> 
>  	rate = (unsigned int)vnt_frame_time[tx_rate];
> 
> -	if (tx_rate <= 3) {
> +	if (tx_rate <= RATE_11M) {
>  		if (preamble_type == 1)
>  			preamble = 96;
>  		else
> --
> 2.20.1

This doesn't apply to my tree :(

Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Oscar Carter]

On Mon, Apr 06, 2020 at 04:22:12PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> > Use the define RATE_11M present in the file "device.h" instead of the
> > magic number 3. So the code is more clear.
> >
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/staging/vt6656/baseband.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> > index 3e4bd637849a..a785f91c1566 100644
> > --- a/drivers/staging/vt6656/baseband.c
> > +++ b/drivers/staging/vt6656/baseband.c
> > @@ -24,6 +24,7 @@
> >
> >  #include <linux/bits.h>
> >  #include <linux/kernel.h>
> > +#include "device.h"
> >  #include "mac.h"
> >  #include "baseband.h"
> >  #include "rf.h"
> > @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> >
> >  	rate = (unsigned int)vnt_frame_time[tx_rate];
> >
> > -	if (tx_rate <= 3) {
> > +	if (tx_rate <= RATE_11M) {
> >  		if (preamble_type == 1)
> >  			preamble = 96;
> >  		else
> > --
> > 2.20.1
>
> This doesn't apply to my tree :(
>
Sorry, but I don't understand what it means. This meant that I need to rebase
this patch against your staging-next branch of your staging tree ? Or it means
something else ?

Thanks,
oscar carter

Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Greg Kroah-Hartman]

On Mon, Apr 06, 2020 at 06:38:36PM +0200, Oscar Carter wrote:
> On Mon, Apr 06, 2020 at 04:22:12PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> > > Use the define RATE_11M present in the file "device.h" instead of the
> > > magic number 3. So the code is more clear.
> > >
> > > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/staging/vt6656/baseband.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> > > index 3e4bd637849a..a785f91c1566 100644
> > > --- a/drivers/staging/vt6656/baseband.c
> > > +++ b/drivers/staging/vt6656/baseband.c
> > > @@ -24,6 +24,7 @@
> > >
> > >  #include <linux/bits.h>
> > >  #include <linux/kernel.h>
> > > +#include "device.h"
> > >  #include "mac.h"
> > >  #include "baseband.h"
> > >  #include "rf.h"
> > > @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> > >
> > >  	rate = (unsigned int)vnt_frame_time[tx_rate];
> > >
> > > -	if (tx_rate <= 3) {
> > > +	if (tx_rate <= RATE_11M) {
> > >  		if (preamble_type == 1)
> > >  			preamble = 96;
> > >  		else
> > > --
> > > 2.20.1
> >
> > This doesn't apply to my tree :(
> >
> Sorry, but I don't understand what it means. This meant that I need to rebase
> this patch against your staging-next branch of your staging tree ?

Yes, and 3/3 as well, because I dropped the 1/3 patch here.

thanks,

greg k-h

Re: [PATCH 2/3] staging: vt6656: Use define instead of magic number for tx_rate

[Oscar Carter]

On Mon, Apr 06, 2020 at 07:58:08PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 06, 2020 at 06:38:36PM +0200, Oscar Carter wrote:
> > On Mon, Apr 06, 2020 at 04:22:12PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 04, 2020 at 04:13:59PM +0200, Oscar Carter wrote:
> > > > Use the define RATE_11M present in the file "device.h" instead of the
> > > > magic number 3. So the code is more clear.
> > > >
> > > > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > > > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > >  drivers/staging/vt6656/baseband.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> > > > index 3e4bd637849a..a785f91c1566 100644
> > > > --- a/drivers/staging/vt6656/baseband.c
> > > > +++ b/drivers/staging/vt6656/baseband.c
> > > > @@ -24,6 +24,7 @@
> > > >
> > > >  #include <linux/bits.h>
> > > >  #include <linux/kernel.h>
> > > > +#include "device.h"
> > > >  #include "mac.h"
> > > >  #include "baseband.h"
> > > >  #include "rf.h"
> > > > @@ -141,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> > > >
> > > >  	rate = (unsigned int)vnt_frame_time[tx_rate];
> > > >
> > > > -	if (tx_rate <= 3) {
> > > > +	if (tx_rate <= RATE_11M) {
> > > >  		if (preamble_type == 1)
> > > >  			preamble = 96;
> > > >  		else
> > > > --
> > > > 2.20.1
> > >
> > > This doesn't apply to my tree :(
> > >
> > Sorry, but I don't understand what it means. This meant that I need to rebase
> > this patch against your staging-next branch of your staging tree ?
>
> Yes, and 3/3 as well, because I dropped the 1/3 patch here.
>
Ok, I will create a new patch series version rebased against your staging-next
branch and I will send it.

> thanks,
>
> greg k-h

thanks,
oscar carter

[PATCH 3/3] staging: vt6656: Remove unnecessary local variable initialization

[Oscar Carter]

Don't initialize the rate variable as it is set a few lines later.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/baseband.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
index a785f91c1566..04393ea6287d 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -135,7 +135,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
 {
 	unsigned int frame_time;
 	unsigned int preamble;
-	unsigned int rate = 0;
+	unsigned int rate;

 	if (tx_rate >= ARRAY_SIZE(vnt_frame_time))
 		return 0;
--
2.20.1

Re: [PATCH 3/3] staging: vt6656: Remove unnecessary local variable initialization

[Dan Carpenter]

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter