[PATCH v2 net 0/2] dpaa2-eth: misc fixes

[PATCH v2 net 0/2] dpaa2-eth: misc fixes

[Ioana Ciornei]

This patch set adds a couple of fixes around updating configuration on MAC
change.  Depending on when MC connects the DPNI to a MAC, both the MAC
address and TX FQIDs should be updated everytime there is a change in
configuration.

Changes in v2:
 - used reverse christmas tree ordering in patch 2/2

Florin Chiculita (1):
  dpaa2-eth: add irq for the dpmac connect/disconnect event

Ioana Radulescu (1):
  dpaa2-eth: Fix TX FQID values

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 48 +++++++++++++++++++++++-
 drivers/net/ethernet/freescale/dpaa2/dpni.h      |  5 ++-
 2 files changed, 51 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH v2 net 1/2] dpaa2-eth: add irq for the dpmac connect/disconnect event

[Ioana Ciornei]

From: Florin Chiculita <florinlaurentiu.chiculita@nxp.com>

Add IRQ for the DPNI endpoint change event, resolving the issue
when a dynamically created DPNI gets a randomly generated hw address
when the endpoint is a DPMAC object.

Signed-off-by: Florin Chiculita <florinlaurentiu.chiculita@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - none

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 6 +++++-
 drivers/net/ethernet/freescale/dpaa2/dpni.h      | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 162d7d8fb295..5acd734a216b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -3306,6 +3306,9 @@ static irqreturn_t dpni_irq0_handler_thread(int irq_num, void *arg)
 	if (status & DPNI_IRQ_EVENT_LINK_CHANGED)
 		link_state_update(netdev_priv(net_dev));
 
+	if (status & DPNI_IRQ_EVENT_ENDPOINT_CHANGED)
+		set_mac_addr(netdev_priv(net_dev));
+
 	return IRQ_HANDLED;
 }
 
@@ -3331,7 +3334,8 @@ static int setup_irqs(struct fsl_mc_device *ls_dev)
 	}
 
 	err = dpni_set_irq_mask(ls_dev->mc_io, 0, ls_dev->mc_handle,
-				DPNI_IRQ_INDEX, DPNI_IRQ_EVENT_LINK_CHANGED);
+				DPNI_IRQ_INDEX, DPNI_IRQ_EVENT_LINK_CHANGED |
+				DPNI_IRQ_EVENT_ENDPOINT_CHANGED);
 	if (err < 0) {
 		dev_err(&ls_dev->dev, "dpni_set_irq_mask(): %d\n", err);
 		goto free_irq;
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni.h b/drivers/net/ethernet/freescale/dpaa2/dpni.h
index fd583911b6c0..ee0711d06b3a 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni.h
@@ -133,9 +133,12 @@ int dpni_reset(struct fsl_mc_io	*mc_io,
  */
 #define DPNI_IRQ_INDEX				0
 /**
- * IRQ event - indicates a change in link state
+ * IRQ events:
+ *       indicates a change in link state
+ *       indicates a change in endpoint
  */
 #define DPNI_IRQ_EVENT_LINK_CHANGED		0x00000001
+#define DPNI_IRQ_EVENT_ENDPOINT_CHANGED		0x00000002
 
 int dpni_set_irq_enable(struct fsl_mc_io	*mc_io,
 			u32			cmd_flags,
-- 
1.9.1

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

[Ioana Ciornei]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Depending on when MC connects the DPNI to a MAC, Tx FQIDs may
not be available during probe time.

Read the FQIDs each time the link goes up to avoid using invalid
values. In case an error occurs or an invalid value is retrieved,
fall back to QDID-based enqueueing.

Fixes: 1fa0f68c9255 ("dpaa2-eth: Use FQ-based DPIO enqueue API")
Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - used reverse christmas tree ordering in update_tx_fqids

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 42 ++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

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);
+
 static int link_state_update(struct dpaa2_eth_priv *priv)
 {
 	struct dpni_link_state state = {0};
@@ -1261,6 +1263,7 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
 		goto out;
 
 	if (state.up) {
+		update_tx_fqids(priv);
 		netif_carrier_on(priv->net_dev);
 		netif_tx_start_all_queues(priv->net_dev);
 	} else {
@@ -2533,6 +2536,45 @@ static int set_pause(struct dpaa2_eth_priv *priv)
 	return 0;
 }
 
+static void update_tx_fqids(struct dpaa2_eth_priv *priv)
+{
+	struct dpni_queue_id qid = {0};
+	struct dpaa2_eth_fq *fq;
+	struct dpni_queue queue;
+	int i, j, err;
+
+	/* We only use Tx FQIDs for FQID-based enqueue, so check
+	 * if DPNI version supports it before updating FQIDs
+	 */
+	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_ENQUEUE_FQID_VER_MAJOR,
+				   DPNI_ENQUEUE_FQID_VER_MINOR) < 0)
+		return;
+
+	for (i = 0; i < priv->num_fqs; i++) {
+		fq = &priv->fq[i];
+		if (fq->type != DPAA2_TX_CONF_FQ)
+			continue;
+		for (j = 0; j < dpaa2_eth_tc_count(priv); j++) {
+			err = dpni_get_queue(priv->mc_io, 0, priv->mc_token,
+					     DPNI_QUEUE_TX, j, fq->flowid,
+					     &queue, &qid);
+			if (err)
+				goto out_err;
+
+			fq->tx_fqid[j] = qid.fqid;
+			if (fq->tx_fqid[j] == 0)
+				goto out_err;
+		}
+	}
+
+	return;
+
+out_err:
+	netdev_info(priv->net_dev,
+		    "Error reading Tx FQID, fallback to QDID-based enqueue");
+	priv->enqueue = dpaa2_eth_enqueue_qd;
+}
+
 /* Configure the DPNI object this interface is associated with */
 static int setup_dpni(struct fsl_mc_device *ls_dev)
 {
-- 
1.9.1

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

[Andrew Lunn]

On Mon, Oct 14, 2019 at 12:25:17PM +0300, Ioana Ciornei wrote:
> From: Ioana Radulescu <ruxandra.radulescu@nxp.com>
> 
> Depending on when MC connects the DPNI to a MAC, Tx FQIDs may
> not be available during probe time.
> 
> Read the FQIDs each time the link goes up to avoid using invalid
> values. In case an error occurs or an invalid value is retrieved,
> fall back to QDID-based enqueueing.
> 
> Fixes: 1fa0f68c9255 ("dpaa2-eth: Use FQ-based DPIO enqueue API")
> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
>  - used reverse christmas tree ordering in update_tx_fqids
> 
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 42 ++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> 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?

Thanks
	Andrew

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

[Jakub Kicinski]

On Mon, 14 Oct 2019 12:25:17 +0300, Ioana Ciornei wrote:
> From: Ioana Radulescu <ruxandra.radulescu@nxp.com>
> 
> Depending on when MC connects the DPNI to a MAC, Tx FQIDs may
> not be available during probe time.
> 
> Read the FQIDs each time the link goes up to avoid using invalid
> values. In case an error occurs or an invalid value is retrieved,
> fall back to QDID-based enqueueing.
> 
> Fixes: 1fa0f68c9255 ("dpaa2-eth: Use FQ-based DPIO enqueue API")
> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
>  - used reverse christmas tree ordering in update_tx_fqids
> 
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 42 ++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> 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);
> 
>  static int link_state_update(struct dpaa2_eth_priv *priv)
>  {
>  	struct dpni_link_state state = {0};
> @@ -1261,6 +1263,7 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
>  		goto out;
>  
>  	if (state.up) {
> +		update_tx_fqids(priv);
>  		netif_carrier_on(priv->net_dev);
>  		netif_tx_start_all_queues(priv->net_dev);
>  	} else {
> @@ -2533,6 +2536,45 @@ static int set_pause(struct dpaa2_eth_priv *priv)
>  	return 0;
>  }
>  
> +static void update_tx_fqids(struct dpaa2_eth_priv *priv)
> +{
> +	struct dpni_queue_id qid = {0};
> +	struct dpaa2_eth_fq *fq;
> +	struct dpni_queue queue;
> +	int i, j, err;
> +
> +	/* We only use Tx FQIDs for FQID-based enqueue, so check
> +	 * if DPNI version supports it before updating FQIDs
> +	 */
> +	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_ENQUEUE_FQID_VER_MAJOR,
> +				   DPNI_ENQUEUE_FQID_VER_MINOR) < 0)
> +		return;
> +
> +	for (i = 0; i < priv->num_fqs; i++) {
> +		fq = &priv->fq[i];
> +		if (fq->type != DPAA2_TX_CONF_FQ)
> +			continue;
> +		for (j = 0; j < dpaa2_eth_tc_count(priv); j++) {
> +			err = dpni_get_queue(priv->mc_io, 0, priv->mc_token,
> +					     DPNI_QUEUE_TX, j, fq->flowid,
> +					     &queue, &qid);
> +			if (err)
> +				goto out_err;
> +
> +			fq->tx_fqid[j] = qid.fqid;
> +			if (fq->tx_fqid[j] == 0)
> +				goto out_err;
> +		}
> +	}
> +
> +	return;
> +
> +out_err:
> +	netdev_info(priv->net_dev,
> +		    "Error reading Tx FQID, fallback to QDID-based enqueue");

Missing new line at the end of this message, I think.

> +	priv->enqueue = dpaa2_eth_enqueue_qd;

Should the enqueue be set to dpaa2_eth_enqueue_fq config was successful?
IOW if there is a transient error we should go back to the preferred
method?

> +}

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

[Jakub Kicinski]

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

But your point obviously stands, and in the future the code should be
structured more carefully :(


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..

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

[Andrew Lunn]

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

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

[Ioana Ciornei]

On 10/15/19 10:36 PM, Jakub Kicinski wrote:
> On Mon, 14 Oct 2019 12:25:17 +0300, Ioana Ciornei wrote:
>> From: Ioana Radulescu <ruxandra.radulescu@nxp.com>
>>
>> Depending on when MC connects the DPNI to a MAC, Tx FQIDs may
>> not be available during probe time.
>>
>> Read the FQIDs each time the link goes up to avoid using invalid
>> values. In case an error occurs or an invalid value is retrieved,
>> fall back to QDID-based enqueueing.
>>
>> Fixes: 1fa0f68c9255 ("dpaa2-eth: Use FQ-based DPIO enqueue API")
>> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> ---
>> Changes in v2:
>>   - used reverse christmas tree ordering in update_tx_fqids
>>
>>   drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 42 ++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> 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);
>>
>>   static int link_state_update(struct dpaa2_eth_priv *priv)
>>   {
>>   	struct dpni_link_state state = {0};
>> @@ -1261,6 +1263,7 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
>>   		goto out;
>>   
>>   	if (state.up) {
>> +		update_tx_fqids(priv);
>>   		netif_carrier_on(priv->net_dev);
>>   		netif_tx_start_all_queues(priv->net_dev);
>>   	} else {
>> @@ -2533,6 +2536,45 @@ static int set_pause(struct dpaa2_eth_priv *priv)
>>   	return 0;
>>   }
>>   
>> +static void update_tx_fqids(struct dpaa2_eth_priv *priv)
>> +{
>> +	struct dpni_queue_id qid = {0};
>> +	struct dpaa2_eth_fq *fq;
>> +	struct dpni_queue queue;
>> +	int i, j, err;
>> +
>> +	/* We only use Tx FQIDs for FQID-based enqueue, so check
>> +	 * if DPNI version supports it before updating FQIDs
>> +	 */
>> +	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_ENQUEUE_FQID_VER_MAJOR,
>> +				   DPNI_ENQUEUE_FQID_VER_MINOR) < 0)
>> +		return;
>> +
>> +	for (i = 0; i < priv->num_fqs; i++) {
>> +		fq = &priv->fq[i];
>> +		if (fq->type != DPAA2_TX_CONF_FQ)
>> +			continue;
>> +		for (j = 0; j < dpaa2_eth_tc_count(priv); j++) {
>> +			err = dpni_get_queue(priv->mc_io, 0, priv->mc_token,
>> +					     DPNI_QUEUE_TX, j, fq->flowid,
>> +					     &queue, &qid);
>> +			if (err)
>> +				goto out_err;
>> +
>> +			fq->tx_fqid[j] = qid.fqid;
>> +			if (fq->tx_fqid[j] == 0)
>> +				goto out_err;
>> +		}
>> +	}
>> +
>> +	return;
>> +
>> +out_err:
>> +	netdev_info(priv->net_dev,
>> +		    "Error reading Tx FQID, fallback to QDID-based enqueue");
> 
> Missing new line at the end of this message, I think.

Will add in v3.

> 
>> +	priv->enqueue = dpaa2_eth_enqueue_qd;
> 
> Should the enqueue be set to dpaa2_eth_enqueue_fq config was successful?
> IOW if there is a transient error we should go back to the preferred
> metho
Good point. Without setting enqueue back to dpaa2_eth_enqueue_fq 
querying the fqids would be for nothing. Will send an update fixing both 
of these things.

Thanks,
Ioana

> 
>> +}
> 
> 

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

[Ioana Ciornei]

On 10/15/19 10:47 PM, Andrew Lunn wrote:
> 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.


Yes, I'll cleanup this in net-next.

Also, no more Ioana's for a while, just me :)

> 
>> 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..

I think that's another change for net-next.

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

Yes, the priv->enqueue is used on the Tx path to enqueue a skb 
(transformed into a frame descriptor) into a queue.
I need to benchmark if transforming the callback into an if-else 
statement is helping.

Ioana C

> 	Andrew
>