| Message ID | 1571045117-26329-3-git-send-email-ioana.ciornei@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | dpaa2-eth: misc fixes | expand |
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
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? > +}
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..
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
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 > >> +} > >
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 >
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) {