Re: [PATCH net-next v12 03/15] ax88179_178a: Fix endianness of pause watermark register

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Commit subject/message mismatch: the patch is titled
  "ax88179_178a: Fix endianness of pause watermark register" but…

--

ax88179_178a: Fix endianness of pause watermark register

This swaps the register addresses bound to AX_PAUSE_WATERLVL_LOW and
AX_PAUSE_WATERLVL_HIGH (0x54 and 0x55), and swaps the two call sites in
ax88179_reset() so that the same values keep landing in the same physical
registers as before.

> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 81d8412ce8e2f..2ed46ab62c006 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -32,8 +32,8 @@
>  #define AX_ACCESS_EEPROM			0x04
>  #define AX_ACCESS_EFUS				0x05
>  #define AX_RELOAD_EEPROM_EFUSE			0x06
> -#define AX_PAUSE_WATERLVL_HIGH			0x54
> -#define AX_PAUSE_WATERLVL_LOW			0x55
> +#define AX_PAUSE_WATERLVL_LOW			0x54
> +#define AX_PAUSE_WATERLVL_HIGH			0x55

[Severity: Medium]
Does the subject line match what this patch does?  The subject says
"Fix endianness of pause watermark register", but the only change is
renaming which macro maps to 0x54 and which maps to 0x55, plus a matching
swap of the two call sites below.

A grep of the tree at this commit shows the macros have exactly two users:

  drivers/net/usb/ax88179_178a.c:
    #define AX_PAUSE_WATERLVL_LOW    0x54
    #define AX_PAUSE_WATERLVL_HIGH   0x55
    ax88179_reset():
      ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH, 1, 1, tmp);
      ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);

Both of those are size 1 writes, and the endianness conversion in
ax88179_write_cmd() only runs for size 2:

  static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
                               u16 size, const void *data)
  {
          if (2 == size) {
                  u16 buf;
                  buf = *((u16 *)data);
                  cpu_to_le16s(&buf);
                  ret = __ax88179_write_cmd(dev, cmd, value, index,
                                            size, &buf);
          } else {
  ...

So before and after this patch the driver writes 0x34 to MAC register 0x55
and 0x52 to MAC register 0x54, and no 16-bit access to 0x54/0x55 exists in
the tree at this point.  The commit message itself says the change is
behaviour preserving:

  "but also swap the configuration values used in the code in order to
   keep the current behaviour"
  "The endianness is relevant for 16-bit writes to the register."

Would a subject describing this as a correction of the register byte-order
definitions, or as preparation for the later 16-bit writes, be more
accurate?  As written, the "Fix ..." subject with no Fixes: tag reads like a
bug fix on a path that is not reachable yet, which could attract a stable
backport of a no-op change.

>  #define PHYSICAL_LINK_STATUS			0x02
>  	#define	AX_USB_SS		0x04
> @@ -1619,11 +1619,10 @@ static int ax88179_reset(struct usbnet *dev)
>  	dev->rx_urb_size = 1024 * 20;
>  
>  	*tmp = 0x34;
> -	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH, 1, 1, tmp);
>  
>  	*tmp = 0x52;
> -	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH,
> -			  1, 1, tmp);
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de