| 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: <ekzmq7y5is7em2zlsmf4bzne4z346dkyvynynmd45m7iqulamq@sle2yzzx7o4t>
Date: Fri, 23 Aug 2024 14:56:46 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Furong Xu <0x1207@...il.com>
Cc: Vladimir Oltean <olteanv@...il.com>,
Alexander Lobakin <aleksander.lobakin@...el.com>, "David S. Miller" <davem@...emloft.net>,
Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
Joao Pinto <jpinto@...opsys.com>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, xfr@...look.com
Subject: Re: [PATCH net-next v6 1/7] net: stmmac: move stmmac_fpe_cfg to
stmmac_priv data
On Fri, Aug 23, 2024 at 06:50:08PM +0800, Furong Xu wrote:
> By moving the fpe_cfg field to the stmmac_priv data, stmmac_fpe_cfg
> becomes platform-data eventually, instead of a run-time config.
>
> Suggested-by: Serge Semin <fancer.lancer@...il.com>
> Signed-off-by: Furong Xu <0x1207@...il.com>
> Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/hwif.h | 2 ++
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 30 ++++++++++++++++++-
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 ++++++-------
> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 16 ++--------
> include/linux/stmmac.h | 28 -----------------
> 5 files changed, 44 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 7e90f34b8c88..d3da82982012 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -26,6 +26,8 @@
> })
>
> struct stmmac_extra_stats;
> +struct stmmac_fpe_cfg;
> +enum stmmac_mpacket_type;
> struct stmmac_priv;
> struct stmmac_safety_stats;
> struct dma_desc;
Not sure whether it's supposed to be alphabetically ordered, but using
additional spaces to align the names seems an abnormal approach. I
failed to find any similar sample in kernel. So seeing the driver
doesn't implement the forward declarations as you suggest I'd convert
this to just:
struct stmmac_extra_stats;
struct stmmac_priv;
struct stmmac_safety_stats;
+struct stmmac_fpe_cfg;
+enum stmmac_mpacket_type;
struct dma_desc;
Other than that the patch looks good:
Reviewed-by: Serge Semin <fancer.lancer@...il.com>
Thanks
-Serge(y)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index b23b920eedb1..458d6b16ce21 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -146,6 +146,33 @@ struct stmmac_channel {
> u32 index;
> };
>
> +/* FPE link state */
> +enum stmmac_fpe_state {
> + FPE_STATE_OFF = 0,
> + FPE_STATE_CAPABLE = 1,
> + FPE_STATE_ENTERING_ON = 2,
> + FPE_STATE_ON = 3,
> +};
> +
> +/* FPE link-partner hand-shaking mPacket type */
> +enum stmmac_mpacket_type {
> + MPACKET_VERIFY = 0,
> + MPACKET_RESPONSE = 1,
> +};
> +
> +enum stmmac_fpe_task_state_t {
> + __FPE_REMOVING,
> + __FPE_TASK_SCHED,
> +};
> +
> +struct stmmac_fpe_cfg {
> + bool enable; /* FPE enable */
> + bool hs_enable; /* FPE handshake enable */
> + enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */
> + enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */
> + u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */
> +};
> +
> struct stmmac_tc_entry {
> bool in_use;
> bool in_hw;
> @@ -339,11 +366,12 @@ struct stmmac_priv {
> struct workqueue_struct *wq;
> struct work_struct service_task;
>
> - /* Workqueue for handling FPE hand-shaking */
> + /* Frame Preemption feature (FPE) */
> unsigned long fpe_task_state;
> struct workqueue_struct *fpe_wq;
> struct work_struct fpe_task;
> char wq_name[IFNAMSIZ + 4];
> + struct stmmac_fpe_cfg fpe_cfg;
>
> /* TC Handling */
> unsigned int tc_entries_max;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d9fca8d1227c..529fe31f8b04 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -968,7 +968,7 @@ static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
>
> static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> {
> - struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> + struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
> enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
> bool *hs_enable = &fpe_cfg->hs_enable;
> @@ -3536,7 +3536,7 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
> if (priv->dma_cap.fpesel) {
> stmmac_fpe_start_wq(priv);
>
> - if (priv->plat->fpe_cfg->enable)
> + if (priv->fpe_cfg.enable)
> stmmac_fpe_handshake(priv, true);
> }
>
> @@ -5982,7 +5982,7 @@ static int stmmac_set_features(struct net_device *netdev,
>
> static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
> {
> - struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> + struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
> enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
> bool *hs_enable = &fpe_cfg->hs_enable;
> @@ -7381,7 +7381,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
> {
> struct stmmac_priv *priv = container_of(work, struct stmmac_priv,
> fpe_task);
> - struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> + struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> enum stmmac_fpe_state *lo_state = &fpe_cfg->lo_fpe_state;
> enum stmmac_fpe_state *lp_state = &fpe_cfg->lp_fpe_state;
> bool *hs_enable = &fpe_cfg->hs_enable;
> @@ -7427,17 +7427,17 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
>
> void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable)
> {
> - if (priv->plat->fpe_cfg->hs_enable != enable) {
> + if (priv->fpe_cfg.hs_enable != enable) {
> if (enable) {
> stmmac_fpe_send_mpacket(priv, priv->ioaddr,
> - priv->plat->fpe_cfg,
> + &priv->fpe_cfg,
> MPACKET_VERIFY);
> } else {
> - priv->plat->fpe_cfg->lo_fpe_state = FPE_STATE_OFF;
> - priv->plat->fpe_cfg->lp_fpe_state = FPE_STATE_OFF;
> + priv->fpe_cfg.lo_fpe_state = FPE_STATE_OFF;
> + priv->fpe_cfg.lp_fpe_state = FPE_STATE_OFF;
> }
>
> - priv->plat->fpe_cfg->hs_enable = enable;
> + priv->fpe_cfg.hs_enable = enable;
> }
> }
>
> @@ -7898,7 +7898,7 @@ int stmmac_suspend(struct device *dev)
> if (priv->dma_cap.fpesel) {
> /* Disable FPE */
> stmmac_fpe_configure(priv, priv->ioaddr,
> - priv->plat->fpe_cfg,
> + &priv->fpe_cfg,
> priv->plat->tx_queues_to_use,
> priv->plat->rx_queues_to_use, false);
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 996f2bcd07a2..9cc41ed01882 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -282,16 +282,6 @@ static int tc_init(struct stmmac_priv *priv)
> if (ret)
> return -ENOMEM;
>
> - if (!priv->plat->fpe_cfg) {
> - priv->plat->fpe_cfg = devm_kzalloc(priv->device,
> - sizeof(*priv->plat->fpe_cfg),
> - GFP_KERNEL);
> - if (!priv->plat->fpe_cfg)
> - return -ENOMEM;
> - } else {
> - memset(priv->plat->fpe_cfg, 0, sizeof(*priv->plat->fpe_cfg));
> - }
> -
> /* Fail silently as we can still use remaining features, e.g. CBS */
> if (!dma_cap->frpsel)
> return 0;
> @@ -1076,7 +1066,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> /* Actual FPE register configuration will be done after FPE handshake
> * is success.
> */
> - priv->plat->fpe_cfg->enable = fpe;
> + priv->fpe_cfg.enable = fpe;
>
> ret = stmmac_est_configure(priv, priv, priv->est,
> priv->plat->clk_ptp_rate);
> @@ -1109,9 +1099,9 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> mutex_unlock(&priv->est_lock);
> }
>
> - priv->plat->fpe_cfg->enable = false;
> + priv->fpe_cfg.enable = false;
> stmmac_fpe_configure(priv, priv->ioaddr,
> - priv->plat->fpe_cfg,
> + &priv->fpe_cfg,
> priv->plat->tx_queues_to_use,
> priv->plat->rx_queues_to_use,
> false);
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 338991c08f00..d79ff252cfdc 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -138,33 +138,6 @@ struct stmmac_txq_cfg {
> int tbs_en;
> };
>
> -/* FPE link state */
> -enum stmmac_fpe_state {
> - FPE_STATE_OFF = 0,
> - FPE_STATE_CAPABLE = 1,
> - FPE_STATE_ENTERING_ON = 2,
> - FPE_STATE_ON = 3,
> -};
> -
> -/* FPE link-partner hand-shaking mPacket type */
> -enum stmmac_mpacket_type {
> - MPACKET_VERIFY = 0,
> - MPACKET_RESPONSE = 1,
> -};
> -
> -enum stmmac_fpe_task_state_t {
> - __FPE_REMOVING,
> - __FPE_TASK_SCHED,
> -};
> -
> -struct stmmac_fpe_cfg {
> - bool enable; /* FPE enable */
> - bool hs_enable; /* FPE handshake enable */
> - enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */
> - enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */
> - u32 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */
> -};
> -
> struct stmmac_safety_feature_cfg {
> u32 tsoee;
> u32 mrxpee;
> @@ -232,7 +205,6 @@ struct plat_stmmacenet_data {
> struct fwnode_handle *port_node;
> struct device_node *mdio_node;
> struct stmmac_dma_cfg *dma_cfg;
> - struct stmmac_fpe_cfg *fpe_cfg;
> struct stmmac_safety_feature_cfg *safety_feat_cfg;
> int clk_csr;
> int has_gmac;
> --
> 2.34.1
>
Powered by blists - more mailing lists