Message-ID: <lpGiEC2yjymw5L8SqZsIw_ocyD4Eo8mJuTPAZufS4mStWL64WYn9-9lUD8SAptAwHzqChexGXFq1AWYMeeT2V4RnfhlzwlcPrOkQAqpdjmc=@protonmail.com>
Date: Sat, 21 Sep 2024 14:59:02 +0000
From: Dominik Karol Piątkowski <dominik.karol.piatkowski@...tonmail.com>
To: Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc: gregkh@...uxfoundation.org, tdavies@...kphysics.net, dan.carpenter@...aro.org, linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/6] staging: rtl8192e: rtllib_rx.c: Fix alignment to open parentheses

Hi Philipp,

On Saturday, September 21st, 2024 at 15:51, Philipp Hortmann <philipp.g.hortmann@...il.com> wrote:
<cut>
> Hi Domenik,
> 
> I hope to not stress you beyond limits.

First of all, I want to thank you for fast, valuable reviews. I hope I am not
overusing your patience.

> 
> Thanks for deviding the patch. I can apply it now to my repo.
> First two patches are looking good.
> 
> I prefer to have a comma at the end of the line. This line does not
> increase readablility to me.
> 
> ((struct rx_reorder_entry *)
> list_entry(list->next, struct rx_reorder_entry, list))->seq_num))

I understand your point. I made this line similar to one that already exists
above, see diff:

diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c
index 8fe224a83dd6..e58be8e07917 100644
--- a/drivers/staging/rtl8192e/rtllib_rx.c
+++ b/drivers/staging/rtl8192e/rtllib_rx.c
@@ -409,12 +409,10 @@ static bool add_reorder_entry(struct rx_ts_record *ts,
 
 	while (list->next != &ts->rx_pending_pkt_list) {
 		if (SN_LESS(reorder_entry->seq_num, ((struct rx_reorder_entry *)
-		    list_entry(list->next, struct rx_reorder_entry,
-		    list))->seq_num))
+		    list_entry(list->next, struct rx_reorder_entry, list))->seq_num))
 			list = list->next;
-		else if (SN_EQUAL(reorder_entry->seq_num,
-			((struct rx_reorder_entry *)list_entry(list->next,
-			struct rx_reorder_entry, list))->seq_num))
+		else if (SN_EQUAL(reorder_entry->seq_num, ((struct rx_reorder_entry *)
+			 list_entry(list->next, struct rx_reorder_entry, list))->seq_num))
 			return false;
 		else
 			break;

I can of course remove this change from patch.

> 
> 
> Sometimes it is better to change less.

True.

> 
> The change does not perfectly fit to the description: There you say
> ...aligns the code to open parentheses... but you do not need to remove
> line breaks or shorten code to achieve this.

I agree. There are a few removed line breaks, where I thought it could
improve code readability. If these changes are not acceptable, I will
of course remove them.

> 
> Smaller patches lead to an earlier acceptance. This typically leads to
> more confidence at the beginning for newbies. There is no question about
> that you know what you are doing. But there are some corners where the
> kernel is special.

Thank you, I will watch out for these corners.

> 
> Find more below.
> 
> > ...
> > @@ -876,9 +874,9 @@ static int rtllib_rx_check_duplicate(struct rtllib_device *ieee,
> > frag = WLAN_GET_SEQ_FRAG(sc);
> > 
> > if (!ieee->ht_info->cur_rx_reorder_enable ||
> > - !ieee->current_network.qos_data.active ||
> > - !is_data_frame(skb->data) ||
> > - is_legacy_data_frame(skb->data)) {
> > + !ieee->current_network.qos_data.active ||
> > + !is_data_frame(skb->data) ||
> > + is_legacy_data_frame(skb->data)) {
> > if (!ieee80211_is_beacon(hdr->frame_control)) {
> > if (is_duplicate_packet(ieee, hdr))
> > return -1;
> > @@ -887,7 +885,7 @@ static int rtllib_rx_check_duplicate(struct rtllib_device *ieee,
> > struct rx_ts_record *ts = NULL;
> > 
> > if (rtllib_get_ts(ieee, (struct ts_common_info **)&ts, hdr->addr2,
> > - (u8)frame_qos_tid((u8 *)(skb->data)), RX_DIR, true)) {
> > + (u8)frame_qos_tid((u8 *)(skb->data)), RX_DIR, true)) {
> 
> 
> I am understanding the logic behind this but I cannot really say that
> this increases the readability. It increases the readability of the if
> condition but I am losing readability of the overall code and it
> increases the issue with the too long lines.

I understand your point. If this change negatively impacts readability
of the overall code, I can of course remove it.
About the issue with too long lines - I am certain that none of the lines
I introduced were longer than 100 columns, unless we are trying to maintain
hard limit of 80 columns wherever possible. I know that line length of
80 columns is still preferred, though. Sometimes, these 20 additional
columns could increase readability, but I understand that it is not granted.

> 
> I have not looked into the remaining patches.
> 
> I need some support from another reviewer.
> 
> Your patches are working fine on hardware.

Once again, thank you for thorough review, and I hope I am not overusing
your patience.

Thanks,

Dominik Karol

> 
> Bye Philipp
> 
> > if ((fc & (1 << 11)) && (frag == ts->rx_last_frag_num) &&
> > (WLAN_GET_SEQ_SEQ(sc) == ts->rx_last_seq_num))
> > return -1;

Powered by blists - more mailing lists