These patches are used to fix some issues for RTL8153.
Hayes Wang (9):
r8152: fix runtime resume for linking change
r8152: reset flow control patch when linking on for RTL8153B
r8152: get default setting of WOL before initializing
r8152: disable U2P3 for RTL8153B
r8152: Disable PLA MCU clock speed down
r8152: disable test IO for RTL8153B
r8152: don't enable U1U2 with USB_SPEED_HIGH for RTL8153B
r8152: avoid the MCU to clear the lanwake
r8152: disable DelayPhyPwrChg
drivers/net/usb/r8152.c | 124 ++++++++++++++++++++++++++++++++++++----
1 file changed, 113 insertions(+), 11 deletions(-)
--
2.21.0
Enable DelayPhyPwrChg let the device fail enter the power saving mode
of P3.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0998b9587943..c999a58ddda9 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -31,7 +31,7 @@
#define NETNEXT_VERSION "11"
/* Information for net */
-#define NET_VERSION "10"
+#define NET_VERSION "11"
#define DRIVER_VERSION "v1." NETNEXT_VERSION "." NET_VERSION
#define DRIVER_AUTHOR "Realtek linux nic maintainers <[email protected]>"
@@ -109,6 +109,7 @@
#define PLA_BP_EN 0xfc38
#define USB_USB2PHY 0xb41e
+#define USB_SSPHYLINK1 0xb426
#define USB_SSPHYLINK2 0xb428
#define USB_U2P3_CTRL 0xb460
#define USB_CSR_DUMMY1 0xb464
@@ -384,6 +385,9 @@
#define USB2PHY_SUSPEND 0x0001
#define USB2PHY_L1 0x0002
+/* USB_SSPHYLINK1 */
+#define DELAY_PHY_PWR_CHG BIT(1)
+
/* USB_SSPHYLINK2 */
#define pwd_dn_scale_mask 0x3ffe
#define pwd_dn_scale(x) ((x) << 1)
@@ -4993,6 +4997,10 @@ static void rtl8153_up(struct r8152 *tp)
ocp_data &= ~LANWAKE_PIN;
ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1);
+ ocp_data &= ~DELAY_PHY_PWR_CHG;
+ ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1, ocp_data);
+
r8153_aldps_en(tp, true);
switch (tp->version) {
--
2.21.0
When linking ON, the patch of flow control has to be reset. This
makes sure the patch works normally.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 115559707683..64efd58279b3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2857,6 +2857,7 @@ static void r8153_set_rx_early_size(struct r8152 *tp)
static int rtl8153_enable(struct r8152 *tp)
{
+ u32 ocp_data;
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return -ENODEV;
@@ -2865,6 +2866,15 @@ static int rtl8153_enable(struct r8152 *tp)
r8153_set_rx_early_timeout(tp);
r8153_set_rx_early_size(tp);
+ if (tp->version == RTL_VER_09) {
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_TASK);
+ ocp_data &= ~FC_PATCH_TASK;
+ ocp_write_word(tp, MCU_TYPE_USB, USB_FW_TASK, ocp_data);
+ usleep_range(1000, 2000);
+ ocp_data |= FC_PATCH_TASK;
+ ocp_write_word(tp, MCU_TYPE_USB, USB_FW_TASK, ocp_data);
+ }
+
return rtl_enable(tp);
}
--
2.21.0
From: Hayes Wang <[email protected]>
Date: Tue, 21 Jan 2020 20:40:28 +0800
> When linking ON, the patch of flow control has to be reset. This
> makes sure the patch works normally.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 115559707683..64efd58279b3 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -2857,6 +2857,7 @@ static void r8153_set_rx_early_size(struct r8152 *tp)
>
> static int rtl8153_enable(struct r8152 *tp)
> {
> + u32 ocp_data;
> if (test_bit(RTL8152_UNPLUG, &tp->flags))
> return -ENODEV;
>
Please put an empty line after the local variable declarations.
Thank you.
On Tue, 2020-01-21 at 13:54 +0100, David Miller wrote:
> From: Hayes Wang <[email protected]>
> Date: Tue, 21 Jan 2020 20:40:28 +0800
>
> > When linking ON, the patch of flow control has to be reset. This
> > makes sure the patch works normally.
[]
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
[]
> > @@ -2857,6 +2857,7 @@ static void r8153_set_rx_early_size(struct r8152 *tp)
> >
> > static int rtl8153_enable(struct r8152 *tp)
> > {
> > + u32 ocp_data;
> > if (test_bit(RTL8152_UNPLUG, &tp->flags))
> > return -ENODEV;
> >
>
> Please put an empty line after the local variable declarations.
Local scoping is generally better.
Perhaps declare ocp_data inside the if branch
where it's used.
Joe Perches [mailto:[email protected]]
> Sent: Tuesday, January 21, 2020 9:01 PM
> To: David Miller; Hayes Wang
[...]
> > > static int rtl8153_enable(struct r8152 *tp)
> > > {
> > > + u32 ocp_data;
> > > if (test_bit(RTL8152_UNPLUG, &tp->flags))
> > > return -ENODEV;
> > >
> >
> > Please put an empty line after the local variable declarations.
>
> Local scoping is generally better.
>
> Perhaps declare ocp_data inside the if branch
> where it's used.
OK. I would move it.
Best Regards,
Hayes
v2:
For patch #2, move declaring the variable "ocp_data".
v1:
These patches are used to fix some issues for RTL8153.
Hayes Wang (9):
r8152: fix runtime resume for linking change
r8152: reset flow control patch when linking on for RTL8153B
r8152: get default setting of WOL before initializing
r8152: disable U2P3 for RTL8153B
r8152: Disable PLA MCU clock speed down
r8152: disable test IO for RTL8153B
r8152: don't enable U1U2 with USB_SPEED_HIGH for RTL8153B
r8152: avoid the MCU to clear the lanwake
r8152: disable DelayPhyPwrChg
drivers/net/usb/r8152.c | 124 ++++++++++++++++++++++++++++++++++++----
1 file changed, 113 insertions(+), 11 deletions(-)
--
2.21.0
Initailization would reset runtime suspend by tp->saved_wolopts, so
the tp->saved_wolopts should be set before initializing.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 504db2348a3e..c3217a5c2fe1 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6739,6 +6739,11 @@ static int rtl8152_probe(struct usb_interface *intf,
intf->needs_remote_wakeup = 1;
+ if (!rtl_can_wakeup(tp))
+ __rtl_set_wol(tp, 0);
+ else
+ tp->saved_wolopts = __rtl_get_wol(tp);
+
tp->rtl_ops.init(tp);
#if IS_BUILTIN(CONFIG_USB_RTL8152)
/* Retry in case request_firmware() is not ready yet. */
@@ -6756,10 +6761,6 @@ static int rtl8152_probe(struct usb_interface *intf,
goto out1;
}
- if (!rtl_can_wakeup(tp))
- __rtl_set_wol(tp, 0);
-
- tp->saved_wolopts = __rtl_get_wol(tp);
if (tp->saved_wolopts)
device_set_wakeup_enable(&udev->dev, true);
else
--
2.21.0
Enable U2P3 may miss zero packet for bulk-in.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c3217a5c2fe1..bc6b2f8aaa7e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3389,7 +3389,6 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable)
r8153b_ups_en(tp, false);
r8153_queue_wake(tp, false);
rtl_runtime_suspend_enable(tp, false);
- r8153_u2p3en(tp, true);
r8153b_u1u2en(tp, true);
}
}
@@ -4688,7 +4687,6 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
r8153_aldps_en(tp, true);
r8152b_enable_fc(tp);
- r8153_u2p3en(tp, true);
set_bit(PHY_RESET, &tp->flags);
}
@@ -5018,7 +5016,6 @@ static void rtl8153b_up(struct r8152 *tp)
ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
r8153_aldps_en(tp, true);
- r8153_u2p3en(tp, true);
r8153b_u1u2en(tp, true);
}
--
2.21.0
For RTL8153B with QFN32, disable test IO. Otherwise, it may casue
abnormal behavior for the device randomly.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1fb85c79bd33..7efeddad1fc8 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -312,6 +312,7 @@
/* PLA_PHY_PWR */
#define TX_10M_IDLE_EN 0x0080
#define PFM_PWM_SWITCH 0x0040
+#define TEST_IO_OFF BIT(4)
/* PLA_MAC_PWR_CTRL */
#define D3_CLK_GATED_EN 0x00004000
@@ -5538,6 +5539,15 @@ static void r8153b_init(struct r8152 *tp)
ocp_data &= ~PLA_MCU_SPDWN_EN;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+ if (tp->version == RTL_VER_09) {
+ /* Disable Test IO for 32QFN */
+ if (ocp_read_byte(tp, MCU_TYPE_PLA, 0xdc00) & BIT(5)) {
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+ ocp_data |= TEST_IO_OFF;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
+ }
+ }
+
set_bit(GREEN_ETHERNET, &tp->flags);
/* rx aggregation */
--
2.21.0
PLA MCU clock speed down could only be enabled when tx/rx are disabled.
Otherwise, the packet lost may occur.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index bc6b2f8aaa7e..1fb85c79bd33 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -324,6 +324,7 @@
#define MAC_CLK_SPDWN_EN BIT(15)
/* PLA_MAC_PWR_CTRL3 */
+#define PLA_MCU_SPDWN_EN BIT(14)
#define PKT_AVAIL_SPDWN_EN 0x0100
#define SUSPEND_SPDWN_EN 0x0004
#define U1U2_SPDWN_EN 0x0002
@@ -5005,6 +5006,8 @@ static void rtl8153_down(struct r8152 *tp)
static void rtl8153b_up(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return;
@@ -5015,17 +5018,27 @@ static void rtl8153b_up(struct r8152 *tp)
r8153_first_init(tp);
ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data &= ~PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
r8153_aldps_en(tp, true);
r8153b_u1u2en(tp, true);
}
static void rtl8153b_down(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
rtl_drop_queued_tx(tp);
return;
}
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data |= PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
r8153b_u1u2en(tp, false);
r8153_u2p3en(tp, false);
r8153b_power_cut_en(tp, false);
@@ -5521,6 +5534,10 @@ static void r8153b_init(struct r8152 *tp)
ocp_data |= MAC_CLK_SPDWN_EN;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2, ocp_data);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data &= ~PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
set_bit(GREEN_ETHERNET, &tp->flags);
/* rx aggregation */
--
2.21.0
For certain platforms, it causes USB reset periodically.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 7efeddad1fc8..b1a00f29455b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3391,7 +3391,8 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable)
r8153b_ups_en(tp, false);
r8153_queue_wake(tp, false);
rtl_runtime_suspend_enable(tp, false);
- r8153b_u1u2en(tp, true);
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
}
}
@@ -5024,7 +5025,9 @@ static void rtl8153b_up(struct r8152 *tp)
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
r8153_aldps_en(tp, true);
- r8153b_u1u2en(tp, true);
+
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
}
static void rtl8153b_down(struct r8152 *tp)
@@ -5527,7 +5530,9 @@ static void r8153b_init(struct r8152 *tp)
ocp_data &= ~CUR_LINK_OK;
ocp_data |= POLL_LINK_CHG;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
- r8153b_u1u2en(tp, true);
+
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
usb_enable_lpm(tp->udev);
/* MAC clock speed down */
--
2.21.0
Avoid the MCU to clear the lanwake after suspending. It may cause the
WOL fail. Disable LANWAKE_CLR_EN before suspending. Besides,enable it
and reset the lanwake status when resuming or initializing.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b1a00f29455b..c037fc7adcea 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -68,6 +68,7 @@
#define PLA_LED_FEATURE 0xdd92
#define PLA_PHYAR 0xde00
#define PLA_BOOT_CTRL 0xe004
+#define PLA_LWAKE_CTRL_REG 0xe007
#define PLA_GPHY_INTR_IMR 0xe022
#define PLA_EEE_CR 0xe040
#define PLA_EEEP_CR 0xe080
@@ -95,6 +96,7 @@
#define PLA_TALLYCNT 0xe890
#define PLA_SFF_STS_7 0xe8de
#define PLA_PHYSTATUS 0xe908
+#define PLA_CONFIG6 0xe90a /* CONFIG6 */
#define PLA_BP_BA 0xfc26
#define PLA_BP_0 0xfc28
#define PLA_BP_1 0xfc2a
@@ -300,6 +302,9 @@
#define LINK_ON_WAKE_EN 0x0010
#define LINK_OFF_WAKE_EN 0x0008
+/* PLA_CONFIG6 */
+#define LANWAKE_CLR_EN BIT(0)
+
/* PLA_CONFIG5 */
#define BWF_EN 0x0040
#define MWF_EN 0x0020
@@ -356,6 +361,9 @@
/* PLA_BOOT_CTRL */
#define AUTOLOAD_DONE 0x0002
+/* PLA_LWAKE_CTRL_REG */
+#define LANWAKE_PIN BIT(7)
+
/* PLA_SUSPEND_FLAG */
#define LINK_CHG_EVENT BIT(0)
@@ -4968,6 +4976,8 @@ static void rtl8152_down(struct r8152 *tp)
static void rtl8153_up(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return;
@@ -4975,6 +4985,15 @@ static void rtl8153_up(struct r8152 *tp)
r8153_u2p3en(tp, false);
r8153_aldps_en(tp, false);
r8153_first_init(tp);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data |= LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+ ocp_data &= ~LANWAKE_PIN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+
r8153_aldps_en(tp, true);
switch (tp->version) {
@@ -4993,11 +5012,17 @@ static void rtl8153_up(struct r8152 *tp)
static void rtl8153_down(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
rtl_drop_queued_tx(tp);
return;
}
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data &= ~LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
r8153_u1u2en(tp, false);
r8153_u2p3en(tp, false);
r8153_power_cut_en(tp, false);
@@ -5458,6 +5483,14 @@ static void r8153_init(struct r8152 *tp)
r8153_mac_clk_spd(tp, false);
usb_enable_lpm(tp->udev);
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data |= LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+ ocp_data &= ~LANWAKE_PIN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+
/* rx aggregation */
ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
--
2.21.0
Enable DelayPhyPwrChg let the device fail enter the power saving mode
of P3.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c037fc7adcea..3f425f974d03 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -31,7 +31,7 @@
#define NETNEXT_VERSION "11"
/* Information for net */
-#define NET_VERSION "10"
+#define NET_VERSION "11"
#define DRIVER_VERSION "v1." NETNEXT_VERSION "." NET_VERSION
#define DRIVER_AUTHOR "Realtek linux nic maintainers <[email protected]>"
@@ -109,6 +109,7 @@
#define PLA_BP_EN 0xfc38
#define USB_USB2PHY 0xb41e
+#define USB_SSPHYLINK1 0xb426
#define USB_SSPHYLINK2 0xb428
#define USB_U2P3_CTRL 0xb460
#define USB_CSR_DUMMY1 0xb464
@@ -384,6 +385,9 @@
#define USB2PHY_SUSPEND 0x0001
#define USB2PHY_L1 0x0002
+/* USB_SSPHYLINK1 */
+#define DELAY_PHY_PWR_CHG BIT(1)
+
/* USB_SSPHYLINK2 */
#define pwd_dn_scale_mask 0x3ffe
#define pwd_dn_scale(x) ((x) << 1)
@@ -4994,6 +4998,10 @@ static void rtl8153_up(struct r8152 *tp)
ocp_data &= ~LANWAKE_PIN;
ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1);
+ ocp_data &= ~DELAY_PHY_PWR_CHG;
+ ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1, ocp_data);
+
r8153_aldps_en(tp, true);
switch (tp->version) {
--
2.21.0
On Tue, Jan 21, 2020 at 4:43 AM Hayes Wang <[email protected]> wrote:
>
> Enable DelayPhyPwrChg let the device fail enter the power saving mode
> of P3.
Hayes,
I'm very curious about this commit message: why would one want this to fail?
Did you mean "don't allow the phy to enter P3 power saving mode"?
If P3 power saving mode is broken, what is the symptom?
How long is the delay when this is still enabled? (to help identify
failures when this is still enabled)
BTW, I've reviewed all the patches and don't see any obvious issues
with them - though I don't have the technical documents to verify any
changes in behavior.
I did see two typos in the commit messages that could be corrected if
you need to send out v3:
[PATCH net 5/9] r8152: Disable PLA MCU clock speed down
s/packet lost/packet loss/
[PATCH net 6/9] r8152: disable test IO for RTL8153B
s/casue/cause
cheers,
grant
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 0998b9587943..c999a58ddda9 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -31,7 +31,7 @@
> #define NETNEXT_VERSION "11"
>
> /* Information for net */
> -#define NET_VERSION "10"
> +#define NET_VERSION "11"
>
> #define DRIVER_VERSION "v1." NETNEXT_VERSION "." NET_VERSION
> #define DRIVER_AUTHOR "Realtek linux nic maintainers <[email protected]>"
> @@ -109,6 +109,7 @@
> #define PLA_BP_EN 0xfc38
>
> #define USB_USB2PHY 0xb41e
> +#define USB_SSPHYLINK1 0xb426
> #define USB_SSPHYLINK2 0xb428
> #define USB_U2P3_CTRL 0xb460
> #define USB_CSR_DUMMY1 0xb464
> @@ -384,6 +385,9 @@
> #define USB2PHY_SUSPEND 0x0001
> #define USB2PHY_L1 0x0002
>
> +/* USB_SSPHYLINK1 */
> +#define DELAY_PHY_PWR_CHG BIT(1)
> +
> /* USB_SSPHYLINK2 */
> #define pwd_dn_scale_mask 0x3ffe
> #define pwd_dn_scale(x) ((x) << 1)
> @@ -4993,6 +4997,10 @@ static void rtl8153_up(struct r8152 *tp)
> ocp_data &= ~LANWAKE_PIN;
> ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
>
> + ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1);
> + ocp_data &= ~DELAY_PHY_PWR_CHG;
> + ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1, ocp_data);
> +
> r8153_aldps_en(tp, true);
>
> switch (tp->version) {
> --
> 2.21.0
>
Grant Grundler [mailto:[email protected]]
> Sent: Wednesday, January 22, 2020 3:03 PM
[...]
> Did you mean "don't allow the phy to enter P3 power saving mode"?
> If P3 power saving mode is broken, what is the symptom?
Yes.
It increases power consumption.
PS. the "phy" is for USB, not for Ethernet.
> How long is the delay when this is still enabled? (to help identify
> failures when this is still enabled)
When this is enable, the device would wait an internal signal which
wouldn't be triggered. Then, the device couldn't enter P3 mode, so the
power consumption is increased.
Best Regards,
Hayes
v3:
1. Fix the typos for patch #5 and #6.
2. Modify the commit message of patch #9.
v2:
For patch #2, move declaring the variable "ocp_data".
v1:
These patches are used to fix some issues for RTL8153.
Hayes Wang (9):
r8152: fix runtime resume for linking change
r8152: reset flow control patch when linking on for RTL8153B
r8152: get default setting of WOL before initializing
r8152: disable U2P3 for RTL8153B
r8152: Disable PLA MCU clock speed down
r8152: disable test IO for RTL8153B
r8152: don't enable U1U2 with USB_SPEED_HIGH for RTL8153B
r8152: avoid the MCU to clear the lanwake
r8152: disable DelayPhyPwrChg
drivers/net/usb/r8152.c | 124 ++++++++++++++++++++++++++++++++++++----
1 file changed, 113 insertions(+), 11 deletions(-)
--
2.21.0
Enable U2P3 may miss zero packet for bulk-in.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c3217a5c2fe1..bc6b2f8aaa7e 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3389,7 +3389,6 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable)
r8153b_ups_en(tp, false);
r8153_queue_wake(tp, false);
rtl_runtime_suspend_enable(tp, false);
- r8153_u2p3en(tp, true);
r8153b_u1u2en(tp, true);
}
}
@@ -4688,7 +4687,6 @@ static void r8153b_hw_phy_cfg(struct r8152 *tp)
r8153_aldps_en(tp, true);
r8152b_enable_fc(tp);
- r8153_u2p3en(tp, true);
set_bit(PHY_RESET, &tp->flags);
}
@@ -5018,7 +5016,6 @@ static void rtl8153b_up(struct r8152 *tp)
ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
r8153_aldps_en(tp, true);
- r8153_u2p3en(tp, true);
r8153b_u1u2en(tp, true);
}
--
2.21.0
PLA MCU clock speed down could only be enabled when tx/rx are disabled.
Otherwise, the packet loss may occur.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index bc6b2f8aaa7e..1fb85c79bd33 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -324,6 +324,7 @@
#define MAC_CLK_SPDWN_EN BIT(15)
/* PLA_MAC_PWR_CTRL3 */
+#define PLA_MCU_SPDWN_EN BIT(14)
#define PKT_AVAIL_SPDWN_EN 0x0100
#define SUSPEND_SPDWN_EN 0x0004
#define U1U2_SPDWN_EN 0x0002
@@ -5005,6 +5006,8 @@ static void rtl8153_down(struct r8152 *tp)
static void rtl8153b_up(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return;
@@ -5015,17 +5018,27 @@ static void rtl8153b_up(struct r8152 *tp)
r8153_first_init(tp);
ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data &= ~PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
r8153_aldps_en(tp, true);
r8153b_u1u2en(tp, true);
}
static void rtl8153b_down(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
rtl_drop_queued_tx(tp);
return;
}
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data |= PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
r8153b_u1u2en(tp, false);
r8153_u2p3en(tp, false);
r8153b_power_cut_en(tp, false);
@@ -5521,6 +5534,10 @@ static void r8153b_init(struct r8152 *tp)
ocp_data |= MAC_CLK_SPDWN_EN;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2, ocp_data);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+ ocp_data &= ~PLA_MCU_SPDWN_EN;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
set_bit(GREEN_ETHERNET, &tp->flags);
/* rx aggregation */
--
2.21.0
For RTL8153B with QFN32, disable test IO. Otherwise, it may cause
abnormal behavior for the device randomly.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1fb85c79bd33..7efeddad1fc8 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -312,6 +312,7 @@
/* PLA_PHY_PWR */
#define TX_10M_IDLE_EN 0x0080
#define PFM_PWM_SWITCH 0x0040
+#define TEST_IO_OFF BIT(4)
/* PLA_MAC_PWR_CTRL */
#define D3_CLK_GATED_EN 0x00004000
@@ -5538,6 +5539,15 @@ static void r8153b_init(struct r8152 *tp)
ocp_data &= ~PLA_MCU_SPDWN_EN;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+ if (tp->version == RTL_VER_09) {
+ /* Disable Test IO for 32QFN */
+ if (ocp_read_byte(tp, MCU_TYPE_PLA, 0xdc00) & BIT(5)) {
+ ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR);
+ ocp_data |= TEST_IO_OFF;
+ ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data);
+ }
+ }
+
set_bit(GREEN_ETHERNET, &tp->flags);
/* rx aggregation */
--
2.21.0
Avoid the MCU to clear the lanwake after suspending. It may cause the
WOL fail. Disable LANWAKE_CLR_EN before suspending. Besides,enable it
and reset the lanwake status when resuming or initializing.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b1a00f29455b..c037fc7adcea 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -68,6 +68,7 @@
#define PLA_LED_FEATURE 0xdd92
#define PLA_PHYAR 0xde00
#define PLA_BOOT_CTRL 0xe004
+#define PLA_LWAKE_CTRL_REG 0xe007
#define PLA_GPHY_INTR_IMR 0xe022
#define PLA_EEE_CR 0xe040
#define PLA_EEEP_CR 0xe080
@@ -95,6 +96,7 @@
#define PLA_TALLYCNT 0xe890
#define PLA_SFF_STS_7 0xe8de
#define PLA_PHYSTATUS 0xe908
+#define PLA_CONFIG6 0xe90a /* CONFIG6 */
#define PLA_BP_BA 0xfc26
#define PLA_BP_0 0xfc28
#define PLA_BP_1 0xfc2a
@@ -300,6 +302,9 @@
#define LINK_ON_WAKE_EN 0x0010
#define LINK_OFF_WAKE_EN 0x0008
+/* PLA_CONFIG6 */
+#define LANWAKE_CLR_EN BIT(0)
+
/* PLA_CONFIG5 */
#define BWF_EN 0x0040
#define MWF_EN 0x0020
@@ -356,6 +361,9 @@
/* PLA_BOOT_CTRL */
#define AUTOLOAD_DONE 0x0002
+/* PLA_LWAKE_CTRL_REG */
+#define LANWAKE_PIN BIT(7)
+
/* PLA_SUSPEND_FLAG */
#define LINK_CHG_EVENT BIT(0)
@@ -4968,6 +4976,8 @@ static void rtl8152_down(struct r8152 *tp)
static void rtl8153_up(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags))
return;
@@ -4975,6 +4985,15 @@ static void rtl8153_up(struct r8152 *tp)
r8153_u2p3en(tp, false);
r8153_aldps_en(tp, false);
r8153_first_init(tp);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data |= LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+ ocp_data &= ~LANWAKE_PIN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+
r8153_aldps_en(tp, true);
switch (tp->version) {
@@ -4993,11 +5012,17 @@ static void rtl8153_up(struct r8152 *tp)
static void rtl8153_down(struct r8152 *tp)
{
+ u32 ocp_data;
+
if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
rtl_drop_queued_tx(tp);
return;
}
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data &= ~LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
r8153_u1u2en(tp, false);
r8153_u2p3en(tp, false);
r8153_power_cut_en(tp, false);
@@ -5458,6 +5483,14 @@ static void r8153_init(struct r8152 *tp)
r8153_mac_clk_spd(tp, false);
usb_enable_lpm(tp->udev);
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
+ ocp_data |= LANWAKE_CLR_EN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
+ ocp_data &= ~LANWAKE_PIN;
+ ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+
/* rx aggregation */
ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
--
2.21.0
When enabling this, the device would wait an internal signal which
wouldn't be triggered. Then, the device couldn't enter P3 mode, so
the power consumption is increased.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index c037fc7adcea..3f425f974d03 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -31,7 +31,7 @@
#define NETNEXT_VERSION "11"
/* Information for net */
-#define NET_VERSION "10"
+#define NET_VERSION "11"
#define DRIVER_VERSION "v1." NETNEXT_VERSION "." NET_VERSION
#define DRIVER_AUTHOR "Realtek linux nic maintainers <[email protected]>"
@@ -109,6 +109,7 @@
#define PLA_BP_EN 0xfc38
#define USB_USB2PHY 0xb41e
+#define USB_SSPHYLINK1 0xb426
#define USB_SSPHYLINK2 0xb428
#define USB_U2P3_CTRL 0xb460
#define USB_CSR_DUMMY1 0xb464
@@ -384,6 +385,9 @@
#define USB2PHY_SUSPEND 0x0001
#define USB2PHY_L1 0x0002
+/* USB_SSPHYLINK1 */
+#define DELAY_PHY_PWR_CHG BIT(1)
+
/* USB_SSPHYLINK2 */
#define pwd_dn_scale_mask 0x3ffe
#define pwd_dn_scale(x) ((x) << 1)
@@ -4994,6 +4998,10 @@ static void rtl8153_up(struct r8152 *tp)
ocp_data &= ~LANWAKE_PIN;
ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1);
+ ocp_data &= ~DELAY_PHY_PWR_CHG;
+ ocp_write_word(tp, MCU_TYPE_USB, USB_SSPHYLINK1, ocp_data);
+
r8153_aldps_en(tp, true);
switch (tp->version) {
--
2.21.0
Initailization would reset runtime suspend by tp->saved_wolopts, so
the tp->saved_wolopts should be set before initializing.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 504db2348a3e..c3217a5c2fe1 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6739,6 +6739,11 @@ static int rtl8152_probe(struct usb_interface *intf,
intf->needs_remote_wakeup = 1;
+ if (!rtl_can_wakeup(tp))
+ __rtl_set_wol(tp, 0);
+ else
+ tp->saved_wolopts = __rtl_get_wol(tp);
+
tp->rtl_ops.init(tp);
#if IS_BUILTIN(CONFIG_USB_RTL8152)
/* Retry in case request_firmware() is not ready yet. */
@@ -6756,10 +6761,6 @@ static int rtl8152_probe(struct usb_interface *intf,
goto out1;
}
- if (!rtl_can_wakeup(tp))
- __rtl_set_wol(tp, 0);
-
- tp->saved_wolopts = __rtl_get_wol(tp);
if (tp->saved_wolopts)
device_set_wakeup_enable(&udev->dev, true);
else
--
2.21.0
For certain platforms, it causes USB reset periodically.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 7efeddad1fc8..b1a00f29455b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3391,7 +3391,8 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable)
r8153b_ups_en(tp, false);
r8153_queue_wake(tp, false);
rtl_runtime_suspend_enable(tp, false);
- r8153b_u1u2en(tp, true);
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
}
}
@@ -5024,7 +5025,9 @@ static void rtl8153b_up(struct r8152 *tp)
ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
r8153_aldps_en(tp, true);
- r8153b_u1u2en(tp, true);
+
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
}
static void rtl8153b_down(struct r8152 *tp)
@@ -5527,7 +5530,9 @@ static void r8153b_init(struct r8152 *tp)
ocp_data &= ~CUR_LINK_OK;
ocp_data |= POLL_LINK_CHG;
ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data);
- r8153b_u1u2en(tp, true);
+
+ if (tp->udev->speed != USB_SPEED_HIGH)
+ r8153b_u1u2en(tp, true);
usb_enable_lpm(tp->udev);
/* MAC clock speed down */
--
2.21.0
From: Hayes Wang <[email protected]>
Date: Wed, 22 Jan 2020 16:02:04 +0800
> v3:
> 1. Fix the typos for patch #5 and #6.
> 2. Modify the commit message of patch #9.
>
> v2:
> For patch #2, move declaring the variable "ocp_data".
>
> v1:
> These patches are used to fix some issues for RTL8153.
Series applied, thank you.
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Wed, 3 Mar 2021 16:39:47 +0800 you wrote:
> This reverts commit 134f98bcf1b898fb9d6f2b91bc85dd2e5478b4b8.
>
> The r8153_mac_clk_spd() is used for RTL8153A only, because the register
> table of RTL8153B is different from RTL8153A. However, this function would
> be called when RTL8153B calls r8153_first_init() and r8153_enter_oob().
> That causes RTL8153B becomes unstable when suspending and resuming. The
> worst case may let the device stop working.
>
> [...]
Here is the summary with links:
- [net] Revert "r8152: adjust the settings about MAC clock speed down for RTL8153"
https://git.kernel.org/netdev/net/c/4b5dc1a94d4f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On 05.03.2021 10:34, Hayes Wang wrote:
> The (0xBAF70000 & 0x00FFF000) << 6 should be (0xf70 << 18).
>
> Fixes: 561535b0f239 ("r8169: fix OCP access on RTL8117")
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index f704da3f214c..7aad0ba53372 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -767,7 +767,7 @@ static void r8168fp_adjust_ocp_cmd(struct rtl8169_private *tp, u32 *cmd, int typ
> if (type == ERIAR_OOB &&
> (tp->mac_version == RTL_GIGA_MAC_VER_52 ||
> tp->mac_version == RTL_GIGA_MAC_VER_53))
> - *cmd |= 0x7f0 << 18;
> + *cmd |= 0xf70 << 18;
> }
>
> DECLARE_RTL_COND(rtl_eriar_cond)
>
Acked-by: Heiner Kallweit <[email protected]>
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Fri, 5 Mar 2021 17:34:41 +0800 you wrote:
> The (0xBAF70000 & 0x00FFF000) << 6 should be (0xf70 << 18).
>
> Fixes: 561535b0f239 ("r8169: fix OCP access on RTL8117")
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Here is the summary with links:
- [net] r8169: fix r8168fp_adjust_ocp_cmd function
https://git.kernel.org/netdev/net/c/abbf9a0ef884
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Fri, 16 Apr 2021 16:04:35 +0800 Hayes Wang wrote:
> Support RTL8153C, RTL8153D, RTL8156A, and RTL8156B. The RTL8156A
> and RTL8156B are the 2.5G ethernet.
>
> Signed-off-by: Hayes Wang <[email protected]>
> + switch (tp->version) {
> + case RTL_VER_10:
> + data = ocp_reg_read(tp, 0xad40);
> + data &= ~0x3ff;
> + data |= BIT(7) | BIT(2);
> + ocp_reg_write(tp, 0xad40, data);
> +
> + data = ocp_reg_read(tp, 0xad4e);
> + data |= BIT(4);
> + ocp_reg_write(tp, 0xad4e, data);
> + data = ocp_reg_read(tp, 0xad16);
> + data &= ~0x3ff;
> + data |= 0x6;
> + ocp_reg_write(tp, 0xad16, data);
> + data = ocp_reg_read(tp, 0xad32);
> + data &= ~0x3f;
> + data |= 6;
> + ocp_reg_write(tp, 0xad32, data);
> + data = ocp_reg_read(tp, 0xac08);
> + data &= ~(BIT(12) | BIT(8));
> + ocp_reg_write(tp, 0xac08, data);
> + data = ocp_reg_read(tp, 0xac8a);
> + data |= BIT(12) | BIT(13) | BIT(14);
> + data &= ~BIT(15);
> + ocp_reg_write(tp, 0xac8a, data);
> + data = ocp_reg_read(tp, 0xad18);
> + data |= BIT(10);
> + ocp_reg_write(tp, 0xad18, data);
> + data = ocp_reg_read(tp, 0xad1a);
> + data |= 0x3ff;
> + ocp_reg_write(tp, 0xad1a, data);
> + data = ocp_reg_read(tp, 0xad1c);
> + data |= 0x3ff;
> + ocp_reg_write(tp, 0xad1c, data);
> +
> + data = sram_read(tp, 0x80ea);
> + data &= ~0xff00;
> + data |= 0xc400;
> + sram_write(tp, 0x80ea, data);
> + data = sram_read(tp, 0x80eb);
> + data &= ~0x0700;
> + data |= 0x0300;
> + sram_write(tp, 0x80eb, data);
> + data = sram_read(tp, 0x80f8);
> + data &= ~0xff00;
> + data |= 0x1c00;
> + sram_write(tp, 0x80f8, data);
> + data = sram_read(tp, 0x80f1);
> + data &= ~0xff00;
> + data |= 0x3000;
> + sram_write(tp, 0x80f1, data);
> + switch (tp->version) {
> + case RTL_VER_12:
> + ocp_reg_write(tp, 0xbf86, 0x9000);
> + data = ocp_reg_read(tp, 0xc402);
> + data |= BIT(10);
> + ocp_reg_write(tp, 0xc402, data);
> + data &= ~BIT(10);
> + ocp_reg_write(tp, 0xc402, data);
> + ocp_reg_write(tp, 0xbd86, 0x1010);
> + ocp_reg_write(tp, 0xbd88, 0x1010);
> + data = ocp_reg_read(tp, 0xbd4e);
> + data &= ~(BIT(10) | BIT(11));
> + data |= BIT(11);
> + ocp_reg_write(tp, 0xbd4e, data);
> + data = ocp_reg_read(tp, 0xbf46);
> + data &= ~0xf00;
> + data |= 0x700;
> + ocp_reg_write(tp, 0xbf46, data);
> + data = r8153_phy_status(tp, 0);
> + switch (data) {
> + case PHY_STAT_EXT_INIT:
> + rtl8152_apply_firmware(tp, true);
> +
> + data = ocp_reg_read(tp, 0xa466);
> + data &= ~BIT(0);
> + ocp_reg_write(tp, 0xa466, data);
What are all these magic constants? :(
> @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface *intf,
> set_ethernet_addr(tp);
>
> usb_set_intfdata(intf, tp);
> - netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> +
> + if (tp->support_2500full)
> + netif_napi_add(netdev, &tp->napi, r8152_poll, 256);
why 256? We have 100G+ drivers all using 64 what's special here?
> + else
> + netif_napi_add(netdev, &tp->napi, r8152_poll, 64);
Jakub Kicinski <[email protected]>
> Sent: Saturday, April 17, 2021 5:50 AM
> > + switch (tp->version) {
> > + case RTL_VER_10:
> > + data = ocp_reg_read(tp, 0xad40);
> > + data &= ~0x3ff;
> > + data |= BIT(7) | BIT(2);
> > + ocp_reg_write(tp, 0xad40, data);
> > +
> > + data = ocp_reg_read(tp, 0xad4e);
> > + data |= BIT(4);
> > + ocp_reg_write(tp, 0xad4e, data);
> > + data = ocp_reg_read(tp, 0xad16);
> > + data &= ~0x3ff;
> > + data |= 0x6;
> > + ocp_reg_write(tp, 0xad16, data);
> > + data = ocp_reg_read(tp, 0xad32);
> > + data &= ~0x3f;
> > + data |= 6;
> > + ocp_reg_write(tp, 0xad32, data);
> > + data = ocp_reg_read(tp, 0xac08);
> > + data &= ~(BIT(12) | BIT(8));
> > + ocp_reg_write(tp, 0xac08, data);
> > + data = ocp_reg_read(tp, 0xac8a);
> > + data |= BIT(12) | BIT(13) | BIT(14);
> > + data &= ~BIT(15);
> > + ocp_reg_write(tp, 0xac8a, data);
> > + data = ocp_reg_read(tp, 0xad18);
> > + data |= BIT(10);
> > + ocp_reg_write(tp, 0xad18, data);
> > + data = ocp_reg_read(tp, 0xad1a);
> > + data |= 0x3ff;
> > + ocp_reg_write(tp, 0xad1a, data);
> > + data = ocp_reg_read(tp, 0xad1c);
> > + data |= 0x3ff;
> > + ocp_reg_write(tp, 0xad1c, data);
> > +
> > + data = sram_read(tp, 0x80ea);
> > + data &= ~0xff00;
> > + data |= 0xc400;
> > + sram_write(tp, 0x80ea, data);
> > + data = sram_read(tp, 0x80eb);
> > + data &= ~0x0700;
> > + data |= 0x0300;
> > + sram_write(tp, 0x80eb, data);
> > + data = sram_read(tp, 0x80f8);
> > + data &= ~0xff00;
> > + data |= 0x1c00;
> > + sram_write(tp, 0x80f8, data);
> > + data = sram_read(tp, 0x80f1);
> > + data &= ~0xff00;
> > + data |= 0x3000;
> > + sram_write(tp, 0x80f1, data);
These are the parameters of PHY.
Some are used for speed down about power saving.
And some are used for performance.
> > + switch (tp->version) {
> > + case RTL_VER_12:
> > + ocp_reg_write(tp, 0xbf86, 0x9000);
> > + data = ocp_reg_read(tp, 0xc402);
> > + data |= BIT(10);
> > + ocp_reg_write(tp, 0xc402, data);
> > + data &= ~BIT(10);
> > + ocp_reg_write(tp, 0xc402, data);
> > + ocp_reg_write(tp, 0xbd86, 0x1010);
> > + ocp_reg_write(tp, 0xbd88, 0x1010);
> > + data = ocp_reg_read(tp, 0xbd4e);
> > + data &= ~(BIT(10) | BIT(11));
> > + data |= BIT(11);
> > + ocp_reg_write(tp, 0xbd4e, data);
> > + data = ocp_reg_read(tp, 0xbf46);
> > + data &= ~0xf00;
> > + data |= 0x700;
> > + ocp_reg_write(tp, 0xbf46, data);
These are used to adjust the clock of GPHY.
It influences the linking.
> > + data = r8153_phy_status(tp, 0);
> > + switch (data) {
> > + case PHY_STAT_EXT_INIT:
> > + rtl8152_apply_firmware(tp, true);
> > +
> > + data = ocp_reg_read(tp, 0xa466);
> > + data &= ~BIT(0);
> > + ocp_reg_write(tp, 0xa466, data);
These let the PHY exit PHY_STAT_EXT_INIT state.
> What are all these magic constants? :(
I think it is difficult for me to make all magic values meaningful.
The PHY setting is very complex. Only PHY engineers know
what are the settings mean.
> > @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface
> *intf,
> > set_ethernet_addr(tp);
> >
> > usb_set_intfdata(intf, tp);
> > - netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> > +
> > + if (tp->support_2500full)
> > + netif_napi_add(netdev, &tp->napi, r8152_poll, 256);
>
> why 256? We have 100G+ drivers all using 64 what's special here?
>
> > + else
> > + netif_napi_add(netdev, &tp->napi, r8152_poll, 64);
We test 2.5G Ethernet on some embedded platform.
And we find 64 is not large enough, and the performance
couldn't reach 2.5 G bits/s.
Best Regards,
Hayes
On Tue, 20 Apr 2021 07:00:39 +0000 Hayes Wang wrote:
> > > @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface *intf,
> > > set_ethernet_addr(tp);
> > >
> > > usb_set_intfdata(intf, tp);
> > > - netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> > > +
> > > + if (tp->support_2500full)
> > > + netif_napi_add(netdev, &tp->napi, r8152_poll, 256);
> >
> > why 256? We have 100G+ drivers all using 64 what's special here?
> >
> > > + else
> > > + netif_napi_add(netdev, &tp->napi, r8152_poll, 64);
>
> We test 2.5G Ethernet on some embedded platform.
> And we find 64 is not large enough, and the performance
> couldn't reach 2.5 G bits/s.
Did you manage to identify what the cause is?
NAPI will keep calling your driver if the budget was exhausted, the
only difference between 64 and 256 should be the setup cost of the
driver's internal loop. And perhaps more frequent GRO flush - what's
the CONFIG_HZ set to?
Jakub Kicinski <[email protected]>
> Sent: Wednesday, April 21, 2021 2:34 AM
[...]
> > We test 2.5G Ethernet on some embedded platform.
> > And we find 64 is not large enough, and the performance
> > couldn't reach 2.5 G bits/s.
>
> Did you manage to identify what the cause is?
>
> NAPI will keep calling your driver if the budget was exhausted, the
> only difference between 64 and 256 should be the setup cost of the
> driver's internal loop. And perhaps more frequent GRO flush - what's
> the CONFIG_HZ set to?
I am not sure. It is more than one year ago.
The CONFIG_HZ may be 250.
First, the CPU of that platform is slower than a x86 platform.
Then, the rx data comes very fast, because of the 2.5G Ethernet.
We find the budget is always exhausted, when the traffic is busy.
Best Regards,
Hayes
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 22 Apr 2021 16:48:02 +0800 you wrote:
> When the timeout occurs, we still have to run the following process
> for releasing patch request. Otherwise, the PHY would keep no link.
> Therefore, use break to stop the loop of loading firmware and
> release the patch request rather than return the function directly.
>
> Fixes: 4a51b0e8a014 ("r8152: support PHY firmware for RTL8156 series")
> Signed-off-by: Hayes Wang <[email protected]>
>
> [...]
Here is the summary with links:
- [net-next] r8152: replace return with break for ram code speedup mode timeout
https://git.kernel.org/netdev/net-next/c/f49c35b89b78
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Sat, 24 Apr 2021 14:09:03 +0800 you wrote:
> Remove DELL_TB_RX_AGG_BUG and LENOVO_MACPASSTHRU flags of rtl8152_flags.
> They are only set when initializing and wouldn't be change. It is enough
> to record them with variables.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Here is the summary with links:
- [net-next] r8152: remove some bit operations
https://git.kernel.org/netdev/net-next/c/9c68011bd7e4
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Fri, May 21, 2021 at 05:07:34PM +0800, Hayes Wang wrote:
> Verify some fields of the USB descriptor to make sure the driver
> could be used by the device.
>
> BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> Reported-by: [email protected]
> Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 71 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 136ea06540ff..f348350f5da1 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8107,6 +8107,69 @@ static void r8156b_init(struct r8152 *tp)
> tp->coalesce = 15000; /* 15 us */
> }
>
> +static bool rtl_check_vendor_ok(struct usb_interface *intf)
> +{
> + struct usb_host_interface *alt = intf->cur_altsetting;
> + struct usb_host_endpoint *in = NULL, *out = NULL, *intr = NULL;
> + unsigned int ep;
> +
> + if (alt->desc.bNumEndpoints < 3) {
> + dev_err(&intf->dev, "Unexpected bNumEndpoints %d\n", alt->desc.bNumEndpoints);
> + return false;
> + }
> +
> + for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
> + struct usb_host_endpoint *e;
> +
> + e = alt->endpoint + ep;
> +
> + /* ignore endpoints which cannot transfer data */
> + if (!usb_endpoint_maxp(&e->desc))
> + continue;
> +
> + switch (e->desc.bmAttributes) {
> + case USB_ENDPOINT_XFER_INT:
> + if (!usb_endpoint_dir_in(&e->desc))
> + continue;
> + if (!intr)
> + intr = e;
> + break;
> + case USB_ENDPOINT_XFER_BULK:
> + if (usb_endpoint_dir_in(&e->desc)) {
> + if (!in)
> + in = e;
> + } else if (!out) {
> + out = e;
> + }
> + break;
> + default:
> + continue;
> + }
> + }
> +
> + if (!in || !out || !intr) {
> + dev_err(&intf->dev, "Miss Endpoints\n");
> + return false;
> + }
> +
> + if ((in->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 1) {
> + dev_err(&intf->dev, "Invalid Rx Endpoints\n");
> + return false;
> + }
> +
> + if ((out->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 2) {
> + dev_err(&intf->dev, "Invalid Tx Endpoints\n");
> + return false;
> + }
> +
> + if ((intr->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 3) {
> + dev_err(&intf->dev, "Invalid interrupt Endpoints\n");
> + return false;
> + }
> +
> + return true;
> +}
We have a USB core function that does all of the above for you, why not
use that instead?
Look at usb_find_common_endpoints() and
usb_find_common_endpoints_reverse() and at the very least
usb_find_bulk_in_endpoint() and related functions. Please don't
open-code this type of logic, it's easy to get things wrong.
thanks,
greg k-h
Greg KH <[email protected]>
> Sent: Friday, May 21, 2021 5:43 PM
[...]
> We have a USB core function that does all of the above for you, why not
> use that instead?
>
> Look at usb_find_common_endpoints() and
> usb_find_common_endpoints_reverse() and at the very least
> usb_find_bulk_in_endpoint() and related functions. Please don't
> open-code this type of logic, it's easy to get things wrong.
Fine. Thanks.
Best Regards,
Hayes
On Sat, May 22, 2021 at 01:24:54PM +0800, Hayes Wang wrote:
> Verify some fields of the USB descriptor to make sure the driver
> could be used by the device.
>
> Besides, remove the check of endpoint number in rtl8152_probe().
> It has been done in rtl_check_vendor_ok().
>
> BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> Reported-by: [email protected]
> Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> v2:
> Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
> code.
Much better, just some tiny grammer changes below:
>
> remove the check of endpoint number in rtl8152_probe(). It has been done
> in rtl_check_vendor_ok().
>
> drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 136ea06540ff..6e5230d6c721 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8107,6 +8107,39 @@ static void r8156b_init(struct r8152 *tp)
> tp->coalesce = 15000; /* 15 us */
> }
>
> +static bool rtl_check_vendor_ok(struct usb_interface *intf)
> +{
> + struct usb_host_interface *alt = intf->cur_altsetting;
> + struct usb_endpoint_descriptor *in, *out, *intr;
> +
> + if (alt->desc.bNumEndpoints < 3) {
> + dev_err(&intf->dev, "Unexpected bNumEndpoints %d\n", alt->desc.bNumEndpoints);
> + return false;
> + }
> +
> + if (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) {
> + dev_err(&intf->dev, "Miss Endpoints\n");
"Miss" feels ackward, how about "Invalid number of endpoints"?
> + return false;
> + }
> +
> + if (usb_endpoint_num(in) != 1) {
> + dev_err(&intf->dev, "Invalid Rx Endpoint\n");
"Invalid number of Rx endpoints"
> + return false;
> + }
> +
> + if (usb_endpoint_num(out) != 2) {
> + dev_err(&intf->dev, "Invalid Tx Endpoint\n");
"Invalid number of RX endpoints"
> + return false;
> + }
> +
> + if (usb_endpoint_num(intr) != 3) {
> + dev_err(&intf->dev, "Invalid interrupt Endpoint\n");
"Invalid number of interrupt endpoints"
But really, this doesn't matter, all is good if you don't want to change
this :)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Sat, May 22, 2021 at 09:32:58AM +0200, Greg Kroah-Hartman wrote:
> On Sat, May 22, 2021 at 01:24:54PM +0800, Hayes Wang wrote:
> > Verify some fields of the USB descriptor to make sure the driver
> > could be used by the device.
> >
> > Besides, remove the check of endpoint number in rtl8152_probe().
> > It has been done in rtl_check_vendor_ok().
> >
> > BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> > Reported-by: [email protected]
> > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> > Signed-off-by: Hayes Wang <[email protected]>
> > ---
> > v2:
> > Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
> > code.
>
> Much better, just some tiny grammer changes below:
>
> >
> > remove the check of endpoint number in rtl8152_probe(). It has been done
> > in rtl_check_vendor_ok().
> >
> > drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 136ea06540ff..6e5230d6c721 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -8107,6 +8107,39 @@ static void r8156b_init(struct r8152 *tp)
> > tp->coalesce = 15000; /* 15 us */
> > }
> >
> > +static bool rtl_check_vendor_ok(struct usb_interface *intf)
> > +{
> > + struct usb_host_interface *alt = intf->cur_altsetting;
> > + struct usb_endpoint_descriptor *in, *out, *intr;
> > +
> > + if (alt->desc.bNumEndpoints < 3) {
> > + dev_err(&intf->dev, "Unexpected bNumEndpoints %d\n", alt->desc.bNumEndpoints);
> > + return false;
> > + }
This check is now redundant and can be removed.
> > +
> > + if (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) {
> > + dev_err(&intf->dev, "Miss Endpoints\n");
>
> "Miss" feels ackward, how about "Invalid number of endpoints"?
The helper also checks the type and direction so perhaps something like
"expected endpoints not found" (or just "missing endpoints") which is
more precise.
> > + return false;
> > + }
> > +
> > + if (usb_endpoint_num(in) != 1) {
> > + dev_err(&intf->dev, "Invalid Rx Endpoint\n");
>
> "Invalid number of Rx endpoints"
Here it is the endpoint number (address) that is being checked so
"number of" would be wrong.
That said, perhaps none of these checks are even needed a bit depending
on how the driver is implemented. That is, if it hardcodes the endpoint
addresses or uses the result from usb_find_common_endpoints() above
(which I realise now that it does not so these checks are probably still
needed).
> > + return false;
> > + }
Johan
Johan Hovold <[email protected]>
> Sent: Saturday, May 22, 2021 4:07 PM
[...]
> > > + if (usb_endpoint_num(in) != 1) {
> > > + dev_err(&intf->dev, "Invalid Rx Endpoint\n");
> >
> > "Invalid number of Rx endpoints"
>
> Here it is the endpoint number (address) that is being checked so
> "number of" would be wrong.
>
> That said, perhaps none of these checks are even needed a bit depending
> on how the driver is implemented. That is, if it hardcodes the endpoint
> addresses or uses the result from usb_find_common_endpoints() above
> (which I realise now that it does not so these checks are probably still
> needed).
The purpose of the checks is to find out the fake devices. That is, even
the device supports in, out, and interrupt endpoints, it is treated as
fake or malicious device, if the addresses of these endpoints are wrong.
Therefore, I would keep the checks.
Best Regards,
Hayes
On Mon, May 24, 2021 at 01:49:33AM +0000, Hayes Wang wrote:
> Johan Hovold <[email protected]>
> > Sent: Saturday, May 22, 2021 4:07 PM
> [...]
> > > > + if (usb_endpoint_num(in) != 1) {
> > > > + dev_err(&intf->dev, "Invalid Rx Endpoint\n");
> > >
> > > "Invalid number of Rx endpoints"
> >
> > Here it is the endpoint number (address) that is being checked so
> > "number of" would be wrong.
> >
> > That said, perhaps none of these checks are even needed a bit depending
> > on how the driver is implemented. That is, if it hardcodes the endpoint
> > addresses or uses the result from usb_find_common_endpoints() above
> > (which I realise now that it does not so these checks are probably still
> > needed).
>
> The purpose of the checks is to find out the fake devices. That is, even
> the device supports in, out, and interrupt endpoints, it is treated as
> fake or malicious device, if the addresses of these endpoints are wrong.
> Therefore, I would keep the checks.
Strictly, you need to check for bad input which could cause your driver
to crash or malfunction. Generally you don't need to verify endpoint
addresses unless the driver is hardcoding those. But since that is
precisely what this particular driver is doing, these checks indeed need
to stay.
Johan
On Mon, May 24, 2021 at 02:49:42PM +0800, Hayes Wang wrote:
> Verify some fields of the USB descriptor to make sure the driver
> could be used by the device.
>
> Besides, remove the check of endpoint number in rtl8152_probe().
> usb_find_common_endpoints() includes it.
>
> BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> Reported-by: [email protected]
> Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> v3:
> Remove the check of endpoint number in rtl_check_vendor_ok().
>
> Adjust the error message and ccommit message.
>
> v2:
> Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
> code.
>
> remove the check of endpoint number in rtl8152_probe(). It has been done
> in rtl_check_vendor_ok().
>
> drivers/net/usb/r8152.c | 42 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 136ea06540ff..f6abb2fbf972 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8107,6 +8107,37 @@ static void r8156b_init(struct r8152 *tp)
> tp->coalesce = 15000; /* 15 us */
> }
>
> +static bool rtl_check_vendor_ok(struct usb_interface *intf)
> +{
> + struct usb_host_interface *alt = intf->cur_altsetting;
> + struct usb_endpoint_descriptor *in, *out, *intr;
> +
> + if (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) {
> + dev_err(&intf->dev, "Expected endpoints are not found\n");
> + return false;
> + }
> +
> + /* Check Rx endpoint address */
> + if (usb_endpoint_num(in) != 1) {
> + dev_err(&intf->dev, "Invalid Rx endpoint address\n");
> + return false;
> + }
> +
> + /* Check Tx endpoint address */
> + if (usb_endpoint_num(out) != 2) {
> + dev_err(&intf->dev, "Invalid Tx endpoint address\n");
> + return false;
> + }
> +
> + /* Check interrupt endpoint address */
> + if (usb_endpoint_num(intr) != 3) {
> + dev_err(&intf->dev, "Invalid interrupt endpoint address\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> static bool rtl_vendor_mode(struct usb_interface *intf)
> {
> struct usb_host_interface *alt = intf->cur_altsetting;
> @@ -8115,12 +8146,15 @@ static bool rtl_vendor_mode(struct usb_interface *intf)
> int i, num_configs;
>
> if (alt->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC)
> - return true;
> + return rtl_check_vendor_ok(intf);
>
> /* The vendor mode is not always config #1, so to find it out. */
> udev = interface_to_usbdev(intf);
> c = udev->config;
> num_configs = udev->descriptor.bNumConfigurations;
> + if (num_configs < 2)
> + return false;
> +
Nit: This check looks unnecessary also as the driver can handle a single
configuration just fine, and by removing it you'd be logging "Unexpected
Device\n" below also in the single config case.
> for (i = 0; i < num_configs; (i++, c++)) {
> struct usb_interface_descriptor *desc = NULL;
>
> @@ -8135,7 +8169,8 @@ static bool rtl_vendor_mode(struct usb_interface *intf)
> }
> }
>
> - WARN_ON_ONCE(i == num_configs);
> + if (i == num_configs)
> + dev_err(&intf->dev, "Unexpected Device\n");
>
> return false;
> }
> @@ -9381,9 +9416,6 @@ static int rtl8152_probe(struct usb_interface *intf,
> if (!rtl_vendor_mode(intf))
> return -ENODEV;
>
> - if (intf->cur_altsetting->desc.bNumEndpoints < 3)
> - return -ENODEV;
> -
> usb_reset_device(udev);
> netdev = alloc_etherdev(sizeof(struct r8152));
> if (!netdev) {
Other than that, looks good to me now.
Reviewed-by: Johan Hovold <[email protected]>
Johan
Johan Hovold <[email protected]>
> Sent: Monday, May 24, 2021 4:01 PM
[...]
> > /* The vendor mode is not always config #1, so to find it out. */
> > udev = interface_to_usbdev(intf);
> > c = udev->config;
> > num_configs = udev->descriptor.bNumConfigurations;
> > + if (num_configs < 2)
> > + return false;
> > +
>
> Nit: This check looks unnecessary also as the driver can handle a single
> configuration just fine, and by removing it you'd be logging "Unexpected
> Device\n" below also in the single config case.
I just want to distinguish the devices.
It is acceptable if the device contains only one configuration.
A mistake occurs if the device has more configurations and
there is no expected one.
I would remove it if you think it is better.
Best Regards,
Hayes
On Mon, May 24, 2021 at 08:54:50AM +0000, Hayes Wang wrote:
> Johan Hovold <[email protected]>
> > Sent: Monday, May 24, 2021 4:01 PM
> [...]
> > > /* The vendor mode is not always config #1, so to find it out. */
> > > udev = interface_to_usbdev(intf);
> > > c = udev->config;
> > > num_configs = udev->descriptor.bNumConfigurations;
> > > + if (num_configs < 2)
> > > + return false;
> > > +
> >
> > Nit: This check looks unnecessary also as the driver can handle a single
> > configuration just fine, and by removing it you'd be logging "Unexpected
> > Device\n" below also in the single config case.
>
> I just want to distinguish the devices.
> It is acceptable if the device contains only one configuration.
> A mistake occurs if the device has more configurations and
> there is no expected one.
> I would remove it if you think it is better.
I'm fine with keeping the check too (e.g. as an optimisation of sort),
it's just a bit inconsistent to not log an error in that one error path.
Johan
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Mon, 24 May 2021 14:49:42 +0800 you wrote:
> Verify some fields of the USB descriptor to make sure the driver
> could be used by the device.
>
> Besides, remove the check of endpoint number in rtl8152_probe().
> usb_find_common_endpoints() includes it.
>
> BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> Reported-by: [email protected]
> Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> Signed-off-by: Hayes Wang <[email protected]>
>
> [...]
Here is the summary with links:
- [net,v3] r8152: check the informaton of the device
https://git.kernel.org/netdev/net/c/1a44fb38cc65
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Tue, 1 Jun 2021 15:37:12 +0800 you wrote:
> Support get_pauseparam and set_pauseparam of ethtool_ops.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 75 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
Here is the summary with links:
- [net-next] r8152: support pauseparam of ethtool_ops
https://git.kernel.org/netdev/net-next/c/163d01c56e80
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 17 Jun 2021 18:00:15 +0800 you wrote:
> Store the information of the pipes to avoid calling usb_rcvctrlpipe(),
> usb_sndctrlpipe(), usb_rcvbulkpipe(), usb_sndbulkpipe(), and
> usb_rcvintpipe() frequently.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
Here is the summary with links:
- [net-next] r8152: store the information of the pipes
https://git.kernel.org/netdev/net-next/c/b67fda9a8280
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html