| Message ID | 20200618064029.32168-7-kurt@linutronix.de |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Hirschmann Hellcreek DSA driver | expand |
On Thu, Jun 18, 2020 at 08:40:26AM +0200, Kurt Kanzenbach wrote: > The switch has registers which are useful for debugging issues: debugfs is not particularly likes. Please try to find other means where possible. Memory usage fits nicely into devlink. See mv88e6xxx which exports the ATU fill for example. Are trace registers counters? > +static int hellcreek_debugfs_init(struct hellcreek *hellcreek) > +{ > + struct dentry *file; > + > + hellcreek->debug_dir = debugfs_create_dir(dev_name(hellcreek->dev), > + NULL); > + if (!hellcreek->debug_dir) > + return -ENOMEM; Just a general comment. You should not check the return value from any debugfs call, since it is totally optional. It will also do the right thing if the previous call has failed. There are numerous emails from GregKH about this. Andrew
Hi Andrew, On Thu Jun 18 2020, Andrew Lunn wrote: > On Thu, Jun 18, 2020 at 08:40:26AM +0200, Kurt Kanzenbach wrote: >> The switch has registers which are useful for debugging issues: > > debugfs is not particularly likes. Please try to find other means > where possible. Memory usage fits nicely into devlink. See mv88e6xxx > which exports the ATU fill for example. OK, I'll have a look at devlink and the mv88e6xxx driver to see if that could be utilized. > Are trace registers counters? No. The trace registers provide bits for error conditions and if packets have been dropped e.g. because of full queues or FCS errors, and so on. > >> +static int hellcreek_debugfs_init(struct hellcreek *hellcreek) >> +{ >> + struct dentry *file; >> + >> + hellcreek->debug_dir = debugfs_create_dir(dev_name(hellcreek->dev), >> + NULL); >> + if (!hellcreek->debug_dir) >> + return -ENOMEM; > > Just a general comment. You should not check the return value from any > debugfs call, since it is totally optional. It will also do the right > thing if the previous call has failed. There are numerous emails from > GregKH about this. OK. Thanks, Kurt
> > Are trace registers counters? > > No. The trace registers provide bits for error conditions and if packets > have been dropped e.g. because of full queues or FCS errors, and so on. Is there some documentation somewhere? A better understanding of what they can do might help figure out the correct API. Andrew
On Fri Jun 19 2020, Andrew Lunn wrote: >> > Are trace registers counters? >> >> No. The trace registers provide bits for error conditions and if packets >> have been dropped e.g. because of full queues or FCS errors, and so on. > > Is there some documentation somewhere? A better understanding of what > they can do might help figure out the correct API. No, not that I'm aware of. Actually there are a few more debugging mechanisms and features which should be exposed somehow. Here's the list: * Trace registers for the error conditions. This feature needs to be configured for which ports should be traced * Memory registers for indicating how many free page and meta pointers are available (read-only) * Limit registers for configuring: * Maximum memory limit per port * Reserved memory for critical traffic * Background traffic rate * Maximum queue depth * Re-prioritization of packets based on the ether type (not mac address) * Packet logging (-> retrieval of packet time stamps) based on port, traffic class and direction * Queue tracking What API would be useful for these mechanisms? Thanks, Kurt
On Mon, Jun 22, 2020 at 02:32:28PM +0200, Kurt Kanzenbach wrote: > On Fri Jun 19 2020, Andrew Lunn wrote: > >> > Are trace registers counters? > >> > >> No. The trace registers provide bits for error conditions and if packets > >> have been dropped e.g. because of full queues or FCS errors, and so on. > > > > Is there some documentation somewhere? A better understanding of what > > they can do might help figure out the correct API. > > No, not that I'm aware of. > > Actually there are a few more debugging mechanisms and features which > should be exposed somehow. Here's the list: > > * Trace registers for the error conditions. This feature needs to be > configured for which ports should be traced > * Memory registers for indicating how many free page and meta pointers > are available (read-only) > * Limit registers for configuring: > * Maximum memory limit per port > * Reserved memory for critical traffic > * Background traffic rate > * Maximum queue depth > * Re-prioritization of packets based on the ether type (not mac address) > * Packet logging (-> retrieval of packet time stamps) based on port, traffic class and direction > * Queue tracking > > What API would be useful for these mechanisms? Hi Kurt You should take a look at devlink. Many of these fit devlink resources. Use that where it fits. But you might end up with an out of tree debugfs patch for your own debugging work. We have something similar of mv88e6xxx. Andrew
Hi Kurt, On Mon, 22 Jun 2020 at 15:34, Kurt Kanzenbach <kurt@linutronix.de> wrote: > > * Re-prioritization of packets based on the ether type (not mac address) This can be done by offloading a tc skbedit priority action, and a "protocol" key (even though I don't understand why you need to mention "not mac address". > * Packet logging (-> retrieval of packet time stamps) based on port, traffic class and direction What does this mean? tcpdump can give you this, for traffic destined to the CPU. Do you want to mirror/sample traffic to the CPU for debug? > > What API would be useful for these mechanisms? > > Thanks, > Kurt Thanks, -Vladimir
Hi Vladimir, On Mon Jun 22 2020, Vladimir Oltean wrote: > Hi Kurt, > > On Mon, 22 Jun 2020 at 15:34, Kurt Kanzenbach <kurt@linutronix.de> wrote: >> >> * Re-prioritization of packets based on the ether type (not mac address) > > This can be done by offloading a tc skbedit priority action, and a > "protocol" key (even though I don't understand why you need to mention > "not mac address". Thanks. That seems like it can be used. I did mention the mac address, because the switch has two ways of doing a re-prioritization either by fdb entries via mac addresses or by the high level inspection (HLI) based on the ether type. > >> * Packet logging (-> retrieval of packet time stamps) based on port, traffic class and direction > > What does this mean? tcpdump can give you this, for traffic destined > to the CPU. Do you want to mirror/sample traffic to the CPU for debug? > The switch can capture timestamps (nanoseconds) when packets have been transmitted or received on all ports. The timestamps are stored in a FIFO and can be retrieved later. As you said tcpdump only works for packets at the CPU port. This feature is useful for debugging latency issues for specific traffic flows. Thanks, Kurt
On Mon Jun 22 2020, Andrew Lunn wrote: > On Mon, Jun 22, 2020 at 02:32:28PM +0200, Kurt Kanzenbach wrote: >> On Fri Jun 19 2020, Andrew Lunn wrote: >> >> > Are trace registers counters? >> >> >> >> No. The trace registers provide bits for error conditions and if packets >> >> have been dropped e.g. because of full queues or FCS errors, and so on. >> > >> > Is there some documentation somewhere? A better understanding of what >> > they can do might help figure out the correct API. >> >> No, not that I'm aware of. >> >> Actually there are a few more debugging mechanisms and features which >> should be exposed somehow. Here's the list: >> >> * Trace registers for the error conditions. This feature needs to be >> configured for which ports should be traced >> * Memory registers for indicating how many free page and meta pointers >> are available (read-only) >> * Limit registers for configuring: >> * Maximum memory limit per port >> * Reserved memory for critical traffic >> * Background traffic rate >> * Maximum queue depth >> * Re-prioritization of packets based on the ether type (not mac address) >> * Packet logging (-> retrieval of packet time stamps) based on port, traffic class and direction >> * Queue tracking >> >> What API would be useful for these mechanisms? > > Hi Kurt > > You should take a look at devlink. Many of these fit devlink > resources. Use that where it fits. But you might end up with an out of > tree debugfs patch for your own debugging work. We have something > similar of mv88e6xxx. I see. Maybe I'll keep the debug stuff out of tree for now and will come back to it later. Thanks, Kurt
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c index 7e678b298f99..a56df65ae486 100644 --- a/drivers/net/dsa/hirschmann/hellcreek.c +++ b/drivers/net/dsa/hirschmann/hellcreek.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/ktime.h> #include <linux/time.h> +#include <linux/debugfs.h> #include <net/dsa.h> #include <net/pkt_sched.h> @@ -1371,6 +1372,195 @@ static int hellcreek_port_setup_tc(struct dsa_switch *ds, int port, return hellcreek_port_del_schedule(ds, port); } +static ssize_t hellcreek_dbg_swtrc_cfg_write(struct file *filp, + const char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + int ret; + u16 reg; + + ret = kstrtou16_from_user(buffer, count, 16, ®); + if (ret) + return ret; + + hellcreek_write(hellcreek, reg, HR_SWTRC_CFG); + + return count; +} + +static ssize_t hellcreek_dbg_swtrc0_write(struct file *filp, + const char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + int ret; + u16 reg; + + ret = kstrtou16_from_user(buffer, count, 16, ®); + if (ret) + return ret; + + hellcreek_write(hellcreek, reg, HR_SWTRC0); + + return count; +} + +static ssize_t hellcreek_dbg_swtrc1_write(struct file *filp, + const char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + int ret; + u16 reg; + + ret = kstrtou16_from_user(buffer, count, 16, ®); + if (ret) + return ret; + + hellcreek_write(hellcreek, reg, HR_SWTRC1); + + return count; +} + +static ssize_t hellcreek_dbg_swtrc0_read(struct file *filp, + char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + char buf[32]; + ssize_t desc = 0; + u16 reg; + + reg = hellcreek_read(hellcreek, HR_SWTRC0); + + desc += snprintf(buf, sizeof(buf), "0x%04x\n", reg); + + return simple_read_from_buffer(buffer, count, ppos, buf, desc); +} + +static ssize_t hellcreek_dbg_swtrc1_read(struct file *filp, + char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + char buf[32]; + ssize_t desc = 0; + u16 reg; + + reg = hellcreek_read(hellcreek, HR_SWTRC1); + + desc += snprintf(buf, sizeof(buf), "0x%04x\n", reg); + + return simple_read_from_buffer(buffer, count, ppos, buf, desc); +} + +static ssize_t hellcreek_dbg_mfree_read(struct file *filp, + char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + char buf[32]; + ssize_t desc = 0; + u16 reg; + + reg = hellcreek_read(hellcreek, HR_MFREE); + + desc += snprintf(buf, sizeof(buf), "0x%04x\n", reg); + + return simple_read_from_buffer(buffer, count, ppos, buf, desc); +} + +static ssize_t hellcreek_dbg_pfree_read(struct file *filp, + char __user *buffer, + size_t count, loff_t *ppos) +{ + struct hellcreek *hellcreek = filp->private_data; + char buf[32]; + ssize_t desc = 0; + u16 reg; + + reg = hellcreek_read(hellcreek, HR_PFREE); + + desc += snprintf(buf, sizeof(buf), "0x%04x\n", reg); + + return simple_read_from_buffer(buffer, count, ppos, buf, desc); +} + +static const struct file_operations hellcreek_dbg_swtrc_cfg_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .write = hellcreek_dbg_swtrc_cfg_write, +}; + +static const struct file_operations hellcreek_dbg_swtrc0_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .read = hellcreek_dbg_swtrc0_read, + .write = hellcreek_dbg_swtrc0_write, +}; + +static const struct file_operations hellcreek_dbg_swtrc1_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .read = hellcreek_dbg_swtrc1_read, + .write = hellcreek_dbg_swtrc1_write, +}; + +static const struct file_operations hellcreek_dbg_pfree_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .read = hellcreek_dbg_pfree_read, +}; + +static const struct file_operations hellcreek_dbg_mfree_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .read = hellcreek_dbg_mfree_read, +}; + +static int hellcreek_debugfs_init(struct hellcreek *hellcreek) +{ + struct dentry *file; + + hellcreek->debug_dir = debugfs_create_dir(dev_name(hellcreek->dev), + NULL); + if (!hellcreek->debug_dir) + return -ENOMEM; + + file = debugfs_create_file("swtrc_cfg", 0200, hellcreek->debug_dir, + hellcreek, &hellcreek_dbg_swtrc_cfg_fops); + if (!file) + return -ENOMEM; + + file = debugfs_create_file("swtrc0", 0600, hellcreek->debug_dir, + hellcreek, &hellcreek_dbg_swtrc0_fops); + if (!file) + return -ENOMEM; + + file = debugfs_create_file("swtrc1", 0600, hellcreek->debug_dir, + hellcreek, &hellcreek_dbg_swtrc1_fops); + if (!file) + return -ENOMEM; + + file = debugfs_create_file("pfree", 0400, hellcreek->debug_dir, + hellcreek, &hellcreek_dbg_pfree_fops); + if (!file) + return -ENOMEM; + + file = debugfs_create_file("mfree", 0400, hellcreek->debug_dir, + hellcreek, &hellcreek_dbg_mfree_fops); + if (!file) + return -ENOMEM; + + return 0; +} + +static void hellcreek_debugfs_exit(struct hellcreek *hellcreek) +{ + debugfs_remove_recursive(hellcreek->debug_dir); +} + static const struct dsa_switch_ops hellcreek_ds_ops = { .get_tag_protocol = hellcreek_get_tag_protocol, .setup = hellcreek_setup, @@ -1475,9 +1665,17 @@ static int hellcreek_probe(struct platform_device *pdev) hellcreek_feature_detect(hellcreek); + ret = hellcreek_debugfs_init(hellcreek); + if (ret) { + dev_err(dev, "Failed to initialize debugfs!\n"); + goto err_debugfs; + } + hellcreek->ds = devm_kzalloc(dev, sizeof(*hellcreek->ds), GFP_KERNEL); - if (!hellcreek->ds) - return -ENOMEM; + if (!hellcreek->ds) { + ret = -ENOMEM; + goto err_debugfs; + } hellcreek->ds->dev = dev; hellcreek->ds->priv = hellcreek; @@ -1488,7 +1686,7 @@ static int hellcreek_probe(struct platform_device *pdev) ret = dsa_register_switch(hellcreek->ds); if (ret) { dev_err(dev, "Unable to register switch\n"); - return ret; + goto err_debugfs; } ret = hellcreek_ptp_setup(hellcreek); @@ -1511,6 +1709,8 @@ static int hellcreek_probe(struct platform_device *pdev) hellcreek_ptp_free(hellcreek); err_ptp_setup: dsa_unregister_switch(hellcreek->ds); +err_debugfs: + hellcreek_debugfs_exit(hellcreek); return ret; } @@ -1519,6 +1719,7 @@ static int hellcreek_remove(struct platform_device *pdev) { struct hellcreek *hellcreek = platform_get_drvdata(pdev); + hellcreek_debugfs_exit(hellcreek); hellcreek_hwtstamp_free(hellcreek); hellcreek_ptp_free(hellcreek); dsa_unregister_switch(hellcreek->ds); diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h index d3d1a1144857..59cc7b59ff2c 100644 --- a/drivers/net/dsa/hirschmann/hellcreek.h +++ b/drivers/net/dsa/hirschmann/hellcreek.h @@ -280,6 +280,7 @@ struct hellcreek { struct ptp_clock_info ptp_clock_info; struct hellcreek_port ports[4]; struct delayed_work overflow_work; + struct dentry *debug_dir; spinlock_t reg_lock; /* Switch IP register lock */ spinlock_t ptp_lock; /* PTP IP register lock */ void __iomem *base;
The switch has registers which are useful for debugging issues: * Trace registers This can be helpful to trace why packets have been filtered or dropped or if there any other serious problems. * Memory registers These registers provide the current switch internal RAM utilization. Especially a unexpected workload with an not appropriate queue setup packets might be dropped due to memory exhaustion. Expose that registers via debugfs. Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- drivers/net/dsa/hirschmann/hellcreek.c | 207 ++++++++++++++++++++++++- drivers/net/dsa/hirschmann/hellcreek.h | 1 + 2 files changed, 205 insertions(+), 3 deletions(-)