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: <05841a52-3d2f-8704-b885-4b02e7a439e0@quicinc.com>
Date:   Thu, 18 Aug 2022 15:25:49 -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 33/50] wifi: ath12k: add mhi.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-34-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220812161003.27279-34-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/mhi.c | 615 ++++++++++++++++++++++++++++++++++
>   1 file changed, 615 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c
> new file mode 100644
> index 000000000000..f77634994d97
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath12k/mhi.c
> @@ -0,0 +1,615 @@
> +// SPDX-License-Identifier: BSD-3-Clause-Clear
> +/*
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/msi.h>
> +#include <linux/pci.h>
> +
> +#include "core.h"
> +#include "debug.h"
> +#include "mhi.h"
> +#include "pci.h"
> +
> +#define MHI_TIMEOUT_DEFAULT_MS	90000
> +
> +static struct mhi_channel_config ath12k_mhi_channels_qcn9274[] = {

should this be const?
in struct mhi_controller_config:
	const struct mhi_channel_config *ch_cfg;

> +	{
> +		.num = 0,
> +		.name = "LOOPBACK",
> +		.num_elements = 32,
> +		.event_ring = 1,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 1,
> +		.name = "LOOPBACK",
> +		.num_elements = 32,
> +		.event_ring = 1,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 20,
> +		.name = "IPCR",
> +		.num_elements = 32,
> +		.event_ring = 1,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 21,
> +		.name = "IPCR",
> +		.num_elements = 32,
> +		.event_ring = 1,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = true,
> +	},
> +};
> +
> +static struct mhi_event_config ath12k_mhi_events_qcn9274[] = {

seems this should be const
but for some reason struct mhi_controller_config has:
	struct mhi_event_config *event_cfg;

(not const) so this can't be const :(

perhaps someone can propose a MHI interface change?
especially since internally to MHI in parse_ev_cfg() we have:
	const struct mhi_event_config *event_cfg;
	[...]
	for (i = 0; i < num; i++) {
		event_cfg = &config->event_cfg[i];

so it is treated as const

> +	{
> +		.num_elements = 32,
> +		.irq_moderation_ms = 0,
> +		.irq = 1,
> +		.data_type = MHI_ER_CTRL,
> +		.mode = MHI_DB_BRST_DISABLE,
> +		.hardware_event = false,
> +		.client_managed = false,
> +		.offload_channel = false,
> +	},
> +	{
> +		.num_elements = 256,
> +		.irq_moderation_ms = 1,
> +		.irq = 2,
> +		.mode = MHI_DB_BRST_DISABLE,
> +		.priority = 1,
> +		.hardware_event = false,
> +		.client_managed = false,
> +		.offload_channel = false,
> +	},
> +};
> +
> +struct mhi_controller_config ath12k_mhi_config_qcn9274 = {
> +	.max_channels = 30,
> +	.timeout_ms = 10000,
> +	.use_bounce_buf = false,
> +	.buf_len = 0,
> +	.num_channels = ARRAY_SIZE(ath12k_mhi_channels_qcn9274),
> +	.ch_cfg = ath12k_mhi_channels_qcn9274,
> +	.num_events = ARRAY_SIZE(ath12k_mhi_events_qcn9274),
> +	.event_cfg = ath12k_mhi_events_qcn9274,
> +};
> +
> +static struct mhi_channel_config ath12k_mhi_channels_wcn7850[] = {

const

> +	{
> +		.num = 0,
> +		.name = "LOOPBACK",
> +		.num_elements = 32,
> +		.event_ring = 0,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 1,
> +		.name = "LOOPBACK",
> +		.num_elements = 32,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 20,
> +		.name = "IPCR",
> +		.num_elements = 64,
> +		.event_ring = 1,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +	},
> +	{
> +		.num = 21,
> +		.name = "IPCR",
> +		.num_elements = 64,
> +		.event_ring = 1,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = 0x4,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = true,
> +	},
> +};
> +
> +static struct mhi_event_config ath12k_mhi_events_wcn7850[] = {

keep not const (for now) :(

> +	{
> +		.num_elements = 32,
> +		.irq_moderation_ms = 0,
> +		.irq = 1,
> +		.mode = MHI_DB_BRST_DISABLE,
> +		.data_type = MHI_ER_CTRL,
> +		.hardware_event = false,
> +		.client_managed = false,
> +		.offload_channel = false,
> +	},
> +	{
> +		.num_elements = 256,
> +		.irq_moderation_ms = 1,
> +		.irq = 2,
> +		.mode = MHI_DB_BRST_DISABLE,
> +		.priority = 1,
> +		.hardware_event = false,
> +		.client_managed = false,
> +		.offload_channel = false,
> +	},
> +};
> +
> +struct mhi_controller_config ath12k_mhi_config_wcn7850 = {

and this one should be const since it is registered via:

int mhi_register_controller(struct mhi_controller *mhi_cntrl,
			const struct mhi_controller_config *config);

> +	.max_channels = 128,
> +	.timeout_ms = 2000,
> +	.use_bounce_buf = false,
> +	.buf_len = 0,
> +	.num_channels = ARRAY_SIZE(ath12k_mhi_channels_wcn7850),
> +	.ch_cfg = ath12k_mhi_channels_wcn7850,
> +	.num_events = ARRAY_SIZE(ath12k_mhi_events_wcn7850),
> +	.event_cfg = ath12k_mhi_events_wcn7850,
> +};
> +

snip

> +static int ath12k_mhi_get_msi(struct ath12k_pci *ab_pci)
> +{
> +	struct ath12k_base *ab = ab_pci->ab;
> +	u32 user_base_data, base_vector;
> +	int ret, num_vectors, i;
> +	int *irq;
> +
> +	ret = ath12k_pci_get_user_msi_assignment(ab,
> +						 "MHI", &num_vectors,
> +						 &user_base_data, &base_vector);
> +	if (ret)
> +		return ret;
> +
> +	ath12k_dbg(ab, ATH12K_DBG_PCI, "Number of assigned MSI for MHI is %d, base vector is %d\n",
> +		   num_vectors, base_vector);
> +
> +	irq = kcalloc(num_vectors, sizeof(int), GFP_KERNEL);

prefer sizeof(*irq)?

> +	if (!irq)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_vectors; i++)
> +		irq[i] = ath12k_pci_get_msi_irq(ab->dev,
> +						base_vector + i);
> +
> +	ab_pci->mhi_ctrl->irq = irq;
> +	ab_pci->mhi_ctrl->nr_irqs = num_vectors;
> +
> +	return 0;
> +}
> +

snip

> +void ath12k_mhi_unregister(struct ath12k_pci *ab_pci)
> +{
> +	struct mhi_controller *mhi_ctrl = ab_pci->mhi_ctrl;
> +
> +	mhi_unregister_controller(mhi_ctrl);
> +	kfree(mhi_ctrl->irq);
> +	mhi_free_controller(mhi_ctrl);

consider setting ab_pci->mhi_ctrl = NULL to avoid dangling pointer?

> +}

rest snipped