| Message ID | 1571313682-28900-4-git-send-email-sheetal.tigadoli@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add OP-TEE based bnxt f/w manager | expand |
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote: > From: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > Driver supports 2 types of core dumps. > > 1. Live dump - Firmware dump when system is up and running. > 2. Crash dump - Dump which is collected during firmware crash > that can be retrieved after recovery. > Crash dump is currently supported only on specific 58800 chips > which can be retrieved using OP-TEE API only, as firmware cannot > access this region directly. > > User needs to set the dump flag using following command before > initiating the dump collection: > > $ ethtool -W|--set-dump eth0 N > > Where N is "0" for live dump and "1" for crash dump > > Command to collect the dump after setting the flag: > > $ ethtool -w eth0 data Filename > > Cc: Michael Chan <michael.chan@broadcom.com> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 ++ > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 +++++++++++++++++++++-- > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 ++ > 3 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > index 0943715..3e7d1fb 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > @@ -1807,6 +1807,9 @@ struct bnxt { > > u8 num_leds; > struct bnxt_led_info leds[BNXT_MAX_LED]; > + u16 dump_flag; > +#define BNXT_DUMP_LIVE 0 > +#define BNXT_DUMP_CRASH 1 > > struct bpf_prog *xdp_prog; > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index 51c1404..1596221 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) > return rc; > } > > +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) > +{ > + struct bnxt *bp = netdev_priv(dev); > + > +#ifndef CONFIG_TEE_BNXT_FW > + return -EOPNOTSUPP; > +#endif if (!IS_ENABLED(...)) return x; reads better IMHO But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n > + if (dump->flag > BNXT_DUMP_CRASH) { > + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); more of an _info than _err, if at all > + return -EINVAL; > + } > + > + bp->dump_flag = dump->flag; > + return 0; > +} > + > static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) > { > struct bnxt *bp = netdev_priv(dev); > @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) > bp->ver_resp.hwrm_fw_bld_8b << 8 | > bp->ver_resp.hwrm_fw_rsvd_8b; > > - return bnxt_get_coredump(bp, NULL, &dump->len); > + dump->flag = bp->dump_flag; > + if (bp->dump_flag == BNXT_DUMP_CRASH) > + dump->len = BNXT_CRASH_DUMP_LEN; > + else > + bnxt_get_coredump(bp, NULL, &dump->len); > + return 0; > } > > static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, > @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, > > memset(buf, 0, dump->len); > > - return bnxt_get_coredump(bp, buf, &dump->len); > + dump->flag = bp->dump_flag; > + if (dump->flag == BNXT_DUMP_CRASH) { > +#ifdef CONFIG_TEE_BNXT_FW > + return tee_bnxt_copy_coredump(buf, 0, dump->len); > +#endif > + } else { > + return bnxt_get_coredump(bp, buf, &dump->len); > + } > + > + return 0; > } > > void bnxt_ethtool_init(struct bnxt *bp) > @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) > .set_phys_id = bnxt_set_phys_id, > .self_test = bnxt_self_test, > .reset = bnxt_reset, > + .set_dump = bnxt_set_dump, > .get_dump_flag = bnxt_get_dump_flag, > .get_dump_data = bnxt_get_dump_data, > }; > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > index b5b65b3..01de7e7 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { > #define HWRM_DBG_CMN_FLAGS_MORE 1 > }; > > +#define BNXT_CRASH_DUMP_LEN (8 << 20) > + > #define BNXT_LED_DFLT_ENA \ > (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ > PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \
On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote: > > From: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > > > Driver supports 2 types of core dumps. > > > > 1. Live dump - Firmware dump when system is up and running. > > 2. Crash dump - Dump which is collected during firmware crash > > that can be retrieved after recovery. > > Crash dump is currently supported only on specific 58800 chips > > which can be retrieved using OP-TEE API only, as firmware cannot > > access this region directly. > > > > User needs to set the dump flag using following command before > > initiating the dump collection: > > > > $ ethtool -W|--set-dump eth0 N > > > > Where N is "0" for live dump and "1" for crash dump > > > > Command to collect the dump after setting the flag: > > > > $ ethtool -w eth0 data Filename > > > > Cc: Michael Chan <michael.chan@broadcom.com> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com> > > --- > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 ++ > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 +++++++++++++++++++++-- > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 ++ > > 3 files changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > > index 0943715..3e7d1fb 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h > > @@ -1807,6 +1807,9 @@ struct bnxt { > > > > u8 num_leds; > > struct bnxt_led_info leds[BNXT_MAX_LED]; > > + u16 dump_flag; > > +#define BNXT_DUMP_LIVE 0 > > +#define BNXT_DUMP_CRASH 1 > > > > struct bpf_prog *xdp_prog; > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > index 51c1404..1596221 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) > > return rc; > > } > > > > +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) > > +{ > > + struct bnxt *bp = netdev_priv(dev); > > + > > +#ifndef CONFIG_TEE_BNXT_FW > > + return -EOPNOTSUPP; > > +#endif > > if (!IS_ENABLED(...)) > return x; > > reads better IMHO Okay. > > But also you seem to be breaking live dump for systems with > CONFIG_TEE_BNXT_FW=n Yes, we are supporting set_dump only if crash dump is supported. > > > + if (dump->flag > BNXT_DUMP_CRASH) { > > + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); > > more of an _info than _err, if at all I made this err, as we are returning error on invalid flag value. I can modify the log to something like "Invalid dump flag. Supports only Live(0) and Crash(1) dumps.\n" to make it more like error log. > > > + return -EINVAL; > > + } > > + > > + bp->dump_flag = dump->flag; > > + return 0; > > +} > > + > > static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) > > { > > struct bnxt *bp = netdev_priv(dev); > > @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) > > bp->ver_resp.hwrm_fw_bld_8b << 8 | > > bp->ver_resp.hwrm_fw_rsvd_8b; > > > > - return bnxt_get_coredump(bp, NULL, &dump->len); > > + dump->flag = bp->dump_flag; > > + if (bp->dump_flag == BNXT_DUMP_CRASH) > > + dump->len = BNXT_CRASH_DUMP_LEN; > > + else > > + bnxt_get_coredump(bp, NULL, &dump->len); > > + return 0; > > } > > > > static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, > > @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, > > > > memset(buf, 0, dump->len); > > > > - return bnxt_get_coredump(bp, buf, &dump->len); > > + dump->flag = bp->dump_flag; > > + if (dump->flag == BNXT_DUMP_CRASH) { > > +#ifdef CONFIG_TEE_BNXT_FW > > + return tee_bnxt_copy_coredump(buf, 0, dump->len); > > +#endif > > + } else { > > + return bnxt_get_coredump(bp, buf, &dump->len); > > + } > > + > > + return 0; > > } > > > > void bnxt_ethtool_init(struct bnxt *bp) > > @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) > > .set_phys_id = bnxt_set_phys_id, > > .self_test = bnxt_self_test, > > .reset = bnxt_reset, > > + .set_dump = bnxt_set_dump, > > .get_dump_flag = bnxt_get_dump_flag, > > .get_dump_data = bnxt_get_dump_data, > > }; > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > > index b5b65b3..01de7e7 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h > > @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { > > #define HWRM_DBG_CMN_FLAGS_MORE 1 > > }; > > > > +#define BNXT_CRASH_DUMP_LEN (8 << 20) > > + > > #define BNXT_LED_DFLT_ENA \ > > (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ > > PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \ > Thanks, Vasundhara
On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote: > On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote: > > On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote: > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > index 51c1404..1596221 100644 > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) > > > return rc; > > > } > > > > > > +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) > > > +{ > > > + struct bnxt *bp = netdev_priv(dev); > > > + > > > +#ifndef CONFIG_TEE_BNXT_FW > > > + return -EOPNOTSUPP; > > > +#endif > > > > if (!IS_ENABLED(...)) > > return x; > > > > reads better IMHO > Okay. > > > > > But also you seem to be breaking live dump for systems with > > CONFIG_TEE_BNXT_FW=n > Yes, we are supporting set_dump only if crash dump is supported. It's wrong. > > > + if (dump->flag > BNXT_DUMP_CRASH) { > > > + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); > > > > more of an _info than _err, if at all > I made this err, as we are returning error on invalid flag value. I > can modify the log to > something like "Invalid dump flag. Supports only Live(0) and Crash(1) > dumps.\n" to make > it more like error log. Not an error.
On Fri, Oct 18, 2019 at 10:31 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote: > > On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote: > > > On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote: > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > index 51c1404..1596221 100644 > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) > > > > return rc; > > > > } > > > > > > > > +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) > > > > +{ > > > > + struct bnxt *bp = netdev_priv(dev); > > > > + > > > > +#ifndef CONFIG_TEE_BNXT_FW > > > > + return -EOPNOTSUPP; > > > > +#endif > > > > > > if (!IS_ENABLED(...)) > > > return x; > > > > > > reads better IMHO > > Okay. > > > > > > > > But also you seem to be breaking live dump for systems with > > > CONFIG_TEE_BNXT_FW=n > > Yes, we are supporting set_dump only if crash dump is supported. > > It's wrong. Sorry not very clear. You are saying that support set_dump all the time and return error, if the config option is not enabled? If yes, I will modify the same way as it makes sense. > > > > > + if (dump->flag > BNXT_DUMP_CRASH) { > > > > + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); > > > > > > more of an _info than _err, if at all > > I made this err, as we are returning error on invalid flag value. I > > can modify the log to > > something like "Invalid dump flag. Supports only Live(0) and Crash(1) > > dumps.\n" to make > > it more like error log. > > Not an error. Okay.
On Mon, 21 Oct 2019 10:05:18 +0530, Vasundhara Volam wrote: > On Fri, Oct 18, 2019 at 10:31 PM Jakub Kicinski wrote: > > On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote: > > > On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote: > > > > On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote: > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > > index 51c1404..1596221 100644 > > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > > > @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) > > > > > return rc; > > > > > } > > > > > > > > > > +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) > > > > > +{ > > > > > + struct bnxt *bp = netdev_priv(dev); > > > > > + > > > > > +#ifndef CONFIG_TEE_BNXT_FW > > > > > + return -EOPNOTSUPP; > > > > > +#endif > > > > > > > > if (!IS_ENABLED(...)) > > > > return x; > > > > > > > > reads better IMHO > > > Okay. > > > > > > > > > > > But also you seem to be breaking live dump for systems with > > > > CONFIG_TEE_BNXT_FW=n > > > Yes, we are supporting set_dump only if crash dump is supported. > > > > It's wrong. > > Sorry not very clear. You are saying that support set_dump all the > time and return error, if the config option is not enabled? If yes, I > will modify the same way as it makes sense. Yes, I think users can expect that set will work even if there is only one dump type/flag supported. Technically implementing the set is not required from functional perspective, however, we could imagine software that always does a set before a get, and the set should not fail.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 0943715..3e7d1fb 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1807,6 +1807,9 @@ struct bnxt { u8 num_leds; struct bnxt_led_info leds[BNXT_MAX_LED]; + u16 dump_flag; +#define BNXT_DUMP_LIVE 0 +#define BNXT_DUMP_CRASH 1 struct bpf_prog *xdp_prog; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; } +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{ + struct bnxt *bp = netdev_priv(dev); + +#ifndef CONFIG_TEE_BNXT_FW + return -EOPNOTSUPP; +#endif + + if (dump->flag > BNXT_DUMP_CRASH) { + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); + return -EINVAL; + } + + bp->dump_flag = dump->flag; + return 0; +} + static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) { struct bnxt *bp = netdev_priv(dev); @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) bp->ver_resp.hwrm_fw_bld_8b << 8 | bp->ver_resp.hwrm_fw_rsvd_8b; - return bnxt_get_coredump(bp, NULL, &dump->len); + dump->flag = bp->dump_flag; + if (bp->dump_flag == BNXT_DUMP_CRASH) + dump->len = BNXT_CRASH_DUMP_LEN; + else + bnxt_get_coredump(bp, NULL, &dump->len); + return 0; } static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, memset(buf, 0, dump->len); - return bnxt_get_coredump(bp, buf, &dump->len); + dump->flag = bp->dump_flag; + if (dump->flag == BNXT_DUMP_CRASH) { +#ifdef CONFIG_TEE_BNXT_FW + return tee_bnxt_copy_coredump(buf, 0, dump->len); +#endif + } else { + return bnxt_get_coredump(bp, buf, &dump->len); + } + + return 0; } void bnxt_ethtool_init(struct bnxt *bp) @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) .set_phys_id = bnxt_set_phys_id, .self_test = bnxt_self_test, .reset = bnxt_reset, + .set_dump = bnxt_set_dump, .get_dump_flag = bnxt_get_dump_flag, .get_dump_data = bnxt_get_dump_data, }; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h index b5b65b3..01de7e7 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { #define HWRM_DBG_CMN_FLAGS_MORE 1 }; +#define BNXT_CRASH_DUMP_LEN (8 << 20) + #define BNXT_LED_DFLT_ENA \ (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \