Re: [PATCH v2 1/4] USB: serial: mxuport: clean up firmware version handling

[Johan Hovold <johan@kernel.org>]

On Tue, Jun 23, 2026 at 04:01:36PM +0800, Crescent Hsieh wrote:
> Add a small helper for parsing firmware versions and use it for the
> bundled firmware image. This avoids open-coded firmware image offsets
> in probe() and validates the image size before reading the version
> bytes.
> 
> Keep the existing version comparison policy unchanged, but store the
> version components explicitly so that the version can be printed
> without unpacking a raw integer.
> 
> Print the firmware version fields in decimal, as these fields represent
> readable version components rather than hexadecimal values.
> 
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---

Thanks for the v2. You seem to have addressed all my comments on v1 of
the series, which is unfortunately not as common as I wish it were. :)

> @@ -82,7 +95,6 @@
>  
>  #define RQ_VENDOR_RESET_DEVICE		0x23 /* Try to reset the device */
>  #define RQ_VENDOR_QUERY_FW_CONFIG	0x24
> -

Nit: this is an unrelated change

I see you add another define here in the next patch, but perhaps you
should keep the newline separators before and after that one.

>  #define RQ_VENDOR_GET_VERSION		0x81 /* Get firmware version */
>  #define RQ_VENDOR_GET_PAGE		0x82 /* Read flash page */
>  #define RQ_VENDOR_GET_ROM_PROC		0x83 /* Get ROM process state */
> @@ -970,8 +982,32 @@ static int mxuport_calc_num_ports(struct usb_serial *serial,
>  	return num_ports;
>  }
>  
> +static void mxuport_set_fw_version(struct mxuport_fw_version *version,
> +				   u8 major, u8 minor, u8 build)

The naming is a bit unfortunate as this is not an inverse of
mxuport_get_fw_version() but that should be ok.

> +{
> +	version->major = major;
> +	version->minor = minor;
> +	version->build = build;
> +	version->value = (major << 16) | (minor << 8) | build;
> +}
> +
> +static int mxuport_parse_fw_version(const struct firmware *fw,
> +				    const u16 *offsets,
> +				    struct mxuport_fw_version *version)
> +{
> +	if (fw->size <= offsets[0] || fw->size <= offsets[1] ||
> +	    fw->size <= offsets[2])
> +		return -EINVAL;

I just merged this sanity check:

	https://lore.kernel.org/all/20260715084611.45995-1-pengpeng@iscas.ac.cn/

so you may want to rebase on my usb-linus branch if there is a v3,
otherwise I can fix it up.

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-linus

> +
> +	mxuport_set_fw_version(version, fw->data[offsets[0]],
> +			       fw->data[offsets[1]], fw->data[offsets[2]]);
> +
> +	return 0;
> +}
> +
>  /* Get the version of the firmware currently running. */
> -static int mxuport_get_fw_version(struct usb_serial *serial, u32 *version)
> +static int mxuport_get_fw_version(struct usb_serial *serial,
> +				  struct mxuport_fw_version *version)
>  {
>  	u8 *ver_buf;
>  	int err;
> @@ -988,7 +1024,7 @@ static int mxuport_get_fw_version(struct usb_serial *serial, u32 *version)
>  		goto out;
>  	}
>  
> -	*version = (ver_buf[0] << 16) | (ver_buf[1] << 8) | ver_buf[2];
> +	mxuport_set_fw_version(version, ver_buf[0], ver_buf[1], ver_buf[2]);
>  	err = 0;
>  out:
>  	kfree(ver_buf);
> @@ -1049,8 +1085,7 @@ static int mxuport_probe(struct usb_serial *serial,
>  {
>  	u16 productid = le16_to_cpu(serial->dev->descriptor.idProduct);
>  	const struct firmware *fw_p = NULL;
> -	u32 version;
> -	int local_ver;
> +	struct mxuport_fw_version version, local_ver;

Nit: I'd move this line above fw_p to maintain reverse xmas style of the
declarations.

>  	char buf[32];
>  	int err;

Johan