Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
On Mon, Nov 02, 2015 at 07:31:34PM +0200, Madalin Bucur wrote:
> diff --git a/drivers/net/ethernet/freescale/dpaa/Makefile
> b/drivers/net/ethernet/freescale/dpaa/Makefile
> new file mode 100644
> index 000..3847ec7
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/dpaa/Makefile
> @@ -0,0 +1,11 @@
> +#
> +# Makefile for the Freescale DPAA Ethernet controllers
> +#
> +
> +# Include FMan headers
> +FMAN= $(srctree)/drivers/net/ethernet/freescale/fman
> +ccflags-y += -I$(FMAN)
...or just put the two parts of the same driver into the same directory.
> +#define DPA_DESCRIPTION "FSL DPAA Ethernet driver"
Avoiding duplicating those four words is not worth the obfuscation of
moving it somewhere else.
> +static u8 debug = -1;
-1 does not fit in a u8. Usually this is declared as int.
> +module_param(debug, byte, S_IRUGO);
> +MODULE_PARM_DESC(debug, "Module/Driver verbosity level");
It would be good to document the range of values here.
> +
> +/* This has to work in tandem with the DPA_CS_THRESHOLD_xxx values. */
> +static u16 tx_timeout = 1000;
> +module_param(tx_timeout, ushort, S_IRUGO);
> +MODULE_PARM_DESC(tx_timeout, "The Tx timeout in ms");
Could you elaborate on the relationship with DPA_CS_THRESHOLD_xxx?
Or even tell me where I can find such a symbol?
> +
> +/* BM */
BM?
> +#define DPAA_ETH_MAX_PAD (L1_CACHE_BYTES * 8)
This isn't used.
> +static u8 dpa_priv_common_bpid;
> +
> +static void _dpa_rx_error(struct net_device *net_dev,
> + const struct dpa_priv_s *priv,
> + struct dpa_percpu_priv_s *percpu_priv,
> + const struct qm_fd *fd,
> + u32 fqid)
Why the leading underscore? Likewise elsewhere.
> +{
> + /* limit common, possibly innocuous Rx FIFO Overflow errors'
> + * interference with zero-loss convergence benchmark results.
> + */
Spamming the console is bad regardless of whether you're running a
certain benchmark...
> + if (likely(fd->status & FM_FD_ERR_PHYSICAL))
> + pr_warn_once("non-zero error counters in fman statistics
> (sysfs)\n");
> + else
> + if (net_ratelimit())
> + netif_err(priv, hw, net_dev, "Err FD status = 0x%08x\n",
> + fd->status & FM_FD_STAT_RX_ERRORS);
Why are you using likely in error paths?
Why do we need to print this error at all? Do other net drivers do that?
> + percpu_priv->stats.rx_errors++;
> +
> + dpa_fd_release(net_dev, fd);
Can we reserve "fd" for file descriptors and call this something else?
> +}
> +
> +static void _dpa_tx_error(struct net_device *net_dev,
> + const struct dpa_priv_s *priv,
> + struct dpa_percpu_priv_s *percpu_priv,
> + const struct qm_fd *fd,
> + u32 fqid)
> +{
> + struct sk_buff *skb;
> +
> + if (net_ratelimit())
> + netif_warn(priv, hw, net_dev, "FD status = 0x%08x\n",
> +fd->status & FM_FD_STAT_TX_ERRORS);
> +
> + percpu_priv->stats.tx_errors++;
> +
> + /* If we intended the buffers from this frame to go into the bpools
> + * when the FMan transmit was done, we need to put it in manually.
> + */
> + if (fd->bpid != 0xff) {
> + dpa_fd_release(net_dev, fd);
> + return;
> + }
Define a symbolic constant for this special 0xff value.
> +static enum qman_cb_dqrr_result
> +priv_rx_error_dqrr(struct qman_portal *portal,
> +struct qman_fq *fq,
> +const struct qm_dqrr_entry *dq)
> +{
> + struct net_device *net_dev;
> + struct dpa_priv_s *priv;
> + struct dpa_percpu_priv_s *percpu_priv;
> + int *count_ptr;
> +
> + net_dev = ((struct dpa_fq *)fq)->net_dev;
Don't open-code the casting of one struct to another like this. Use
container_of().
> +static enum qman_cb_dqrr_result
> +priv_rx_default_dqrr(struct qman_portal *portal,
> + struct qman_fq *fq,
> + const struct qm_dqrr_entry *dq)
> +{
> + struct net_device *net_dev;
> + struct dpa_priv_s *priv;
> + struct dpa_percpu_priv_s *percpu_priv;
> + int *count_ptr;
> + struct dpa_bp *dpa_bp;
> +
> + net_dev = ((struct dpa_fq *)fq)->net_dev;
> + priv = netdev_priv(net_dev);
> + dpa_bp = priv->dpa_bp;
> +
> + /* IRQ handler, non-migratable; safe to use raw_cpu_ptr here */
> + percpu_priv = raw_cpu_ptr(priv->percpu_priv);
> + count_ptr = raw_cpu_ptr(dpa_bp->percpu_count);
But why do you *need* to use raw?
> +
> + if (unlikely(dpaa_eth_napi_schedule(percpu_priv, portal)))
> + return qman_cb_dqrr_stop;
If this is always an "IRQ handler" then why is it "unlikely" that this
will return non-zero?
> +static const struct dpa_fq_cbs_t private_fq_cbs = {
> + .rx_defq = { .cb = { .dqrr = priv_rx_default_dqrr } },
> + .tx_defq = { .cb = { .dqrr = priv_tx_conf_defau
RE: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
>
> On Tue, 2015-11-03 at 09:37 +, Madalin-Cristian Bucur wrote:
> > > -Original Message-
> > > From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
> > >
> > > On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > > > + if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> > > > + if (net_ratelimit())
> > > > + netif_warn(priv, hw, net_dev, "FD status =
> > > 0x%08x\n",
> > > > + fd_status &
> FM_FD_STAT_RX_ERRORS);
> > > > +
> > > > + percpu_stats->rx_errors++;
> > > > + goto _release_frame;
> > > > + }
> > >
> > > I cannot find any detailed error accounting(maybe I am not looking
> hard
> > > enough) but I
> > > would appreciate if both TX and RX errors where better
> > > accounted(rx_length_errors, rx_frame_errors,
> > > rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in
> the
> > > past diagnosing
> > > board HW problems.
> > >
> > > Jocke
> >
> > Hi Jocke,
> >
> > There are some error counters exported through ethtool (used to be
> debugfs).
> > FMan HW provides more debug information than we currently export, that
> will be
> > improved in the future but given the current priority of having a
> codebase as
> > small and reviewable as possible we had to drop some things from the
> initial
> > submission.
>
> I know, but ethtool is not always available.
> Even the old fec_main.c has it:
> if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO |
> BD_ENET_RX_CR | BD_ENET_RX_OV)) {
> ndev->stats.rx_errors++;
> if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH)) {
> /* Frame too long or too short. */
> ndev->stats.rx_length_errors++;
> }
> if (status & BD_ENET_RX_NO) /* Frame alignment */
> ndev->stats.rx_frame_errors++;
> if (status & BD_ENET_RX_CR) /* CRC Error */
> ndev->stats.rx_crc_errors++;
> if (status & BD_ENET_RX_OV) /* FIFO overrun */
> ndev->stats.rx_fifo_errors++;
> }
> so it is just a few more lines ... Pretty please ? :)
>
> Jocke
It may be more that just a few lines to add complete debug details.
Your request is noted and will be among the first features to work on
after the driver is accepted upstream.
Thanks,
Madalin
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
On Tue, 2015-11-03 at 09:37 +, Madalin-Cristian Bucur wrote:
> > -Original Message-
> > From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
> >
> > On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > > + if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> > > + if (net_ratelimit())
> > > + netif_warn(priv, hw, net_dev, "FD status =
> > 0x%08x\n",
> > > + fd_status & FM_FD_STAT_RX_ERRORS);
> > > +
> > > + percpu_stats->rx_errors++;
> > > + goto _release_frame;
> > > + }
> >
> > I cannot find any detailed error accounting(maybe I am not looking hard
> > enough) but I
> > would appreciate if both TX and RX errors where better
> > accounted(rx_length_errors, rx_frame_errors,
> > rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in the
> > past diagnosing
> > board HW problems.
> >
> > Jocke
>
> Hi Jocke,
>
> There are some error counters exported through ethtool (used to be debugfs).
> FMan HW provides more debug information than we currently export, that will be
> improved in the future but given the current priority of having a codebase as
> small and reviewable as possible we had to drop some things from the initial
> submission.
I know, but ethtool is not always available.
Even the old fec_main.c has it:
if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO |
BD_ENET_RX_CR | BD_ENET_RX_OV)) {
ndev->stats.rx_errors++;
if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH)) {
/* Frame too long or too short. */
ndev->stats.rx_length_errors++;
}
if (status & BD_ENET_RX_NO) /* Frame alignment */
ndev->stats.rx_frame_errors++;
if (status & BD_ENET_RX_CR) /* CRC Error */
ndev->stats.rx_crc_errors++;
if (status & BD_ENET_RX_OV) /* FIFO overrun */
ndev->stats.rx_fifo_errors++;
}
so it is just a few more lines ... Pretty please ? :)
Jocke
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
> -Original Message-
> From: Joakim Tjernlund [mailto:joakim.tjernl...@transmode.se]
>
> On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> > + if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> > + if (net_ratelimit())
> > + netif_warn(priv, hw, net_dev, "FD status =
> 0x%08x\n",
> > + fd_status & FM_FD_STAT_RX_ERRORS);
> > +
> > + percpu_stats->rx_errors++;
> > + goto _release_frame;
> > + }
>
> I cannot find any detailed error accounting(maybe I am not looking hard
> enough) but I
> would appreciate if both TX and RX errors where better
> accounted(rx_length_errors, rx_frame_errors,
> rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in the
> past diagnosing
> board HW problems.
>
> Jocke
Hi Jocke,
There are some error counters exported through ethtool (used to be debugfs).
FMan HW provides more debug information than we currently export, that will be
improved in the future but given the current priority of having a codebase as
small and reviewable as possible we had to drop some things from the initial
submission.
Madalin
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> + if (unlikely(fd_status & FM_FD_STAT_RX_ERRORS) != 0) {
> + if (net_ratelimit())
> + netif_warn(priv, hw, net_dev, "FD status = 0x%08x\n",
> + fd_status & FM_FD_STAT_RX_ERRORS);
> +
> + percpu_stats->rx_errors++;
> + goto _release_frame;
> + }
I cannot find any detailed error accounting(maybe I am not looking hard enough)
but I
would appreciate if both TX and RX errors where better
accounted(rx_length_errors, rx_frame_errors,
rx_crc_errors, rx_fifo_errors etc.). This has helped me many times in the past
diagnosing
board HW problems.
Jocke--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> This introduces the Freescale Data Path Acceleration Architecture
> (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> the Freescale DPAA QorIQ platforms.
[]
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
[]
> +static void _dpa_rx_error(struct net_device *net_dev,
> + const struct dpa_priv_s *priv,
> + struct dpa_percpu_priv_s *percpu_priv,
> + const struct qm_fd *fd,
> + u32 fqid)
> +{
> + /* limit common, possibly innocuous Rx FIFO Overflow errors'
> + * interference with zero-loss convergence benchmark results.
> + */
> + if (likely(fd->status & FM_FD_ERR_PHYSICAL))
> + pr_warn_once("non-zero error counters in fman statistics
> (sysfs)\n");
> + else
> + if (net_ratelimit())
> + netif_err(priv, hw, net_dev, "Err FD status = 0x%08x\n",
> + fd->status & FM_FD_STAT_RX_ERRORS);
It's a bit of a pity the logging message code is
a mix of pr_, dev_, netdev_
and netif_
Perhaps netif__ratelimited macros should be added.
Something like:
---
include/linux/netdevice.h | 54 +++
1 file changed, 54 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 210d11a..555471d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4025,6 +4025,60 @@ do {
\
})
#endif
+#define netif_level_ratelimited(level, priv, type, dev, fmt, args...) \
+do { \
+ if (netif_msg_##type(priv) && net_ratelimit()) \
+ netdev_##level(dev, fmt, ##args); \
+} while (0)
+
+#define netif_emerg_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(emerg, priv, type, dev, fmt, ##args)
+#define netif_alert_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(alert, priv, type, dev, fmt, ##args)
+#define netif_crit_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(crit, priv, type, dev, fmt, ##args)
+#define netif_err_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(err, priv, type, dev, fmt, ##args)
+#define netif_warn_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(warn, priv, type, dev, fmt, ##args)
+#define netif_notice_ratelimited(priv, type, dev, fmt, args...)
\
+ netif_level_ratelimited(notice, priv, type, dev, fmt, ##args)
+#define netif_info_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(info, priv, type, dev, fmt, ##args)
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+/* descriptor check is first to prevent flooding with "callbacks suppressed" */
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
+ if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) &&\
+ netif_msg_##type(priv) && net_ratelimit()) \
+ __dynamic_netdev_dbg(&descriptor, dev, fmt, ##args);\
+} while (0)
+#elif defined(DEBUG)
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ if (netif_msg_##type(priv) && net_ratelimit()) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#else
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ if (0) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
+#if defined(VERBOSE_DEBUG)
+#define netif_vdbg_ratelimited netif_dbg_ratelimited
+#else
+#define netif_vdbg(priv, type, dev, fmt, args...) \
+do { \
+ if (0) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
/*
* The list of packet types we will receive (as opposed to discard)
* and the routines to invoke.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
