qlge: refactor qlge_change_rx_buffers() in qlge_main.c
PATCH 1/1 qlge: refactor qlge_change_rx_buffers() in qlge_main.c
Greetings. As I understand, qlge_change_rx_buffers() waits 4 times for an outstanding reset to complete. And if it cannot, reports about timeout. I assume this can be written using for loop instead of while, wich looks nicer for me. Hope my guess was right.
--- drivers/staging/qlge/qlge_main.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/staging/qlge/qlge_main.c +6 -9
@@ -4012,19 +4012,16 @@ static int qlge_change_rx_buffers(struct qlge_adapter *qdev)/* Wait for an outstanding reset to complete. */ if (!test_bit (QL_ADAPTER_UP , &qdev->flags)) { -int i =4 ; - -while (--i && !test_bit (QL_ADAPTER_UP , &qdev->flags)) { +for (int i =4 ; !test_bit (QL_ADAPTER_UP , &qdev->flags); i--) { +if (!i) { +netif_err (qdev, ifup, qdev->ndev, +"Timed out waiting for adapter UP\n" ); +return -ETIMEDOUT ; + }netif_err (qdev, ifup, qdev->ndev,"Waiting for adapter UP...\n" );ssleep (1 ); } - -if (!i) { -netif_err (qdev, ifup, qdev->ndev, -"Timed out waiting for adapter UP\n" ); -return -ETIMEDOUT ; - } } status =qlge_adapter_down (qdev);
Signature · 1 line
2.34.0
Hi, Adam!
Looks like you have missed all maintainers in your CC list :) Please, use ./script/get_mainainer.pl to get list of people who are responsible for reviewing your code
Quoted 5 lines · Adam Kandur
Greetings. As I understand,
qlge_change_rx_buffers()waits 4 times for an outstanding reset to complete. And if it cannot, reports about timeout. I assume this can be written using for loop instead of while, wich looks nicer for me. Hope my guess was right.
Commit message should only explain why you do this change. You can leave your unrelated thoughts/questions below ---, since this part won't be visible in gitlog
Quoted 2 lines
Signed-off-by Adam Kandur <[email protected]>---
<--- here
drivers/staging/qlge/qlge_main.c static int qlge_change_rx_buffers(struct qlge_adapter *qdev) · line 4014
Quoted 11 lines
drivers/staging/qlge/qlge_main.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)drivers/staging/qlge/qlge_main.c +0
-0@@ -4012,19 +4012,16 @@ static int qlge_change_rx_buffers(struct qlge_adapter *qdev)/* Wait for an outstanding reset to complete. */ if (!test_bit (QL_ADAPTER_UP , &qdev->flags)) {
Aren't there 5 checks actually? It seems unlikely that this bit will be set after 3-4 instructions
drivers/staging/qlge/qlge_main.c static int qlge_change_rx_buffers(struct qlge_adapter *qdev) · line 4015
Quoted 4 lines
drivers/staging/qlge/qlge_main.c +1
-3-int i =4 ; - -while (--i && !test_bit (QL_ADAPTER_UP , &qdev->flags)) { +for (int i =4 ; !test_bit (QL_ADAPTER_UP , &qdev->flags); i--) {
Have you compiled this patch? Compiler should have warn you about mixed declaration and code.
With regards,
Pavel Skripkin
Quoted 2 lines · Adam Kandur
I assume this can be written using for loop instead of while, wich looks nicer for me.
The original code was better than your code.
Also we have a published style guidelines. We don't really encourage people to start enforcing their own weird style preferences which are not in the style guidelines. Once you are a maintainer then no one can stop you from having opinions but until then it's best to stick to the style guideline.
regards,
dan carpenter