| Message ID | 20201008115808.91850-3-coiby.xu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver | expand |
On Thu, Oct 08, 2020 at 07:58:04PM +0800, Coiby Xu wrote: > -static int > -qlge_reporter_coredump(struct devlink_health_reporter *reporter, > - struct devlink_fmsg *fmsg, void *priv_ctx, > - struct netlink_ext_ack *extack) > +static int fill_seg_(struct devlink_fmsg *fmsg, > + struct mpi_coredump_segment_header *seg_header, > + u32 *reg_data) > { > - return 0; > + int i; > + int header_size = sizeof(struct mpi_coredump_segment_header); Please use the sizeof() directly in the code. Don't introduce indirection if you can help it. > + int regs_num = (seg_header->seg_size - header_size) / sizeof(u32); > + int err; > + regards, dan carpenter
On Thu, Oct 08, 2020 at 04:39:40PM +0300, Dan Carpenter wrote: >On Thu, Oct 08, 2020 at 07:58:04PM +0800, Coiby Xu wrote: >> -static int >> -qlge_reporter_coredump(struct devlink_health_reporter *reporter, >> - struct devlink_fmsg *fmsg, void *priv_ctx, >> - struct netlink_ext_ack *extack) >> +static int fill_seg_(struct devlink_fmsg *fmsg, >> + struct mpi_coredump_segment_header *seg_header, >> + u32 *reg_data) >> { >> - return 0; >> + int i; >> + int header_size = sizeof(struct mpi_coredump_segment_header); > >Please use the sizeof() directly in the code. Don't introduce >indirection if you can help it. > >> + int regs_num = (seg_header->seg_size - header_size) / sizeof(u32); >> + int err; >> + > Thank you for the suggestion! >regards, >dan carpenter -- Best regards, Coiby
On 2020-10-08 19:58 +0800, Coiby Xu wrote: > $ devlink health dump show DEVICE reporter coredump -p -j > { > "Core Registers": { > "segment": 1, > "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 > ,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] > }, > "Test Logic Regs": { > "segment": 2, > "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] > }, > "RMII Registers": { > "segment": 3, > "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] > }, > ... > "Sem Registers": { > "segment": 50, > "values": [ 0,0,0,0 ] > } > } > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com> > --- > drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++-- > 1 file changed, 125 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c > index aa45e7e368c0..91b6600b94a9 100644 > --- a/drivers/staging/qlge/qlge_devlink.c > +++ b/drivers/staging/qlge/qlge_devlink.c > @@ -1,16 +1,135 @@ > #include "qlge.h" > #include "qlge_devlink.h" > > -static int > -qlge_reporter_coredump(struct devlink_health_reporter *reporter, > - struct devlink_fmsg *fmsg, void *priv_ctx, > - struct netlink_ext_ack *extack) > +static int fill_seg_(struct devlink_fmsg *fmsg, Please include the "qlge_" prefix. > + struct mpi_coredump_segment_header *seg_header, > + u32 *reg_data) > { > - return 0; > + int i; > + int header_size = sizeof(struct mpi_coredump_segment_header); > + int regs_num = (seg_header->seg_size - header_size) / sizeof(u32); > + int err; > + > + err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description); > + if (err) > + return err; > + err = devlink_fmsg_obj_nest_start(fmsg); > + if (err) > + return err; > + err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num); > + if (err) > + return err; > + err = devlink_fmsg_arr_pair_nest_start(fmsg, "values"); > + if (err) > + return err; > + for (i = 0; i < regs_num; i++) { > + err = devlink_fmsg_u32_put(fmsg, *reg_data); > + if (err) > + return err; > + reg_data++; > + } > + err = devlink_fmsg_obj_nest_end(fmsg); > + if (err) > + return err; > + err = devlink_fmsg_arr_pair_nest_end(fmsg); > + if (err) > + return err; > + err = devlink_fmsg_pair_nest_end(fmsg); > + return err; > +} > + > +#define fill_seg(seg_hdr, seg_regs) \ considering that this macro accesses local variables, it is not really "function-like". I think an all-caps name would be better to tip-off the reader. > + do { \ > + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ > + if (err) { \ > + kvfree(dump); \ > + return err; \ > + } \ > + } while (0) > + > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, > + struct devlink_fmsg *fmsg, void *priv_ctx, > + struct netlink_ext_ack *extack) > +{ > + int err = 0; > + > + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); Please name this variable ql_devlink, like in qlge_probe().
On Sat, Oct 10, 2020 at 04:48:09PM +0900, Benjamin Poirier wrote: >On 2020-10-08 19:58 +0800, Coiby Xu wrote: >> $ devlink health dump show DEVICE reporter coredump -p -j >> { >> "Core Registers": { >> "segment": 1, >> "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 >> ,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] >> }, >> "Test Logic Regs": { >> "segment": 2, >> "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] >> }, >> "RMII Registers": { >> "segment": 3, >> "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] >> }, >> ... >> "Sem Registers": { >> "segment": 50, >> "values": [ 0,0,0,0 ] >> } >> } >> >> Signed-off-by: Coiby Xu <coiby.xu@gmail.com> >> --- >> drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++-- >> 1 file changed, 125 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c >> index aa45e7e368c0..91b6600b94a9 100644 >> --- a/drivers/staging/qlge/qlge_devlink.c >> +++ b/drivers/staging/qlge/qlge_devlink.c >> @@ -1,16 +1,135 @@ >> #include "qlge.h" >> #include "qlge_devlink.h" >> >> -static int >> -qlge_reporter_coredump(struct devlink_health_reporter *reporter, >> - struct devlink_fmsg *fmsg, void *priv_ctx, >> - struct netlink_ext_ack *extack) >> +static int fill_seg_(struct devlink_fmsg *fmsg, > >Please include the "qlge_" prefix. > >> + struct mpi_coredump_segment_header *seg_header, >> + u32 *reg_data) >> { >> - return 0; >> + int i; >> + int header_size = sizeof(struct mpi_coredump_segment_header); >> + int regs_num = (seg_header->seg_size - header_size) / sizeof(u32); >> + int err; >> + >> + err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description); >> + if (err) >> + return err; >> + err = devlink_fmsg_obj_nest_start(fmsg); >> + if (err) >> + return err; >> + err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num); >> + if (err) >> + return err; >> + err = devlink_fmsg_arr_pair_nest_start(fmsg, "values"); >> + if (err) >> + return err; >> + for (i = 0; i < regs_num; i++) { >> + err = devlink_fmsg_u32_put(fmsg, *reg_data); >> + if (err) >> + return err; >> + reg_data++; >> + } >> + err = devlink_fmsg_obj_nest_end(fmsg); >> + if (err) >> + return err; >> + err = devlink_fmsg_arr_pair_nest_end(fmsg); >> + if (err) >> + return err; >> + err = devlink_fmsg_pair_nest_end(fmsg); >> + return err; >> +} >> + >> +#define fill_seg(seg_hdr, seg_regs) \ > >considering that this macro accesses local variables, it is not really >"function-like". I think an all-caps name would be better to tip-off the >reader. > Thank you for this suggestion! >> + do { \ >> + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ >> + if (err) { \ >> + kvfree(dump); \ >> + return err; \ >> + } \ >> + } while (0) >> + >> +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, >> + struct devlink_fmsg *fmsg, void *priv_ctx, >> + struct netlink_ext_ack *extack) >> +{ >> + int err = 0; >> + >> + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); > >Please name this variable ql_devlink, like in qlge_probe(). I happened to find the following text in drivers/staging/qlge/TODO > * in terms of namespace, the driver uses either qlge_, ql_ (used by > other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with > clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" > prefix. So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink. -- Best regards, Coiby
On 2020-10-10 18:02 +0800, Coiby Xu wrote: [...] > > > + do { \ > > > + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ > > > + if (err) { \ > > > + kvfree(dump); \ > > > + return err; \ > > > + } \ > > > + } while (0) > > > + > > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, > > > + struct devlink_fmsg *fmsg, void *priv_ctx, > > > + struct netlink_ext_ack *extack) > > > +{ > > > + int err = 0; > > > + > > > + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); > > > > Please name this variable ql_devlink, like in qlge_probe(). > > I happened to find the following text in drivers/staging/qlge/TODO > > * in terms of namespace, the driver uses either qlge_, ql_ (used by > > other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with > > clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" > > prefix. This comment applies to global identifiers, not local variables. > > So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink. Up to you but personally, I think ql_devlink is better. In any case, "dev" is too general and often used for struct net_device pointers instead.
On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote: >On 2020-10-10 18:02 +0800, Coiby Xu wrote: >[...] >> > > + do { \ >> > > + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ >> > > + if (err) { \ >> > > + kvfree(dump); \ >> > > + return err; \ >> > > + } \ >> > > + } while (0) >> > > + >> > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, >> > > + struct devlink_fmsg *fmsg, void *priv_ctx, >> > > + struct netlink_ext_ack *extack) >> > > +{ >> > > + int err = 0; >> > > + >> > > + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); >> > >> > Please name this variable ql_devlink, like in qlge_probe(). >> >> I happened to find the following text in drivers/staging/qlge/TODO >> > * in terms of namespace, the driver uses either qlge_, ql_ (used by >> > other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with >> > clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" >> > prefix. > >This comment applies to global identifiers, not local variables. Thank you for the explanation! Are you suggesting we should choose different naming styles so we better tell global identifiers from local variables? > >> >> So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink. > >Up to you but personally, I think ql_devlink is better. In any case, >"dev" is too general and often used for struct net_device pointers >instead. Thank you for the suggestion. Another reason to use qlge_dl is many other network drivers supporting devlink interface also adopt this kind of style. -- Best regards, Coiby
On 2020-10-12 19:51 +0800, Coiby Xu wrote: > On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote: > > On 2020-10-10 18:02 +0800, Coiby Xu wrote: > > [...] > > > > > + do { \ > > > > > + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ > > > > > + if (err) { \ > > > > > + kvfree(dump); \ > > > > > + return err; \ > > > > > + } \ > > > > > + } while (0) > > > > > + > > > > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, > > > > > + struct devlink_fmsg *fmsg, void *priv_ctx, > > > > > + struct netlink_ext_ack *extack) > > > > > +{ > > > > > + int err = 0; > > > > > + > > > > > + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); > > > > > > > > Please name this variable ql_devlink, like in qlge_probe(). > > > > > > I happened to find the following text in drivers/staging/qlge/TODO > > > > * in terms of namespace, the driver uses either qlge_, ql_ (used by > > > > other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with > > > > clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" > > > > prefix. > > > > This comment applies to global identifiers, not local variables. > > Thank you for the explanation! Are you suggesting we should choose > different naming styles so we better tell global identifiers from local > variables? That's not the main purpose IMO. Using a consistent prefix for global identifiers (ex. "qlge_") is to avoid clashes (two drivers using the same name, as in the examples above). Strictly speaking, it is not a problem for symbols with internal linkage (ex. static functions) or type definitions in local header files but it makes the code clearer because it shows immediately that this identifier was defined in the driver. For local variables, the name is more a matter of personal taste I think but it should be consistent within the driver and with other users of the same api, where applicable. A prefix is not needed but the name is sometimes a simpler version of a type name which includes a prefix. > > > So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink. > > > > Up to you but personally, I think ql_devlink is better. In any case, > > "dev" is too general and often used for struct net_device pointers > > instead. > > Thank you for the suggestion. Another reason to use qlge_dl is many > other network drivers supporting devlink interface also adopt this kind > of style. Sounds good. On second thought I regretted suggesting ql_devlink. While local variable don't need any prefix; if they do have one, better not mix qlge_ and ql_.
diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c index aa45e7e368c0..91b6600b94a9 100644 --- a/drivers/staging/qlge/qlge_devlink.c +++ b/drivers/staging/qlge/qlge_devlink.c @@ -1,16 +1,135 @@ #include "qlge.h" #include "qlge_devlink.h" -static int -qlge_reporter_coredump(struct devlink_health_reporter *reporter, - struct devlink_fmsg *fmsg, void *priv_ctx, - struct netlink_ext_ack *extack) +static int fill_seg_(struct devlink_fmsg *fmsg, + struct mpi_coredump_segment_header *seg_header, + u32 *reg_data) { - return 0; + int i; + int header_size = sizeof(struct mpi_coredump_segment_header); + int regs_num = (seg_header->seg_size - header_size) / sizeof(u32); + int err; + + err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description); + if (err) + return err; + err = devlink_fmsg_obj_nest_start(fmsg); + if (err) + return err; + err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num); + if (err) + return err; + err = devlink_fmsg_arr_pair_nest_start(fmsg, "values"); + if (err) + return err; + for (i = 0; i < regs_num; i++) { + err = devlink_fmsg_u32_put(fmsg, *reg_data); + if (err) + return err; + reg_data++; + } + err = devlink_fmsg_obj_nest_end(fmsg); + if (err) + return err; + err = devlink_fmsg_arr_pair_nest_end(fmsg); + if (err) + return err; + err = devlink_fmsg_pair_nest_end(fmsg); + return err; +} + +#define fill_seg(seg_hdr, seg_regs) \ + do { \ + err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ + if (err) { \ + kvfree(dump); \ + return err; \ + } \ + } while (0) + +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, + struct devlink_fmsg *fmsg, void *priv_ctx, + struct netlink_ext_ack *extack) +{ + int err = 0; + + struct qlge_devlink *dev = devlink_health_reporter_priv(reporter); + struct ql_adapter *qdev = dev->qdev; + struct ql_mpi_coredump *dump; + + if (!netif_running(qdev->ndev)) + return 0; + + dump = kvmalloc(sizeof(*dump), GFP_KERNEL); + if (!dump) + return -ENOMEM; + + err = ql_core_dump(qdev, dump); + if (err) { + kvfree(dump); + return err; + } + + fill_seg(core_regs_seg_hdr, mpi_core_regs); + fill_seg(test_logic_regs_seg_hdr, test_logic_regs); + fill_seg(rmii_regs_seg_hdr, rmii_regs); + fill_seg(fcmac1_regs_seg_hdr, fcmac1_regs); + fill_seg(fcmac2_regs_seg_hdr, fcmac2_regs); + fill_seg(fc1_mbx_regs_seg_hdr, fc1_mbx_regs); + fill_seg(ide_regs_seg_hdr, ide_regs); + fill_seg(nic1_mbx_regs_seg_hdr, nic1_mbx_regs); + fill_seg(smbus_regs_seg_hdr, smbus_regs); + fill_seg(fc2_mbx_regs_seg_hdr, fc2_mbx_regs); + fill_seg(nic2_mbx_regs_seg_hdr, nic2_mbx_regs); + fill_seg(i2c_regs_seg_hdr, i2c_regs); + fill_seg(memc_regs_seg_hdr, memc_regs); + fill_seg(pbus_regs_seg_hdr, pbus_regs); + fill_seg(mde_regs_seg_hdr, mde_regs); + fill_seg(nic_regs_seg_hdr, nic_regs); + fill_seg(nic2_regs_seg_hdr, nic2_regs); + fill_seg(xgmac1_seg_hdr, xgmac1); + fill_seg(xgmac2_seg_hdr, xgmac2); + fill_seg(code_ram_seg_hdr, code_ram); + fill_seg(memc_ram_seg_hdr, memc_ram); + fill_seg(xaui_an_hdr, serdes_xaui_an); + fill_seg(xaui_hss_pcs_hdr, serdes_xaui_hss_pcs); + fill_seg(xfi_an_hdr, serdes_xfi_an); + fill_seg(xfi_train_hdr, serdes_xfi_train); + fill_seg(xfi_hss_pcs_hdr, serdes_xfi_hss_pcs); + fill_seg(xfi_hss_tx_hdr, serdes_xfi_hss_tx); + fill_seg(xfi_hss_rx_hdr, serdes_xfi_hss_rx); + fill_seg(xfi_hss_pll_hdr, serdes_xfi_hss_pll); + + err = fill_seg_(fmsg, &dump->misc_nic_seg_hdr, + (u32 *)&dump->misc_nic_info); + if (err) { + kvfree(dump); + return err; + } + + fill_seg(intr_states_seg_hdr, intr_states); + fill_seg(cam_entries_seg_hdr, cam_entries); + fill_seg(nic_routing_words_seg_hdr, nic_routing_words); + fill_seg(ets_seg_hdr, ets); + fill_seg(probe_dump_seg_hdr, probe_dump); + fill_seg(routing_reg_seg_hdr, routing_regs); + fill_seg(mac_prot_reg_seg_hdr, mac_prot_regs); + fill_seg(xaui2_an_hdr, serdes2_xaui_an); + fill_seg(xaui2_hss_pcs_hdr, serdes2_xaui_hss_pcs); + fill_seg(xfi2_an_hdr, serdes2_xfi_an); + fill_seg(xfi2_train_hdr, serdes2_xfi_train); + fill_seg(xfi2_hss_pcs_hdr, serdes2_xfi_hss_pcs); + fill_seg(xfi2_hss_tx_hdr, serdes2_xfi_hss_tx); + fill_seg(xfi2_hss_rx_hdr, serdes2_xfi_hss_rx); + fill_seg(xfi2_hss_pll_hdr, serdes2_xfi_hss_pll); + fill_seg(sem_regs_seg_hdr, sem_regs); + + kvfree(dump); + return err; } static const struct devlink_health_reporter_ops qlge_reporter_ops = { - .name = "dummy", + .name = "coredump", .dump = qlge_reporter_coredump, };
$ devlink health dump show DEVICE reporter coredump -p -j { "Core Registers": { "segment": 1, "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] }, "Test Logic Regs": { "segment": 2, "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] }, "RMII Registers": { "segment": 3, "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ] }, ... "Sem Registers": { "segment": 50, "values": [ 0,0,0,0 ] } } Signed-off-by: Coiby Xu <coiby.xu@gmail.com> --- drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++-- 1 file changed, 125 insertions(+), 6 deletions(-)