Re: [PATCH 28/50] wifi: ath12k: add htc.h

Jeff Johnson
2022-08-18T21:11:06Z

On 8/12/2022 9:09 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
>
> (Patches split into one patch per file for easier review, but the final
> commit will be one big patch. See the cover letter for more info.)
>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/htc.h | 311 ++++++++++++++++++++++++++++++++++
> 1 file changed, 311 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath12k/htc.h b/drivers/net/wireless/ath/ath12k/htc.h

snip

> +#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK GENMASK(1, 0)
> +#define ATH12K_HTC_CONN_FLAGS_RECV_ALLOC GENMASK(15, 8)
> +
> +enum ath12k_htc_conn_flags {
> + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_FOURTH = 0x0,
> + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_ONE_HALF = 0x1,
> + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_THREE_FOURTHS = 0x2,
> + ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_UNITY = 0x3,

it seems strange to call a 2-bit field a flag

> + ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE = 1 << 2,
> + ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL = 1 << 3
> +};

and it seems strange to have an enum which incorporates the values of
the ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK along with definitions of
other mask flags.

perhaps these would be more logically defined as macros just after
ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK:
#define ATH12K_HTC_CONN_FLAGS_THRESHOLD_LEVEL_MASK GENMASK(1, 0)
#define ATH12K_HTC_CONN_FLAGS_REDUCE_CREDIT_DRIBBLE BIT(2)
#define ATH12K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL BIT(3)

and then rename ath12k_htc_conn_flags to be more specific that it just
has threshold level values?

> +
> +enum ath12k_htc_conn_svc_status {
> + ATH12K_HTC_CONN_SVC_STATUS_SUCCESS = 0,
> + ATH12K_HTC_CONN_SVC_STATUS_NOT_FOUND = 1,
> + ATH12K_HTC_CONN_SVC_STATUS_FAILED = 2,
> + ATH12K_HTC_CONN_SVC_STATUS_NO_RESOURCES = 3,
> + ATH12K_HTC_CONN_SVC_STATUS_NO_MORE_EP = 4
> +};
> +
> +struct ath12k_htc_ready {
> + __le32 id_credit_count;
> + __le32 size_ep;
> +} __packed;
> +
> +struct ath12k_htc_ready_extended {
> + struct ath12k_htc_ready base;
> + __le32 ver_bundle;
> +} __packed;
> +
> +struct ath12k_htc_conn_svc {
> + __le32 msg_svc_id;
> + __le32 flags_len;
> +} __packed;
> +
> +struct ath12k_htc_conn_svc_resp {
> + __le32 msg_svc_id;
> + __le32 flags_len;
> + __le32 svc_meta_pad;
> +} __packed;
> +
> +struct ath12k_htc_setup_complete_extended {
> + __le32 msg_id;

is there a reason this isn't msg_svc_id to be consistent with the other
htc messages?

or even have every htc msg begin with a struct ath12k_htc_msg hdr?

perhaps also consider some naming conventions similar to what were
adopted for WMI?

> + __le32 flags;
> + __le32 max_msgs_per_bundled_recv;
> +} __packed;
> +
> +struct ath12k_htc_msg {
> + __le32 msg_svc_id;
> + __le32 flags_len;
> +} __packed __aligned(4);
> +
> +enum ath12k_htc_record_id {
> + ATH12K_HTC_RECORD_NULL = 0,
> + ATH12K_HTC_RECORD_CREDITS = 1
> +};
> +
> +struct ath12k_htc_record_hdr {
> + u8 id; /* @enum ath12k_htc_record_id */
> + u8 len;
> + u8 pad0;
> + u8 pad1;
> +} __packed;
> +
> +struct ath12k_htc_credit_report {
> + u8 eid; /* @enum ath12k_htc_ep_id */
> + u8 credits;
> + u8 pad0;
> + u8 pad1;
> +} __packed;
> +
> +struct ath12k_htc_record {
> + struct ath12k_htc_record_hdr hdr;
> + union {
> + struct ath12k_htc_credit_report credit_report[0];
> + u8 pauload[0];

s/pauload/payload/?

s/[0]/[]/ per
<https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays>

> + };
> +} __packed __aligned(4);
> +
> +/* note: the trailer offset is dynamic depending
> + * on payload length. this is only a struct layout draft
> + */
> +struct ath12k_htc_frame {
> + struct ath12k_htc_hdr hdr;
> + union {
> + struct ath12k_htc_msg msg;
> + u8 payload[0];

s/[0]/[]/

> + };
> + struct ath12k_htc_record trailer[0];

s/[0]/[]/

but this struct makes no sense. you can't have a variable-length payload
that isn't at the end of the struct

if this is just supposed to be some sort of pseudo-documentation then
perhaps put it in a comment so that the compiler won't see this?

or perhaps even better draw an ASCII representation?

> +} __packed __aligned(4);

rest snipped