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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ