Messages in this thread | | | From | Coiby Xu <> | Date | Sat, 10 Oct 2020 18:02:58 +0800 | Subject | Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter |
| |
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
| |