From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:06
From: Leon Romanovsky <leonro@nvidia.com>
Hi,
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
It will cancel the need to deal with merge conflicts for my next devlink series :).
Thanks
Leon Romanovsky (6):
bnxt_en: Check devlink allocation and registration status
bnxt_en: Properly remove port parameter support
devlink: Delete not used port parameters APIs
devlink: Remove single line function obfuscations
ice: Delete always true check of PF pointer
qed: Don't ignore devlink allocation failures
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +-
.../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 26 +---
.../net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 --
drivers/net/ethernet/intel/ice/ice_main.c | 3 -
drivers/net/ethernet/qlogic/qede/qede_main.c | 12 +-
drivers/scsi/qedf/qedf_main.c | 2 +
include/net/devlink.h | 6 -
net/core/devlink.c | 123 +++++-------------
8 files changed, 47 insertions(+), 143 deletions(-)
--
2.31.1
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:11
From: Leon Romanovsky <leonro@nvidia.com>
devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.
Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.
Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
3 files changed, 10 insertions(+), 21 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:15
From: Leon Romanovsky <leonro@nvidia.com>
There is no in-kernel users for the devlink port parameters API,
so let's remove it.
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
include/net/devlink.h | 6 ------
net/core/devlink.c | 42 ------------------------------------------
2 files changed, 48 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:18
From: Leon Romanovsky <leonro@nvidia.com>
There is no need in extra one line functions to call relevant
functions only once.
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
net/core/devlink.c | 89 ++++++++++++++++++----------------------------
1 file changed, 34 insertions(+), 55 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:30
From: Leon Romanovsky <leonro@nvidia.com>
This driver doesn't have any port parameters and registers
devlink port parameters with empty table. Remove the useless
calls to devlink_port_params_register and _unregister.
Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
1 file changed, 13 deletions(-)
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:35
From: Leon Romanovsky <leonro@nvidia.com>
devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.
Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.
Fixes: 755f982bb1ff ("qed/qede: make devlink survive recovery")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/qlogic/qede/qede_main.c | 12 +++++-------
drivers/scsi/qedf/qedf_main.c | 2 ++
2 files changed, 7 insertions(+), 7 deletions(-)
@@ -1176,19 +1176,17 @@ static int __qede_probe(struct pci_dev *pdev, u32 dp_module, u8 dp_level,edev->devlink=qed_ops->common->devlink_register(cdev);if(IS_ERR(edev->devlink)){DP_NOTICE(edev,"Cannot register devlink\n");+rc=PTR_ERR(edev->devlink);edev->devlink=NULL;-/* Go on, we can live without devlink */+gotoerr3;}}else{structnet_device*ndev=pci_get_drvdata(pdev);+structqed_devlink*qdl;edev=netdev_priv(ndev);--if(edev->devlink){-structqed_devlink*qdl=devlink_priv(edev->devlink);--qdl->cdev=cdev;-}+qdl=devlink_priv(edev->devlink);+qdl->cdev=cdev;edev->cdev=cdev;memset(&edev->stats,0,sizeof(edev->stats));memcpy(&edev->dev_info,&dev_info,sizeof(dev_info));
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 18:13:38
From: Leon Romanovsky <leonro@nvidia.com>
PF pointer is always valid when PCI core calls its .shutdown() and
.remove() callbacks. There is no need to check it again.
Fixes: 837f08fdecbe ("ice: Add basic driver framework for Intel(R) E800 Series")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 3 ---
1 file changed, 3 deletions(-)
On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky [off-list ref] wrote:
quoted hunk
From: Leon Romanovsky <leonro@nvidia.com>
devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.
Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.
Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
3 files changed, 10 insertions(+), 21 deletions(-)
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
quoted hunk
if (BNXT_PF(bp)) {
bnxt_dl_params_unregister(bp);
devlink_port_unregister(&bp->dl_port);
On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky [off-list ref] wrote:
quoted hunk
From: Leon Romanovsky <leonro@nvidia.com>
This driver doesn't have any port parameters and registers
devlink port parameters with empty table. Remove the useless
calls to devlink_port_params_register and _unregister.
Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
1 file changed, 13 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-09-23 22:55:53
On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
It will cancel the need to deal with merge conflicts for my next devlink series :).
Not sure how Dave will feel about adding fixes to net-next,
we do merge the trees weekly after all.
Otherwise the patches look fine.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 23:11:29
On Thu, Sep 23, 2021 at 02:11:40PM -0700, Edwin Peer wrote:
On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky [off-list ref] wrote:
quoted
From: Leon Romanovsky <leonro@nvidia.com>
devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.
Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.
Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
3 files changed, 10 insertions(+), 21 deletions(-)
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.
<...>
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-23 23:16:16
On Thu, Sep 23, 2021 at 03:55:47PM -0700, Jakub Kicinski wrote:
On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
quoted
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
It will cancel the need to deal with merge conflicts for my next devlink series :).
Not sure how Dave will feel about adding fixes to net-next,
we do merge the trees weekly after all.
My almost ready submission queue is:
➜ kernel git:(m/devlink) git l
693c1a9ac5b3 (HEAD -> m/devlink) devlink: Delete reload enable/disable interface
6d39354f8b44 net/mlx5: Register separate reload devlink ops for multiport device
1ac4e8811fd5 devlink: Allow set specific ops callbacks dynamically
de1849d3b348 devlink: Allow modification of devlink ops
7439a45dce72 net: dsa: Move devlink registration to be last devlink command
7dd23a327395 staging: qlge: Move devlink registration to be last devlink command
77f074c98b0d ptp: ocp: Move devlink registration to be last devlink command
fb3f4d40ad49 net: wwan: iosm: Move devlink_register to be last devlink command
87e95ee9275b netdevsim: Move devlink registration to be last devlink command
4173205af399 net: ethernet: ti: Move devlink registration to be last devlink command
bc633a0759f6 qed: Move devlink registration to be last devlink command
ead4e2027164 ionic: Move devlink registration to be last devlink command
bc5272ccc378 nfp: Move delink_register to be last command
a6521bf133d9 net: mscc: ocelot: delay devlink registration to the end
e0ca9a29cc20 mlxsw: core: Register devlink instance last
681ac1457516 net/mlx5: Accept devlink user input after driver initialization complete
9b1a2f4abaef net/mlx4: Move devlink_register to be the last initialization command
a3b2d9a95a51 net/prestera: Split devlink and traps registrations to separate routines
bbdf4842432f octeontx2: Move devlink registration to be last devlink command
5297e23f19e9 ice: Open devlink when device is ready
18af77a99cea net: hinic: Open device for the user access when it is ready
91a03cdc92e2 dpaa2-eth: Register devlink instance at the end of probe
dd5af984e53c liquidio: Overcome missing device lock protection in init/remove flows
efea109ba32e bnxt_en: Register devlink instance at the end devlink configuration
6a2b139bcf01 devlink: Notify users when objects are accessible
+ a couple of patches that removes "published" field from devlink parameters
and fix of old devlink bug where parameters were netlink notifications
were sent twice.
So it will be very helpful to keep this series in net-next.
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.
From: Leon Romanovsky <leonro@nvidia.com>
Hi,
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
It will cancel the need to deal with merge conflicts for my next devlink series :).
ok, but just this one time.
I much rather this kind of stuff goes to net and we deal with the merge
conflicts that arise.
Thsnks!
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Thu, 23 Sep 2021 21:12:47 +0300 you wrote:
From: Leon Romanovsky <leonro@nvidia.com>
Hi,
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
[...]
On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski [off-list ref] wrote:
On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
quoted
quoted
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.
+1, fwiw
I appreciate that being more explicit can improve visibility, but it
does not make error handling inherently less error prone, nor is it
simpler (ie. the opposite isn't true). Idempotency is orthogonal to
unwind flow or the presence or not of a particular unwind handler (one
can still enforce either in review). But, if release handlers are
independent (most in bnxt are), then permitting other orderings can be
perfectly valid and places less burden on achieving the canonical form
for correctness (ie. usage is simpler and less error prone). That's
not to say we should throw caution to the wind and allow arbitrary
unwind flows, but it does mean certain mistakes don't result in actual
bugs. There are other flexibility benefits too. A single, unwind
everything, handler can be reused in more than one context.
That said, isn't the more important question what style and
assumptions the surrounding code has adopted? In this particular case,
I checked that this change wouldn't introduce the possibility of a
double unwind, but in other contexts in this driver code base,
changing error handling in this piecemeal way might actually introduce
a bug in contexts where the caller has assumed the overall function is
idempotent. Isn't local consistency of style a more important concern,
especially given that you are not predominantly responsible for
maintenance of this driver? Dealing with this exception to the norm in
our driver certainly places an additional burden on us to remember to
treat this particular case with special care. We should either rework
all of bnxt error handling to adopt the more accepted canonical form,
or we should adopt the surrounding conventions. What we shouldn't do
is mix approaches in one driver.
Regards,
Edwin Peer
From: Leon Romanovsky <leonro@nvidia.com>
Hi,
I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.
It will cancel the need to deal with merge conflicts for my next devlink series :).
ok, but just this one time.
Thanks
I much rather this kind of stuff goes to net and we deal with the merge
conflicts that arise.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-09-25 10:01:44
On Fri, Sep 24, 2021 at 10:20:32AM -0700, Edwin Peer wrote:
On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
quoted
quoted
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.
+1, fwiw
I appreciate that being more explicit can improve visibility, but it
does not make error handling inherently less error prone, nor is it
simpler (ie. the opposite isn't true). Idempotency is orthogonal to
unwind flow or the presence or not of a particular unwind handler (one
can still enforce either in review). But, if release handlers are
independent (most in bnxt are), then permitting other orderings can be
perfectly valid and places less burden on achieving the canonical form
for correctness (ie. usage is simpler and less error prone). That's
not to say we should throw caution to the wind and allow arbitrary
unwind flows, but it does mean certain mistakes don't result in actual
bugs. There are other flexibility benefits too. A single, unwind
everything, handler can be reused in more than one context.
And this is where the fun begins. Different context means different
lifetime expectations, maybe need of locking and unpredictable flows
from reader perspective.
For example, in this devlink case, it took me time to check all driver
to see that pf can't be null.
The idea that adding code that maybe will be used can be seen as
anti-pattern.
Thanks