| 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: <aed8af81-852c-1e75-b522-76e52ca12a39@linux.intel.com>
Date: Wed, 31 Aug 2022 18:16:14 +0530
From: "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.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>,
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 8/30/2022 7:23 AM, Sergey Ryazanov wrote:
> 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?
It should be possible to reuse the existing handshake workqueue.
modified workqueue might look as below [1]. We could reuse the
md->exp_id inside the workqueue to decide MD or AP handshake flow.
[1]
static void t7xx_hk_wq(struct work_struct *work)
{`
struct t7xx_modem *md = container_of(work, struct t7xx_modem,
handshake_work);
struct t7xx_fsm_ctl *ctl = md->fsm_ctl;
if (md->exp_id & D2H_INT_ASYNC_MD_HK) {
//modem handshake
..
md->exp_id &= ~D2H_INT_ASYNC_MD_HK;
}
if (md->exp_id & D2H_INT_ASYNC_AP_HK) {
//AP handshake
..
md->exp_id &= ~D2H_INT_ASYNC_AP_HK;
}
}
--
Chetan
Powered by blists - more mailing lists