netdev - Re: [PATCH net-next v3 5/5] dpll: zl3073x: Implement devlink flash callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250818192943.342ad511@kernel.org>
Date: Mon, 18 Aug 2025 19:29:43 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Ivan Vecera <ivecera@...hat.com>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Jonathan Corbet
 <corbet@....net>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, Michal Schmidt
 <mschmidt@...hat.com>, Petr Oros <poros@...hat.com>, Przemek Kitszel
 <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net-next v3 5/5] dpll: zl3073x: Implement devlink flash
 callback

On Wed, 13 Aug 2025 19:44:08 +0200 Ivan Vecera wrote:
> +	struct zl3073x_dev *zldev = devlink_priv(devlink);
> +	struct zl3073x_fw_component *util;
> +	struct zl3073x_fw *zlfw;
> +	int rc = 0;
> +
> +	/* Load firmware */

Please drop the comments which more or less repeat the name 
of the function called.

> +	zlfw = zl3073x_fw_load(zldev, params->fw->data, params->fw->size,
> +			       extack);
> +	if (IS_ERR(zlfw))
> +		return PTR_ERR(zlfw);
> +
> +	util = zlfw->component[ZL_FW_COMPONENT_UTIL];
> +	if (!util) {
> +		zl3073x_devlink_flash_notify(zldev,
> +					     "Utility is missing in firmware",
> +					     NULL, 0, 0);
> +		rc = -EOPNOTSUPP;

I'd think -EINVAL would be more appropriate.
If you want to be fancy maybe ENOEXEC ?

> +		goto error;
> +	}
> +
> +	/* Stop normal operation during flash */
> +	zl3073x_dev_stop(zldev);
> +
> +	/* Enter flashing mode */
> +	rc = zl3073x_flash_mode_enter(zldev, util->data, util->size, extack);
> +	if (!rc) {
> +		/* Flash the firmware */
> +		rc = zl3073x_fw_flash(zldev, zlfw, extack);

this error code seems to be completely ignored, no?

> +		/* Leave flashing mode */
> +		zl3073x_flash_mode_leave(zldev, extack);
> +	}
> +
> +	/* Restart normal operation */
> +	rc = zl3073x_dev_start(zldev, true);
> +	if (rc)
> +		dev_warn(zldev->dev, "Failed to re-start normal operation\n");

And also we can't really cleanly handle the failure case.

This is why I was speculating about implementing the down/up portion
in the devlink core. Add a flag that the driver requires reload_down
to be called before the flashing operation, and reload_up after.
This way not only core handles some of the error handling, but also
it can mark the device as reload_failed if things go sideways, which 
is a nicer way to surface this sort of permanent error state.

Not feeling strongly about it, but I think it'd be cleaner, so bringing
it up in case my previous comment from a while back wasn't clear.

> +error:
> +	/* Free flash context */
> +	zl3073x_fw_free(zlfw);
> +
> +	zl3073x_devlink_flash_notify(zldev,
> +				     rc ? "Flashing failed" : "Flashing done",
> +				     NULL, 0, 0);