Fix indentation issue to adhere to Linux kernel coding style,
Issue found by checkpatch. Change the long for loop into 3 lines. And
optimize by avoiding the multiplication.
Signed-off-by: Binyi Han <[email protected]>
---
v2:
- Change the long for loop into 3 lines.
v3:
- Align page_entries in the for loop to open parenthesis.
- Optimize by avoiding the multiplication.
drivers/staging/qlge/qlge_main.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 1a378330d775..4b166c66cfc5 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
tmp = (u64)rx_ring->lbq.base_dma;
base_indirect_ptr = rx_ring->lbq.base_indirect;
- for (page_entries = 0; page_entries <
- MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
- base_indirect_ptr[page_entries] =
- cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
+ for (page_entries = 0;
+ page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
+ page_entries++) {
+ base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
+ tmp += DB_PAGE_SIZE;
+ }
cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
cqicb->lbq_buf_size =
cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
@@ -3022,10 +3024,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
tmp = (u64)rx_ring->sbq.base_dma;
base_indirect_ptr = rx_ring->sbq.base_indirect;
- for (page_entries = 0; page_entries <
- MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
- base_indirect_ptr[page_entries] =
- cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
+ for (page_entries = 0;
+ page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
+ page_entries++) {
+ base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
+ tmp += DB_PAGE_SIZE;
+ }
cqicb->sbq_addr =
cpu_to_le64(rx_ring->sbq.base_indirect_dma);
cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
--
2.25.1
On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> Fix indentation issue to adhere to Linux kernel coding style,
> Issue found by checkpatch. Change the long for loop into 3 lines. And
> optimize by avoiding the multiplication.
>
> Signed-off-by: Binyi Han <[email protected]>
> ---
> v2:
> - Change the long for loop into 3 lines.
> v3:
> - Align page_entries in the for loop to open parenthesis.
> - Optimize by avoiding the multiplication.
Please do not mix coding style fixes with "optimizations" or logical
changes. This should be multiple patches.
Also, did you test this change on real hardware? At first glance, it's
not obvious that the code is still doing the same thing, so "proof" of
that would be nice to have.
thanks,
greg k-h
On Mon, 2022-07-11 at 10:05 +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> > Fix indentation issue to adhere to Linux kernel coding style,
> > Issue found by checkpatch. Change the long for loop into 3 lines. And
> > optimize by avoiding the multiplication.
> >
> > Signed-off-by: Binyi Han <[email protected]>
> > ---
> > v2:
> > - Change the long for loop into 3 lines.
> > v3:
> > - Align page_entries in the for loop to open parenthesis.
> > - Optimize by avoiding the multiplication.
>
> Please do not mix coding style fixes with "optimizations" or logical
> changes. This should be multiple patches.
>
> Also, did you test this change on real hardware? At first glance, it's
> not obvious that the code is still doing the same thing, so "proof" of
> that would be nice to have.
I read the code and suggested the optimization. It's the same logic.
On Mon, Jul 11, 2022 at 01:55:24PM -0700, Joe Perches wrote:
> On Mon, 2022-07-11 at 10:05 +0200, Greg Kroah-Hartman wrote:
> > On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> > > Fix indentation issue to adhere to Linux kernel coding style,
> > > Issue found by checkpatch. Change the long for loop into 3 lines. And
> > > optimize by avoiding the multiplication.
> > >
> > > Signed-off-by: Binyi Han <[email protected]>
> > > ---
> > > v2:
> > > - Change the long for loop into 3 lines.
> > > v3:
> > > - Align page_entries in the for loop to open parenthesis.
> > > - Optimize by avoiding the multiplication.
> >
> > Please do not mix coding style fixes with "optimizations" or logical
> > changes. This should be multiple patches.
> >
> > Also, did you test this change on real hardware? At first glance, it's
> > not obvious that the code is still doing the same thing, so "proof" of
> > that would be nice to have.
>
> I read the code and suggested the optimization. It's the same logic.
>
>
I appreciate the review, but it looks quite different from the original
so it should be 2 different patches, one for coding style changes, and
the second for the "optimization".
thanks,
greg k-h
On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> Fix indentation issue to adhere to Linux kernel coding style,
> Issue found by checkpatch. Change the long for loop into 3 lines. And
> optimize by avoiding the multiplication.
There is no possible way this optimization helps benchmarks. Better to
focus on readability.
>
> Signed-off-by: Binyi Han <[email protected]>
> ---
> v2:
> - Change the long for loop into 3 lines.
> v3:
> - Align page_entries in the for loop to open parenthesis.
> - Optimize by avoiding the multiplication.
>
> drivers/staging/qlge/qlge_main.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 1a378330d775..4b166c66cfc5 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
> tmp = (u64)rx_ring->lbq.base_dma;
> base_indirect_ptr = rx_ring->lbq.base_indirect;
>
> - for (page_entries = 0; page_entries <
> - MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> - base_indirect_ptr[page_entries] =
> - cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> + for (page_entries = 0;
> + page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> + page_entries++) {
> + base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> + tmp += DB_PAGE_SIZE;
I've previously said that using "int i;" is clearer here. You would
kind of expect "page_entries" to be the number of entries, so it's kind
of misleading. In other words, it's not just harmless wordiness and
needless exposition, it's actively bad.
I would probably just put it on one line:
for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
base_indirect_ptr[i] = cpu_to_le64(tmp + (i * DB_PAGE_SIZE));
But if you want to break it up you could do:
for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
base_indirect_ptr[i] = cpu_to_le64(tmp +
(i * DB_PAGE_SIZE));
"tmp" is kind of a bad name. Also "base_indirect_ptr" would be better
as "base_indirect".
regards,
dan carpenter
On Tue, 2022-07-12 at 16:46 +0300, Dan Carpenter wrote:
> On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> > Fix indentation issue to adhere to Linux kernel coding style,
> > Issue found by checkpatch. Change the long for loop into 3 lines. And
> > optimize by avoiding the multiplication.
>
> There is no possible way this optimization helps benchmarks. Better to
> focus on readability.
I think removing the multiply _improves_ readability.
> > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
[]
> > @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
> > tmp = (u64)rx_ring->lbq.base_dma;
> > base_indirect_ptr = rx_ring->lbq.base_indirect;
> >
> > - for (page_entries = 0; page_entries <
> > - MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> > - base_indirect_ptr[page_entries] =
> > - cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> > + for (page_entries = 0;
> > + page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> > + page_entries++) {
> > + base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> > + tmp += DB_PAGE_SIZE;
>
> I've previously said that using "int i;" is clearer here. You would
> kind of expect "page_entries" to be the number of entries, so it's kind
> of misleading. In other words, it's not just harmless wordiness and
> needless exposition, it's actively bad.
Likely true.
> I would probably just put it on one line:
>
> for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
> base_indirect_ptr[i] = cpu_to_le64(tmp + (i * DB_PAGE_SIZE));
>
> But if you want to break it up you could do:
>
> for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
> base_indirect_ptr[i] = cpu_to_le64(tmp +
> (i * DB_PAGE_SIZE));
>
> "tmp" is kind of a bad name. Also "base_indirect_ptr" would be better
> as "base_indirect".
tmp is a poor name here. Maybe dma would be better.
MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) is also a poorly named macro
where all the existing uses are QLGE_BQ_LEN.
And there's base_indirect_ptr and base_indirect_dma in the struct
so just base_indirect might not be the best.
tmp = (u64)rx_ring->lbq.base_dma;
base_indirect_ptr = rx_ring->lbq.base_indirect;
And clarity is good.
Though here, clarity to value for effort though is dubious.
btw: this code got moved to staging 3 years ago.
Maybe it's getting closer to removal time.
On Tue, Jul 12, 2022 at 11:44:39AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 11, 2022 at 01:55:24PM -0700, Joe Perches wrote:
> > On Mon, 2022-07-11 at 10:05 +0200, Greg Kroah-Hartman wrote:
> > > On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> > > > Fix indentation issue to adhere to Linux kernel coding style,
> > > > Issue found by checkpatch. Change the long for loop into 3 lines. And
> > > > optimize by avoiding the multiplication.
> > > >
> > > > Signed-off-by: Binyi Han <[email protected]>
> > > > ---
> > > > v2:
> > > > - Change the long for loop into 3 lines.
> > > > v3:
> > > > - Align page_entries in the for loop to open parenthesis.
> > > > - Optimize by avoiding the multiplication.
> > >
> > > Please do not mix coding style fixes with "optimizations" or logical
> > > changes. This should be multiple patches.
> > >
> > > Also, did you test this change on real hardware? At first glance, it's
> > > not obvious that the code is still doing the same thing, so "proof" of
> > > that would be nice to have.
I don't have access to a real hardware, so can't provide a "proof" of that.
I agree with Joe that's the same logic, and it compiles ok. But I
understand if you don't apply this patch, it's more safe with some
testing.
> >
> > I read the code and suggested the optimization. It's the same logic.
> >
> >
>
> I appreciate the review, but it looks quite different from the original
> so it should be 2 different patches, one for coding style changes, and
> the second for the "optimization".
>
> thanks,
>
> greg k-h
Thank you for the review. I sent a patchset, following the instruction
here:
https://kernelnewbies.org/FirstKernelPatch
And I didn't find a good way to send a patchset by mutt in a single
command, so I run "mutt -H PatchFile" several times to send it, and I hope
that is okay.
Best,
Binyi
On Tue, Jul 12, 2022 at 07:14:55AM -0700, Joe Perches wrote:
> On Tue, 2022-07-12 at 16:46 +0300, Dan Carpenter wrote:
> > On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> > > Fix indentation issue to adhere to Linux kernel coding style,
> > > Issue found by checkpatch. Change the long for loop into 3 lines. And
> > > optimize by avoiding the multiplication.
> >
> > There is no possible way this optimization helps benchmarks. Better to
> > focus on readability.
>
> I think removing the multiply _improves_ readability.
>
> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> []
> > > @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
> > > tmp = (u64)rx_ring->lbq.base_dma;
> > > base_indirect_ptr = rx_ring->lbq.base_indirect;
> > >
> > > - for (page_entries = 0; page_entries <
> > > - MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> > > - base_indirect_ptr[page_entries] =
> > > - cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> > > + for (page_entries = 0;
> > > + page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> > > + page_entries++) {
> > > + base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> > > + tmp += DB_PAGE_SIZE;
> >
> > I've previously said that using "int i;" is clearer here. You would
> > kind of expect "page_entries" to be the number of entries, so it's kind
> > of misleading. In other words, it's not just harmless wordiness and
> > needless exposition, it's actively bad.
>
> Likely true.
>
I agree it could be misleading. But if "page_entries" is in the for loop I
would assume it's some kind of index variable, and still it provides some
information (page entry) for the index, probably page_entry_idx could be
better name but still makes the for loop a very long one. I guess I would
leave it be.
> > I would probably just put it on one line:
> >
> > for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
> > base_indirect_ptr[i] = cpu_to_le64(tmp + (i * DB_PAGE_SIZE));
> >
> > But if you want to break it up you could do:
> >
> > for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
> > base_indirect_ptr[i] = cpu_to_le64(tmp +
> > (i * DB_PAGE_SIZE));
> >
> > "tmp" is kind of a bad name. Also "base_indirect_ptr" would be better
> > as "base_indirect".
>
> tmp is a poor name here. Maybe dma would be better.
>
Yeah, I think so.
> MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) is also a poorly named macro
> where all the existing uses are QLGE_BQ_LEN.
>
> And there's base_indirect_ptr and base_indirect_dma in the struct
> so just base_indirect might not be the best.
>
> tmp = (u64)rx_ring->lbq.base_dma;
> base_indirect_ptr = rx_ring->lbq.base_indirect;
>
> And clarity is good.
> Though here, clarity to value for effort though is dubious.
>
> btw: this code got moved to staging 3 years ago.
>
> Maybe it's getting closer to removal time.
>
That sounds sad.
Thank you for reviewing!
Best,
Binyi
On Thu, Jul 14, 2022 at 11:04:57PM -0700, binyi wrote:
> On Tue, Jul 12, 2022 at 07:14:55AM -0700, Joe Perches wrote:
> > On Tue, 2022-07-12 at 16:46 +0300, Dan Carpenter wrote:
> > > > @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
> > > > tmp = (u64)rx_ring->lbq.base_dma;
> > > > base_indirect_ptr = rx_ring->lbq.base_indirect;
> > > >
> > > > - for (page_entries = 0; page_entries <
> > > > - MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> > > > - base_indirect_ptr[page_entries] =
> > > > - cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> > > > + for (page_entries = 0;
> > > > + page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> > > > + page_entries++) {
> > > > + base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> > > > + tmp += DB_PAGE_SIZE;
> > >
> > > I've previously said that using "int i;" is clearer here. You would
> > > kind of expect "page_entries" to be the number of entries, so it's kind
> > > of misleading. In other words, it's not just harmless wordiness and
> > > needless exposition, it's actively bad.
> >
> > Likely true.
> >
>
> I agree it could be misleading. But if "page_entries" is in the for loop I
> would assume it's some kind of index variable, and still it provides some
> information (page entry) for the index, probably page_entry_idx could be
> better name but still makes the for loop a very long one. I guess I would
> leave it be.
It does not "provide some information". That's what I was trying to
explain. It's the opposite of information. Information is good. No
information is neutral. But anti-information is bad.
Like there are so many times in life where you listen to someone and you
think you are learning something but you end up stupider than before.
They have done studies on TV news where it can make you less informed
than people who don't watch it. And other studies say if you stop
watching TV news for even a month then your brain starts to heal.
I don't really care about one line of code, but what I'm trying to say
is learn to recognize anti-information and delete it. Comments for
example are often useless.
regards,
dan carpenter
On Fri, Jul 15, 2022 at 12:28:47PM +0300, Dan Carpenter wrote:
> On Thu, Jul 14, 2022 at 11:04:57PM -0700, binyi wrote:
> > On Tue, Jul 12, 2022 at 07:14:55AM -0700, Joe Perches wrote:
> > > On Tue, 2022-07-12 at 16:46 +0300, Dan Carpenter wrote:
> > > > > @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
> > > > > tmp = (u64)rx_ring->lbq.base_dma;
> > > > > base_indirect_ptr = rx_ring->lbq.base_indirect;
> > > > >
> > > > > - for (page_entries = 0; page_entries <
> > > > > - MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> > > > > - base_indirect_ptr[page_entries] =
> > > > > - cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> > > > > + for (page_entries = 0;
> > > > > + page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> > > > > + page_entries++) {
> > > > > + base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> > > > > + tmp += DB_PAGE_SIZE;
> > > >
> > > > I've previously said that using "int i;" is clearer here. You would
> > > > kind of expect "page_entries" to be the number of entries, so it's kind
> > > > of misleading. In other words, it's not just harmless wordiness and
> > > > needless exposition, it's actively bad.
> > >
> > > Likely true.
> > >
> >
> > I agree it could be misleading. But if "page_entries" is in the for loop I
> > would assume it's some kind of index variable, and still it provides some
> > information (page entry) for the index, probably page_entry_idx could be
> > better name but still makes the for loop a very long one. I guess I would
> > leave it be.
>
> It does not "provide some information". That's what I was trying to
> explain. It's the opposite of information. Information is good. No
> information is neutral. But anti-information is bad.
I see. Indeed, being neutral is better than being bad.
> Like there are so many times in life where you listen to someone and you
> think you are learning something but you end up stupider than before.
> They have done studies on TV news where it can make you less informed
> than people who don't watch it. And other studies say if you stop
> watching TV news for even a month then your brain starts to heal.
>
> I don't really care about one line of code, but what I'm trying to say
> is learn to recognize anti-information and delete it. Comments for
> example are often useless.
It makes more sense to me now. Thanks for explaining!
Best,
Binyi