Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org E7EEEC433C6
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Aloka Dixit <alokad@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org, ath11k@lists.infradead.org,
        John Crispin <john@phrozen.org>
Subject: Re: [PATCH V5 1/2] ath11k: add WMI calls to manually add/del/pause/resume TWT dialogs
References: <20210222192651.1782-1-alokad@codeaurora.org>
        <20210222192651.1782-2-alokad@codeaurora.org>
Date:   Tue, 09 Mar 2021 12:57:23 +0200
In-Reply-To: <20210222192651.1782-2-alokad@codeaurora.org> (Aloka Dixit's
        message of "Mon, 22 Feb 2021 11:26:50 -0800")
Message-ID: <87sg54zjak.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Aloka Dixit <alokad@codeaurora.org> writes:

> From: John Crispin <john@phrozen.org>
>
> These calls are used for debugging and will be required for WFA
> certification tests.
>
> Signed-off-by: John Crispin <john@phrozen.org>
> Co-developed-by: Aloka Dixit <alokad@codeaurora.org>
> Signed-off-by: Aloka Dixit <alokad@codeaurora.org>

[...]

> +int ath11k_wmi_send_twt_add_dialog_cmd(struct ath11k *ar,
> +				       struct wmi_twt_add_dialog_params *params)
> +{
> +	struct ath11k_pdev_wmi *wmi = ar->wmi;
> +	struct ath11k_base *ab = wmi->wmi_ab->ab;
> +	struct wmi_twt_add_dialog_params_cmd *cmd;
> +	struct sk_buff *skb;
> +	int ret, len;
> +
> +	len = sizeof(*cmd);
> +
> +	skb = ath11k_wmi_alloc_skb(wmi->wmi_ab, len);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	cmd = (struct wmi_twt_add_dialog_params_cmd *)skb->data;
> +	cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG, WMI_TAG_TWT_ADD_DIALOG_CMD) |
> +			  FIELD_PREP(WMI_TLV_LEN, len - TLV_HDR_SIZE);
> +
> +	cmd->vdev_id = params->vdev_id;
> +	ether_addr_copy(cmd->peer_macaddr.addr, params->peer_macaddr);
> +	cmd->dialog_id = params->dialog_id;
> +	cmd->wake_intvl_us = params->wake_intvl_us;
> +	cmd->wake_intvl_mantis = params->wake_intvl_mantis;
> +	cmd->wake_dura_us = params->wake_dura_us;
> +	cmd->sp_offset_us = params->sp_offset_us;
> +	cmd->flags = params->twt_cmd;
> +	if (params->flag_bcast)
> +		cmd->flags |= WMI_TWT_ADD_DIALOG_FLAG_BCAST;
> +	if (params->flag_trigger)
> +		cmd->flags |= WMI_TWT_ADD_DIALOG_FLAG_TRIGGER;
> +	if (params->flag_flow_type)
> +		cmd->flags |= WMI_TWT_ADD_DIALOG_FLAG_FLOW_TYPE;
> +	if (params->flag_protection)
> +		cmd->flags |= WMI_TWT_ADD_DIALOG_FLAG_PROTECTION;
> +
> +	ath11k_dbg(ar->ab, ATH11K_DBG_WMI,
> +		   "WMI add TWT dialog, vdev %u, dialog id %u,\n"
> +		   "wake interval %u, mantissa %u, wake duration %u,\n"
> +		   "service period offset %u, flags 0x%x\n",

In debug messages please use all lower case, and no commas nor \n (ie.
all in one line).

> +	ret = ath11k_wmi_cmd_send(wmi, skb, WMI_TWT_ADD_DIALOG_CMDID);
> +
> +	if (ret) {
> +		ath11k_warn(ab,
> +			    "Failed to send WMI command to add TWT dialog\n");

Warning and error messages should follow style:

"failed to send add TWT dialog WMI command: %d", ret

> +static void ath11k_wmi_twt_add_dialog_event(struct ath11k_base *ab,
> +					    struct sk_buff *skb)
> +{
> +	const char *status[] = {
> +		"OK", "TWT_NOT_ENABLED", "USED_DIALOG_ID", "INVALID_PARAM",
> +		"NOT_READY", "NO_RESOURCE", "NO_ACK", "NO_RESPONSE",
> +		"DENIED", "UNKNOWN_ERROR"
> +	};
> +	const void **tb;
> +	const struct wmi_twt_add_dialog_event *ev;
> +	int ret;
> +
> +	tb = ath11k_wmi_tlv_parse_alloc(ab, skb->data, skb->len, GFP_ATOMIC);
> +	if (IS_ERR(tb)) {
> +		ret = PTR_ERR(tb);
> +		ath11k_warn(ab,
> +			    "failed to parse TWT add dialog status event tlv: %d\n", ret);
> +		return;
> +	}
> +
> +	ev = tb[WMI_TAG_TWT_ADD_DIALOG_COMPLETE_EVENT];
> +	if (!ev) {
> +		ath11k_warn(ab, "failed to fetch TWT add dialog event\n");
> +		goto exit;
> +	}
> +
> +	ath11k_dbg(ab, ATH11K_DBG_WMI,
> +		   "TWT Add Dialog Event - vdev: %d, dialog id: %d, status: %s\n",
> +		   ev->vdev_id, ev->dialog_id, status[ev->status]);

You are not checking that status array is not used out of bounds. I
would just remove the string conversion to keep things simple, but if
you want to print in strings please add a helper function to do the
conversion and add status values as enum to wmi.h.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches