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 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
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