Re: [PATCH v2 net 2/2] dpaa2-eth: Fix TX FQID values

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Oct 15, 2019 at 12:40:17PM -0700, Jakub Kicinski wrote:
> On Tue, 15 Oct 2019 21:29:23 +0200, Andrew Lunn wrote:
> > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > > index 5acd734a216b..c3c2c06195ae 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > > @@ -1235,6 +1235,8 @@ static void dpaa2_eth_set_rx_taildrop(struct dpaa2_eth_priv *priv, bool enable)
> > >  	priv->rx_td_enabled = enable;
> > >  }
> > >  
> > > +static void update_tx_fqids(struct dpaa2_eth_priv *priv);
> > > +  
> > 
> > Hi Ioana and Ioana
> > 
> > Using forward declarations is generally not liked. Is there something
> > which is preventing you from having it earlier in the file?
> 
> Ha! I was just about to ask the same question 😊
> 
> +out_err:
> +	netdev_info(priv->net_dev,
> +		    "Error reading Tx FQID, fallback to QDID-based enqueue");
> +	priv->enqueue = dpaa2_eth_enqueue_qd;
> +}
> 
> Here dpaa2_eth_enqueue_qd is a function pointer which is is defined
> towards the end of the file :S

Hi Jakub

Thanks, i was too lazy to look.

So this is O.K. for net, since fixes should be minimal.

Ioana's please could you submit a patch to net-next, once this has
been merged, to move the code around to remove the forward
declarations.

> Also can I point out that this:
> 
> static inline int dpaa2_eth_enqueue_qd(struct dpaa2_eth_priv *priv,             
>                                        struct dpaa2_eth_fq *fq,                 
>                                        struct dpaa2_fd *fd, u8 prio)            
> {                                                                               
>         return dpaa2_io_service_enqueue_qd(fq->channel->dpio,                   
>                                            priv->tx_qdid, prio,                 
>                                            fq->tx_qdbin, fd);                   
> }                                                                               
>                                                                                 
> static inline int dpaa2_eth_enqueue_fq(struct dpaa2_eth_priv *priv,             
>                                        struct dpaa2_eth_fq *fq,                 
>                                        struct dpaa2_fd *fd, u8 prio)            
> {                                                                               
>         return dpaa2_io_service_enqueue_fq(fq->channel->dpio,                   
>                                            fq->tx_fqid[prio], fd);              
> }                                                                               
>                                                                                 
> static void set_enqueue_mode(struct dpaa2_eth_priv *priv)                       
> {                                                                               
>         if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_ENQUEUE_FQID_VER_MAJOR,           
>                                    DPNI_ENQUEUE_FQID_VER_MINOR) < 0)            
>                 priv->enqueue = dpaa2_eth_enqueue_qd;                           
>         else                                                                    
>                 priv->enqueue = dpaa2_eth_enqueue_fq; 
> }
> 
> Could be the most pointless use of the inline keyword possible :)
> Both dpaa2_eth_enqueue_qd() and dpaa2_eth_enqueue_fq() are only ever
> called via a pointer..

Is priv->enqueue used on the hotpath? SPECTRA/Meltdown etc make that
expensive.

	Andrew