On 9/11/25 09:09, Jianbo Liu wrote:
On 9/11/2025 8:45 AM, Jakub Kicinski wrote:
On Wed, 10 Sep 2025 11:23:09 +0800 Jianbo Liu wrote:
On 9/10/2025 9:23 AM, Jakub Kicinski wrote:
On Mon, 8 Sep 2025 13:07:04 +0300 Tariq Toukan wrote:
+ struct net_device *netdev = mlx5_uplink_netdev_get(dev);
+ struct mlx5e_priv *priv;
+ int err;
+
+ if (!netdev)
+ return 0;
Please don't call in variable init functions which require cleanup
or error checking.
But in this function, a NULL return from mlx5_uplink_netdev_get is a
valid condition where it should simply return 0. No cleanup or error
check is needed.
You have to check if it succeeded, and if so, you need to clean up
later. Do no hide meaningful code in variable init.
My focus was on the NULL case, but I see now that the real issue is
ensuring the corresponding cleanup (_put) happens on the successful
path. Hiding the _get call in the initializer makes that less clear.
I will refactor the code to follow the correct pattern, like this:
struct net_device *netdev;
netdev = mlx5_uplink_netdev_get(dev);
if (!netdev)
return 0;
Thank you for the explanation.
that would be much better, and make it obvious that there is
matched get() and put() calls
would be also great to minify stacktrace
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
