| Message ID | 20231017105341.415466-11-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: retain error in struct devlink_fmsg | expand |
On Tue, 17 Oct 2023 12:53:40 +0200 Przemek Kitszel wrote: > Drop unneeded error checking. > > devlink_fmsg_*() family of functions is now retaining errors, > so there is no need to check for them after each call. Humpf. Unrelated to the set, when did qlge grow devlink support?! Coiby, do you still use this HW? It looks like the driver was moved to staging on account of being old and unused, and expecting that we'll delete it. Clearly that's not the case if people are adding devlink support, so should we move it back?
On 2023-10-17 18:15 -0700, Jakub Kicinski wrote: > On Tue, 17 Oct 2023 12:53:40 +0200 Przemek Kitszel wrote: > > Drop unneeded error checking. > > > > devlink_fmsg_*() family of functions is now retaining errors, > > so there is no need to check for them after each call. > > Humpf. Unrelated to the set, when did qlge grow devlink support?! > > Coiby, do you still use this HW? > > It looks like the driver was moved to staging on account of being > old and unused, and expecting that we'll delete it. Clearly that's > not the case if people are adding devlink support, so should we > move it back? AFAIK this was done by Coiby as an exercise in kernel programming. Improving the debugging dump facilities was one of the tasks in the TODO file. I moved the driver to staging because it had many problems and it had been abandoned by the vendor. There might be some qlge users left but is that reason enough to move the driver back to drivers/net/ if there is no one who is interested in doing more than checkpatch fixes on the driver?
On Thu, 19 Oct 2023 10:28:03 -0400 Benjamin Poirier wrote: > > Humpf. Unrelated to the set, when did qlge grow devlink support?! > > > > Coiby, do you still use this HW? > > > > It looks like the driver was moved to staging on account of being > > old and unused, and expecting that we'll delete it. Clearly that's > > not the case if people are adding devlink support, so should we > > move it back? > > AFAIK this was done by Coiby as an exercise in kernel programming. > Improving the debugging dump facilities was one of the tasks in the TODO > file. > > I moved the driver to staging because it had many problems and it had > been abandoned by the vendor. There might be some qlge users left but is > that reason enough to move the driver back to drivers/net/ > if there is no one who is interested in doing more than checkpatch > fixes on the driver? Staging is usually an area for code entering the kernel, not leaving. We should either suffer with it under drivers/net/ or delete it, as you say, nobody is working on significant improvements so having the driver in staging is serving no purpose. How about we delete it completely, and if someone complains bring it back under drivers/net ?
On 2023-10-19 07:42 -0700, Jakub Kicinski wrote: > On Thu, 19 Oct 2023 10:28:03 -0400 Benjamin Poirier wrote: > > > Humpf. Unrelated to the set, when did qlge grow devlink support?! > > > > > > Coiby, do you still use this HW? > > > > > > It looks like the driver was moved to staging on account of being > > > old and unused, and expecting that we'll delete it. Clearly that's > > > not the case if people are adding devlink support, so should we > > > move it back? > > > > AFAIK this was done by Coiby as an exercise in kernel programming. > > Improving the debugging dump facilities was one of the tasks in the TODO > > file. > > > > I moved the driver to staging because it had many problems and it had > > been abandoned by the vendor. There might be some qlge users left but is > > that reason enough to move the driver back to drivers/net/ > > if there is no one who is interested in doing more than checkpatch > > fixes on the driver? > > Staging is usually an area for code entering the kernel, not leaving. > We should either suffer with it under drivers/net/ or delete it, > as you say, nobody is working on significant improvements so having > the driver in staging is serving no purpose. > > How about we delete it completely, and if someone complains bring > it back under drivers/net ? That sounds like a reasonable way forward, thank you. I'll send a patch to do the removal.
diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c index 0ab02d6d3817..0b19363ca2e9 100644 --- a/drivers/staging/qlge/qlge_devlink.c +++ b/drivers/staging/qlge/qlge_devlink.c @@ -2,51 +2,29 @@ #include "qlge.h" #include "qlge_devlink.h" -static int qlge_fill_seg_(struct devlink_fmsg *fmsg, - struct mpi_coredump_segment_header *seg_header, - u32 *reg_data) +static void qlge_fill_seg_(struct devlink_fmsg *fmsg, + struct mpi_coredump_segment_header *seg_header, + u32 *reg_data) { int regs_num = (seg_header->seg_size - sizeof(struct mpi_coredump_segment_header)) / sizeof(u32); - int err; int i; - 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; + devlink_fmsg_pair_nest_start(fmsg, seg_header->description); + devlink_fmsg_obj_nest_start(fmsg); + devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num); + devlink_fmsg_arr_pair_nest_start(fmsg, "values"); for (i = 0; i < regs_num; i++) { - err = devlink_fmsg_u32_put(fmsg, *reg_data); - if (err) - return err; + devlink_fmsg_u32_put(fmsg, *reg_data); 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; + devlink_fmsg_obj_nest_end(fmsg); + devlink_fmsg_arr_pair_nest_end(fmsg); + devlink_fmsg_pair_nest_end(fmsg); } -#define FILL_SEG(seg_hdr, seg_regs) \ - do { \ - err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \ - if (err) { \ - kvfree(dump); \ - return err; \ - } \ - } while (0) +#define FILL_SEG(seg_hdr, seg_regs) \ + qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs) static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, struct devlink_fmsg *fmsg, void *priv_ctx, @@ -114,14 +92,8 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, 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 = qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr, - (u32 *)&dump->misc_nic_info); - if (err) { - kvfree(dump); - return err; - } - + qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr, + (u32 *)&dump->misc_nic_info); 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); @@ -140,7 +112,7 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter, FILL_SEG(sem_regs_seg_hdr, sem_regs); kvfree(dump); - return err; + return 0; } static const struct devlink_health_reporter_ops qlge_reporter_ops = {
Drop unneeded error checking. devlink_fmsg_*() family of functions is now retaining errors, so there is no need to check for them after each call. Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> --- drivers/staging/qlge/qlge_devlink.c | 60 ++++++++--------------------- 1 file changed, 16 insertions(+), 44 deletions(-)