Hi Philipp,
On Saturday, September 21st, 2024 at 15:51, Philipp Hortmann <philipp.g.hortmann@xxxxxxxxx> 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;
