Re: [PATCH v2 3/6] staging: rtl8192e: rtllib_rx.c: Fix alignment to open parentheses

[Philipp Hortmann]

On 9/20/24 21:36, Dominik Karol Piątkowski wrote:
> This patch aligns the code to open parentheses to improve readability.
>
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@xxxxxxxxxxxxxx>
> ---
>   drivers/staging/rtl8192e/rtllib_rx.c | 91 +++++++++++++---------------
>   1 file changed, 42 insertions(+), 49 deletions(-)
>
> 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;


Hi Domenik,

I hope to not stress you beyond limits.

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))

Sometimes it is better to change less.

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.

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.

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 have not looked into the remaining patches.

I need some support from another reviewer.

Your patches are working fine on hardware.

Bye Philipp

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