| Message ID | 20191112212200.5572-1-olteanv@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: dsa: sja1105: Print the reset reason | expand |
On 11/12/19 1:22 PM, Vladimir Oltean wrote: > Sometimes it can be quite opaque even for me why the driver decided to > reset the switch. So instead of adding dump_stack() calls each time for > debugging, just add a reset reason to sja1105_static_config_reload > calls which gets printed to the console. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
From: Vladimir Oltean <olteanv@gmail.com> Date: Tue, 12 Nov 2019 23:22:00 +0200 > Sometimes it can be quite opaque even for me why the driver decided to > reset the switch. So instead of adding dump_stack() calls each time for > debugging, just add a reset reason to sja1105_static_config_reload > calls which gets printed to the console. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Applied.
On Tue, Nov 12, 2019 at 11:22:00PM +0200, Vladimir Oltean wrote: > Sometimes it can be quite opaque even for me why the driver decided to > reset the switch. So instead of adding dump_stack() calls each time for > debugging, just add a reset reason to sja1105_static_config_reload > calls which gets printed to the console. > +int sja1105_static_config_reload(struct sja1105_private *priv, > + enum sja1105_reset_reason reason) > { > struct ptp_system_timestamp ptp_sts_before; > struct ptp_system_timestamp ptp_sts_after; > @@ -1405,6 +1413,10 @@ int sja1105_static_config_reload(struct sja1105_private *priv) > out_unlock_ptp: > mutex_unlock(&priv->ptp_data.lock); > > + dev_info(priv->ds->dev, > + "Reset switch and programmed static config. Reason: %s\n", > + sja1105_reset_reasons[reason]); If this is for debugging, maybe dev_dbg() would be better? Andrew
Hi Andrew, On Wed, 13 Nov 2019 at 05:53, Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Nov 12, 2019 at 11:22:00PM +0200, Vladimir Oltean wrote: > > Sometimes it can be quite opaque even for me why the driver decided to > > reset the switch. So instead of adding dump_stack() calls each time for > > debugging, just add a reset reason to sja1105_static_config_reload > > calls which gets printed to the console. > > > +int sja1105_static_config_reload(struct sja1105_private *priv, > > + enum sja1105_reset_reason reason) > > { > > struct ptp_system_timestamp ptp_sts_before; > > struct ptp_system_timestamp ptp_sts_after; > > @@ -1405,6 +1413,10 @@ int sja1105_static_config_reload(struct sja1105_private *priv) > > out_unlock_ptp: > > mutex_unlock(&priv->ptp_data.lock); > > > > + dev_info(priv->ds->dev, > > + "Reset switch and programmed static config. Reason: %s\n", > > + sja1105_reset_reasons[reason]); > > If this is for debugging, maybe dev_dbg() would be better? > > Andrew This should not be a debugging print, a reset is an important event in the life of the switch and I would like the user to be aware of it. When I said "debugging" I meant "figure out the reason why it needed to reset this time". Thanks, -Vladimir
On Thu, Nov 14, 2019 at 02:53:21PM +0200, Vladimir Oltean wrote: > Hi Andrew, > > On Wed, 13 Nov 2019 at 05:53, Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Tue, Nov 12, 2019 at 11:22:00PM +0200, Vladimir Oltean wrote: > > > Sometimes it can be quite opaque even for me why the driver decided to > > > reset the switch. So instead of adding dump_stack() calls each time for > > > debugging, just add a reset reason to sja1105_static_config_reload > > > calls which gets printed to the console. > > > > > +int sja1105_static_config_reload(struct sja1105_private *priv, > > > + enum sja1105_reset_reason reason) > > > { > > > struct ptp_system_timestamp ptp_sts_before; > > > struct ptp_system_timestamp ptp_sts_after; > > > @@ -1405,6 +1413,10 @@ int sja1105_static_config_reload(struct sja1105_private *priv) > > > out_unlock_ptp: > > > mutex_unlock(&priv->ptp_data.lock); > > > > > > + dev_info(priv->ds->dev, > > > + "Reset switch and programmed static config. Reason: %s\n", > > > + sja1105_reset_reasons[reason]); > > > > If this is for debugging, maybe dev_dbg() would be better? > > > > Andrew > > This should not be a debugging print, a reset is an important event in > the life of the switch and I would like the user to be aware of it. > When I said "debugging" I meant "figure out the reason why it needed > to reset this time". Ah, O.K. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h index 64b3ee7b9771..1a3722971b61 100644 --- a/drivers/net/dsa/sja1105/sja1105.h +++ b/drivers/net/dsa/sja1105/sja1105.h @@ -115,7 +115,15 @@ typedef enum { } sja1105_spi_rw_mode_t; /* From sja1105_main.c */ -int sja1105_static_config_reload(struct sja1105_private *priv); +enum sja1105_reset_reason { + SJA1105_VLAN_FILTERING = 0, + SJA1105_RX_HWTSTAMPING, + SJA1105_AGEING_TIME, + SJA1105_SCHEDULING, +}; + +int sja1105_static_config_reload(struct sja1105_private *priv, + enum sja1105_reset_reason reason); /* From sja1105_spi.c */ int sja1105_xfer_buf(const struct sja1105_private *priv, diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 475cc2d8b0e8..b60224c55244 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -1341,13 +1341,21 @@ static void sja1105_bridge_leave(struct dsa_switch *ds, int port, sja1105_bridge_member(ds, port, br, false); } +static const char * const sja1105_reset_reasons[] = { + [SJA1105_VLAN_FILTERING] = "VLAN filtering", + [SJA1105_RX_HWTSTAMPING] = "RX timestamping", + [SJA1105_AGEING_TIME] = "Ageing time", + [SJA1105_SCHEDULING] = "Time-aware scheduling", +}; + /* For situations where we need to change a setting at runtime that is only * available through the static configuration, resetting the switch in order * to upload the new static config is unavoidable. Back up the settings we * modify at runtime (currently only MAC) and restore them after uploading, * such that this operation is relatively seamless. */ -int sja1105_static_config_reload(struct sja1105_private *priv) +int sja1105_static_config_reload(struct sja1105_private *priv, + enum sja1105_reset_reason reason) { struct ptp_system_timestamp ptp_sts_before; struct ptp_system_timestamp ptp_sts_after; @@ -1405,6 +1413,10 @@ int sja1105_static_config_reload(struct sja1105_private *priv) out_unlock_ptp: mutex_unlock(&priv->ptp_data.lock); + dev_info(priv->ds->dev, + "Reset switch and programmed static config. Reason: %s\n", + sja1105_reset_reasons[reason]); + /* Configure the CGU (PLLs) for MII and RMII PHYs. * For these interfaces there is no dynamic configuration * needed, since PLLs have same settings at all speeds. @@ -1599,7 +1611,7 @@ static int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled) l2_lookup_params = table->entries; l2_lookup_params->shared_learn = !enabled; - rc = sja1105_static_config_reload(priv); + rc = sja1105_static_config_reload(priv, SJA1105_VLAN_FILTERING); if (rc) dev_err(ds->dev, "Failed to change VLAN Ethertype\n"); @@ -1871,7 +1883,7 @@ static int sja1105_set_ageing_time(struct dsa_switch *ds, l2_lookup_params->maxage = maxage; - return sja1105_static_config_reload(priv); + return sja1105_static_config_reload(priv, SJA1105_AGEING_TIME); } static int sja1105_port_setup_tc(struct dsa_switch *ds, int port, diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c index 0a35813f9328..6b9b2bef8a7b 100644 --- a/drivers/net/dsa/sja1105/sja1105_ptp.c +++ b/drivers/net/dsa/sja1105/sja1105_ptp.c @@ -102,7 +102,7 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv, priv->tagger_data.stampable_skb = NULL; } - return sja1105_static_config_reload(priv); + return sja1105_static_config_reload(priv, SJA1105_RX_HWTSTAMPING); } int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr) diff --git a/drivers/net/dsa/sja1105/sja1105_tas.c b/drivers/net/dsa/sja1105/sja1105_tas.c index 33eca6a82ec5..d846fb5c4e4d 100644 --- a/drivers/net/dsa/sja1105/sja1105_tas.c +++ b/drivers/net/dsa/sja1105/sja1105_tas.c @@ -352,7 +352,7 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port, if (rc < 0) return rc; - return sja1105_static_config_reload(priv); + return sja1105_static_config_reload(priv, SJA1105_SCHEDULING); } /* The cycle time extension is the amount of time the last cycle from @@ -400,7 +400,7 @@ int sja1105_setup_tc_taprio(struct dsa_switch *ds, int port, if (rc < 0) return rc; - return sja1105_static_config_reload(priv); + return sja1105_static_config_reload(priv, SJA1105_SCHEDULING); } void sja1105_tas_setup(struct dsa_switch *ds)
Sometimes it can be quite opaque even for me why the driver decided to reset the switch. So instead of adding dump_stack() calls each time for debugging, just add a reset reason to sja1105_static_config_reload calls which gets printed to the console. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/net/dsa/sja1105/sja1105.h | 10 +++++++++- drivers/net/dsa/sja1105/sja1105_main.c | 18 +++++++++++++++--- drivers/net/dsa/sja1105/sja1105_ptp.c | 2 +- drivers/net/dsa/sja1105/sja1105_tas.c | 4 ++-- 4 files changed, 27 insertions(+), 7 deletions(-)