| Message ID | 20220424092842.101307-1-wanjiabing@vivo.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | FDDI: defxx: simplify if-if to if-else | expand |
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 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 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
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]);
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(-)