| Message ID | 20231018202647.44769-1-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | devlink: retain error in struct devlink_fmsg | expand |
Wed, Oct 18, 2023 at 10:26:36PM CEST, przemyslaw.kitszel@intel.com wrote: >Extend devlink fmsg to retain error (patch 1), >so drivers could omit error checks after devlink_fmsg_*() (patches 2-10), >and finally enforce future uses to follow this practice by change to >return void (patch 11) > >Note that it was compile tested only. > >bloat-o-meter for whole series: >add/remove: 8/18 grow/shrink: 23/40 up/down: 2017/-5833 (-3816) > >changelog: >v3: set err to correct value, thanks to Simon and smatch > (mlx5 patch, final patch); 2 nits: - always better to have per-patch changelog so it is clear for the reviewers what exactly did you change and where. - if you do any change in a patch, you should drop the acked/reviewed/signedoff tags and get them again from people. that being said: set- Reviewed-by: Jiri Pirko <jiri@nvidia.com>
On 10/19/23 15:44, Jiri Pirko wrote: > Wed, Oct 18, 2023 at 10:26:36PM CEST, przemyslaw.kitszel@intel.com wrote: >> Extend devlink fmsg to retain error (patch 1), >> so drivers could omit error checks after devlink_fmsg_*() (patches 2-10), >> and finally enforce future uses to follow this practice by change to >> return void (patch 11) >> >> Note that it was compile tested only. >> >> bloat-o-meter for whole series: >> add/remove: 8/18 grow/shrink: 23/40 up/down: 2017/-5833 (-3816) >> >> changelog: >> v3: set err to correct value, thanks to Simon and smatch >> (mlx5 patch, final patch); > > 2 nits: > - always better to have per-patch changelog so it is clear for the > reviewers what exactly did you change and where. agree that adding changelog also to patches would be better > - if you do any change in a patch, you should drop the > acked/reviewed/signedoff tags and get them again from people. Here opinions differ widely, and my understanding was "it depends". Noted to always drop yours. On one side you have just rebased patches (different context of review), then with trivial conflict fixed, then with minor change (as here, 0-init to retval, those are things that I believe most reviewers don't want to look again at. Then patches with some improvement that somebody suggested (as reviewer, I want to see what could have been done better and I didn't notice). In the above cases there is both time to react and no much harm keeping RB. Things I think that most reviewers and maintainers would like to drop RB start at "significant changes such as 'business' logic change", which is of course a fuzzy line. Always dropping is an easy solution, perhaps too easy ;) > > that being said: > set- > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Thank you!
Thu, Oct 19, 2023 at 11:50:03PM CEST, przemyslaw.kitszel@intel.com wrote: >On 10/19/23 15:44, Jiri Pirko wrote: >> Wed, Oct 18, 2023 at 10:26:36PM CEST, przemyslaw.kitszel@intel.com wrote: >> > Extend devlink fmsg to retain error (patch 1), >> > so drivers could omit error checks after devlink_fmsg_*() (patches 2-10), >> > and finally enforce future uses to follow this practice by change to >> > return void (patch 11) >> > >> > Note that it was compile tested only. >> > >> > bloat-o-meter for whole series: >> > add/remove: 8/18 grow/shrink: 23/40 up/down: 2017/-5833 (-3816) >> > >> > changelog: >> > v3: set err to correct value, thanks to Simon and smatch >> > (mlx5 patch, final patch); >> >> 2 nits: >> - always better to have per-patch changelog so it is clear for the >> reviewers what exactly did you change and where. > >agree that adding changelog also to patches would be better > >> - if you do any change in a patch, you should drop the >> acked/reviewed/signedoff tags and get them again from people. > >Here opinions differ widely, and my understanding was "it depends". >Noted to always drop yours. > >On one side you have just rebased patches (different context of review), >then with trivial conflict fixed, then with minor change (as here, >0-init to retval, those are things that I believe most reviewers don't >want to look again at. >Then patches with some improvement that somebody suggested (as reviewer, >I want to see what could have been done better and I didn't notice). > >In the above cases there is both time to react and no much harm keeping >RB. Things I think that most reviewers and maintainers would like to >drop RB start at "significant changes such as 'business' logic change", >which is of course a fuzzy line. > >Always dropping is an easy solution, perhaps too easy ;) Quoting Documentation/process/submitting-patches.rst: " Both Tested-by and Reviewed-by tags, once received on mailing list from tester or reviewer, should be added by author to the applicable patches when sending next versions. However if the patch has changed substantially in following version, these tags might not be applicable anymore and thus should be removed. Usually removal of someone's Tested-by or Reviewed-by tags should be mentioned in the patch changelog (after the '---' separator). " I guess that in this case you are right, it the change is not likely "substantial". Thanks! > >> >> that being said: >> set- >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> >> > >Thank you!
Hello: This series was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 18 Oct 2023 22:26:36 +0200 you wrote: > Extend devlink fmsg to retain error (patch 1), > so drivers could omit error checks after devlink_fmsg_*() (patches 2-10), > and finally enforce future uses to follow this practice by change to > return void (patch 11) > > Note that it was compile tested only. > > [...] Here is the summary with links: - [net-next,v3,01/11] devlink: retain error in struct devlink_fmsg (no matching commit) - [net-next,v3,02/11] netdevsim: devlink health: use retained error fmsg API (no matching commit) - [net-next,v3,03/11] pds_core: devlink health: use retained error fmsg API https://git.kernel.org/netdev/net-next/c/47957bb3f783 - [net-next,v3,04/11] bnxt_en: devlink health: use retained error fmsg API (no matching commit) - [net-next,v3,05/11] hinic: devlink health: use retained error fmsg API (no matching commit) - [net-next,v3,06/11] octeontx2-af: devlink health: use retained error fmsg API https://git.kernel.org/netdev/net-next/c/d8cf03fca341 - [net-next,v3,07/11] mlxsw: core: devlink health: use retained error fmsg API https://git.kernel.org/netdev/net-next/c/1d434b48495d - [net-next,v3,08/11] net/mlx5: devlink health: use retained error fmsg API (no matching commit) - [net-next,v3,09/11] qed: devlink health: use retained error fmsg API https://git.kernel.org/netdev/net-next/c/18256cb2d4a0 - [net-next,v3,10/11] staging: qlge: devlink health: use retained error fmsg API https://git.kernel.org/netdev/net-next/c/3465915e9985 - [net-next,v3,11/11] devlink: convert most of devlink_fmsg_*() to return void (no matching commit) You are awesome, thank you!
Extend devlink fmsg to retain error (patch 1), so drivers could omit error checks after devlink_fmsg_*() (patches 2-10), and finally enforce future uses to follow this practice by change to return void (patch 11) Note that it was compile tested only. bloat-o-meter for whole series: add/remove: 8/18 grow/shrink: 23/40 up/down: 2017/-5833 (-3816) changelog: v3: set err to correct value, thanks to Simon and smatch (mlx5 patch, final patch); v2: extend series by two more drivers (qed, qlge); add final cleanup patch, since now whole series should be merged in one part (thanks Jiri for encouragement here); v1: https://lore.kernel.org/netdev/20231010104318.3571791-3-przemyslaw.kitszel@intel.com v2: https://lore.kernel.org/netdev/20231017105341.415466-1-przemyslaw.kitszel@intel.com Przemek Kitszel (11): devlink: retain error in struct devlink_fmsg netdevsim: devlink health: use retained error fmsg API pds_core: devlink health: use retained error fmsg API bnxt_en: devlink health: use retained error fmsg API hinic: devlink health: use retained error fmsg API octeontx2-af: devlink health: use retained error fmsg API mlxsw: core: devlink health: use retained error fmsg API net/mlx5: devlink health: use retained error fmsg API qed: devlink health: use retained error fmsg API staging: qlge: devlink health: use retained error fmsg API devlink: convert most of devlink_fmsg_*() to return void drivers/net/ethernet/amd/pds_core/devlink.c | 29 +- .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 93 ++-- .../net/ethernet/huawei/hinic/hinic_devlink.c | 217 +++----- .../marvell/octeontx2/af/rvu_devlink.c | 464 +++++------------- .../mellanox/mlx5/core/diag/fw_tracer.c | 49 +- .../mellanox/mlx5/core/diag/reporter_vnic.c | 118 ++--- .../mellanox/mlx5/core/diag/reporter_vnic.h | 6 +- .../ethernet/mellanox/mlx5/core/en/health.c | 187 ++----- .../ethernet/mellanox/mlx5/core/en/health.h | 14 +- .../mellanox/mlx5/core/en/reporter_rx.c | 426 ++++------------ .../mellanox/mlx5/core/en/reporter_tx.c | 346 ++++--------- .../net/ethernet/mellanox/mlx5/core/en_rep.c | 5 +- .../net/ethernet/mellanox/mlx5/core/health.c | 127 ++--- drivers/net/ethernet/mellanox/mlxsw/core.c | 171 ++----- drivers/net/ethernet/qlogic/qed/qed_devlink.c | 6 +- drivers/net/netdevsim/health.c | 118 ++--- drivers/staging/qlge/qlge_devlink.c | 60 +-- include/net/devlink.h | 60 +-- net/devlink/health.c | 387 +++++---------- 19 files changed, 823 insertions(+), 2060 deletions(-)