[PATCH] stmmac: Don't access tx_q->dirty_tx before netif_tx_lock
This is the possible reason for different hard to reproduce
problems on my ARMv7-SMP test system.
The symptoms are in recent kernels imprecise external aborts,
and in older kernels various kinds of network stalls and
unexpected page allocation failures.
My testing indicates that the trouble started between v4.5 and v4.6
and prevails up to v4.14.
Using the dirty_tx before acquiring the spin lock is clearly
wrong and was first introduced with v4.6.
Fixes: e3ad57c96715 ("stmmac: review RX/TX ring management")
Signed-off-by: Bernd Edlinger <bernd.edlin...@hotmail.de>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1763e48..c8e280fa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1800,12 +1800,13 @@ static void stmmac_tx_clean(struct stmmac_priv *priv,
u32 queue)
{
struct stmmac_tx_queue *tx_q = >tx_queue[queue];
unsigned int bytes_compl = 0, pkts_compl = 0;
- unsigned int entry = tx_q->dirty_tx;
+ unsigned int entry;
netif_tx_lock(priv->dev);
priv->xstats.tx_clean++;
+ entry = tx_q->dirty_tx;
while (entry != tx_q->cur_tx) {
struct sk_buff *skb = tx_q->tx_skbuff[entry];
struct dma_desc *p;
--
1.9.1
[PATCHv2] Add a driver for Renesas uPD60620 and uPD60620A PHYs
Signed-off-by: Bernd Edlinger <bernd.edlin...@hotmail.de>
---
drivers/net/phy/Kconfig| 5 +++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/uPD60620.c | 109 +
3 files changed, 115 insertions(+)
create mode 100644 drivers/net/phy/uPD60620.c
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cd931cf..e2cf8ff 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -366,6 +366,11 @@ config REALTEK_PHY
---help---
Supports the Realtek 821x PHY.
+config RENESAS_PHY
+ tristate "Driver for Renesas PHYs"
+ ---help---
+ Supports the Renesas PHYs uPD60620 and uPD60620A.
+
config ROCKCHIP_PHY
tristate "Driver for Rockchip Ethernet PHYs"
---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 416df92..1404ad3 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
obj-$(CONFIG_NATIONAL_PHY) += national.o
obj-$(CONFIG_QSEMI_PHY)+= qsemi.o
obj-$(CONFIG_REALTEK_PHY) += realtek.o
+obj-$(CONFIG_RENESAS_PHY) += uPD60620.o
obj-$(CONFIG_ROCKCHIP_PHY) += rockchip.o
obj-$(CONFIG_SMSC_PHY) += smsc.o
obj-$(CONFIG_STE10XP) += ste10Xp.o
diff --git a/drivers/net/phy/uPD60620.c b/drivers/net/phy/uPD60620.c
new file mode 100644
index 000..96b3347
--- /dev/null
+++ b/drivers/net/phy/uPD60620.c
@@ -0,0 +1,109 @@
+/*
+ * Driver for the Renesas PHY uPD60620.
+ *
+ * Copyright (C) 2015 Softing Industrial Automation GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include
+#include
+#include
+
+#define UPD60620_PHY_ID0xb8242824
+
+/* Extended Registers and values */
+/* PHY Special Control/Status*/
+#define PHY_PHYSCR 0x1F /* PHY.31 */
+#define PHY_PHYSCR_10MB0x0004/* PHY speed = 10mb */
+#define PHY_PHYSCR_100MB 0x0008/* PHY speed = 100mb */
+#define PHY_PHYSCR_DUPLEX 0x0010/* PHY Duplex */
+
+/* PHY Special Modes */
+#define PHY_SPM0x12 /* PHY.18 */
+
+/* Init PHY */
+
+static int upd60620_config_init(struct phy_device *phydev)
+{
+ /* Enable support for passive HUBs (could be a strap option) */
+ /* PHYMODE: All speeds, HD in parallel detect */
+ return phy_write(phydev, PHY_SPM, 0x0180 | phydev->mdio.addr);
+}
+
+/* Get PHY status from common registers */
+
+static int upd60620_read_status(struct phy_device *phydev)
+{
+ int phy_state;
+
+ /* Read negotiated state */
+ phy_state = phy_read(phydev, MII_BMSR);
+ if (phy_state < 0)
+ return phy_state;
+
+ phydev->link = 0;
+ phydev->lp_advertising = 0;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ if (phy_state & (BMSR_ANEGCOMPLETE | BMSR_LSTATUS)) {
+ phy_state = phy_read(phydev, PHY_PHYSCR);
+ if (phy_state < 0)
+ return phy_state;
+
+ if (phy_state & (PHY_PHYSCR_10MB | PHY_PHYSCR_100MB)) {
+ phydev->link = 1;
+ phydev->speed = SPEED_10;
+ phydev->duplex = DUPLEX_HALF;
+
+ if (phy_state & PHY_PHYSCR_100MB)
+ phydev->speed = SPEED_100;
+ if (phy_state & PHY_PHYSCR_DUPLEX)
+ phydev->duplex = DUPLEX_FULL;
+
+ phy_state = phy_read(phydev, MII_LPA);
+ if (phy_state < 0)
+ return phy_state;
+
+ phydev->lp_advertising
+ = mii_lpa_to_ethtool_lpa_t(phy_state);
+
+ if (phydev->duplex == DUPLEX_FULL) {
+ if (phy_state & LPA_PAUSE_CAP)
+ phydev->pause = 1;
+ if (phy_state & LPA_PAUSE_ASYM)
+ phydev->asym_pause = 1;
+ }
+ }
+ }
+ return 0;
+}
+
+MODULE_DESCRIPTION("Renesas uPD60620 PHY driver");
+MODULE_AUTHOR("Bernd Edlinger <bernd.edlin...@hotmail.de>");
+MODULE_LICENSE("GPL");
+
+static struct phy_driver upd60620_driver[1] = { {
+ .phy_id = UPD60620_PHY_ID,
+ .phy_id_mask= 0xfffe,
+ .name = "Renesas uPD60620",
+ .features = PHY_BASIC_FEATURES,
+ .flags = 0,
+ .config_init= upd60620_config_init,
+ .config_aneg= genphy_confi
Re: [PATCH] Add a driver for Renesas uPD60620 and uPD60620A PHYs
Hi Andrew, sorry for delayed reply. Looks like I did not receive a copy of your e-mail. >> Do you suggest that there are cases where auto negotiation does not >> reach completion, and still provides a usable link status? > > My experience is that it often return 10/half, since everything should > support that. And depending on what the MAC is doing, packets can > sometime get across the link. >> >> I have tried to connect to link partners with fixed configuration >> but even then the auto negotiation always competes normally. > > Which is a bit odd. > > There are a few different possibilities here. The peer PHY driver is > broken. Rather than doing fixed, it actually set the possible > negotiation options to just the one setting you tried to fix it > to. And hence the uPD60620 device negotiated fine. Or the uPD60620 is > broken is said it negotiated, but in fact it failed. > > What was the result? 10/Half, or the fixed values you set the peer to? This is a dual-channel PHY, so I did just connect both ports and played with the mii-tool -F / -A in different combinations on each port and observed what happens when the cable is plugged in. What happens is that the port with autonegotiation enabled detects the correct speed and always half duplex, so the ASIC _pretends_ that autonegotiatiation completes, when in fact only parallel detection succeeded. Of course the other phy may be in full-duplex mode, but that can not be detected by parallel detection. The duplex mode would be full duplex by default, but my initialization overrides a possible strap option and changes that to half duplex: + /* Enable support for passive HUBs (could be a strap option) */ + /* PHYMODE: All speeds, HD in parallel detect */ + return phy_write(phydev, PHY_SPM, 0x0180 | phydev->mdio.addr); >> >> Signed-off-by: Bernd Edlinger <bernd.edlin...@hotmail.de> > > Please send this is a new patch. If we were to take this is is, all > the comments above would end up in the commit message. > > --- > > Under the --- you can however add comments which don't go into the > commit log. Good practice is to list the things you changed since the > previous version. Thanks, I did not know that. I will re-send the patch in a new thread. Bernd.
Re: [PATCH] Add a driver for Renesas uPD60620 and uPD60620A PHYs
On 09/22/17 19:59, Andrew Lunn wrote:
> On Fri, Sep 22, 2017 at 05:08:45PM +0000, Bernd Edlinger wrote:
>>
>> +config RENESAS_PHY
>> +tristate "Driver for Renesas PHYs"
>> +---help---
>> + Supports the uPD60620 and uPD60620A PHYs.
>> +
>
> Hi Bernd
>
> Please call this "Reneseas PHYs" and place in it alphabetical order.
>
Done.
>> +
>> +/* Extended Registers and values */
>> +/* PHY Special Control/Status*/
>> +#define PHY_PHYSCR 0x1F /* PHY.31 */
>> +#define PHY_PHYSCR_10MB0x0004/* PHY speed = 10mb */
>> +#define PHY_PHYSCR_100MB 0x0008/* PHY speed = 100mb */
>> +#define PHY_PHYSCR_DUPLEX 0x0010/* PHY Duplex */
>> +#define PHY_PHYSCR_RSVD5 0x0020/* Reserved Bit 5 */
>> +#define PHY_PHYSCR_MIIMOD 0x0040/* Enable 4B5B MII mode */
>
> Are any of these comments actually useful. It seems like the defines
> are pretty obvious.
>
>> +#define PHY_PHYSCR_RSVD7 0x0080/* Reserved Bit 7 */
>> +#define PHY_PHYSCR_RSVD8 0x0100/* Reserved Bit 8 */
>> +#define PHY_PHYSCR_RSVD9 0x0200/* Reserved Bit 9 */
>> +#define PHY_PHYSCR_RSVD10 0x0400/* Reserved Bit 10 */
>> +#define PHY_PHYSCR_RSVD11 0x0800/* Reserved Bit 11 */
>> +#define PHY_PHYSCR_ANDONE 0x1000/* Auto negotiation done */
>> +#define PHY_PHYSCR_RSVD13 0x2000/* Reserved Bit 13 */
>> +#define PHY_PHYSCR_RSVD14 0x4000/* Reserved Bit 14 */
>> +#define PHY_PHYSCR_RSVD15 0x8000/* Reserved Bit 15 */
>
> It looks like the only register you use is SCR and SPM. Maybe delete
> all the rest? Or do you plan to add more features making use of these
> registers?
>
No, I removed all unused defines for now.
>> +phydev->link = 0;
>> +phydev->lp_advertising = 0;
>> +phydev->pause = 0;
>> +phydev->asym_pause = 0;
>> +
>> +if (phy_state & BMSR_ANEGCOMPLETE) {
>
> It is worth comparing this against genphy_read_status() which is the
> reference implementation. You would normally check if auto negotiation
> is enabled, not if it has completed. If it is enabled you read the
> current negotiated state, even if it is not completed.
>
Do you suggest that there are cases where auto negotiation does not
reach completion, and still provides a usable link status?
I have tried to connect to link partners with fixed configuration
but even then the auto negotiation always competes normally.
>> +phy_state = phy_read(phydev, PHY_PHYSCR);
>> +if (phy_state < 0)
>> +return phy_state;
>> +
>> +if (phy_state & (PHY_PHYSCR_10MB | PHY_PHYSCR_100MB)) {
>> +phydev->link = 1;
>> +phydev->speed = SPEED_10;
>> +phydev->duplex = DUPLEX_HALF;
>> +
>> +if (phy_state & PHY_PHYSCR_100MB)
>> +phydev->speed = SPEED_100;
>> +if (phy_state & PHY_PHYSCR_DUPLEX)
>> +phydev->duplex = DUPLEX_FULL;
>> +
>> +phy_state = phy_read(phydev, MII_LPA);
>> +if (phy_state < 0)
>> +return phy_state;
>> +
>> +phydev->lp_advertising
>> += mii_lpa_to_ethtool_lpa_t(phy_state);
>> +
>> +if (phydev->duplex == DUPLEX_FULL) {
>> +if (phy_state & LPA_PAUSE_CAP)
>> +phydev->pause = 1;
>> +if (phy_state & LPA_PAUSE_ASYM)
>> +phydev->asym_pause = 1;
>> +}
>> +}
>> +} else if (phy_state & BMSR_LSTATUS) {
>
> The else clause is then for a fixed configuration. Since all you are
> looking at is BMCR, you can probably just cut/paste from
> genphy_read_status().
>
I think I can fold the fixed speed case in the auto negotiation case:
The PHYSCR has always the correct values for fixed settings.
I was initially unsure if I should look at it while autonegotiation is
not complete, but as you pointed out, that is the generally accepted
practice.
Thanks
Bernd.
>From 2e101aed8466b314251972d1eaccfb43cf177078 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlin...@hotmail.de>
Date: Thu, 21 Sep 2017 15:46:16 +0200
Subject: [PATCH 2/5] Add a driver for Renesas uPD60620 and uPD60620A PHYs.
Signed-off-by: Bernd Edlinger <bernd.edlin...@hotmail.de>
---
drivers/net/p
[PATCH] Add a driver for Renesas uPD60620 and uPD60620A PHYs
Signed-off-by: Bernd Edlinger <bernd.edlin...@hotmail.de> --- drivers/net/phy/Kconfig| 5 + drivers/net/phy/Makefile | 1 + drivers/net/phy/uPD60620.c | 226 + 3 files changed, 232 insertions(+) create mode 100644 drivers/net/phy/uPD60620.c diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index a9d16a3..25089f0 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -287,6 +287,11 @@ config DP83867_PHY ---help--- Currently supports the DP83867 PHY. +config RENESAS_PHY + tristate "Driver for Renesas PHYs" + ---help--- + Supports the uPD60620 and uPD60620A PHYs. + config FIXED_PHY tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs" depends on PHYLIB diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 416df92..1404ad3 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -72,6 +72,7 @@ obj-$(CONFIG_MICROSEMI_PHY) += mscc.o obj-$(CONFIG_NATIONAL_PHY)+= national.o obj-$(CONFIG_QSEMI_PHY) += qsemi.o obj-$(CONFIG_REALTEK_PHY) += realtek.o +obj-$(CONFIG_RENESAS_PHY) += uPD60620.o obj-$(CONFIG_ROCKCHIP_PHY)+= rockchip.o obj-$(CONFIG_SMSC_PHY)+= smsc.o obj-$(CONFIG_STE10XP) += ste10Xp.o diff --git a/drivers/net/phy/uPD60620.c b/drivers/net/phy/uPD60620.c new file mode 100644 index 000..b3d900c --- /dev/null +++ b/drivers/net/phy/uPD60620.c @@ -0,0 +1,226 @@ +/* + * Driver for the Renesas PHY uPD60620. + * + * Copyright (C) 2015 Softing Industrial Automation GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#include +#include +#include + +#define UPD60620_PHY_ID0xb8242824 + +/* Extended Registers and values */ +/* PHY Special Control/Status*/ +#define PHY_PHYSCR 0x1F /* PHY.31 */ +#define PHY_PHYSCR_10MB0x0004/* PHY speed = 10mb */ +#define PHY_PHYSCR_100MB 0x0008/* PHY speed = 100mb */ +#define PHY_PHYSCR_DUPLEX 0x0010/* PHY Duplex */ +#define PHY_PHYSCR_RSVD5 0x0020/* Reserved Bit 5 */ +#define PHY_PHYSCR_MIIMOD 0x0040/* Enable 4B5B MII mode */ +#define PHY_PHYSCR_RSVD7 0x0080/* Reserved Bit 7 */ +#define PHY_PHYSCR_RSVD8 0x0100/* Reserved Bit 8 */ +#define PHY_PHYSCR_RSVD9 0x0200/* Reserved Bit 9 */ +#define PHY_PHYSCR_RSVD10 0x0400/* Reserved Bit 10 */ +#define PHY_PHYSCR_RSVD11 0x0800/* Reserved Bit 11 */ +#define PHY_PHYSCR_ANDONE 0x1000/* Auto negotiation done */ +#define PHY_PHYSCR_RSVD13 0x2000/* Reserved Bit 13 */ +#define PHY_PHYSCR_RSVD14 0x4000/* Reserved Bit 14 */ +#define PHY_PHYSCR_RSVD15 0x8000/* Reserved Bit 15 */ + +/* PHY Global Config Mapping */ +#define PHY_GLOBAL_CONFIG 0x07 +/* PHY GPIO Config Register 1 */ +#define PHY_GPIO_CONFIG1 0x01 /* PHY 7.1 */ +#define PHY_GPIO4_INT0 0x000d /* GPIO4 configuration */ +#define PHY_GPIO5_INT1 0x00d0 /* GPIO5 configuration */ + +/* PHY Interrupt Control Register */ +#define PHY_ICR0x1e /* PHY.30 */ +#define PHY_ICR_RSVD0 0x0001/* Reserved bit 0 */ +#define PHY_ICR_ANCPRRN0x0002/* Auto negotiation paged received */ +#define PHY_ICR_PDFEN 0x0004/* Parallel detection fault */ +#define PHY_ICR_ANCLPAEN 0x0008/* Auto negotiation last page ack */ +#define PHY_ICR_LNKINTEN 0x0010/* Link down */ +#define PHY_ICR_REMFD 0x0020/* Remote fault detected */ +#define PHY_ICR_ANCINTEN 0x0040/* Auto negotiation complete */ +#define PHY_ICR_EOEN 0x0080/* Energy on generated */ +#define PHY_ICR_RSVD8 0x0100/* Reserved bit 8 */ +#define PHY_ICR_FEQTRGEN 0x0200/* FEQ Trigger */ +#define PHY_ICR_BERTRGEN 0x0400/* BER Counter Trigger */ +#define PHY_ICR_MLINTEN0x0800/* Maxlvl */ +#define PHY_ICR_CLPINTEN 0x1000/* Clipping */ +#define PHY_ICR_RSVD13 0x2000/* Reserved bit 13 */ +#define PHY_ICR_RSVD14 0x4000/* Reserved bit 14 */ +#define PHY_ICR_RSVD15 0x8000/* Reserved bit 15 */ + +/* PHY Interrupt Status Register */ +#define PHY_ISR0x1d /* PHY.29 */ +#define PHY_ISR_DUPINT 0x/* Placeholder for Duplex/Speed intr */ +#define PHY_ISR_RSVD0 0x0001/* Reserved bit 0 */ +#define PHY_ISR_ANCPR 0x0002/* Auto negotiation paged received */ +#define PHY_ISR_PDF0x0004/* Parallel detection fault */ +#define PHY_ISR_ANCLPA 0x0008/* Auto negotiation last page ack */ +#define PHY_ISR_LNKINT 0x0010/* Link down */ +#define PHY_ISR_REMFD 0x0020/* Remote fault detected */ +#define PHY_ISR_ANCINT 0x0040/* Auto negotiation compl
