| 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: <d795c765-2f55-4970-95d4-dc0637ab88aa@ti.com>
Date: Mon, 9 Dec 2024 16:09:32 +0530
From: Meghana Malladi <m-malladi@...com>
To: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
CC: <vigneshr@...com>, <jan.kiszka@...mens.com>,
Roger Quadros
<rogerq@...nel.org>, <javier.carrasco.cruz@...il.com>,
<diogo.ivo@...mens.com>, <jacob.e.keller@...el.com>,
<horms@...nel.org>, <pabeni@...hat.com>, <kuba@...nel.org>,
<edumazet@...gle.com>, <davem@...emloft.net>, <andrew+netdev@...n.ch>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
<danishanwar@...com>
Subject: Re: [EXTERNAL] Re: [PATCH net v3 1/2] net: ti: icssg-prueth: Fix
firmware load sequence.
On 05/12/24 14:40, Kalesh Anakkur Purayil wrote:
> On Thu, Dec 5, 2024 at 1:59 PM Meghana Malladi <m-malladi@...com> wrote:
>>
>> From: MD Danish Anwar <danishanwar@...com>
>>
>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>> respective firmwares to PRU cores and whenever interface goes down, we
>> stop the resective cores. Due to this, when SLICE0 goes down while
>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>> stopped. This results in clock jump for SLICE1 interface as the timesync
>> related operations are no longer running.
>>
>> As there are interdependencies between SLICE0 and SLICE1 firmwares,
>> fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>> ICSSG interface is up. Add new flag in prueth struct to check if all
>> firmwares are running.
>>
>> Use emacs_initialized as reference count to load the firmwares for the
>> first and last interface up/down. Moving init_emac_mode and fw_offload_mode
>> API outside of icssg_config to icssg_common_start API as they need
>> to be called only once per firmware boot.
>>
>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>> Signed-off-by: Meghana Malladi <m-malladi@...com>
>> ---
>>
>> Hi all,
>>
>> This patch is based on net-next tagged next-20241128.
>> v2:https://lore.kernel.org/all/20241128122931.2494446-2-m-malladi@ti.com/
>>
>> * Changes since v2 (v3-v2):
>> - error handling in caller function of prueth_emac_common_start()
>> - Use prus_running flag check before stopping the firmwares
>> Both suggested by Roger Quadros <rogerq@...nel.org>
>>
>> drivers/net/ethernet/ti/icssg/icssg_config.c | 45 ++++--
>> drivers/net/ethernet/ti/icssg/icssg_config.h | 1 +
>> drivers/net/ethernet/ti/icssg/icssg_prueth.c | 157 ++++++++++++-------
>> drivers/net/ethernet/ti/icssg/icssg_prueth.h | 5 +
>> 4 files changed, 140 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index 5d2491c2943a..342150756cf7 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -397,7 +397,7 @@ static int prueth_emac_buffer_setup(struct prueth_emac *emac)
>> return 0;
>> }
>>
>> -static void icssg_init_emac_mode(struct prueth *prueth)
>> +void icssg_init_emac_mode(struct prueth *prueth)
>> {
>> /* When the device is configured as a bridge and it is being brought
>> * back to the emac mode, the host mac address has to be set as 0.
>> @@ -406,9 +406,6 @@ static void icssg_init_emac_mode(struct prueth *prueth)
>> int i;
>> u8 mac[ETH_ALEN] = { 0 };
>>
>> - if (prueth->emacs_initialized)
>> - return;
>> -
>> /* Set VLAN TABLE address base */
>> regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK,
>> addr << SMEM_VLAN_OFFSET);
>> @@ -423,15 +420,13 @@ static void icssg_init_emac_mode(struct prueth *prueth)
>> /* Clear host MAC address */
>> icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
>> }
>> +EXPORT_SYMBOL_GPL(icssg_init_emac_mode);
>>
>> -static void icssg_init_fw_offload_mode(struct prueth *prueth)
>> +void icssg_init_fw_offload_mode(struct prueth *prueth)
>> {
>> u32 addr = prueth->shram.pa + EMAC_ICSSG_SWITCH_DEFAULT_VLAN_TABLE_OFFSET;
>> int i;
>>
>> - if (prueth->emacs_initialized)
>> - return;
>> -
>> /* Set VLAN TABLE address base */
>> regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK,
>> addr << SMEM_VLAN_OFFSET);
>> @@ -448,6 +443,7 @@ static void icssg_init_fw_offload_mode(struct prueth *prueth)
>> icssg_class_set_host_mac_addr(prueth->miig_rt, prueth->hw_bridge_dev->dev_addr);
>> icssg_set_pvid(prueth, prueth->default_vlan, PRUETH_PORT_HOST);
>> }
>> +EXPORT_SYMBOL_GPL(icssg_init_fw_offload_mode);
>>
>> int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>> {
>> @@ -455,11 +451,6 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>> struct icssg_flow_cfg __iomem *flow_cfg;
>> int ret;
>>
>> - if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> - icssg_init_fw_offload_mode(prueth);
>> - else
>> - icssg_init_emac_mode(prueth);
>> -
>> memset_io(config, 0, TAS_GATE_MASK_LIST0);
>> icssg_miig_queues_init(prueth, slice);
>>
>> @@ -786,3 +777,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>> writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>> }
>> EXPORT_SYMBOL_GPL(icssg_set_pvid);
>> +
>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
>> +{
>> + struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
>> + int slice = prueth_emac_slice(emac);
>> + struct mgmt_cmd fdb_cmd = { 0 };
>> + int ret = 0;
> [Kalesh] There is no need to initialize "ret" here
Yes, I will remove it.
>> +
>> + fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
>> + fdb_cmd.type = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
>> + fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
>> + fdb_cmd.param = 0;
>> +
>> + fdb_cmd.param |= (slice << 4);
>> + fdb_cmd.cmd_args[0] = 0;
>> +
>> + ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
>> +
> [Kalesh] There is no need of an new line here
Ok, I will remove it.
>> + if (ret)
>> + return ret;
>> +
>> + WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
>> + if (fdb_cmd_rsp.status == 1)
>> + return 0;
>> +
>> + return -EINVAL;
> [Kalesh] Maybe you can simplify this as:
> return fdb_cmd_rsp.status == 1 ? 0 : -EINVAL;
Yes, this looks good. I will update it.
>> +}
>> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> index 92c2deaa3068..c884e9fa099e 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>> #define ICSSG_FW_MGMT_FDB_CMD_TYPE 0x03
>> #define ICSSG_FW_MGMT_CMD_TYPE 0x04
>> #define ICSSG_FW_MGMT_PKT 0x80000000
>> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW 0x05
>>
>> struct icssg_r30_cmd {
>> u32 cmd[4];
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index c568c84a032b..2e22e793b01a 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -164,11 +164,11 @@ static struct icssg_firmwares icssg_emac_firmwares[] = {
>> }
>> };
>>
>> -static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> +static int prueth_emac_start(struct prueth *prueth, int slice)
>> {
>> struct icssg_firmwares *firmwares;
>> struct device *dev = prueth->dev;
>> - int slice, ret;
>> + int ret;
>>
>> if (prueth->is_switch_mode)
>> firmwares = icssg_switch_firmwares;
>> @@ -177,16 +177,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> else
>> firmwares = icssg_emac_firmwares;
>>
>> - slice = prueth_emac_slice(emac);
>> - if (slice < 0) {
>> - netdev_err(emac->ndev, "invalid port\n");
>> - return -EINVAL;
>> - }
>> -
>> - ret = icssg_config(prueth, emac, slice);
>> - if (ret)
>> - return ret;
>> -
>> ret = rproc_set_firmware(prueth->pru[slice], firmwares[slice].pru);
>> ret = rproc_boot(prueth->pru[slice]);
>> if (ret) {
>> @@ -208,7 +198,6 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> goto halt_rtu;
>> }
>>
>> - emac->fw_running = 1;
>> return 0;
>>
>> halt_rtu:
>> @@ -220,6 +209,80 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>> return ret;
>> }
>>
>> +static int prueth_emac_common_start(struct prueth *prueth)
>> +{
>> + struct prueth_emac *emac;
>> + int ret = 0;
>> + int slice;
>> +
>> + if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1])
>> + return -EINVAL;
>> +
>> + /* clear SMEM and MSMC settings for all slices */
>> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size);
>> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS);
>> +
>> + icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>> + icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>> +
>> + if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> + icssg_init_fw_offload_mode(prueth);
>> + else
>> + icssg_init_emac_mode(prueth);
>> +
>> + for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
>> + emac = prueth->emac[slice];
>> + if (emac) {
>> + ret |= icssg_config(prueth, emac, slice);
>> + if (ret)
>> + return ret;
>> + }
>> + ret |= prueth_emac_start(prueth, slice);
>> + }
>> + if (!ret)
>> + prueth->prus_running = 1;
>> + else
>> + return ret;
> [Kalesh] This will read better if you change the condition check like:
> if (ret)
> return ret;
> prueth->prus_running = 1;
Thanks, I will update it.
>> +
>> + emac = prueth->emac[ICSS_SLICE0] ? prueth->emac[ICSS_SLICE0] :
>> + prueth->emac[ICSS_SLICE1];
>> + ret = icss_iep_init(emac->iep, &prueth_iep_clockops,
>> + emac, IEP_DEFAULT_CYCLE_TIME_NS);
>> + if (ret) {
>> + dev_err(prueth->dev, "Failed to initialize IEP module\n");
>> + return ret;
>> + }
>> +
>> + return 0;
> [Kalesh] You can "return ret" here and remove the return from above if
> condition.
I will update it.
>> +}
>> +
>> +static int prueth_emac_common_stop(struct prueth *prueth)
>> +{
>> + struct prueth_emac *emac;
>> + int slice;
>> +
>> + if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1])
>> + return -EINVAL;
>> +
>> + icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
>> + icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
>> +
>> + for (slice = 0; slice < PRUETH_NUM_MACS; slice++) {
>> + if (prueth->prus_running) {
>> + rproc_shutdown(prueth->txpru[slice]);
>> + rproc_shutdown(prueth->rtu[slice]);
>> + rproc_shutdown(prueth->pru[slice]);
>> + }
>> + }
>> + prueth->prus_running = 0;
>> +
>> + emac = prueth->emac[ICSS_SLICE0] ? prueth->emac[ICSS_SLICE0] :
>> + prueth->emac[ICSS_SLICE1];
>> + icss_iep_exit(emac->iep);
>> +
>> + return 0;
>> +}
>> +
>> /* called back by PHY layer if there is change in link state of hw port*/
>> static void emac_adjust_link(struct net_device *ndev)
>> {
>> @@ -369,12 +432,13 @@ static void prueth_iep_settime(void *clockops_data, u64 ns)
>> {
>> struct icssg_setclock_desc __iomem *sc_descp;
>> struct prueth_emac *emac = clockops_data;
>> + struct prueth *prueth = emac->prueth;
>> struct icssg_setclock_desc sc_desc;
>> u64 cyclecount;
>> u32 cycletime;
>> int timeout;
>>
>> - if (!emac->fw_running)
>> + if (!prueth->prus_running)
>> return;
>>
>> sc_descp = emac->prueth->shram.va + TIMESYNC_FW_WC_SETCLOCK_DESC_OFFSET;
>> @@ -543,23 +607,17 @@ static int emac_ndo_open(struct net_device *ndev)
>> {
>> struct prueth_emac *emac = netdev_priv(ndev);
>> int ret, i, num_data_chn = emac->tx_ch_num;
>> + struct icssg_flow_cfg __iomem *flow_cfg;
>> struct prueth *prueth = emac->prueth;
>> int slice = prueth_emac_slice(emac);
>> struct device *dev = prueth->dev;
>> int max_rx_flows;
>> int rx_flow;
>>
>> - /* clear SMEM and MSMC settings for all slices */
>> - if (!prueth->emacs_initialized) {
>> - memset_io(prueth->msmcram.va, 0, prueth->msmcram.size);
>> - memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS);
>> - }
>> -
>> /* set h/w MAC as user might have re-configured */
>> ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>
>> icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>> - icssg_class_default(prueth->miig_rt, slice, 0, false);
>> icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>
>> /* Notify the stack of the actual queue counts. */
>> @@ -597,18 +655,23 @@ static int emac_ndo_open(struct net_device *ndev)
>> goto cleanup_napi;
>> }
>>
>> - /* reset and start PRU firmware */
>> - ret = prueth_emac_start(prueth, emac);
>> - if (ret)
>> - goto free_rx_irq;
>> + if (!prueth->emacs_initialized) {
>> + ret = prueth_emac_common_start(prueth);
>> + if (ret)
>> + goto stop;
>> + }
>>
>> - icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>> + flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>> + writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>> + ret = emac_fdb_flow_id_updated(emac);
>>
>> - if (!prueth->emacs_initialized) {
>> - ret = icss_iep_init(emac->iep, &prueth_iep_clockops,
>> - emac, IEP_DEFAULT_CYCLE_TIME_NS);
>> + if (ret) {
>> + netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>> + goto stop;
>> }
>>
>> + icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>> +
>> ret = request_threaded_irq(emac->tx_ts_irq, NULL, prueth_tx_ts_irq,
>> IRQF_ONESHOT, dev_name(dev), emac);
>> if (ret)
>> @@ -653,8 +716,7 @@ static int emac_ndo_open(struct net_device *ndev)
>> free_tx_ts_irq:
>> free_irq(emac->tx_ts_irq, emac);
>> stop:
>> - prueth_emac_stop(emac);
>> -free_rx_irq:
>> + prueth_emac_common_stop(prueth);
>> free_irq(emac->rx_chns.irq[rx_flow], emac);
>> cleanup_napi:
>> prueth_ndev_del_tx_napi(emac, emac->tx_ch_num);
>> @@ -689,8 +751,6 @@ static int emac_ndo_stop(struct net_device *ndev)
>> if (ndev->phydev)
>> phy_stop(ndev->phydev);
>>
>> - icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
>> -
>> if (emac->prueth->is_hsr_offload_mode)
>> __dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
>> else
>> @@ -728,11 +788,9 @@ static int emac_ndo_stop(struct net_device *ndev)
>> /* Destroying the queued work in ndo_stop() */
>> cancel_delayed_work_sync(&emac->stats_work);
>>
>> - if (prueth->emacs_initialized == 1)
>> - icss_iep_exit(emac->iep);
>> -
>> /* stop PRUs */
>> - prueth_emac_stop(emac);
>> + if (prueth->emacs_initialized == 1)
>> + prueth_emac_common_stop(prueth);
>>
>> free_irq(emac->tx_ts_irq, emac);
>>
>> @@ -1069,16 +1127,10 @@ static void prueth_emac_restart(struct prueth *prueth)
>> icssg_set_port_state(emac1, ICSSG_EMAC_PORT_DISABLE);
>>
>> /* Stop both pru cores for both PRUeth ports*/
>> - prueth_emac_stop(emac0);
>> - prueth->emacs_initialized--;
>> - prueth_emac_stop(emac1);
>> - prueth->emacs_initialized--;
>> + prueth_emac_common_stop(prueth);
>>
>> /* Start both pru cores for both PRUeth ports */
>> - prueth_emac_start(prueth, emac0);
>> - prueth->emacs_initialized++;
>> - prueth_emac_start(prueth, emac1);
>> - prueth->emacs_initialized++;
>> + prueth_emac_common_start(prueth);
>>
>> /* Enable forwarding for both PRUeth ports */
>> icssg_set_port_state(emac0, ICSSG_EMAC_PORT_FORWARD);
>> @@ -1413,13 +1465,10 @@ static int prueth_probe(struct platform_device *pdev)
>> prueth->pa_stats = NULL;
>> }
>>
>> - if (eth0_node) {
>> + if (eth0_node || eth1_node) {
>> ret = prueth_get_cores(prueth, ICSS_SLICE0, false);
>> if (ret)
>> goto put_cores;
>> - }
>> -
>> - if (eth1_node) {
>> ret = prueth_get_cores(prueth, ICSS_SLICE1, false);
>> if (ret)
>> goto put_cores;
>> @@ -1618,14 +1667,12 @@ static int prueth_probe(struct platform_device *pdev)
>> pruss_put(prueth->pruss);
>>
>> put_cores:
>> - if (eth1_node) {
>> - prueth_put_cores(prueth, ICSS_SLICE1);
>> - of_node_put(eth1_node);
>> - }
>> -
>> - if (eth0_node) {
>> + if (eth0_node || eth1_node) {
>> prueth_put_cores(prueth, ICSS_SLICE0);
>> of_node_put(eth0_node);
>> +
>> + prueth_put_cores(prueth, ICSS_SLICE1);
>> + of_node_put(eth1_node);
>> }
>>
>> return ret;
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index f5c1d473e9f9..b30f2e9a73d8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -257,6 +257,7 @@ struct icssg_firmwares {
>> * @is_switchmode_supported: indicates platform support for switch mode
>> * @switch_id: ID for mapping switch ports to bridge
>> * @default_vlan: Default VLAN for host
>> + * @prus_running: flag to indicate if all pru cores are running
>> */
>> struct prueth {
>> struct device *dev;
>> @@ -298,6 +299,7 @@ struct prueth {
>> int default_vlan;
>> /** @vtbl_lock: Lock for vtbl in shared memory */
>> spinlock_t vtbl_lock;
>> + bool prus_running;
>> };
>>
>> struct emac_tx_ts_response {
>> @@ -361,6 +363,8 @@ int icssg_set_port_state(struct prueth_emac *emac,
>> enum icssg_port_state_cmd state);
>> void icssg_config_set_speed(struct prueth_emac *emac);
>> void icssg_config_half_duplex(struct prueth_emac *emac);
>> +void icssg_init_emac_mode(struct prueth *prueth);
>> +void icssg_init_fw_offload_mode(struct prueth *prueth);
>>
>> /* Buffer queue helpers */
>> int icssg_queue_pop(struct prueth *prueth, u8 queue);
>> @@ -377,6 +381,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>> u8 untag_mask, bool add);
>> u16 icssg_get_pvid(struct prueth_emac *emac);
>> void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>> #define prueth_napi_to_tx_chn(pnapi) \
>> container_of(pnapi, struct prueth_tx_chn, napi_tx)
>>
>> --
>> 2.25.1
>>
>>
>
>
Powered by blists - more mailing lists