netdev - Re: [PATCH net-next v4 2/5] dpll: zl3073x: Add low-level flash functions

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: <2948c2d9-1600-444d-89c9-c129ddfba109@redhat.com>
Date: Sat, 6 Sep 2025 08:57:37 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
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 v4 2/5] dpll: zl3073x: Add low-level flash
 functions

On 06. 09. 25 4:19 dop., Jakub Kicinski wrote:
> On Wed,  3 Sep 2025 12:08:57 +0200 Ivan Vecera wrote:
>> +/**
>> + * zl3073x_flash_download_block - Download image block to device memory
>> + * @zldev: zl3073x device structure
>> + * @image: image to be downloaded
>> + * @start: start position (in 32-bit words)
>> + * @size: size to download (in 32-bit words)
>> + * @extack: netlink extack pointer to report errors
>> + *
>> + * Returns 0 in case of success or negative value otherwise.
>> + */
>> +static int
>> +zl3073x_flash_download(struct zl3073x_dev *zldev, const char *component,
>> +		       u32 addr, const void *data, size_t size,
>> +		       struct netlink_ext_ack *extack)
> 
> function name doesn't match kdoc, and "Returns" -> "Return:"
> 
> No idea why the kernel-doc script doesn't catch this..

Will fix...

>> +		rc = zl3073x_write_hwreg(zldev, addr, *(const u32 *)ptr);
> 
> you're sure data is 4B aligned? Otherwise get_unaligned()

Yes, this should be always aligned but you are right, using
get_unaligned() here is the safest.

>> +		if (time_after(jiffies, timeout)) {
> 
> time_after_jiffies() ?

I miss that macros, thanks for pointing out.

Anyway:
time_after(jiffies,...) -> time_is_before_jiffies(...)

Will use.

>> +			if (signal_pending(current)) {
>> +				ZL_FLASH_ERR_MSG(extack,
>> +						 "Flashing interrupted");
>> +				return -EINTR;
>> +			}
> 
> Is the flash dual-banked? Normally random signals interrupting flashing
> is recipe for bricked parts.

The download is safe operation... During this the driver downloads
block from host memory to device memory (RAM) and it is safe to break
this operation. (What should not be interrupted is flash itself (device
memory to internal flash memory).

> A little odd to use "timeout" for periodic check. check_time?

Will rename.

>> +	/* Return if no error occurred */
>> +	if (!count)
>> +		return 0;
> 
> Did I already accus^W ask you if AI helped you write this ? :D
> This level of commenting makes me think of code generators :)

:-D no, I didn't really use AI for code generation :-D

As the zl3073x is the first standalone DPLL driver I tried from the
start to write well commented code :-) But maybe I overdid it a bit :-)

> +	/* Enable host control */
> +	rc = zl3073x_flash_host_ctrl_enable(zldev);
> +	if (rc) {
> +		ZL_FLASH_ERR_MSG(extack, "cannot enable host control");
> +		goto error;
> +	}
> +
> +	zl3073x_devlink_flash_notify(zldev, "Flash mode enabled", "utility",
> +				     0, 0);
> +
> +	return 0;
> +
> +error:
> +	rc = zl3073x_flash_mode_leave(zldev, extack);
> +	if (rc)
> +		ZL_FLASH_ERR_MSG(extack,
> +				 "failed to switch back to normal mode");
> +
> +	return rc;
> 
> Should we be overriding rc here if there was an error on entering
> but we cleanly left? If so that _is_ worth commenting on..

Oops, this is an error, we should not override final rc here... Instead
of this the driver should make its best to revert back to normal mode.

Will fix this.

Thanks for the review,
Ivan