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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Don't initialize the rate variable as it is set a few lines later.
Signed-off-by: Oscar Carter <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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 <[email protected]>
regards,
dan carpenter
Reviewed-by: Dan Carpenter <[email protected]>
regards,
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 <[email protected]>
> Reviewed-by: Dan Carpenter <[email protected]>
> ---
> 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 :(
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
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 <[email protected]>
> > Reviewed-by: Dan Carpenter <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > Reviewed-by: Dan Carpenter <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > Reviewed-by: Dan Carpenter <[email protected]>
> > > > ---
> > > > 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