Re: [PATCH net-next v2 3/4] r8152: Add irq mitigation for RTL8157/9

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: Add irq mitigation for RTL8157/9

This commit clears bits 0, 1, 2, and 7 of USB register 0xcf04 in
r8156_init() for RTL_VER_16 and later, with the stated intent of avoiding
intr URB completions with urb->status == -ESHUTDOWN on RTL8157 and
RTL8159.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2a07dde289e2..9fcad3dac76f 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8452,6 +8452,12 @@ static void r8156_init(struct r8152 *tp)
>  	else
>  		r8153_u2p3en(tp, false);
>  
> +	if (tp->version >= RTL_VER_16) {
> +		/* Disable Interrupt Mitigation */
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
> +				  BIT(0) | BIT(1) | BIT(2) | BIT(7));
> +	}
> +

The subject and changelog describe this as adding "irq mitigation" /
"interrupt mitigation code", but the in-tree comment says "Disable
Interrupt Mitigation" and the operation is ocp_byte_clr_bits(), which
turns a hardware feature off.

Would it be clearer to phrase the subject along the lines of "r8152:
disable hardware interrupt mitigation on RTL8157/9" so that someone
bisecting or searching the log for when this feature was "added" is not
led in the opposite direction?

The same sequence already exists in rtl8153_unload():

	static void rtl8153_unload(struct r8152 *tp)
	{
		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
			return;

		r8153_power_cut_en(tp, false);

		if (tp->version >= RTL_VER_16) {
			/* Disable Interrupt Mitigation */
			ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) | BIT(1) | BIT(2) | BIT(7));
		}
	}

That copy was introduced by fd3c7d080df5 ("r8152: Add support for the
RTL8157 hardware"). Now that the bits are cleared during init, does the
unload-path copy still serve a purpose, or can it be removed as part of
this change?

The register 0xcf04 and the bit selection BIT(0) | BIT(1) | BIT(2) |
BIT(7) are bare numeric literals, while most of the file uses named
defines (USB_MSC_TIMER, USB_U2P3_CTRL, U2P3_ENABLE, and so on). Could
this change also introduce a symbolic name for the register and for the
individual bits being cleared, so reviewers can check the mask against
the datasheet and tell which bit is the enable versus threshold versus
reserved?

On the causal chain:

The changelog links the register write to intr URB completions with
status -ESHUTDOWN / -108, but does not explain how a MAC-side
interrupt-mitigation setting produces that status.

-ESHUTDOWN on an intr URB is normally posted by the USB core when an
endpoint has been disabled or the device is disconnecting. Could the
commit message describe the mechanism by which clearing these four bits
prevents the host controller from observing endpoint shutdown, and cite
the datasheet naming for bits 0, 1, 2, and 7 of 0xcf04 so it is clear
why exactly those bits (and no others) are the correct set to clear?