| 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:
<MEYP282MB26975FCCAAEC504D72278F8BBB692@MEYP282MB2697.AUSP282.PROD.OUTLOOK.COM>
Date: Wed, 10 Jan 2024 07:07:15 +0000
From: Jinjian Song <SongJinJian@...mail.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"chandrashekar.devegowda@...el.com" <chandrashekar.devegowda@...el.com>,
"chiranjeevi.rapolu@...ux.intel.com" <chiranjeevi.rapolu@...ux.intel.com>,
"haijun.liu@...iatek.com" <haijun.liu@...iatek.com>,
"m.chetan.kumar@...ux.intel.com" <m.chetan.kumar@...ux.intel.com>,
"ricardo.martinez@...ux.intel.com" <ricardo.martinez@...ux.intel.com>,
"loic.poulain@...aro.org" <loic.poulain@...aro.org>,
"johannes@...solutions.net" <johannes@...solutions.net>,
"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>, "linux-kernel@...r.kernel.com"
<linux-kernel@...r.kernel.com>, "vsankar@...ovo.com" <vsankar@...ovo.com>,
"danielwinkler@...gle.com" <danielwinkler@...gle.com>, "nmarupaka@...gle.com"
<nmarupaka@...gle.com>, "joey.zhao@...ocom.com" <joey.zhao@...ocom.com>,
"liuqf@...ocom.com" <liuqf@...ocom.com>, "felix.yan@...ocom.com"
<felix.yan@...ocom.com>, Jinjian Song <jinjian.song@...ocom.com>
Subject: Re: [net-next v3 3/3] net: wwan: t7xx: Add fastboot WWAN port
>Hello Jinjian,
>thank you for sharing this implementation. I believe this patch requires further splitting since its hard to distinguish between generic changes and fastboot port introduction. And here a lot of changes to the common driver codebase. At least the ports configuration rework worth a dedicated patch and probably modem boot state tracking also requires a dedicated patch or should go to the modem state exporting patch.
Yes, please let me split this patch, this patch include the infrastructure for early port configuration/ fastboot port creation according to boot stage/a little common driver codebase.
Early port configuration make it possible to surpport fastboot channel when device booting to LK stage, we create it's dedicated queue.
As normal, device booting to Linux stage, driver using another queue to communicate with device.
1.Iinfrastructure for early port configuration according to boot stage patch.
2.Fastboot port configuration patch.
>Please find a few nitpicks below.
>On 28.12.2023 11:44, Jinjian Song wrote:
> [skipped]
>> @@ -82,6 +91,7 @@ struct cldma_queue {
>> wait_queue_head_t req_wq; /* Only for TX */
>> struct workqueue_struct *worker;
>> struct work_struct cldma_work;
>> + int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb);
>> };
>>
>> struct cldma_ctrl {
>> @@ -101,24 +111,22 @@ struct cldma_ctrl {
>> struct md_pm_entity *pm_entity;
>> struct t7xx_cldma_hw hw_info;
>> bool is_late_init;
>> - int (*recv_skb)(struct cldma_queue *queue, struct sk_buff *skb);
>> };
>What was the purpose of moving the recv_skb callback between layers?
Fastboot channel using a dedicate queue for raw data, we need moving to the queue associate layers to identify the data channel.
Origin design only support one CLDMA queue.
> [skipped]
>> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c
>> b/drivers/net/wwan/t7xx/t7xx_pci.c
>> index d5f6a8638aba..f51e5e2b41e1 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>> @@ -170,7 +170,8 @@ static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
>> pm_runtime_set_autosuspend_delay(&pdev->dev, PM_AUTOSUSPEND_MS);
>> pm_runtime_use_autosuspend(&pdev->dev);
>>
>> - return t7xx_wait_pm_config(t7xx_dev);
>> + t7xx_wait_pm_config(t7xx_dev);
>> + return 0;
>What was the purpose of breaking the error reporting?
Yeah, I should delete the t7xx_wait_pm_config(t7xx_dev) here, return directly, this function only used for when we
sending PM control commands to device, we don't need it here. like follow:
iowrite32(T7XX_L1_BIT(0), IREG_BASE(t7xx_dev) + DISABLE_ASPM_LOWPWR);
t7xx_wait_pm_config(t7xx_dev);
>> }
>>
>> void t7xx_pci_pm_init_late(struct t7xx_pci_dev *t7xx_dev) diff --git
>> a/drivers/net/wwan/t7xx/t7xx_port.h
>> b/drivers/net/wwan/t7xx/t7xx_port.h
>> index 4ae8a00a8532..09acb1ef144d 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
>> @@ -75,6 +75,8 @@ enum port_ch {
>> PORT_CH_DSS6_TX = 0x20df,
>> PORT_CH_DSS7_RX = 0x20e0,
>> PORT_CH_DSS7_TX = 0x20e1,
>> +
>> + PORT_CH_ID_UNIMPORTANT = 0xffff,
>Do we need this '_ID_' inside the name?
Please let me remove this '_ID_'.
>> };
> [skipped]
>> +static int t7xx_port_ctrl_start(struct wwan_port *port)
>Please use consistent prefixes, when it is possible. Half of objects below have 't7xx_port_fastboot_' not 't7xx_port_ctrl_' prefix. And this is t7xx_port_fastboot.c file, what make more sence for the former prefix.
Please let me unify the prefix.
> [skipped]
>> +static const struct wwan_port_ops wwan_ops = {
>Should it be called t7xx_port_fastboot_ops?
Please let me modify that.
>> + .start = t7xx_port_ctrl_start,
>> + .stop = t7xx_port_ctrl_stop,
>> + .tx = t7xx_port_ctrl_tx,
>> +};
> [skipped]
>> +struct port_ops fastboot_port_ops = {
>> + .init = t7xx_port_fastboot_init,
>> + .recv_skb = t7xx_port_fastboot_recv_skb,
>> + .uninit = t7xx_port_fastboot_uninit,
>> + .enable_chl = t7xx_port_fastboot_enable_chl,
>> + .disable_chl = t7xx_port_fastboot_disable_chl, };
>> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> index 274846d39fbf..0525a70acc81 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> @@ -100,6 +100,21 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
>> },
> > };
> >
>> +static struct t7xx_port_conf t7xx_early_port_conf[] = {
>Seems this array should be a constant.
Yes, please let modify.
>> + {
>> + .tx_ch = PORT_CH_ID_UNIMPORTANT,
>> + .rx_ch = PORT_CH_ID_UNIMPORTANT,
>> + .txq_index = CLDMA_Q_IDX_DUMP,
>> + .rxq_index = CLDMA_Q_IDX_DUMP,
>> + .txq_exp_index = CLDMA_Q_IDX_DUMP,
>> + .rxq_exp_index = CLDMA_Q_IDX_DUMP,
>> + .path_id = CLDMA_ID_AP,
>> + .ops = &fastboot_port_ops,
>> + .name = "FASTBOOT",
>Is FASTBOOT abbriviation? Why is it spelled in capital?
No, I want use FASTBOOT for fastboot protocol and be consistent with the definition in wwan_core.c
@@ -328,6 +328,10 @@ static const struct {
.name = "XMMRPC",
.devsuf = "xmmrpc",
},
+ [WWAN_PORT_FASTBOOT] = {
+ .name = "FASTBOOT",
+ .devsuf = "fastboot",
+ },
Should I modify name to 'fastboot' both?
>> + .port_type = WWAN_PORT_FASTBOOT,
>> + },
>> +};
> [skipped]
>> static int t7xx_proxy_alloc(struct t7xx_modem *md)
>> {
>> + unsigned int early_port_count = ARRAY_SIZE(t7xx_early_port_conf);
>> unsigned int port_count = ARRAY_SIZE(t7xx_port_conf);
>> struct device *dev = &md->t7xx_dev->pdev->dev;
>> struct port_proxy *port_prox;
>> - int i;
>>
>> + port_count = max(port_count, early_port_count);
>> port_prox = devm_kzalloc(dev, sizeof(*port_prox) + sizeof(struct t7xx_port) * port_count,
>> GFP_KERNEL);
>If you need just a maximum size of two sets of ports, then it is possible to achieve this with a macro:
>#define T7XX_MAX_POSSIBLE_PORTS_NUM \
(max(ARRAY_SIZE(t7xx_port_conf), ARRAY_SIZE(t7xx_early_port_conf)))
>devm_kzalloc(dev, sizeof(*port_prox) +
sizeof(struct t7xx_port) * T7XX_MAX_POSSIBLE_PORTS_NUM,
GFP_KERNEL);
Please let me do that modify.
> [skipped]
>> #define T7XX_PCIE_MISC_DEV_STATUS 0x0d1c
>> -#define MISC_STAGE_MASK GENMASK(2, 0)
>> -#define MISC_RESET_TYPE_PLDR BIT(26)
>> #define MISC_RESET_TYPE_FLDR BIT(27)
>> -#define LINUX_STAGE 4
>> +#define MISC_RESET_TYPE_PLDR BIT(26)
>> +#define MISC_LK_EVENT_MASK GENMASK(11, 8)
>> +#define HOST_EVENT_MASK GENMASK(31, 28)
>> +
>> +enum lk_event_id {
>> + LK_EVENT_NORMAL = 0,
>> + LK_EVENT_CREATE_PD_PORT = 1,
>> + LK_EVENT_CREATE_POST_DL_PORT = 2,
>> + LK_EVENT_RESET = 7,
>> +};
>> +
>> +#define MISC_STAGE_MASK GENMASK(2, 0)
>Why does MISC_STAGE_MASK seems to be recreated? Some whitespace changes or just a git-diff glitch?
MISC_STAGE_MASK is related to the emum t7xx_device_stage define, so recreate it and put the two of them together.
I can restore it, if needed.
>> +enum t7xx_device_stage {
>> + T7XX_DEV_STAGE_INIT = 0,
>> + T7XX_DEV_STAGE_BROM_PRE = 1,
>> + T7XX_DEV_STAGE_BROM_POST = 2,
>> + T7XX_DEV_STAGE_LK = 3,
>> + T7XX_DEV_STAGE_LINUX = 4,
>> +};
> [skipped]
>> - ret = read_poll_timeout(ioread32, dev_status,
>> - (dev_status & MISC_STAGE_MASK) == LINUX_STAGE, 20000, 2000000,
>> - false, IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
>> + ret = read_poll_timeout(ioread32, status,
>> + ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LINUX) ||
>> + ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LK), 100000,
> >+ 20000000, false, IREG_BASE(md->t7xx_dev) +
>> + T7XX_PCIE_MISC_DEV_STATUS);
>Can it be (re-)wrapped in a more readable way? Or can we use a macro here to make it readable again?
Yeah, this function read the device stage from T7XX_PCIE_MISC_DEV_STATUS register and expect device stage
T7XX_DEV_STAGE_LINUX/ T7XX_DEV_STAGE_LK to go on.
I can wrapped in a more readable way using a macro here or function.
> [skipped]
>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> b/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> index b0b3662ae6d7..9421bbd2f117 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> @@ -96,6 +96,7 @@ struct t7xx_fsm_ctl {
>> bool exp_flg;
>> spinlock_t notifier_lock; /* Protects notifier list */
>> struct list_head notifier_list;
>> + u32 prev_status;
>Why is it called 'previous status' do we store 'current status' in the same structure?
Yes, we don't store the current status in the same structure, it store the status last reading by to follow 'status', I can rename it 'status'.
+ ret = read_poll_timeout(ioread32, status,
+ ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LINUX) ||
+ ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LK), 100000,
+ 20000000, false, IREG_BASE(md->t7xx_dev) +
+ T7XX_PCIE_MISC_DEV_STATUS);
Powered by blists - more mailing lists