| 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
| ||
|
Date: Wed, 31 Aug 2022 17:04:40 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.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 Wed, Aug 31, 2022 at 3:46 PM Kumar, M Chetan
<m.chetan.kumar@...ux.intel.com> wrote:
> 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;
> }
> }
I did not mean reuse the _work_, I meant reuse the _workqueue_. Just replace
queue_work(md->ap_handshake_wq, &md->ap_handshake_work);
by
queue_work(md->handshake_wq, &md->ap_handshake_work);
to avoid creating ap_handshake_wq and simplify the driver
initialization. Does it make sense?
--
Sergey
Powered by blists - more mailing lists