Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <959339da-3574-8638-02e6-e1459559fa45@quicinc.com>
Date:   Fri, 19 Aug 2022 13:45:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 46/50] wifi: ath12k: add wmi.c
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>, <linux-wireless@vger.kernel.org>
CC:     <ath12k@lists.infradead.org>
References: <20220812161003.27279-1-kvalo@kernel.org>
 <20220812161003.27279-47-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220812161003.27279-47-kvalo@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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/wmi.c | 6558 +++++++++++++++++++++++++++++++++

[...]

I notice an inconsistency in logging, and wonder if there should be 
consistency. Some commands have a debug log after ath12k_wmi_cmd_send() 
and some have a debug log before:

> +int ath12k_wmi_vdev_create(struct ath12k *ar, u8 *macaddr,
> +			   struct ath12k_wmi_vdev_create_arg *args)
> +{

[... after case ...]

> +	ret = ath12k_wmi_cmd_send(wmi, skb, WMI_VDEV_CREATE_CMDID);
> +	if (ret) {
> +		ath12k_warn(ar->ab,
> +			    "failed to submit WMI_VDEV_CREATE_CMDID\n");
> +		dev_kfree_skb(skb);
> +	}
> +
> +	ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> +		   "WMI vdev create: id %d type %d subtype %d macaddr %pM pdevid %d\n",
> +		   args->if_id, args->type, args->subtype,
> +		   macaddr, args->pdev_id);
> +
> +	return ret;
> +}

[...]

> +int ath12k_wmi_send_peer_delete_cmd(struct ath12k *ar,
> +				    const u8 *peer_addr, u8 vdev_id)
> +{

[... before case ...]

> +	ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
> +		   "WMI peer delete vdev_id %d peer_addr %pM\n",
> +		   vdev_id,  peer_addr);
> +
> +	ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_DELETE_CMDID);
> +	if (ret) {
> +		ath12k_warn(ar->ab, "failed to send WMI_PEER_DELETE cmd\n");
> +		dev_kfree_skb(skb);
> +	}
> +
> +	return ret;
> +}

in the case where an error is reported I think it would make more sense 
to have the error log after the debug log.

For the "after" case above, in the error path, you'd have logs:
failed to submit WMI_VDEV_CREATE_CMDID
WMI vdev create: id %d type %d subtype %d macaddr %pM pdevid %d

IMO this is confusing since it tells you it failed, but then seems to be 
telling you it did somethig

For the "before" case above, in the error path, you'd have:
WMI peer delete vdev_id %d peer_addr %pM
failed to send WMI_PEER_DELETE cmd

This seems to make more sense since it tells you it is doing something 
and then tells you that what it was trying to do failed. no ambiguity.