drivers/net/fddi/defxx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Use if and else instead of if(A) and if (!A).
Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
drivers/net/fddi/defxx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
index b584ffe38ad6..3edb2e96f763 100644
--- a/drivers/net/fddi/defxx.c
+++ b/drivers/net/fddi/defxx.c
@@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
bp->mmio = false;
dfx_get_bars(bp, bar_start, bar_len);
}
- }
- if (!dfx_use_mmio)
+ } else {
region = request_region(bar_start[0], bar_len[0],
bdev->driver->name);
+ }
if (!region) {
dfx_register_res_err(print_name, dfx_use_mmio,
bar_start[0], bar_len[0]);
--
2.35.1On Sun, 24 Apr 2022, Wan Jiabing wrote:
> diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
> index b584ffe38ad6..3edb2e96f763 100644
> --- a/drivers/net/fddi/defxx.c
> +++ b/drivers/net/fddi/defxx.c
> @@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
> bp->mmio = false;
> dfx_get_bars(bp, bar_start, bar_len);
> }
> - }
> - if (!dfx_use_mmio)
> + } else {
> region = request_region(bar_start[0], bar_len[0],
> bdev->driver->name);
> + }
NAK. The first conditional optionally sets `bp->mmio = false', which
changes the value of `dfx_use_mmio' in some configurations:
#if defined(CONFIG_EISA) || defined(CONFIG_PCI)
#define dfx_use_mmio bp->mmio
#else
#define dfx_use_mmio true
#endif
Maciej
On Sun, Apr 24, 2022 at 11:39:50AM +0100, Maciej W. Rozycki wrote:
> On Sun, 24 Apr 2022, Wan Jiabing wrote:
>
> > diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
> > index b584ffe38ad6..3edb2e96f763 100644
> > --- a/drivers/net/fddi/defxx.c
> > +++ b/drivers/net/fddi/defxx.c
> > @@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
> > bp->mmio = false;
> > dfx_get_bars(bp, bar_start, bar_len);
> > }
> > - }
> > - if (!dfx_use_mmio)
> > + } else {
> > region = request_region(bar_start[0], bar_len[0],
> > bdev->driver->name);
> > + }
>
> NAK. The first conditional optionally sets `bp->mmio = false', which
> changes the value of `dfx_use_mmio' in some configurations:
>
> #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
> #define dfx_use_mmio bp->mmio
> #else
> #define dfx_use_mmio true
> #endif
Which is just asking for trouble like this.
Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint
something horrible is going on.
It probably won't stop the robots finding this if (x) if (!x), but
there is a chance the robot drivers will wonder why it is upper case.
Andrew
On Mon, 25 Apr 2022, Andrew Lunn wrote: > > NAK. The first conditional optionally sets `bp->mmio = false', which > > changes the value of `dfx_use_mmio' in some configurations: > > > > #if defined(CONFIG_EISA) || defined(CONFIG_PCI) > > #define dfx_use_mmio bp->mmio > > #else > > #define dfx_use_mmio true > > #endif > > Which is just asking for trouble like this. > > Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint > something horrible is going on. There's legacy behind it, `dfx_use_mmio' used to be a proper variable and references were retained not to obfuscate the changes that ultimately led to the current arrangement. I guess at this stage it could be changed to a function-like macro or a static inline function taking `bp' as the argument. > It probably won't stop the robots finding this if (x) if (!x), but > there is a chance the robot drivers will wonder why it is upper case. Well, blindly relying on automation is bound to cause trouble. There has to be a piece of intelligence signing the results off at the end. And there's nothing wrong with if (x) if (!x) in the first place; any sane compiler will produce reasonable output from it. Don't fix what ain't broke! And watch out for volatiles! Maciej
On Mon, Apr 25, 2022 at 12:26:10AM +0100, Maciej W. Rozycki wrote:
> On Mon, 25 Apr 2022, Andrew Lunn wrote:
>
> > > NAK. The first conditional optionally sets `bp->mmio = false', which
> > > changes the value of `dfx_use_mmio' in some configurations:
> > >
> > > #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
> > > #define dfx_use_mmio bp->mmio
> > > #else
> > > #define dfx_use_mmio true
> > > #endif
> >
> > Which is just asking for trouble like this.
> >
> > Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint
> > something horrible is going on.
>
> There's legacy behind it, `dfx_use_mmio' used to be a proper variable and
> references were retained not to obfuscate the changes that ultimately led
> to the current arrangement. I guess at this stage it could be changed to
> a function-like macro or a static inline function taking `bp' as the
> argument.
Yes, something like that would be good.
> > It probably won't stop the robots finding this if (x) if (!x), but
> > there is a chance the robot drivers will wonder why it is upper case.
>
> Well, blindly relying on automation is bound to cause trouble.
Unfortunately, there are a number of bot drivers who do blindly rely
on automation. We have had to undo the same broken bot driven changes
a few times, and ended up adding extra comments to catch the eye of
the bot drivers.
Andrew
On 2022/4/25 7:26, Maciej W. Rozycki wrote: > On Mon, 25 Apr 2022, Andrew Lunn wrote: > >>> NAK. The first conditional optionally sets `bp->mmio = false', which >>> changes the value of `dfx_use_mmio' in some configurations: >>> >>> #if defined(CONFIG_EISA) || defined(CONFIG_PCI) >>> #define dfx_use_mmio bp->mmio >>> #else >>> #define dfx_use_mmio true >>> #endif Yes, it's my fault. I didn't notice "dfx_use_mmio" is a MACRO, sorry for this wrong patch. >> It probably won't stop the robots finding this if (x) if (!x), but >> there is a chance the robot drivers will wonder why it is upper case. > Well, blindly relying on automation is bound to cause trouble. There has > to be a piece of intelligence signing the results off at the end. You are right and I'll be more careful to review the result before submitting. > > And there's nothing wrong with if (x) if (!x) in the first place; any > sane compiler will produce reasonable output from it. Don't fix what > ain't broke! And watch out for volatiles! Yes, there's nothing wrong with if (x) if (!x), but I want to do is reducing the complexity of the code. There would be less instructions when using "if and else" rather than "if (A) and if (!A)" as I tested: Use if(A) and if(!A): ldr w0, [sp, 28] cmp w0, 0 beq .L2 ldr w0, [sp, 28] add w0, w0, 1 str w0, [sp, 28] .L2: ldr w0, [sp, 28] <------ one more ldr instruction cmp w0, 0 <------ one more cmp instruction bne .L3 ldr w0, [sp, 28] add w0, w0, 2 str w0, [sp, 28] .L3: ldr w0, [sp, 28] mov w1, w0 adrp x0, .LC1 add x0, x0, :lo12:.LC1 bl printf Use if(A) and else: ldr w0, [sp, 28] cmp w0, 0 beq .L2 ldr w0, [sp, 28] add w0, w0, 1 str w0, [sp, 28] <------ reduce two instructions b .L3 .L2: ldr w0, [sp, 28] add w0, w0, 2 str w0, [sp, 28] .L3: ldr w0, [sp, 28] mov w1, w0 adrp x0, .LC1 add x0, x0, :lo12:.LC1 bl printf I also use "pmccabe" , a tool from gcc, to calculate the complexity of the code. It shows this patch can reduce the statements in function. Use if(A) and if(!A): pmccabe -v test.c Modified McCabe Cyclomatic Complexity | Traditional McCabe Cyclomatic Complexity | | # Statements in function | | | First line of function | | | | # lines in function | | | | | filename(definition line number):function | | | | | | 3 3 8 4 17 test.c(4): main Use if(A) and else: pmccabe -v test.c Modified McCabe Cyclomatic Complexity | Traditional McCabe Cyclomatic Complexity | | # Statements in function | | | First line of function | | | | # lines in function | | | | | filename(definition line number):function | | | | | | 2 2 7 4 16 test.c(4): main So I think this type of patchs is meaningful. Thanks, Wan Jiabing
© 2016 - 2026 Red Hat, Inc.