| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHNKnsSUp=E0VMf8sjFoG7uoxdW_O_Lwu57NOiPj1it-mYZnNQ@mail.gmail.com>
Date: Tue, 30 Aug 2022 04:53:07 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: M Chetan Kumar <m.chetan.kumar@...el.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
David Miller <davem@...emloft.net>,
Johannes Berg <johannes@...solutions.net>,
Loic Poulain <loic.poulain@...aro.org>,
"Sudi, Krishna C" <krishna.c.sudi@...el.com>,
M Chetan Kumar <m.chetan.kumar@...ux.intel.com>,
Intel Corporation <linuxwwan@...el.com>,
Haijun Liu <haijun.liu@...iatek.com>,
Madhusmita Sahu <madhusmita.sahu@...el.com>,
Moises Veleta <moises.veleta@...ux.intel.com>,
Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH net-next 1/5] net: wwan: t7xx: Add AP CLDMA
On Tue, Aug 16, 2022 at 7:11 AM <m.chetan.kumar@...el.com> wrote:
> From: Haijun Liu <haijun.liu@...iatek.com>
>
> The t7xx device contains two Cross Layer DMA (CLDMA) interfaces to
> communicate with AP and Modem processors respectively. So far only
> MD-CLDMA was being used, this patch enables AP-CLDMA.
>
> Rename small Application Processor (sAP) to AP.
>
> Signed-off-by: Haijun Liu <haijun.liu@...iatek.com>
> Co-developed-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> Signed-off-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> Signed-off-by: Moises Veleta <moises.veleta@...ux.intel.com>
> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Reviewed-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
And just one nitpick/question below.
> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> index 3458af31e864..c5a3c95004bd 100644
> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> @@ -44,6 +44,7 @@
> #include "t7xx_state_monitor.h"
>
> #define RT_ID_MD_PORT_ENUM 0
> +#define RT_ID_AP_PORT_ENUM 1
> /* Modem feature query identification code - "ICCC" */
> #define MD_FEATURE_QUERY_ID 0x49434343
>
> @@ -296,6 +297,7 @@ static void t7xx_md_exception(struct t7xx_modem *md, enum hif_ex_stage stage)
> }
>
> t7xx_cldma_exception(md->md_ctrl[CLDMA_ID_MD], stage);
> + t7xx_cldma_exception(md->md_ctrl[CLDMA_ID_AP], stage);
>
> if (stage == HIF_EX_INIT)
> t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_EXCEPTION_ACK);
> @@ -424,7 +426,7 @@ static int t7xx_parse_host_rt_data(struct t7xx_fsm_ctl *ctl, struct t7xx_sys_inf
> if (ft_spt_st != MTK_FEATURE_MUST_BE_SUPPORTED)
> return -EINVAL;
>
> - if (i == RT_ID_MD_PORT_ENUM)
> + if (i == RT_ID_MD_PORT_ENUM || i == RT_ID_AP_PORT_ENUM)
> t7xx_port_enum_msg_handler(ctl->md, rt_feature->data);
> }
>
> @@ -454,12 +456,12 @@ static int t7xx_core_reset(struct t7xx_modem *md)
> return 0;
> }
>
> -static void t7xx_core_hk_handler(struct t7xx_modem *md, struct t7xx_fsm_ctl *ctl,
> +static void t7xx_core_hk_handler(struct t7xx_modem *md, struct t7xx_sys_info *core_info,
> + struct t7xx_fsm_ctl *ctl,
> enum t7xx_fsm_event_state event_id,
> enum t7xx_fsm_event_state err_detect)
> {
> struct t7xx_fsm_event *event = NULL, *event_next;
> - struct t7xx_sys_info *core_info = &md->core_md;
> struct device *dev = &md->t7xx_dev->pdev->dev;
> unsigned long flags;
> int ret;
> @@ -529,19 +531,33 @@ static void t7xx_md_hk_wq(struct work_struct *work)
> t7xx_cldma_start(md->md_ctrl[CLDMA_ID_MD]);
> t7xx_fsm_broadcast_state(ctl, MD_STATE_WAITING_FOR_HS2);
> md->core_md.handshake_ongoing = true;
> - t7xx_core_hk_handler(md, ctl, FSM_EVENT_MD_HS2, FSM_EVENT_MD_HS2_EXIT);
> + t7xx_core_hk_handler(md, &md->core_md, ctl, FSM_EVENT_MD_HS2, FSM_EVENT_MD_HS2_EXIT);
> +}
> +
> +static void t7xx_ap_hk_wq(struct work_struct *work)
> +{
> + struct t7xx_modem *md = container_of(work, struct t7xx_modem, ap_handshake_work);
> + struct t7xx_fsm_ctl *ctl = md->fsm_ctl;
> +
> + /* Clear the HS2 EXIT event appended in t7xx_core_reset(). */
> + t7xx_fsm_clr_event(ctl, FSM_EVENT_AP_HS2_EXIT);
> + t7xx_cldma_stop(md->md_ctrl[CLDMA_ID_AP]);
> + t7xx_cldma_switch_cfg(md->md_ctrl[CLDMA_ID_AP]);
> + t7xx_cldma_start(md->md_ctrl[CLDMA_ID_AP]);
> + md->core_ap.handshake_ongoing = true;
> + t7xx_core_hk_handler(md, &md->core_ap, ctl, FSM_EVENT_AP_HS2, FSM_EVENT_AP_HS2_EXIT);
> }
>
> void t7xx_md_event_notify(struct t7xx_modem *md, enum md_event_id evt_id)
> {
> struct t7xx_fsm_ctl *ctl = md->fsm_ctl;
> - void __iomem *mhccif_base;
> unsigned int int_sta;
> unsigned long flags;
>
> switch (evt_id) {
> case FSM_PRE_START:
> - t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_PORT_ENUM);
> + t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_PORT_ENUM | D2H_INT_ASYNC_MD_HK |
> + D2H_INT_ASYNC_AP_HK);
> break;
>
> case FSM_START:
> @@ -554,16 +570,26 @@ void t7xx_md_event_notify(struct t7xx_modem *md, enum md_event_id evt_id)
> ctl->exp_flg = true;
> md->exp_id &= ~D2H_INT_EXCEPTION_INIT;
> md->exp_id &= ~D2H_INT_ASYNC_MD_HK;
> + md->exp_id &= ~D2H_INT_ASYNC_AP_HK;
> } else if (ctl->exp_flg) {
> md->exp_id &= ~D2H_INT_ASYNC_MD_HK;
> - } else if (md->exp_id & D2H_INT_ASYNC_MD_HK) {
> - queue_work(md->handshake_wq, &md->handshake_work);
> - md->exp_id &= ~D2H_INT_ASYNC_MD_HK;
> - mhccif_base = md->t7xx_dev->base_addr.mhccif_rc_base;
> - iowrite32(D2H_INT_ASYNC_MD_HK, mhccif_base + REG_EP2RC_SW_INT_ACK);
> - t7xx_mhccif_mask_set(md->t7xx_dev, D2H_INT_ASYNC_MD_HK);
> + md->exp_id &= ~D2H_INT_ASYNC_AP_HK;
> } else {
> - t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_ASYNC_MD_HK);
> + void __iomem *mhccif_base = md->t7xx_dev->base_addr.mhccif_rc_base;
> +
> + if (md->exp_id & D2H_INT_ASYNC_MD_HK) {
> + queue_work(md->handshake_wq, &md->handshake_work);
> + md->exp_id &= ~D2H_INT_ASYNC_MD_HK;
> + iowrite32(D2H_INT_ASYNC_MD_HK, mhccif_base + REG_EP2RC_SW_INT_ACK);
> + t7xx_mhccif_mask_set(md->t7xx_dev, D2H_INT_ASYNC_MD_HK);
> + }
> +
> + if (md->exp_id & D2H_INT_ASYNC_AP_HK) {
> + queue_work(md->ap_handshake_wq, &md->ap_handshake_work);
> + md->exp_id &= ~D2H_INT_ASYNC_AP_HK;
> + iowrite32(D2H_INT_ASYNC_AP_HK, mhccif_base + REG_EP2RC_SW_INT_ACK);
> + t7xx_mhccif_mask_set(md->t7xx_dev, D2H_INT_ASYNC_AP_HK);
> + }
> }
> spin_unlock_irqrestore(&md->exp_lock, flags);
>
> @@ -576,6 +602,7 @@ void t7xx_md_event_notify(struct t7xx_modem *md, enum md_event_id evt_id)
>
> case FSM_READY:
> t7xx_mhccif_mask_set(md->t7xx_dev, D2H_INT_ASYNC_MD_HK);
> + t7xx_mhccif_mask_set(md->t7xx_dev, D2H_INT_ASYNC_AP_HK);
> break;
>
> default:
> @@ -627,6 +654,19 @@ static struct t7xx_modem *t7xx_md_alloc(struct t7xx_pci_dev *t7xx_dev)
> md->core_md.feature_set[RT_ID_MD_PORT_ENUM] &= ~FEATURE_MSK;
> md->core_md.feature_set[RT_ID_MD_PORT_ENUM] |=
> FIELD_PREP(FEATURE_MSK, MTK_FEATURE_MUST_BE_SUPPORTED);
> +
> + md->ap_handshake_wq = alloc_workqueue("%s", WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI,
> + 0, "ap_hk_wq");
> + if (!md->ap_handshake_wq) {
> + destroy_workqueue(md->handshake_wq);
> + return NULL;
> + }
> +
> + INIT_WORK(&md->ap_handshake_work, t7xx_ap_hk_wq);
> + md->core_ap.feature_set[RT_ID_AP_PORT_ENUM] &= ~FEATURE_MSK;
> + md->core_ap.feature_set[RT_ID_AP_PORT_ENUM] |=
> + FIELD_PREP(FEATURE_MSK, MTK_FEATURE_MUST_BE_SUPPORTED);
> +
> return md;
> }
>
> @@ -638,6 +678,7 @@ int t7xx_md_reset(struct t7xx_pci_dev *t7xx_dev)
> md->exp_id = 0;
> t7xx_fsm_reset(md);
> t7xx_cldma_reset(md->md_ctrl[CLDMA_ID_MD]);
> + t7xx_cldma_reset(md->md_ctrl[CLDMA_ID_AP]);
> t7xx_port_proxy_reset(md->port_prox);
> md->md_init_finish = true;
> return t7xx_core_reset(md);
> @@ -667,6 +708,10 @@ int t7xx_md_init(struct t7xx_pci_dev *t7xx_dev)
> if (ret)
> goto err_destroy_hswq;
>
> + ret = t7xx_cldma_alloc(CLDMA_ID_AP, t7xx_dev);
> + if (ret)
> + goto err_destroy_hswq;
> +
> ret = t7xx_fsm_init(md);
> if (ret)
> goto err_destroy_hswq;
> @@ -679,12 +724,16 @@ int t7xx_md_init(struct t7xx_pci_dev *t7xx_dev)
> if (ret)
> goto err_uninit_ccmni;
>
> - ret = t7xx_port_proxy_init(md);
> + ret = t7xx_cldma_init(md->md_ctrl[CLDMA_ID_AP]);
> if (ret)
> goto err_uninit_md_cldma;
>
> + ret = t7xx_port_proxy_init(md);
> + if (ret)
> + goto err_uninit_ap_cldma;
> +
> ret = t7xx_fsm_append_cmd(md->fsm_ctl, FSM_CMD_START, 0);
> - if (ret) /* fsm_uninit flushes cmd queue */
> + if (ret) /* t7xx_fsm_uninit() flushes cmd queue */
> goto err_uninit_proxy;
>
> t7xx_md_sys_sw_init(t7xx_dev);
> @@ -694,6 +743,9 @@ int t7xx_md_init(struct t7xx_pci_dev *t7xx_dev)
> err_uninit_proxy:
> t7xx_port_proxy_uninit(md->port_prox);
>
> +err_uninit_ap_cldma:
> + t7xx_cldma_exit(md->md_ctrl[CLDMA_ID_AP]);
> +
> err_uninit_md_cldma:
> t7xx_cldma_exit(md->md_ctrl[CLDMA_ID_MD]);
>
> @@ -705,6 +757,7 @@ int t7xx_md_init(struct t7xx_pci_dev *t7xx_dev)
>
> err_destroy_hswq:
> destroy_workqueue(md->handshake_wq);
> + destroy_workqueue(md->ap_handshake_wq);
> dev_err(&t7xx_dev->pdev->dev, "Modem init failed\n");
> return ret;
> }
> @@ -720,8 +773,10 @@ void t7xx_md_exit(struct t7xx_pci_dev *t7xx_dev)
>
> t7xx_fsm_append_cmd(md->fsm_ctl, FSM_CMD_PRE_STOP, FSM_CMD_FLAG_WAIT_FOR_COMPLETION);
> t7xx_port_proxy_uninit(md->port_prox);
> + t7xx_cldma_exit(md->md_ctrl[CLDMA_ID_AP]);
> t7xx_cldma_exit(md->md_ctrl[CLDMA_ID_MD]);
> t7xx_ccmni_exit(t7xx_dev);
> t7xx_fsm_uninit(md);
> destroy_workqueue(md->handshake_wq);
> + destroy_workqueue(md->ap_handshake_wq);
> }
> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.h b/drivers/net/wwan/t7xx/t7xx_modem_ops.h
> index 7469ed636ae8..c93e870ce696 100644
> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.h
> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.h
> @@ -66,10 +66,13 @@ struct t7xx_modem {
> struct cldma_ctrl *md_ctrl[CLDMA_NUM];
> struct t7xx_pci_dev *t7xx_dev;
> struct t7xx_sys_info core_md;
> + struct t7xx_sys_info core_ap;
> bool md_init_finish;
> bool rgu_irq_asserted;
> struct workqueue_struct *handshake_wq;
> struct work_struct handshake_work;
> + struct workqueue_struct *ap_handshake_wq;
Does the driver really need this dedicated workqueue for the AP
handshake? Or can the existing handshake workqueue be reused for the
AP handshake work scheduling too?
> + struct work_struct ap_handshake_work;
> struct t7xx_fsm_ctl *fsm_ctl;
> struct port_proxy *port_prox;
> unsigned int exp_id;
--
Sergey
Powered by blists - more mailing lists