Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org B1DFC60271
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Larry Finger <Larry.Finger@lwfinger.net>
Cc:     linux-wireless@vger.kernel.org, pkshih@realtek.com
Subject: Re: [RFC] rtlwifi: rtl8821ae: Use inline routines rather than macros for descriptor word 0
References: <20190506173916.16486-1-Larry.Finger@lwfinger.net>
Date:   Tue, 28 May 2019 15:08:48 +0300
In-Reply-To: <20190506173916.16486-1-Larry.Finger@lwfinger.net> (Larry
        Finger's message of "Mon, 6 May 2019 12:39:16 -0500")
Message-ID: <87y32q4w8f.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Larry Finger <Larry.Finger@lwfinger.net> writes:

> The driver uses complicated macros to set parts of word 0 of the TX and RX
> descriptors. These are changed into inline routines.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> Kalle,
>
> Based on your comment on how much you dislike those "byte macros", I have
> converted a few of them from rtl8821ae into static inline functions.
>
> Is this what you had in mind, and do you consider these changes to
> improve the code?

[...]

> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> @@ -14,25 +14,67 @@
>  #define USB_HWDESC_HEADER_LEN			40
>  #define CRCLENGTH						4
>  
> -#define SET_TX_DESC_PKT_SIZE(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 0, 16, __val)
> -#define SET_TX_DESC_OFFSET(__pdesc, __val)			\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 16, 8, __val)
> -#define SET_TX_DESC_BMC(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 24, 1, __val)
> -#define SET_TX_DESC_HTC(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 25, 1, __val)
> -#define SET_TX_DESC_LAST_SEG(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 26, 1, __val)
> -#define SET_TX_DESC_FIRST_SEG(__pdesc, __val)		\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 27, 1, __val)
> -#define SET_TX_DESC_LINIP(__pdesc, __val)			\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 28, 1, __val)
> -#define SET_TX_DESC_OWN(__pdesc, __val)				\
> -	SET_BITS_TO_LE_4BYTE(__pdesc, 31, 1, __val)
> -
> -#define GET_TX_DESC_OWN(__pdesc)					\
> -	LE_BITS_TO_4BYTE(__pdesc, 31, 1)
> +/* Set packet size (16 bits) in TX descriptor word 0 */
> +static inline void set_tx_desc_pkt_size(__le32 *__pdesc, u16 __val)
> +{
> +	*__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(0, 15)) |
> +			       __val);
> +}

This was not exactly what I had mind. My point was that the firmware
command or event should be handled as a complete structure, not parsed
one (or four) bytes at a time. To show what I mean here's a random
example from iwlwifi:

struct iwl_alive_resp {
	u8 ucode_minor;
	u8 ucode_major;
	__le16 reserved1;
	u8 sw_rev[8];
	u8 ver_type;
	u8 ver_subtype;			/* not "9" for runtime alive */
	__le16 reserved2;
	__le32 log_event_table_ptr;	/* SRAM address for event log */
	__le32 error_event_table_ptr;	/* SRAM address for error log */
	__le32 timestamp;
	__le32 is_valid;
} __packed;

This a nice, clean and robust way both use AND document an event coming
from the firmware. And the driver can pass around 'struct iwl_alive_res
*resp' pointer to other functions to make it easy to understand what
event the buffer contains. Compared to rtlwifi where there's just 'u8
buf[]' and nobody has any clue what it contains, and need to spend at
least five minutes figuring that out everytime they are looking at a
function.

But to be honest I'm not sure if it's worth trying to cleanup rtlwifi,
it really would need a full rewrite to become a clean driver. IMHO much
better to put more effort on rtl8xxxxu and rtw88, which both already are
clean drivers.

-- 
Kalle Valo