Re: [BUG]: WARNING: CPU: 5 PID: 0 at net/sched/sch_generic.c:442 dev_watchdog+0x24d/0x260
On 13.04.2021 22:59, Xose Vazquez Perez wrote: > A non-recurring bug, on 5.11.12-300.fc34.x86_64 (Fedora kernel). > > Thanks. > > > 0c:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168] (rev 06) > > [ 2.968280] libphy: r8169: probed > [ 2.968844] r8169 :0c:00.0 eth0: RTL8168e/8111e, 2c:41:38:9e:98:93, > XID 2c2, IRQ 47 > [ 2.968849] r8169 :0c:00.0 eth0: jumbo features [frames: 9194 bytes, > tx checksumming: ko] > [ 4.071966] RTL8211DN Gigabit Ethernet r8169-c00:00: attached PHY driver > (mii_bus:phy_addr=r8169-c00:00, irq=IGNORE) > [ 4.323834] r8169 :0c:00.0 eth0: Link is Down > [ 6.729111] r8169 :0c:00.0 eth0: Link is Up - 1Gbps/Full - flow > control rx/tx > > [106378.638739] [ cut here ] > [106378.638757] NETDEV WATCHDOG: eth0 (r8169): transmit queue 0 timed out This is a standard tx timeout and can have very different reasons. Few questions: - Is this a regression? If yes, can you bisect? - Can you reproduce it? If yes, which type of activity triggers it?
Re: [PATCH] phy: nxp-c45: add driver for tja1103
On 09.04.2021 20:41, Radu Pirea (NXP OSS) wrote: > Add driver for tja1103 driver and for future NXP C45 PHYs. > > Signed-off-by: Radu Pirea (NXP OSS) > --- > MAINTAINERS | 6 + > drivers/net/phy/Kconfig | 6 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/nxp-c45.c | 622 ++ > 4 files changed, 635 insertions(+) > create mode 100644 drivers/net/phy/nxp-c45.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a008b70f3c16..082a5eca8913 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12518,6 +12518,12 @@ F: drivers/nvmem/ > F: include/linux/nvmem-consumer.h > F: include/linux/nvmem-provider.h > > +NXP C45 PHY DRIVER > +M: Radu Pirea > +L: net...@vger.kernel.org > +S: Maintained > +F: drivers/net/phy/nxp-c45.c > + > NXP FSPI DRIVER > M: Ashish Kumar > R: Yogesh Gaur > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 698bea312adc..fd2da80b5339 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -228,6 +228,12 @@ config NATIONAL_PHY > help > Currently supports the DP83865 PHY. > > +config NXP_C45_PHY > + tristate "NXP C45 PHYs" > + help > + Enable support for NXP C45 PHYs. > + Currently supports only the TJA1103 PHY. > + > config NXP_TJA11XX_PHY > tristate "NXP TJA11xx PHYs support" > depends on HWMON > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index a13e402074cf..a18f095748b5 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_MICROCHIP_PHY) += microchip.o > obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o > obj-$(CONFIG_MICROSEMI_PHY) += mscc/ > obj-$(CONFIG_NATIONAL_PHY) += national.o > +obj-$(CONFIG_NXP_C45_PHY)+= nxp-c45.o > obj-$(CONFIG_NXP_TJA11XX_PHY)+= nxp-tja11xx.o > obj-$(CONFIG_QSEMI_PHY) += qsemi.o > obj-$(CONFIG_REALTEK_PHY)+= realtek.o > diff --git a/drivers/net/phy/nxp-c45.c b/drivers/net/phy/nxp-c45.c > new file mode 100644 > index ..2961799f7d05 > --- /dev/null > +++ b/drivers/net/phy/nxp-c45.c > @@ -0,0 +1,622 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* NXP C45 PHY driver > + * Copyright (C) 2021 NXP > + * Copyright (C) 2021 Radu Pirea > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PHY_ID_BASE_T1 0x001BB010 > + > +#define B100T1_PMAPMD_CTL0x0834 > +#define B100T1_PMAPMD_CONFIG_EN BIT(15) > +#define B100T1_PMAPMD_MASTER BIT(14) > +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN | > B100T1_PMAPMD_MASTER) > +#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN) > + > +#define DEVICE_CONTROL 0x0040 > +#define DEVICE_CONTROL_RESET BIT(15) > +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14) > +#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13) > +#define RESET_POLL_NS(250 * NSEC_PER_MSEC) > + > +#define PHY_CONTROL 0x8100 > +#define PHY_CONFIG_ENBIT(14) > +#define PHY_START_OP BIT(0) > + > +#define PHY_CONFIG 0x8108 > +#define PHY_CONFIG_AUTO BIT(0) > + > +#define SIGNAL_QUALITY 0x8320 > +#define SQI_VALIDBIT(14) > +#define SQI_MASK GENMASK(2, 0) > +#define MAX_SQI SQI_MASK > + > +#define CABLE_TEST 0x8330 > +#define CABLE_TEST_ENABLEBIT(15) > +#define CABLE_TEST_START BIT(14) > +#define CABLE_TEST_VALID BIT(13) > +#define CABLE_TEST_OK0x00 > +#define CABLE_TEST_SHORTED 0x01 > +#define CABLE_TEST_OPEN 0x02 > +#define CABLE_TEST_UNKNOWN 0x07 > + > +#define PORT_CONTROL 0x8040 > +#define PORT_CONTROL_EN BIT(14) > + > +#define PORT_INFRA_CONTROL 0xAC00 > +#define PORT_INFRA_CONTROL_ENBIT(14) > + > +#define VND1_RXID0xAFCC > +#define VND1_TXID0xAFCD > +#define ID_ENABLEBIT(15) > + > +#define ABILITIES0xAFC4 > +#define RGMII_ID_ABILITY BIT(15) > +#define RGMII_ABILITYBIT(14) > +#define RMII_ABILITY BIT(10) > +#define REVMII_ABILITY BIT(9) > +#define MII_ABILITY BIT(8) > +#define SGMII_ABILITYBIT(0) > + > +#define MII_BASIC_CONFIG 0xAFC6 > +#define MII_BASIC_CONFIG_REV BIT(8) > +#define MII_BASIC_CONFIG_SGMII 0x9 > +#define MII_BASIC_CONFIG_RGMII 0x7 > +#define MII_BASIC_CONFIG_RMII0x5 > +#define MII_BASIC_CONFIG_MII 0x4 > + >
Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"
On 08.04.2021 20:35, Sven Van Asbroeck wrote:
> Hi George,
>
> On Thu, Apr 8, 2021 at 2:26 PM Sven Van Asbroeck wrote:
>>
>> George, I will send a patch for you to try shortly. Except if you're
>> already ahead :)
>
> Would this work for you? It does for me.
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index dbdfabff3b00..7b6794aa8ea9 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct
> lan743x_adapter *adapter, int new_mtu)
> }
>
> mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_);
> - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) &
> - MAC_RX_MAX_SIZE_MASK_);
> + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN)
> + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_);
> lan743x_csr_write(adapter, MAC_RX, mac_rx);
>
> if (enabled) {
> @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct
> lan743x_rx *rx, int index)
> struct sk_buff *skb;
> dma_addr_t dma_ptr;
>
> - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
> + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
> RX_HEAD_PADDING;
>
A completely unrelated question:
How about VLAN packets with a 802.1Q tag? Should VLAN_ETH_HLEN be used?
> descriptor = >ring_cpu_ptr[index];
> buffer_info = >buffer_info[index];
> @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int
> frame_length)
> dev_kfree_skb_irq(skb);
> return NULL;
> }
> - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2);
> + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN);
> if (skb->len > frame_length) {
> skb->tail -= skb->len - frame_length;
> skb->len = frame_length;
>
Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"
On 08.04.2021 20:00, George McCollister wrote: > On Thu, Apr 8, 2021 at 12:46 PM Sven Van Asbroeck wrote: >> >> Hi George, >> >> On Thu, Apr 8, 2021 at 1:36 PM George McCollister >> wrote: >>> >>> Can you explain the difference in behavior with what I was observing >>> on the LAN7431? >> >> I'm not using DSA in my application, so I cannot test or replicate >> what you were observing. It would be great if we could work together >> and settle on a solution that is acceptable to both of us. > > Sounds good. > >> >>> I'll retest but if this is reverted I'm going to start >>> seeing 2 extra bytes on the end of frames and it's going to break DSA >>> with the LAN7431 again. >>> >> >> Seen from my point of view, your patch is a regression. But perhaps my >> patch set is a regression for you? Catch 22... >> >> Would you be able to identify which patch broke your DSA behaviour? >> Was it one of mine? Perhaps we can start from there. > > Yes, first I'm going to confirm that what is in the net branch still > works (unlikely but perhaps something else could have broken it since > last I tried it). > Then I'll confirm the patch which I believe broke it actually did and > report back. > >> >> Sven Just an idea: RX_HEAD_PADDING is an alias for NET_IP_ALIGN that can have two values: 0 and 2 The two systems you use may have different NET_IP_ALIGN values. This could explain the behavior. Then what I proposed should work for both of you: frame_length - ETH_FCS_LEN
Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"
On 08.04.2021 19:23, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck
>
> This reverts commit 3e21a10fdea3c2e4e4d1b72cb9d720256461af40.
>
> The reverted patch completely breaks all network connectivity on the
> lan7430. tcpdump indicates missing bytes when receiving ping
> packets from an external host:
>
> host$ ping $lan7430_ip
> lan7430$ tcpdump -v
> IP truncated-ip - 2 bytes missing! (tos 0x0, ttl 64, id 21715,
> offset 0, flags [DF], proto ICMP (1), length 84)
>
> Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2")
> Signed-off-by: Sven Van Asbroeck
> ---
>
> To: Bryan Whitehead
> To: "David S. Miller"
> To: Jakub Kicinski
> To: George McCollister
> Cc: unglinuxdri...@microchip.com
> Cc: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 1c3e204d727c..dbdfabff3b00 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int
> frame_length)
> dev_kfree_skb_irq(skb);
> return NULL;
> }
> - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4);
> + frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2);
> if (skb->len > frame_length) {
> skb->tail -= skb->len - frame_length;
> skb->len = frame_length;
>
Can't we use frame_length - ETH_FCS_LEN direcctly here?
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 07.04.2021 12:05, Joakim Zhang wrote:
>
> Hi Heiner,
>
>> -Original Message-
>> From: Joakim Zhang
>> Sent: 2021年4月7日 15:46
>> To: Heiner Kallweit ; christian.me...@t2data.com;
>> and...@lunn.ch; li...@armlinux.org.uk; da...@davemloft.net;
>> k...@kernel.org
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>> ; Florian Fainelli
>> Subject: RE: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume
>> back
>>
>>
>> Hi Heiner,
>>
>>> -Original Message-
>>> From: Heiner Kallweit
>>> Sent: 2021年4月7日 15:12
>>> To: Joakim Zhang ;
>>> christian.me...@t2data.com; and...@lunn.ch; li...@armlinux.org.uk;
>>> da...@davemloft.net; k...@kernel.org
>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>>> ; Florian Fainelli
>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus
>>> resume back
>>>
>>> On 07.04.2021 03:43, Joakim Zhang wrote:
>>>>
>>>> Hi Heiner,
>>>>
>>>>> -Original Message-
>>>>> From: Heiner Kallweit
>>>>> Sent: 2021年4月7日 2:22
>>>>> To: Joakim Zhang ;
>>>>> christian.me...@t2data.com; and...@lunn.ch; li...@armlinux.org.uk;
>>>>> da...@davemloft.net; k...@kernel.org; Russell King - ARM Linux
>>>>>
>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>> dl-linux-imx ; Florian Fainelli
>>>>>
>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO
>>>>> bus resume back
>>>>>
>>>>> On 06.04.2021 13:42, Heiner Kallweit wrote:
>>>>>> On 06.04.2021 12:07, Joakim Zhang wrote:
>>>>>>>
>>>>>>>> -Original Message-
>>>>>>>> From: Heiner Kallweit
>>>>>>>> Sent: 2021年4月6日 14:29
>>>>>>>> To: Joakim Zhang ;
>>>>>>>> christian.me...@t2data.com; and...@lunn.ch;
>>>>>>>> li...@armlinux.org.uk; da...@davemloft.net; k...@kernel.org
>>>>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>>>>> dl-linux-imx
>>>>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after
>>>>>>>> MDIO bus resume back
>>>>>>>>
>>>>>>>> On 06.04.2021 04:07, Joakim Zhang wrote:
>>>>>>>>>
>>>>>>>>> Hi Heiner,
>>>>>>>>>
>>>>>>>>>> -Original Message-
>>>>>>>>>> From: Heiner Kallweit
>>>>>>>>>> Sent: 2021年4月5日 20:10
>>>>>>>>>> To: christian.me...@t2data.com; Joakim Zhang
>>>>>>>>>> ; and...@lunn.ch;
>>>>>>>>>> li...@armlinux.org.uk; da...@davemloft.net; k...@kernel.org
>>>>>>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>>>>>>> dl-linux-imx
>>>>>>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after
>>>>>>>>>> MDIO bus resume back
>>>>>>>>>>
>>>>>>>>>> On 05.04.2021 10:43, Christian Melki wrote:
>>>>>>>>>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>>>>>>>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>>>>>>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>>>>>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram
>>>>> may
>>>>>>>>>>>>>> cut off PHY power") invokes phy_init_hw() when MDIO bus
>>>>>>>>>>>>>> resume, it will soft reset PHY if PHY driver implements
>>>>>>>>>>>>>> soft_reset
>>>>> callback.
>>>>>>>>>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset
>>>>>>>>>>>>>> callback to genphy_soft_reset for KSZ8081") adds
>>>>>>>>>>>>>> soft_reset for
>>>>> KSZ8081.
>>>>>>>>>>>>>> After these two patches, I found i.MX6
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 07.04.2021 03:43, Joakim Zhang wrote:
>
> Hi Heiner,
>
>> -Original Message-
>> From: Heiner Kallweit
>> Sent: 2021年4月7日 2:22
>> To: Joakim Zhang ; christian.me...@t2data.com;
>> and...@lunn.ch; li...@armlinux.org.uk; da...@davemloft.net;
>> k...@kernel.org; Russell King - ARM Linux
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>> ; Florian Fainelli
>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume
>> back
>>
>> On 06.04.2021 13:42, Heiner Kallweit wrote:
>>> On 06.04.2021 12:07, Joakim Zhang wrote:
>>>>
>>>>> -Original Message-
>>>>> From: Heiner Kallweit
>>>>> Sent: 2021年4月6日 14:29
>>>>> To: Joakim Zhang ;
>>>>> christian.me...@t2data.com; and...@lunn.ch; li...@armlinux.org.uk;
>>>>> da...@davemloft.net; k...@kernel.org
>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>> dl-linux-imx
>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO
>>>>> bus resume back
>>>>>
>>>>> On 06.04.2021 04:07, Joakim Zhang wrote:
>>>>>>
>>>>>> Hi Heiner,
>>>>>>
>>>>>>> -Original Message-
>>>>>>> From: Heiner Kallweit
>>>>>>> Sent: 2021年4月5日 20:10
>>>>>>> To: christian.me...@t2data.com; Joakim Zhang
>>>>>>> ; and...@lunn.ch; li...@armlinux.org.uk;
>>>>>>> da...@davemloft.net; k...@kernel.org
>>>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>>>> dl-linux-imx
>>>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO
>>>>>>> bus resume back
>>>>>>>
>>>>>>> On 05.04.2021 10:43, Christian Melki wrote:
>>>>>>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>>>>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>>>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram
>> may
>>>>>>>>>>> cut off PHY power") invokes phy_init_hw() when MDIO bus
>>>>>>>>>>> resume, it will soft reset PHY if PHY driver implements soft_reset
>> callback.
>>>>>>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset
>>>>>>>>>>> callback to genphy_soft_reset for KSZ8081") adds soft_reset for
>> KSZ8081.
>>>>>>>>>>> After these two patches, I found i.MX6UL 14x14 EVK which
>>>>>>>>>>> connected to KSZ8081RNB doesn't work any more when system
>>>>> resume
>>>>>>>>>>> back, MAC
>>>>>>> driver is fec_main.c.
>>>>>>>>>>>
>>>>>>>>>>> It's obvious that initializing PHY hardware when MDIO bus
>>>>>>>>>>> resume back would introduce some regression when PHY
>>>>>>>>>>> implements soft_reset. When I
>>>>>>>>>>
>>>>>>>>>> Why is this obvious? Please elaborate on why a soft reset
>>>>>>>>>> should break something.
>>>>>>>>>>
>>>>>>>>>>> am debugging, I found PHY works fine if MAC doesn't support
>>>>>>>>>>> suspend/resume or phy_stop()/phy_start() doesn't been called
>>>>>>>>>>> during suspend/resume. This let me realize, PHY state machine
>>>>>>>>>>> phy_state_machine() could do something breaks the PHY.
>>>>>>>>>>>
>>>>>>>>>>> As we known, MAC resume first and then MDIO bus resume when
>>>>>>>>>>> system resume back from suspend. When MAC resume, usually it
>>>>>>>>>>> will invoke
>>>>>>>>>>> phy_start() where to change PHY state to PHY_UP, then trigger
>>>>>>>>>>> the
>>>>>>>>>>> stat> machine to run now. In phy_state_machine(), it will
>>>>>>>>>>> start/config auto-nego, then change PHY state to PHY_NOLINK,
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 06.04.2021 20:32, Florian Fainelli wrote:
>
>
> On 4/6/2021 4:42 AM, Heiner Kallweit wrote:
>>
>> Waiting for ANEG_COMPLETE to be set wouldn't be a good option. Aneg may never
>> complete for different reasons, e.g. no physical link. And even if we use a
>> timeout this may add unwanted delays.
>>
>>> Do you have any other insights that can help me further locate the issue?
>>> Thanks.
>>>
>>
>> I think current MAC/PHY PM handling isn't perfect. Often we have the
>> following
>> scenario:
>>
>> *suspend*
>> 1. PHY is suspended (mdio_bus_phy_suspend)
>> 2. MAC suspend callback (typically involving phy_stop())
>>
>> *resume*
>> 1. MAC resume callback (typically involving phy_start())
>> 2. PHY is resumed (mdio_bus_phy_resume), incl. calling phy_init_hw()
>>
>> Calling phy_init_hw() after phy_start() doesn't look right.
>> It seems to work in most cases, but there's a certain risk
>> that phy_init_hw() overwrites something, e.g. the advertised
>> modes.
>> I think we have two valid scenarios:
>>
>> 1. phylib PM callbacks are used, then the MAC driver shouldn't
>>touch the PHY in its PM callbacks, especially not call
>>phy_stop/phy_start.
>>
>> 2. MAC PM callbacks take care also of the PHY. Then I think we would
>>need a flag at the phy_device telling it to make the PHY PM
>>callbacks a no-op.
>
> Maybe part of the problem is that the FEC is calling phy_{stop,start} in
> its suspend/resume callbacks instead of phy_{suspend,resume} which would
> play nice and tell the MDIO bus PM callbacks that the PHY has already
> been suspended.
>
This basically is what I just proposed to test.
> I am also suspicious about whether Wake-on-LAN actually works with the
> FEC, you cannot wake from LAN if the PHY is stopped and powered down.
>
phy_stop() calls phy_suspend() which checks for WoL. Therefore this
should not be a problem.
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 06.04.2021 13:42, Heiner Kallweit wrote:
> On 06.04.2021 12:07, Joakim Zhang wrote:
>>
>>> -Original Message-
>>> From: Heiner Kallweit
>>> Sent: 2021年4月6日 14:29
>>> To: Joakim Zhang ; christian.me...@t2data.com;
>>> and...@lunn.ch; li...@armlinux.org.uk; da...@davemloft.net;
>>> k...@kernel.org
>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>>>
>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume
>>> back
>>>
>>> On 06.04.2021 04:07, Joakim Zhang wrote:
>>>>
>>>> Hi Heiner,
>>>>
>>>>> -Original Message-
>>>>> From: Heiner Kallweit
>>>>> Sent: 2021年4月5日 20:10
>>>>> To: christian.me...@t2data.com; Joakim Zhang
>>>>> ; and...@lunn.ch; li...@armlinux.org.uk;
>>>>> da...@davemloft.net; k...@kernel.org
>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>> dl-linux-imx
>>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus
>>>>> resume back
>>>>>
>>>>> On 05.04.2021 10:43, Christian Melki wrote:
>>>>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may
>>>>>>>>> cut off PHY power") invokes phy_init_hw() when MDIO bus resume,
>>>>>>>>> it will soft reset PHY if PHY driver implements soft_reset callback.
>>>>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback
>>>>>>>>> to genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081.
>>>>>>>>> After these two patches, I found i.MX6UL 14x14 EVK which
>>>>>>>>> connected to KSZ8081RNB doesn't work any more when system
>>> resume
>>>>>>>>> back, MAC
>>>>> driver is fec_main.c.
>>>>>>>>>
>>>>>>>>> It's obvious that initializing PHY hardware when MDIO bus resume
>>>>>>>>> back would introduce some regression when PHY implements
>>>>>>>>> soft_reset. When I
>>>>>>>>
>>>>>>>> Why is this obvious? Please elaborate on why a soft reset should
>>>>>>>> break something.
>>>>>>>>
>>>>>>>>> am debugging, I found PHY works fine if MAC doesn't support
>>>>>>>>> suspend/resume or phy_stop()/phy_start() doesn't been called
>>>>>>>>> during suspend/resume. This let me realize, PHY state machine
>>>>>>>>> phy_state_machine() could do something breaks the PHY.
>>>>>>>>>
>>>>>>>>> As we known, MAC resume first and then MDIO bus resume when
>>>>>>>>> system resume back from suspend. When MAC resume, usually it will
>>>>>>>>> invoke
>>>>>>>>> phy_start() where to change PHY state to PHY_UP, then trigger the
>>>>>>>>> stat> machine to run now. In phy_state_machine(), it will
>>>>>>>>> start/config auto-nego, then change PHY state to PHY_NOLINK, what
>>>>>>>>> to next is periodically check PHY link status. When MDIO bus
>>>>>>>>> resume, it will initialize PHY hardware, including soft_reset,
>>>>>>>>> what would soft_reset affect seems various from different PHYs.
>>>>>>>>> For KSZ8081RNB, when it in PHY_NOLINK state and then perform a
>>>>>>>>> soft reset,
>>>>> it will never complete auto-nego.
>>>>>>>>
>>>>>>>> Why? That would need to be checked in detail. Maybe chip errata
>>>>>>>> documentation provides a hint.
>>>>>>>>
>>>>>>>
>>>>>>> The KSZ8081 spec says the following about bit BMCR_PDOWN:
>>>>>>>
>>>>>>> If software reset (Register 0.15) is used to exit power-down mode
>>>>>>> (Register 0.11 = 1), two software reset writes (Register 0.15 = 1)
>>>>>>> are required. The first write cl
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 06.04.2021 12:07, Joakim Zhang wrote:
>
>> -Original Message-
>> From: Heiner Kallweit
>> Sent: 2021年4月6日 14:29
>> To: Joakim Zhang ; christian.me...@t2data.com;
>> and...@lunn.ch; li...@armlinux.org.uk; da...@davemloft.net;
>> k...@kernel.org
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>>
>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume
>> back
>>
>> On 06.04.2021 04:07, Joakim Zhang wrote:
>>>
>>> Hi Heiner,
>>>
>>>> -Original Message-
>>>> From: Heiner Kallweit
>>>> Sent: 2021年4月5日 20:10
>>>> To: christian.me...@t2data.com; Joakim Zhang
>>>> ; and...@lunn.ch; li...@armlinux.org.uk;
>>>> da...@davemloft.net; k...@kernel.org
>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> dl-linux-imx
>>>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus
>>>> resume back
>>>>
>>>> On 05.04.2021 10:43, Christian Melki wrote:
>>>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may
>>>>>>>> cut off PHY power") invokes phy_init_hw() when MDIO bus resume,
>>>>>>>> it will soft reset PHY if PHY driver implements soft_reset callback.
>>>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback
>>>>>>>> to genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081.
>>>>>>>> After these two patches, I found i.MX6UL 14x14 EVK which
>>>>>>>> connected to KSZ8081RNB doesn't work any more when system
>> resume
>>>>>>>> back, MAC
>>>> driver is fec_main.c.
>>>>>>>>
>>>>>>>> It's obvious that initializing PHY hardware when MDIO bus resume
>>>>>>>> back would introduce some regression when PHY implements
>>>>>>>> soft_reset. When I
>>>>>>>
>>>>>>> Why is this obvious? Please elaborate on why a soft reset should
>>>>>>> break something.
>>>>>>>
>>>>>>>> am debugging, I found PHY works fine if MAC doesn't support
>>>>>>>> suspend/resume or phy_stop()/phy_start() doesn't been called
>>>>>>>> during suspend/resume. This let me realize, PHY state machine
>>>>>>>> phy_state_machine() could do something breaks the PHY.
>>>>>>>>
>>>>>>>> As we known, MAC resume first and then MDIO bus resume when
>>>>>>>> system resume back from suspend. When MAC resume, usually it will
>>>>>>>> invoke
>>>>>>>> phy_start() where to change PHY state to PHY_UP, then trigger the
>>>>>>>> stat> machine to run now. In phy_state_machine(), it will
>>>>>>>> start/config auto-nego, then change PHY state to PHY_NOLINK, what
>>>>>>>> to next is periodically check PHY link status. When MDIO bus
>>>>>>>> resume, it will initialize PHY hardware, including soft_reset,
>>>>>>>> what would soft_reset affect seems various from different PHYs.
>>>>>>>> For KSZ8081RNB, when it in PHY_NOLINK state and then perform a
>>>>>>>> soft reset,
>>>> it will never complete auto-nego.
>>>>>>>
>>>>>>> Why? That would need to be checked in detail. Maybe chip errata
>>>>>>> documentation provides a hint.
>>>>>>>
>>>>>>
>>>>>> The KSZ8081 spec says the following about bit BMCR_PDOWN:
>>>>>>
>>>>>> If software reset (Register 0.15) is used to exit power-down mode
>>>>>> (Register 0.11 = 1), two software reset writes (Register 0.15 = 1)
>>>>>> are required. The first write clears power-down mode; the second
>>>>>> write resets the chip and re-latches the pin strapping pin values.
>>>>>>
>>>>>> Maybe this causes the issue you see and genphy_soft_reset() isn't
>>>>>> appropriate for this PHY. Please re-test with the KSZ8081 soft
>>>>>&g
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 06.04.2021 04:07, Joakim Zhang wrote:
>
> Hi Heiner,
>
>> -Original Message-
>> From: Heiner Kallweit
>> Sent: 2021年4月5日 20:10
>> To: christian.me...@t2data.com; Joakim Zhang ;
>> and...@lunn.ch; li...@armlinux.org.uk; da...@davemloft.net;
>> k...@kernel.org
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>>
>> Subject: Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume
>> back
>>
>> On 05.04.2021 10:43, Christian Melki wrote:
>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may cut
>>>>>> off PHY power") invokes phy_init_hw() when MDIO bus resume, it will
>>>>>> soft reset PHY if PHY driver implements soft_reset callback.
>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback to
>>>>>> genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081. After
>>>>>> these two patches, I found i.MX6UL 14x14 EVK which connected to
>>>>>> KSZ8081RNB doesn't work any more when system resume back, MAC
>> driver is fec_main.c.
>>>>>>
>>>>>> It's obvious that initializing PHY hardware when MDIO bus resume
>>>>>> back would introduce some regression when PHY implements
>>>>>> soft_reset. When I
>>>>>
>>>>> Why is this obvious? Please elaborate on why a soft reset should
>>>>> break something.
>>>>>
>>>>>> am debugging, I found PHY works fine if MAC doesn't support
>>>>>> suspend/resume or phy_stop()/phy_start() doesn't been called during
>>>>>> suspend/resume. This let me realize, PHY state machine
>>>>>> phy_state_machine() could do something breaks the PHY.
>>>>>>
>>>>>> As we known, MAC resume first and then MDIO bus resume when system
>>>>>> resume back from suspend. When MAC resume, usually it will invoke
>>>>>> phy_start() where to change PHY state to PHY_UP, then trigger the
>>>>>> stat> machine to run now. In phy_state_machine(), it will
>>>>>> start/config auto-nego, then change PHY state to PHY_NOLINK, what
>>>>>> to next is periodically check PHY link status. When MDIO bus
>>>>>> resume, it will initialize PHY hardware, including soft_reset, what
>>>>>> would soft_reset affect seems various from different PHYs. For
>>>>>> KSZ8081RNB, when it in PHY_NOLINK state and then perform a soft reset,
>> it will never complete auto-nego.
>>>>>
>>>>> Why? That would need to be checked in detail. Maybe chip errata
>>>>> documentation provides a hint.
>>>>>
>>>>
>>>> The KSZ8081 spec says the following about bit BMCR_PDOWN:
>>>>
>>>> If software reset (Register 0.15) is
>>>> used to exit power-down mode
>>>> (Register 0.11 = 1), two software
>>>> reset writes (Register 0.15 = 1) are
>>>> required. The first write clears
>>>> power-down mode; the second
>>>> write resets the chip and re-latches
>>>> the pin strapping pin values.
>>>>
>>>> Maybe this causes the issue you see and genphy_soft_reset() isn't
>>>> appropriate for this PHY. Please re-test with the KSZ8081 soft reset
>>>> following the spec comment.
>>>>
>>>
>>> Interesting. Never expected that behavior.
>>> Thanks for catching it. Skimmed through the datasheets/erratas.
>>> This is what I found (micrel.c):
>>>
>>> 10/100:
>>> 8001 - Unaffected?
>>> 8021/8031 - Double reset after PDOWN.
>>> 8041 - Errata. PDOWN broken. Recommended do not use. Unclear if reset
>>> solves the issue since errata says no error after reset but is also
>>> claiming that only toggling PDOWN (may) or power will help.
>>> 8051 - Double reset after PDOWN.
>>> 8061 - Double reset after PDOWN.
>>> 8081 - Double reset after PDOWN.
>>> 8091 - Double reset after PDOWN.
>>>
>>> 10/100/1000:
>>> Nothing in gigabit afaics.
>>>
>>> Switches:
>>> 8862 - Not affected?
>>> 8863 - Errata. PDOWN broken. Reset will not help. Workaround exists.
>>> 8864 - Not affected?
&
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 05.04.2021 15:53, Christian Melki wrote:
> On 4/5/21 2:09 PM, Heiner Kallweit wrote:
>> On 05.04.2021 10:43, Christian Melki wrote:
>>> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>>>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may cut
>>>>>> off PHY power") invokes phy_init_hw() when MDIO bus resume, it will
>>>>>> soft reset PHY if PHY driver implements soft_reset callback.
>>>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback to
>>>>>> genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081. After these
>>>>>> two patches, I found i.MX6UL 14x14 EVK which connected to KSZ8081RNB
>>>>>> doesn't
>>>>>> work any more when system resume back, MAC driver is fec_main.c.
>>>>>>
>>>>>> It's obvious that initializing PHY hardware when MDIO bus resume back
>>>>>> would introduce some regression when PHY implements soft_reset. When I
>>>>>
>>>>> Why is this obvious? Please elaborate on why a soft reset should break
>>>>> something.
>>>>>
>>>>>> am debugging, I found PHY works fine if MAC doesn't support
>>>>>> suspend/resume
>>>>>> or phy_stop()/phy_start() doesn't been called during suspend/resume. This
>>>>>> let me realize, PHY state machine phy_state_machine() could do something
>>>>>> breaks the PHY.
>>>>>>
>>>>>> As we known, MAC resume first and then MDIO bus resume when system
>>>>>> resume back from suspend. When MAC resume, usually it will invoke
>>>>>> phy_start() where to change PHY state to PHY_UP, then trigger the stat>
>>>>>> machine to run now. In phy_state_machine(), it will start/config
>>>>>> auto-nego, then change PHY state to PHY_NOLINK, what to next is
>>>>>> periodically check PHY link status. When MDIO bus resume, it will
>>>>>> initialize PHY hardware, including soft_reset, what would soft_reset
>>>>>> affect seems various from different PHYs. For KSZ8081RNB, when it in
>>>>>> PHY_NOLINK state and then perform a soft reset, it will never complete
>>>>>> auto-nego.
>>>>>
>>>>> Why? That would need to be checked in detail. Maybe chip errata
>>>>> documentation provides a hint.
>>>>>
>>>>
>>>> The KSZ8081 spec says the following about bit BMCR_PDOWN:
>>>>
>>>> If software reset (Register 0.15) is
>>>> used to exit power-down mode
>>>> (Register 0.11 = 1), two software
>>>> reset writes (Register 0.15 = 1) are
>>>> required. The first write clears
>>>> power-down mode; the second
>>>> write resets the chip and re-latches
>>>> the pin strapping pin values.
>>>>
>>>> Maybe this causes the issue you see and genphy_soft_reset() isn't
>>>> appropriate for this PHY. Please re-test with the KSZ8081 soft reset
>>>> following the spec comment.
>>>>
>>>
>>> Interesting. Never expected that behavior.
>>> Thanks for catching it. Skimmed through the datasheets/erratas.
>>> This is what I found (micrel.c):
>>>
>>> 10/100:
>>> 8001 - Unaffected?
>>> 8021/8031 - Double reset after PDOWN.
>>> 8041 - Errata. PDOWN broken. Recommended do not use. Unclear if reset
>>> solves the issue since errata says no error after reset but is also
>>> claiming that only toggling PDOWN (may) or power will help.
>>> 8051 - Double reset after PDOWN.
>>> 8061 - Double reset after PDOWN.
>>> 8081 - Double reset after PDOWN.
>>> 8091 - Double reset after PDOWN.
>>>
>>> 10/100/1000:
>>> Nothing in gigabit afaics.
>>>
>>> Switches:
>>> 8862 - Not affected?
>>> 8863 - Errata. PDOWN broken. Reset will not help. Workaround exists.
>>> 8864 - Not affected?
>>> 8873 - Errata. PDOWN broken. Reset will not help. Workaround exists.
>>> 9477 - Errata. PDOWN broken. Will randomly cause link failure on
>>> adjacent links. Do not use.
>>>
>>> This certainly explains a lot.
>>>
>>>>>>
>>>>>> This patch changes PHY state to PHY_UP when MDIO bus resume back, it
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 05.04.2021 10:43, Christian Melki wrote:
> On 4/5/21 12:48 AM, Heiner Kallweit wrote:
>> On 04.04.2021 16:09, Heiner Kallweit wrote:
>>> On 04.04.2021 12:07, Joakim Zhang wrote:
>>>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may cut
>>>> off PHY power") invokes phy_init_hw() when MDIO bus resume, it will
>>>> soft reset PHY if PHY driver implements soft_reset callback.
>>>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback to
>>>> genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081. After these
>>>> two patches, I found i.MX6UL 14x14 EVK which connected to KSZ8081RNB
>>>> doesn't
>>>> work any more when system resume back, MAC driver is fec_main.c.
>>>>
>>>> It's obvious that initializing PHY hardware when MDIO bus resume back
>>>> would introduce some regression when PHY implements soft_reset. When I
>>>
>>> Why is this obvious? Please elaborate on why a soft reset should break
>>> something.
>>>
>>>> am debugging, I found PHY works fine if MAC doesn't support suspend/resume
>>>> or phy_stop()/phy_start() doesn't been called during suspend/resume. This
>>>> let me realize, PHY state machine phy_state_machine() could do something
>>>> breaks the PHY.
>>>>
>>>> As we known, MAC resume first and then MDIO bus resume when system
>>>> resume back from suspend. When MAC resume, usually it will invoke
>>>> phy_start() where to change PHY state to PHY_UP, then trigger the stat>
>>>> machine to run now. In phy_state_machine(), it will start/config
>>>> auto-nego, then change PHY state to PHY_NOLINK, what to next is
>>>> periodically check PHY link status. When MDIO bus resume, it will
>>>> initialize PHY hardware, including soft_reset, what would soft_reset
>>>> affect seems various from different PHYs. For KSZ8081RNB, when it in
>>>> PHY_NOLINK state and then perform a soft reset, it will never complete
>>>> auto-nego.
>>>
>>> Why? That would need to be checked in detail. Maybe chip errata
>>> documentation provides a hint.
>>>
>>
>> The KSZ8081 spec says the following about bit BMCR_PDOWN:
>>
>> If software reset (Register 0.15) is
>> used to exit power-down mode
>> (Register 0.11 = 1), two software
>> reset writes (Register 0.15 = 1) are
>> required. The first write clears
>> power-down mode; the second
>> write resets the chip and re-latches
>> the pin strapping pin values.
>>
>> Maybe this causes the issue you see and genphy_soft_reset() isn't
>> appropriate for this PHY. Please re-test with the KSZ8081 soft reset
>> following the spec comment.
>>
>
> Interesting. Never expected that behavior.
> Thanks for catching it. Skimmed through the datasheets/erratas.
> This is what I found (micrel.c):
>
> 10/100:
> 8001 - Unaffected?
> 8021/8031 - Double reset after PDOWN.
> 8041 - Errata. PDOWN broken. Recommended do not use. Unclear if reset
> solves the issue since errata says no error after reset but is also
> claiming that only toggling PDOWN (may) or power will help.
> 8051 - Double reset after PDOWN.
> 8061 - Double reset after PDOWN.
> 8081 - Double reset after PDOWN.
> 8091 - Double reset after PDOWN.
>
> 10/100/1000:
> Nothing in gigabit afaics.
>
> Switches:
> 8862 - Not affected?
> 8863 - Errata. PDOWN broken. Reset will not help. Workaround exists.
> 8864 - Not affected?
> 8873 - Errata. PDOWN broken. Reset will not help. Workaround exists.
> 9477 - Errata. PDOWN broken. Will randomly cause link failure on
> adjacent links. Do not use.
>
> This certainly explains a lot.
>
>>>>
>>>> This patch changes PHY state to PHY_UP when MDIO bus resume back, it
>>>> should be reasonable after PHY hardware re-initialized. Also give state
>>>> machine a chance to start/config auto-nego again.
>>>>
>>>
>>> If the MAC driver calls phy_stop() on suspend, then phydev->suspended
>>> is true and mdio_bus_phy_may_suspend() returns false. As a consequence
>>> phydev->suspended_by_mdio_bus is false and mdio_bus_phy_resume()
>>> skips the PHY hw initialization.
>>> Please also note that mdio_bus_phy_suspend() calls phy_stop_machine()
>>> that sets the state to PHY_UP.
>>>
>>
>> Forgot that MDIO bus suspend is done before MAC driver suspend.
>> Therefore disregard this part for now.
>>
>>> Havin
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 04.04.2021 16:09, Heiner Kallweit wrote:
> On 04.04.2021 12:07, Joakim Zhang wrote:
>> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may cut
>> off PHY power") invokes phy_init_hw() when MDIO bus resume, it will
>> soft reset PHY if PHY driver implements soft_reset callback.
>> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback to
>> genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081. After these
>> two patches, I found i.MX6UL 14x14 EVK which connected to KSZ8081RNB doesn't
>> work any more when system resume back, MAC driver is fec_main.c.
>>
>> It's obvious that initializing PHY hardware when MDIO bus resume back
>> would introduce some regression when PHY implements soft_reset. When I
>
> Why is this obvious? Please elaborate on why a soft reset should break
> something.
>
>> am debugging, I found PHY works fine if MAC doesn't support suspend/resume
>> or phy_stop()/phy_start() doesn't been called during suspend/resume. This
>> let me realize, PHY state machine phy_state_machine() could do something
>> breaks the PHY.
>>
>> As we known, MAC resume first and then MDIO bus resume when system
>> resume back from suspend. When MAC resume, usually it will invoke
>> phy_start() where to change PHY state to PHY_UP, then trigger the stat>
>> machine to run now. In phy_state_machine(), it will start/config
>> auto-nego, then change PHY state to PHY_NOLINK, what to next is
>> periodically check PHY link status. When MDIO bus resume, it will
>> initialize PHY hardware, including soft_reset, what would soft_reset
>> affect seems various from different PHYs. For KSZ8081RNB, when it in
>> PHY_NOLINK state and then perform a soft reset, it will never complete
>> auto-nego.
>
> Why? That would need to be checked in detail. Maybe chip errata
> documentation provides a hint.
>
The KSZ8081 spec says the following about bit BMCR_PDOWN:
If software reset (Register 0.15) is
used to exit power-down mode
(Register 0.11 = 1), two software
reset writes (Register 0.15 = 1) are
required. The first write clears
power-down mode; the second
write resets the chip and re-latches
the pin strapping pin values.
Maybe this causes the issue you see and genphy_soft_reset() isn't
appropriate for this PHY. Please re-test with the KSZ8081 soft reset
following the spec comment.
>>
>> This patch changes PHY state to PHY_UP when MDIO bus resume back, it
>> should be reasonable after PHY hardware re-initialized. Also give state
>> machine a chance to start/config auto-nego again.
>>
>
> If the MAC driver calls phy_stop() on suspend, then phydev->suspended
> is true and mdio_bus_phy_may_suspend() returns false. As a consequence
> phydev->suspended_by_mdio_bus is false and mdio_bus_phy_resume()
> skips the PHY hw initialization.
> Please also note that mdio_bus_phy_suspend() calls phy_stop_machine()
> that sets the state to PHY_UP.
>
Forgot that MDIO bus suspend is done before MAC driver suspend.
Therefore disregard this part for now.
> Having said that the current argumentation isn't convincing. I'm not
> aware of such issues on other systems, therefore it's likely that
> something is system-dependent.
>
> Please check the exact call sequence on your system, maybe it
> provides a hint.
>
>> Signed-off-by: Joakim Zhang
>> ---
>> drivers/net/phy/phy_device.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index cc38e326405a..312a6f662481 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -306,6 +306,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct
>> device *dev)
>> ret = phy_resume(phydev);
>> if (ret < 0)
>> return ret;
>> +
>> +/* PHY state could be changed to PHY_NOLINK from MAC controller resume
>> + * rounte with phy_start(), here change to PHY_UP after re-initializing
>> + * PHY hardware, let PHY state machine to start/config auto-nego again.
>> + */
>> +phydev->state = PHY_UP;
>> +
>> no_resume:
>> if (phydev->attached_dev && phydev->adjust_link)
>> phy_start_machine(phydev);
>>
>
Re: [PATCH] net: phy: fix PHY possibly unwork after MDIO bus resume back
On 04.04.2021 12:07, Joakim Zhang wrote:
> commit 4c0d2e96ba055 ("net: phy: consider that suspend2ram may cut
> off PHY power") invokes phy_init_hw() when MDIO bus resume, it will
> soft reset PHY if PHY driver implements soft_reset callback.
> commit 764d31cacfe4 ("net: phy: micrel: set soft_reset callback to
> genphy_soft_reset for KSZ8081") adds soft_reset for KSZ8081. After these
> two patches, I found i.MX6UL 14x14 EVK which connected to KSZ8081RNB doesn't
> work any more when system resume back, MAC driver is fec_main.c.
>
> It's obvious that initializing PHY hardware when MDIO bus resume back
> would introduce some regression when PHY implements soft_reset. When I
Why is this obvious? Please elaborate on why a soft reset should break
something.
> am debugging, I found PHY works fine if MAC doesn't support suspend/resume
> or phy_stop()/phy_start() doesn't been called during suspend/resume. This
> let me realize, PHY state machine phy_state_machine() could do something
> breaks the PHY.
>
> As we known, MAC resume first and then MDIO bus resume when system
> resume back from suspend. When MAC resume, usually it will invoke
> phy_start() where to change PHY state to PHY_UP, then trigger the stat>
> machine to run now. In phy_state_machine(), it will start/config
> auto-nego, then change PHY state to PHY_NOLINK, what to next is
> periodically check PHY link status. When MDIO bus resume, it will
> initialize PHY hardware, including soft_reset, what would soft_reset
> affect seems various from different PHYs. For KSZ8081RNB, when it in
> PHY_NOLINK state and then perform a soft reset, it will never complete
> auto-nego.
Why? That would need to be checked in detail. Maybe chip errata
documentation provides a hint.
>
> This patch changes PHY state to PHY_UP when MDIO bus resume back, it
> should be reasonable after PHY hardware re-initialized. Also give state
> machine a chance to start/config auto-nego again.
>
If the MAC driver calls phy_stop() on suspend, then phydev->suspended
is true and mdio_bus_phy_may_suspend() returns false. As a consequence
phydev->suspended_by_mdio_bus is false and mdio_bus_phy_resume()
skips the PHY hw initialization.
Please also note that mdio_bus_phy_suspend() calls phy_stop_machine()
that sets the state to PHY_UP.
Having said that the current argumentation isn't convincing. I'm not
aware of such issues on other systems, therefore it's likely that
something is system-dependent.
Please check the exact call sequence on your system, maybe it
provides a hint.
> Signed-off-by: Joakim Zhang
> ---
> drivers/net/phy/phy_device.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index cc38e326405a..312a6f662481 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -306,6 +306,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct
> device *dev)
> ret = phy_resume(phydev);
> if (ret < 0)
> return ret;
> +
> + /* PHY state could be changed to PHY_NOLINK from MAC controller resume
> + * rounte with phy_start(), here change to PHY_UP after re-initializing
> + * PHY hardware, let PHY state machine to start/config auto-nego again.
> + */
> + phydev->state = PHY_UP;
> +
> no_resume:
> if (phydev->attached_dev && phydev->adjust_link)
> phy_start_machine(phydev);
>
Re: [PATCH] ethernet/realtek/r8169: Fix a double free in rtl8169_start_xmit
On 29.03.2021 11:02, Lv Yunlong wrote:
> In rtl8169_start_xmit, it calls rtl8169_tso_csum_v2(tp, skb, opts) and
> rtl8169_tso_csum_v2() calls __skb_put_padto(skb, padto, false). If
> __skb_put_padto() failed, it will free the skb in the first time and
> return error. Then rtl8169_start_xmit will goto err_dma_0.
>
No, the skb isn't freed here in case of error. Have a look at the
implementation of __skb_put_padto() and see also cc6528bc9a0c
("r8169: fix potential skb double free in an error path").
> But in err_dma_0 label, the skb is freed by dev_kfree_skb_any(skb) in
> the second time.
>
> My patch adds a new label inside the old err_dma_0 label to avoid the
> double free and renames the error labels to keep the origin function
> unchanged.
>
> Fixes: b8447abc4c8fb ("r8169: factor out rtl8169_tx_map")
> Signed-off-by: Lv Yunlong
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index f704da3f214c..91c43399712b 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -4217,13 +4217,13 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff
> *skb,
>
> if (unlikely(rtl8169_tx_map(tp, opts, skb_headlen(skb), skb->data,
> entry, false)))
> - goto err_dma_0;
> + goto err_dma_1;
>
> txd_first = tp->TxDescArray + entry;
>
> if (frags) {
> if (rtl8169_xmit_frags(tp, skb, opts, entry))
> - goto err_dma_1;
> + goto err_dma_2;
> entry = (entry + frags) % NUM_TX_DESC;
> }
>
> @@ -4270,10 +4270,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff
> *skb,
>
> return NETDEV_TX_OK;
>
> -err_dma_1:
> +err_dma_2:
> rtl8169_unmap_tx_skb(tp, entry);
> -err_dma_0:
> +err_dma_1:
> dev_kfree_skb_any(skb);
> +err_dma_0:
> dev->stats.tx_dropped++;
> return NETDEV_TX_OK;
>
>
Re: [PATCH] PCI: Remove pci_try_set_mwi
On 26.03.2021 22:26, Bjorn Helgaas wrote: > [+cc Randy, Andrew (though I'm sure you have zero interest in this > ancient question :))] > > On Wed, Dec 09, 2020 at 09:31:21AM +0100, Heiner Kallweit wrote: >> pci_set_mwi() and pci_try_set_mwi() do exactly the same, just that the >> former one is declared as __must_check. However also some callers of >> pci_set_mwi() have a comment that it's an optional feature. I don't >> think there's much sense in this separation and the use of >> __must_check. Therefore remove pci_try_set_mwi() and remove the >> __must_check attribute from pci_set_mwi(). >> I don't expect either function to be used in new code anyway. > > There's not much I like better than removing things. But some > significant thought went into adding pci_try_set_mwi() in the first > place, so I need a little more convincing about why it's safe to > remove it. > Thanks for the link to the 13 yrs old discussion. Unfortunately it doesn't mention any real argument for the __must_check, just: "And one of the reasons for adding the __must_check annotation is to weed out design errors." And the very next response in the discussion calls this a "non-argument". Plus not mentioning what the other reasons could be. Currently we have three ancient drivers that bail out if the call fails. Most callers of pci_set_mwi() use the return code only to emit an error message, but they proceed normally. Majority of users calls pci_try_set_mwi(). And as stated in the commit message I don't expect any new usage of pci_set_mwi(). > The argument should cite the discussion about adding it. I think one > of the earliest conversations is here: > https://lore.kernel.org/linux-ide/20070404213704.224128ec.randy.dun...@oracle.com/ > >> Signed-off-by: Heiner Kallweit >> --- >> patch applies on top of pci/misc for v5.11 >> --- >> Documentation/PCI/pci.rst | 5 + >> drivers/ata/pata_cs5530.c | 2 +- >> drivers/ata/sata_mv.c | 2 +- >> drivers/dma/dw/pci.c | 2 +- >> drivers/dma/hsu/pci.c | 2 +- >> drivers/ide/cs5530.c | 2 +- >> drivers/mfd/intel-lpss-pci.c | 2 +- >> drivers/net/ethernet/adaptec/starfire.c | 2 +- >> drivers/net/ethernet/alacritech/slicoss.c | 2 +- >> drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + >> drivers/net/ethernet/sun/cassini.c| 4 ++-- >> drivers/net/wireless/intersil/p54/p54pci.c| 2 +- >> .../intersil/prism54/islpci_hotplug.c | 3 +-- >> .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- >> drivers/pci/pci.c | 19 --- >> drivers/scsi/3w-9xxx.c| 4 ++-- >> drivers/scsi/3w-sas.c | 4 ++-- >> drivers/scsi/csiostor/csio_init.c | 2 +- >> drivers/scsi/lpfc/lpfc_init.c | 2 +- >> drivers/scsi/qla2xxx/qla_init.c | 8 >> drivers/scsi/qla2xxx/qla_mr.c | 2 +- >> drivers/tty/serial/8250/8250_lpss.c | 2 +- >> drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- >> drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- >> drivers/usb/gadget/udc/net2280.c | 2 +- >> drivers/usb/gadget/udc/pch_udc.c | 2 +- >> include/linux/pci.h | 5 ++--- >> 27 files changed, 33 insertions(+), 60 deletions(-) >> >> diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst >> index 814b40f83..120362cc9 100644 >> --- a/Documentation/PCI/pci.rst >> +++ b/Documentation/PCI/pci.rst >> @@ -226,10 +226,7 @@ If the PCI device can use the PCI >> Memory-Write-Invalidate transaction, >> call pci_set_mwi(). This enables the PCI_COMMAND bit for Mem-Wr-Inval >> and also ensures that the cache line size register is set correctly. >> Check the return value of pci_set_mwi() as not all architectures >> -or chip-sets may support Memory-Write-Invalidate. Alternatively, >> -if Mem-Wr-Inval would be nice to have but is not required, call >> -pci_try_set_mwi() to have the system do its best effort at enabling >> -Mem-Wr-Inval. >> +or chip-sets may support Memory-Write-Invalidate. >> >> >> Request MMIO/IOP resources >> diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c >> index ad75d02b6..8654b3ae1 100644 >> --- a/drivers/ata/pata_cs5530.c >> +++ b/drivers/ata/pata_cs5530.c >> @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) >
Re: [PATCH net-next 1/2] net: phy: add genphy_c45_loopback
On 23.03.2021 17:46, Wong Vee Khee wrote: > Add generic code to enable C45 PHY loopback into the common phy-c45.c > file. This will allow C45 PHY drivers aceess this by setting > .set_loopback. > > Suggested-by: Heiner Kallweit > Signed-off-by: Wong Vee Khee > --- > drivers/net/phy/phy-c45.c | 8 > include/linux/phy.h | 1 + > 2 files changed, 9 insertions(+) > LGTM Reviewed-by: Heiner Kallweit
Re: [PATCH net-next 1/1] net: phy: marvell10g: Add PHY loopback support for 88E2110 PHY
On 23.03.2021 09:48, Wong Vee Khee wrote:
> From: Tan Tee Min
>
> Add support for PHY loopback for the Marvell 88E2110 PHY.
>
> This allow user to perform selftest using ethtool.
>
> Signed-off-by: Tan Tee Min
> Signed-off-by: Wong Vee Khee
> ---
> drivers/net/phy/marvell10g.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index b1bb9b8e1e4e..c45a8f11bdcf 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -89,6 +89,8 @@ enum {
> MV_V2_TEMP_CTRL_DISABLE = 0xc000,
> MV_V2_TEMP = 0xf08c,
> MV_V2_TEMP_UNKNOWN = 0x9600, /* unknown function */
> +
> + MV_LOOPBACK = BIT(14), /* Loopback (88E2110 only) */
Why do you state 88E2110 only?
This is the standard PCS loopback bit as described in clause 45.2.3.1.2
It's defined already as MDIO_PCS_CTRL1_LOOPBACK.
E.g. the 88x3310 spec also describes this bit.
> };
>
> struct mv3310_priv {
> @@ -765,6 +767,15 @@ static int mv3310_set_tunable(struct phy_device *phydev,
> }
> }
>
> +static int mv3310_loopback(struct phy_device *phydev, bool enable)
> +{
> + if (phydev->drv->phy_id != MARVELL_PHY_ID_88E2110)
> + return -EOPNOTSUPP;
If you use the function in the 2110 PHY driver only, then why this check?
And why name it 3310 if it can be used with 2110 only?
This function uses c45 standard functionality only, therefore it should
go to generic code (similar to genphy_loopback for c22).
> +
> + return phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T,
> + MV_LOOPBACK, enable ? MV_LOOPBACK : 0);
> +}
> +
> static struct phy_driver mv3310_drivers[] = {
> {
> .phy_id = MARVELL_PHY_ID_88X3310,
> @@ -796,6 +807,7 @@ static struct phy_driver mv3310_drivers[] = {
> .get_tunable= mv3310_get_tunable,
> .set_tunable= mv3310_set_tunable,
> .remove = mv3310_remove,
> + .set_loopback = mv3310_loopback,
> },
> };
>
>
Re: [PATCH net V2 1/1] net: phy: fix invalid phy id when probe using C22
On 18.03.2021 10:09, Wong Vee Khee wrote:
> When using Clause-22 to probe for PHY devices such as the Marvell
> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> which caused the PHY framework failed to attach the Marvell PHY
> driver.
>
> Fixed this by adding a check of PHY ID equals to all zeroes.
>
> Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID
> handling")
> Cc: sta...@vger.kernel.org
> Reviewed-by: Voon Weifeng
> Signed-off-by: Wong Vee Khee
> ---
> v2 changelog:
> - added fixes tag
> - marked for net instead of net-next
> ---
> drivers/net/phy/phy_device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index cc38e326405a..c12c30254c11 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -809,8 +809,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr,
> u32 *phy_id)
>
> *phy_id |= phy_reg;
>
> - /* If the phy_id is mostly Fs, there is no device there */
> - if ((*phy_id & 0x1fff) == 0x1fff)
> + /* If the phy_id is mostly Fs or all zeroes, there is no device there */
> + if (((*phy_id & 0x1fff) == 0x1fff) || (*phy_id == 0))
> return -ENODEV;
>
> return 0;
>
+ the authors of 0cc8fecf041d ("net: phy: Allow mdio buses to auto-probe c45
devices")
In case of MDIOBUS_C22_C45 we probe c22 first, and then c45.
This causes problems with c45 PHY's that have rudimentary c22 support
and return 0 when reading the c22 PHY ID registers.
Is there a specific reason why c22 is probed first? Reversing the order
would solve the issue we speak about here.
c45-probing of c22-only PHY's shouldn't return false positives
(at least at a first glance).
Re: [PATCH v2 net-next] net: phy: at803x: remove at803x_aneg_done()
On 18.03.2021 20:44, Michael Walle wrote: > Here is what Vladimir says about it: > > at803x_aneg_done() keeps the aneg reporting as "not done" even when > the copper-side link was reported as up, but the in-band autoneg has > not finished. > > That was the _intended_ behavior when that code was introduced, and > Heiner have said about it [1]: > > | That's not nice from the PHY: > | It signals "link up", and if the system asks the PHY for link details, > | then it sheepishly says "well, link is *almost* up". > > If the specification of phy_aneg_done behavior does not include > in-band autoneg (and it doesn't), then this piece of code does not > belong here. > > The fact that we can no longer trigger this code from phylib is yet > another reason why it fails at its intended (and wrong) purpose and > should be removed. > > Removing the SGMII link check, would just keep the call to > genphy_aneg_done(), which is also the fallback. Thus we can just remove > at803x_aneg_done() altogether. > > [1] > https://lore.kernel.org/netdev/fdf0074a-2572-5914-6f3e-77202cbf9...@gmail.com/ > > Suggested-by: Vladimir Oltean > Signed-off-by: Michael Walle > --- Reviewed-by: Heiner Kallweit
Re: [PATCH net-next] net: phy: at803x: remove at803x_aneg_done()
On 18.03.2021 18:04, Vladimir Oltean wrote: > On Thu, Mar 18, 2021 at 05:38:13PM +0100, Michael Walle wrote: >> Am 2021-03-18 17:21, schrieb Heiner Kallweit: >>> On 18.03.2021 16:17, Vladimir Oltean wrote: >>>> On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: >>>>> On 18.03.2021 15:23, Michael Walle wrote: >>>>>> at803x_aneg_done() is pretty much dead code since the patch series >>>>>> "net: phy: improve and simplify phylib state machine" [1]. >>>>>> Remove it. >>>>>> >>>>> >>>>> Well, it's not dead, it's resting .. There are few places where >>>>> phy_aneg_done() is used. So you would need to explain: >>>>> - why these users can't be used with this PHY driver >>>>> - or why the aneg_done callback isn't needed here and the >>>>> genphy_aneg_done() fallback is sufficient >>>> >>>> The piece of code that Michael is removing keeps the aneg reporting as >>>> "not done" even when the copper-side link was reported as up, but the >>>> in-band autoneg has not finished. >>>> >>>> That was the _intended_ behavior when that code was introduced, and >>>> you >>>> have said about it: >>>> https://www.spinics.net/lists/stable/msg389193.html >>>> >>>> | That's not nice from the PHY: >>>> | It signals "link up", and if the system asks the PHY for link details, >>>> | then it sheepishly says "well, link is *almost* up". >>>> >>>> If the specification of phy_aneg_done behavior does not include >>>> in-band >>>> autoneg (and it doesn't), then this piece of code does not belong >>>> here. >>>> >>>> The fact that we can no longer trigger this code from phylib is yet >>>> another reason why it fails at its intended (and wrong) purpose and >>>> should be removed. >>>> >>> I don't argue against the change, I just think that the current commit >>> description isn't sufficient. What you just said I would have expected >>> in the commit description. >> >> I'll come up with a better one, Vladimir, may I use parts of the text >> above? > > My words aren't copyrighted, so feel free, however you might want to > check with Heiner too for his part, you never know. > I'm not paid for the content of my mails, so feel free to quote.
Re: [PATCH net V2 1/1] net: phy: fix invalid phy id when probe using C22
On 18.03.2021 17:02, Florian Fainelli wrote: > > > On 3/18/2021 6:25 AM, Heiner Kallweit wrote: >> On 18.03.2021 10:09, Wong Vee Khee wrote: >>> When using Clause-22 to probe for PHY devices such as the Marvell >>> 88E2110, PHY ID with value 0 is read from the MII PHYID registers >>> which caused the PHY framework failed to attach the Marvell PHY >>> driver. >>> >>> Fixed this by adding a check of PHY ID equals to all zeroes. >>> >> >> I was wondering whether we have, and may break, use cases where a PHY, >> for whatever reason, reports PHY ID 0, but works with the genphy >> driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore >> the patch may break the fixed phy. >> Having said that I think your patch is ok, but we need a change of >> the PHY ID reported by swphy_read_reg() first. >> At a first glance changing the PHY ID to 0x0001 in swphy_read_reg() >> should be sufficient. This value shouldn't collide with any real world >> PHY ID. > > It most likely would not, but it could be considered an ABI breakage, > unless we filter out what we report to user-space via SIOGCMIIREG and > /sys/class/mdio_bus/*/*/phy_id > > Ideally we would have assigned an unique PHY OUI to the fixed PHY but > that would have required registering Linux as a vendor, and the process > is not entirely clear to me about how to go about doing that. > -- In the OUI list I found entry 58-9C-FC, belonging to FreeBSD Foundation. Not sure what they use it for, but it seems adding Linux as a vendor wouldn't be a total exception. > Florian > Heiner
Re: [PATCH net-next] net: phy: at803x: remove at803x_aneg_done()
On 18.03.2021 16:17, Vladimir Oltean wrote: > On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: >> On 18.03.2021 15:23, Michael Walle wrote: >>> at803x_aneg_done() is pretty much dead code since the patch series >>> "net: phy: improve and simplify phylib state machine" [1]. Remove it. >>> >> >> Well, it's not dead, it's resting .. There are few places where >> phy_aneg_done() is used. So you would need to explain: >> - why these users can't be used with this PHY driver >> - or why the aneg_done callback isn't needed here and the >> genphy_aneg_done() fallback is sufficient > > The piece of code that Michael is removing keeps the aneg reporting as > "not done" even when the copper-side link was reported as up, but the > in-band autoneg has not finished. > > That was the _intended_ behavior when that code was introduced, and you > have said about it: > https://www.spinics.net/lists/stable/msg389193.html > > | That's not nice from the PHY: > | It signals "link up", and if the system asks the PHY for link details, > | then it sheepishly says "well, link is *almost* up". > > If the specification of phy_aneg_done behavior does not include in-band > autoneg (and it doesn't), then this piece of code does not belong here. > > The fact that we can no longer trigger this code from phylib is yet > another reason why it fails at its intended (and wrong) purpose and > should be removed. > I don't argue against the change, I just think that the current commit description isn't sufficient. What you just said I would have expected in the commit description.
Re: [PATCH net-next] net: phy: at803x: remove at803x_aneg_done()
On 18.03.2021 15:23, Michael Walle wrote:
> at803x_aneg_done() is pretty much dead code since the patch series
> "net: phy: improve and simplify phylib state machine" [1]. Remove it.
>
Well, it's not dead, it's resting .. There are few places where
phy_aneg_done() is used. So you would need to explain:
- why these users can't be used with this PHY driver
- or why the aneg_done callback isn't needed here and the
genphy_aneg_done() fallback is sufficient
> [1]
> https://lore.kernel.org/netdev/922c223b-7bc0-e0ec-345d-2034b796a...@gmail.com/
>
> Suggested-by: Vladimir Oltean
> Signed-off-by: Michael Walle
> ---
> drivers/net/phy/at803x.c | 31 ---
> 1 file changed, 31 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index c2aa4c92edde..d7799beb811c 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -751,36 +751,6 @@ static void at803x_link_change_notify(struct phy_device
> *phydev)
> }
> }
>
> -static int at803x_aneg_done(struct phy_device *phydev)
> -{
> - int ccr;
> -
> - int aneg_done = genphy_aneg_done(phydev);
> - if (aneg_done != BMSR_ANEGCOMPLETE)
> - return aneg_done;
> -
> - /*
> - * in SGMII mode, if copper side autoneg is successful,
> - * also check SGMII side autoneg result
> - */
> - ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> - if ((ccr & AT803X_MODE_CFG_MASK) != AT803X_MODE_CFG_SGMII)
> - return aneg_done;
> -
> - /* switch to SGMII/fiber page */
> - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
> -
> - /* check if the SGMII link is OK. */
> - if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) {
> - phydev_warn(phydev, "803x_aneg_done: SGMII link is not ok\n");
> - aneg_done = 0;
> - }
> - /* switch back to copper page */
> - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);
> -
> - return aneg_done;
> -}
> -
> static int at803x_read_status(struct phy_device *phydev)
> {
> int ss, err, old_link = phydev->link;
> @@ -1198,7 +1168,6 @@ static struct phy_driver at803x_driver[] = {
> .resume = at803x_resume,
> /* PHY_GBIT_FEATURES */
> .read_status= at803x_read_status,
> - .aneg_done = at803x_aneg_done,
> .config_intr= _config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> .get_tunable= at803x_get_tunable,
>
Re: [PATCH net V2 1/1] net: phy: fix invalid phy id when probe using C22
On 18.03.2021 10:09, Wong Vee Khee wrote:
> When using Clause-22 to probe for PHY devices such as the Marvell
> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> which caused the PHY framework failed to attach the Marvell PHY
> driver.
>
> Fixed this by adding a check of PHY ID equals to all zeroes.
>
I was wondering whether we have, and may break, use cases where a PHY,
for whatever reason, reports PHY ID 0, but works with the genphy
driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
the patch may break the fixed phy.
Having said that I think your patch is ok, but we need a change of
the PHY ID reported by swphy_read_reg() first.
At a first glance changing the PHY ID to 0x0001 in swphy_read_reg()
should be sufficient. This value shouldn't collide with any real world
PHY ID.
> Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID
> handling")
> Cc: sta...@vger.kernel.org
> Reviewed-by: Voon Weifeng
> Signed-off-by: Wong Vee Khee
> ---
> v2 changelog:
> - added fixes tag
> - marked for net instead of net-next
> ---
> drivers/net/phy/phy_device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index cc38e326405a..c12c30254c11 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -809,8 +809,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr,
> u32 *phy_id)
>
> *phy_id |= phy_reg;
>
> - /* If the phy_id is mostly Fs, there is no device there */
> - if ((*phy_id & 0x1fff) == 0x1fff)
> + /* If the phy_id is mostly Fs or all zeroes, there is no device there */
> + if (((*phy_id & 0x1fff) == 0x1fff) || (*phy_id == 0))
> return -ENODEV;
>
> return 0;
>
Re: [PATCH net-next 1/1] net: phy: fix invalid phy id when probe using C22
On 16.03.2021 09:57, Wong Vee Khee wrote: > When using Clause-22 to probe for PHY devices such as the Marvell > 88E2110, PHY ID with value 0 is read from the MII PHYID registers > which caused the PHY framework failed to attach the Marvell PHY > driver. > The issue occurs with a MAC driver that sets MDIO bus capability flag MDIOBUS_C22_C45, like stmmac? Or what is the affected MAC driver? And if you state it's a fix, a Fixes tag would be needed. > Fixed this by adding a check of PHY ID equals to all zeroes. > > Cc: sta...@vger.kernel.org > Reviewed-by: Voon Weifeng > Signed-off-by: Wong Vee Khee > --- > drivers/net/phy/phy_device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index a009d1769b08..f1afc00fcba2 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -820,8 +820,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, > u32 *phy_id) > > *phy_id |= phy_reg; > > - /* If the phy_id is mostly Fs, there is no device there */ > - if ((*phy_id & 0x1fff) == 0x1fff) > + /* If the phy_id is mostly Fs or all zeroes, there is no device there */ > + if (((*phy_id & 0x1fff) == 0x1fff) || (*phy_id == 0)) > return -ENODEV; > > return 0; >
Re: [PATCH] net: netlink: fix error return code of netlink_proto_init()
On 09.03.2021 09:33, Jia-Ju Bai wrote:
> When kcalloc() returns NULL to nl_table, no error return code of
> netlink_proto_init() is assigned.
> To fix this bug, err is assigned with -ENOMEM in this case.
>
Didn't we talk enough about your incorrect patches yesterday?
This one is incorrect again. panic() never returns.
Stop sending patches until you understand the code you're changing!
> Fixes: fab2caf62ed0 ("[NETLINK]: Call panic if nl_table allocation fails")
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
> ---
> net/netlink/af_netlink.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index dd488938447f..9ab66cfb1037 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2880,8 +2880,10 @@ static int __init netlink_proto_init(void)
> BUILD_BUG_ON(sizeof(struct netlink_skb_parms) > sizeof_field(struct
> sk_buff, cb));
>
> nl_table = kcalloc(MAX_LINKS, sizeof(*nl_table), GFP_KERNEL);
> - if (!nl_table)
> + if (!nl_table) {
> + err = -ENOMEM;
> goto panic;
> + }
>
> for (i = 0; i < MAX_LINKS; i++) {
> if (rhashtable_init(_table[i].hash,
>
Re: [PATCH] net: ieee802154: fix error return code of dgram_sendmsg()
On 08.03.2021 13:18, Jia-Ju Bai wrote:
>
>
> On 2021/3/8 18:19, Heiner Kallweit wrote:
>> On 08.03.2021 10:31, Jia-Ju Bai wrote:
>>> When sock_alloc_send_skb() returns NULL to skb, no error return code of
>>> dgram_sendmsg() is assigned.
>>> To fix this bug, err is assigned with -ENOMEM in this case.
>>>
>> Please stop sending such nonsense. Basically all such patches you
>> sent so far are false positives. You have to start thinking,
>> don't blindly trust your robot.
>> In the case here the err variable is populated by sock_alloc_send_skb().
>
> Ah, sorry, it is my fault :(
> I did not notice that the err variable is populated by sock_alloc_send_skb().
> I will think more carefully before sending patches.
>
> By the way, I wonder how to report and discuss possible bugs that I am not
> quite sure of?
> Some people told me that sending patches is better than reporting bugs via
> Bugzilla, so I write the patches of these possible bugs...
> Do you have any advice?
>
If you're quite sure that something is a bug then sending a patch is fine.
Your submissions more or less all being false positives shows that this
takes more than just forwarding bot findings, especially if you have no
idea yet regarding the quality of the bot.
Alternatively you can contact the maintainer and respective mailing list.
But again, maintainers typically are very busy and you should have done
all you can to analyze the suspected bug.
What I'd do being in your shoes:
Take the first 10 findings of a new bot and analyze in detail whether
findings are correct or false positives. Of course this means you
need to get familiar with the affected code in the respective driver.
If false positive ratio is > 5% I wouldn't send out patches w/o more
detailed analysis per finding.
Worst case a maintainer is busy and can't review your submission in time,
and the incorrect fix is applied and breaks the driver.
Typically this shouldn't happen however because Dave/Jakub won't apply
a patch w/o Ack from the respective maintainer.
Disclaimer:
I can only speak for myself. Other maintainers may see this differently.
> Thanks a lot!
>
>
> Best wishes,
> Jia-Ju Bai
>>
>>> Fixes: 78f821b64826 ("ieee802154: socket: put handling into one file")
>>> Reported-by: TOTE Robot
>>> Signed-off-by: Jia-Ju Bai
>>> ---
>>> net/ieee802154/socket.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
>>> index a45a0401adc5..a750b37c7e73 100644
>>> --- a/net/ieee802154/socket.c
>>> +++ b/net/ieee802154/socket.c
>>> @@ -642,8 +642,10 @@ static int dgram_sendmsg(struct sock *sk, struct
>>> msghdr *msg, size_t size)
>>> skb = sock_alloc_send_skb(sk, hlen + tlen + size,
>>> msg->msg_flags & MSG_DONTWAIT,
>>> );
>>> - if (!skb)
>>> + if (!skb) {
>>> + err = -ENOMEM;
>>> goto out_dev;
>>> + }
>>> skb_reserve(skb, hlen);
>>>
>
Re: [PATCH] net: ieee802154: fix error return code of dgram_sendmsg()
On 08.03.2021 10:31, Jia-Ju Bai wrote:
> When sock_alloc_send_skb() returns NULL to skb, no error return code of
> dgram_sendmsg() is assigned.
> To fix this bug, err is assigned with -ENOMEM in this case.
>
Please stop sending such nonsense. Basically all such patches you
sent so far are false positives. You have to start thinking,
don't blindly trust your robot.
In the case here the err variable is populated by sock_alloc_send_skb().
> Fixes: 78f821b64826 ("ieee802154: socket: put handling into one file")
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
> ---
> net/ieee802154/socket.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> index a45a0401adc5..a750b37c7e73 100644
> --- a/net/ieee802154/socket.c
> +++ b/net/ieee802154/socket.c
> @@ -642,8 +642,10 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr
> *msg, size_t size)
> skb = sock_alloc_send_skb(sk, hlen + tlen + size,
> msg->msg_flags & MSG_DONTWAIT,
> );
> - if (!skb)
> + if (!skb) {
> + err = -ENOMEM;
> goto out_dev;
> + }
>
> skb_reserve(skb, hlen);
>
>
Re: [PATCH] ath: ath6kl: fix error return code of ath6kl_htc_rx_bundle()
On 07.03.2021 10:31, Jia-Ju Bai wrote:
> Hi Leon,
>
> I am quite sorry for my incorrect patches...
> My static analysis tool reports some possible bugs about error handling code,
> and thus I write some patches for the bugs that seem to be true in my opinion.
> Because I am not familiar with many device drivers, some of my reported bugs
> can be false positives...
Then, before posting a patch for a driver, get familiar with it to
an extent that you can identify false positives. Relying on others
to detect the false positives is not the best approach.
>
>
> Best wishes,
> Jia-Ju Bai
>
> On 2021/3/7 17:18, Leon Romanovsky wrote:
>> On Sun, Mar 07, 2021 at 01:07:57AM -0800, Jia-Ju Bai wrote:
>>> When hif_scatter_req_get() returns NULL to scat_req, no error return
>>> code of ath6kl_htc_rx_bundle() is assigned.
>>> To fix this bug, status is assigned with -EINVAL in this case.
>>>
>>> Reported-by: TOTE Robot
>>> Signed-off-by: Jia-Ju Bai
>>> ---
>>> drivers/net/wireless/ath/ath6kl/htc_mbox.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
>>> b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
>>> index 998947ef63b6..3f8857d19a0c 100644
>>> --- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
>>> +++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
>>> @@ -1944,8 +1944,10 @@ static int ath6kl_htc_rx_bundle(struct htc_target
>>> *target,
>>>
>>> scat_req = hif_scatter_req_get(target->dev->ar);
>>>
>>> - if (scat_req == NULL)
>>> + if (scat_req == NULL) {
>>> + status = -EINVAL;
>> I'm not sure about it.
>>
>> David. Jakub,
>> Please be warned that patches from this guy are not so great.
>> I looked on 4 patches and 3 of them were wrong (2 in RDMA and 1 for mlx5)
>> plus this patch most likely is incorrect too.
>>
Re: [PATCH] net: mellanox: mlxsw: fix error return code of mlxsw_sp_router_nve_promote_decap()
On 06.03.2021 15:07, Jia-Ju Bai wrote:
> When fib_entry is NULL, no error return code of
> mlxsw_sp_router_nve_promote_decap() is assigned.
> To fix this bug, err is assigned with -EINVAL in this case.
>
Again, are you sure this is a bug? To me it looks like it is
intentional to not return an error code if fib_entry is NULL.
Please don't blindly trust the robot results, there may
always be false positives.
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
> ---
> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> index 9ce90841f92d..7b260e25df1b 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -1981,8 +1981,10 @@ int mlxsw_sp_router_nve_promote_decap(struct mlxsw_sp
> *mlxsw_sp, u32 ul_tb_id,
> fib_entry = mlxsw_sp_router_ip2me_fib_entry_find(mlxsw_sp, ul_tb_id,
>ul_proto, ul_sip,
>type);
> - if (!fib_entry)
> + if (!fib_entry) {
> + err = -EINVAL;
> goto out;
> + }
>
> fib_entry->decap.tunnel_index = tunnel_index;
> fib_entry->type = MLXSW_SP_FIB_ENTRY_TYPE_NVE_DECAP;
>
Re: [PATCH net] r8169: fix r8168fp_adjust_ocp_cmd function
On 05.03.2021 10:34, Hayes Wang wrote:
> The (0xBAF7 & 0x00FFF000) << 6 should be (0xf70 << 18).
>
> Fixes: 561535b0f239 ("r8169: fix OCP access on RTL8117")
> Signed-off-by: Hayes Wang
> ---
> 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
Re: [PATCH] net: mellanox: mlx5: fix error return code in mlx5_fpga_device_start()
On 04.03.2021 15:18, Jia-Ju Bai wrote:
> When mlx5_is_fpga_lookaside() returns a non-zero value, no error
> return code is assigned.
> To fix this bug, err is assigned with -EINVAL as error return code.
>
To me it looks like the current behavior is intentional.
Did you verify that it's actually an error condition if the
function returns true? Please don't blindly trust such code checkers.
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
> ---
> drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
> b/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
> index 2ce4241459ce..c9e6da97126f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
> @@ -198,8 +198,10 @@ int mlx5_fpga_device_start(struct mlx5_core_dev *mdev)
> mlx5_fpga_info(fdev, "FPGA card %s:%u\n", mlx5_fpga_name(fpga_id),
> fpga_id);
>
> /* No QPs if FPGA does not participate in net processing */
> - if (mlx5_is_fpga_lookaside(fpga_id))
> + if (mlx5_is_fpga_lookaside(fpga_id)) {
> + err = -EINVAL;
> goto out;
> + }
>
> mlx5_fpga_info(fdev, "%s(%d): image, version %u; SBU %06x:%04x version
> %d\n",
> mlx5_fpga_image_name(fdev->last_oper_image),
>
Re: next-20210302 - build issue with linux-firmware and rtl_nic/ firmware.
On 03.03.2021 08:39, Valdis Klētnieks wrote: > On Wed, 03 Mar 2021 07:51:06 +0100, Heiner Kallweit said: >>> # Firmware loader >>> CONFIG_EXTRA_FIRMWARE="rtl_nic/rtl8106e-1.fw" >>> CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware" >>> >> This is wrong, simply remove it. > >>> But then I take a closer look at drivers/net/ethernet/realtek/r8169_main.c >>> #define FIRMWARE_8168D_1"rtl_nic/rtl8168d-1.fw" >>> #define FIRMWARE_8168D_2"rtl_nic/rtl8168d-2.fw" >>> #define FIRMWARE_8168E_1"rtl_nic/rtl8168e-1.fw" > > Yes, but then how are *these* filenames supposed to work? Is a userspace > helper > supposed to be smart enough to append the .xz, and the EXTRA_FIRMWARE variant > for embedding out-of-tree firmware has to point at an uncompressed version? > There is no such thing as compressed firmware files, see here: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/rtl_nic Your issue may be distro-specific, so you should check in a support forum of your respective distro.
Re: next-20210302 - build issue with linux-firmware and rtl_nic/ firmware.
On 03.03.2021 07:09, Valdis Klētnieks wrote: > So my kernel build died.. > > UPD drivers/base/firmware_loader/builtin/rtl_nic/rtl8106e-1.fw.gen.S > make[4]: *** No rule to make target '/lib/firmware/rtl_nic/rtl8106e-1.fw', > needed by 'drivers/base/firmware_loader/builtin/rtl_nic/rtl8106e-1.fw.gen.o'. > Stop. > make[3]: *** [scripts/Makefile.build:514: > drivers/base/firmware_loader/builtin] Error 2 > > I tracked it down to a linux-firmware update that shipped everything with .xz > compression: > > % rpm2cpio linux-firmware-20201218-116.fc34.noarch.rpm | cpio -itv | grep > 8106e-1 > -rw-r--r-- 1 root root 1856 Dec 19 04:43 > ./usr/lib/firmware/rtl_nic/rtl8106e-1.fw > 631034 blocks > % rpm2cpio linux-firmware-20210208-117.fc34.noarch.rpm | cpio -itv| grep > 8106e-1 > -rw-r--r-- 1 root root 848 Feb 8 16:38 > ./usr/lib/firmware/rtl_nic/rtl8106e-1.fw.xz > 340217 blocks > > and my .config shows it's self-inflicted (no, I don't remember why it's in > there): > > # Firmware loader > CONFIG_EXTRA_FIRMWARE="rtl_nic/rtl8106e-1.fw" > CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware" > This is wrong, simply remove it. > But then I take a closer look at drivers/net/ethernet/realtek/r8169_main.c > #define FIRMWARE_8168D_1 "rtl_nic/rtl8168d-1.fw" > #define FIRMWARE_8168D_2 "rtl_nic/rtl8168d-2.fw" > #define FIRMWARE_8168E_1 "rtl_nic/rtl8168e-1.fw" > > So now I'm mystified how this compressing all the firmware files is supposed > to work... >
Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
On 26.02.2021 13:18, Kai-Heng Feng wrote: > On Fri, Feb 26, 2021 at 8:10 PM Heiner Kallweit wrote: >> >> On 26.02.2021 08:12, Kalle Valo wrote: >>> Kai-Heng Feng writes: >>> >>>> Now we have a generic D3 shutdown quirk, so convert the original >>>> approach to a PCI quirk. >>>> >>>> Signed-off-by: Kai-Heng Feng >>>> --- >>>> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- >>>> drivers/pci/quirks.c | 6 ++ >>>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> It would have been nice to CC linux-wireless also on patches 1-2. I only >>> saw patch 3 and had to search the rest of patches from lkml. >>> >>> I assume this goes via the PCI tree so: >>> >>> Acked-by: Kalle Valo >>> >> >> To me it looks odd to (mis-)use the quirk mechanism to set a device >> to D3cold on shutdown. As I see it the quirk mechanism is used to work >> around certain device misbehavior. And setting a device to a D3 >> state on shutdown is a normal activity, and the shutdown() callback >> seems to be a good place for it. >> I miss an explanation what the actual benefit of the change is. > > To make putting device to D3 more generic, as there are more than one > device need the quirk. > > Here's the discussion: > https://lore.kernel.org/linux-usb/00de6927-3fa6-a9a3-2d65-2b4d4e8f0...@linux.intel.com/ > Thanks for the link. For the AMD USB use case I don't have a strong opinion, what's considered the better option may be a question of personal taste. For rtw88 however I'd still consider it over-engineering to replace a simple call to pci_set_power_state() with a PCI quirk. I may be biased here because I find it sometimes bothering if I want to look up how a device is handled and in addition to checking the respective driver I also have to grep through quirks.c whether there's any special handling. > Kai-Heng > Heiner
Re: [PATCH 3/3] PCI: Convert rtw88 power cycle quirk to shutdown quirk
On 26.02.2021 08:12, Kalle Valo wrote: > Kai-Heng Feng writes: > >> Now we have a generic D3 shutdown quirk, so convert the original >> approach to a PCI quirk. >> >> Signed-off-by: Kai-Heng Feng >> --- >> drivers/net/wireless/realtek/rtw88/pci.c | 2 -- >> drivers/pci/quirks.c | 6 ++ >> 2 files changed, 6 insertions(+), 2 deletions(-) > > It would have been nice to CC linux-wireless also on patches 1-2. I only > saw patch 3 and had to search the rest of patches from lkml. > > I assume this goes via the PCI tree so: > > Acked-by: Kalle Valo > To me it looks odd to (mis-)use the quirk mechanism to set a device to D3cold on shutdown. As I see it the quirk mechanism is used to work around certain device misbehavior. And setting a device to a D3 state on shutdown is a normal activity, and the shutdown() callback seems to be a good place for it. I miss an explanation what the actual benefit of the change is.
Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
On 20.02.2021 10:02, Serge Semin wrote:
> On Thu, Feb 11, 2021 at 10:39:41AM +, Russell King - ARM Linux admin
> wrote:
>> On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
>>> On Tue, Feb 09, 2021 at 10:56:46AM +, Russell King - ARM Linux admin
>>> wrote:
>>>> On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
>>>>> Right, adding something like a genphy_{read,write}_mmd() doesn't make
>>>>> too much sense for now. What I meant is just exporting mmd_phy_indirect().
>>>>> Then you don't have to open-code the first three steps of a mmd
>>>>> read/write.
>>>>> And it requires no additional code in phylib.
>>>>
>>>> ... but at the cost that the compiler can no longer inline that code,
>>>> as I mentioned in my previous reply. (However, the cost of the accesses
>>>> will be higher.) On the plus side, less I-cache footprint, and smaller
>>>> kernel code.
>>>
>>> Just to note mmd_phy_indirect() isn't defined with inline specifier,
>>> but just as static and it's used twice in the
>>> drivers/net/phy/phy-core.c unit. So most likely the compiler won't
>>> inline the function code in there.
>>
>> You can't always tell whether the compiler will inline a static function
>> or not.
>
> Andrew, Heiner, Russell, what is your final decision about this? Shall
> we export the mmd_phy_indirect() method, implement new
> genphy_{read,write}_mmd() or just leave the patch as is manually
> accessing the MMD register in the driver?
>
If in doubt, leaving the patch as is would be fine with me.
> -Sergey
>
>>
>>> Anyway it's up to the PHY
>>> library maintainers to decide. Please settle the issue with Heiner and
>>> Andrew then. I am ok with both solutions and will do as you decide.
>>
>> FYI, *I* am one of the phylib maintainers.
>>
>> --
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net-next v2 3/3] net: phy: mscc: coma mode disabled for VSC8514
On 15.02.2021 17:58, Bjarni Jonasson wrote:
> The 'coma mode' (configurable through sw or hw) provides an
> optional feature that may be used to control when the PHYs become active.
> The typical usage is to synchronize the link-up time across
> all PHY instances. This patch releases coma mode if not done by hardware,
> otherwise the phys will not link-up
>
> Signed-off-by: Steen Hegelund
> Signed-off-by: Bjarni Jonasson
> Fixes: e4f9ba642f0b ("net: phy: mscc: add support for VSC8514 PHY.")
> ---
> v1 -> v2:
> Modified coma mode config
> Changed net to net-next
>
> drivers/net/phy/mscc/mscc.h | 3 +++
> drivers/net/phy/mscc/mscc_main.c | 16
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index 9d8ee387739e..2b70ccd1b256 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -160,6 +160,9 @@ enum rgmii_clock_delay {
> #define MSCC_PHY_PAGE_TR 0x52b5 /* Token ring registers */
> #define MSCC_PHY_GPIO_CONTROL_214
>
> +#define MSCC_PHY_COMA_MODE 0x2000 /* input(1) / output(0) */
> +#define MSCC_PHY_COMA_OUTPUT 0x1000 /* value to output */
> +
> /* Extended Page 1 Registers */
> #define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT18
> #define VALID_CRC_CNT_CRC_MASK GENMASK(13, 0)
> diff --git a/drivers/net/phy/mscc/mscc_main.c
> b/drivers/net/phy/mscc/mscc_main.c
> index 03181542bcb7..29302ccf7e7b 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -1520,6 +1520,21 @@ static void vsc8584_get_base_addr(struct phy_device
> *phydev)
> vsc8531->addr = addr;
> }
>
> +static void vsc85xx_coma_mode_release(struct phy_device *phydev)
> +{
> + /* The coma mode (pin or reg) provides an optional feature that
> + * may be used to control when the PHYs become active.
> + * Alternatively the COMA_MODE pin may be connected low
> + * so that the PHYs are fully active once out of reset.
> + */
> + phy_unlock_mdio_bus(phydev);
> + /* Enable output (mode=0) and write zero to it */
> + phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> + MSCC_PHY_GPIO_CONTROL_2,
> + MSCC_PHY_COMA_MODE | MSCC_PHY_COMA_OUTPUT, 0);
> + phy_lock_mdio_bus(phydev);
The temporary unlock is a little bit hacky. Better do:
vsc85xx_phy_write_page(MSCC_PHY_PAGE_EXTENDED_GPIO)
__phy_modify()
vsc85xx_phy_write_page(default page)
Alternatively we could add __phy_modify_paged(). But this may not
be worth the effort for now.
> +}
> +
> static int vsc8584_config_init(struct phy_device *phydev)
> {
> struct vsc8531_private *vsc8531 = phydev->priv;
> @@ -2604,6 +2619,7 @@ static int vsc8514_config_init(struct phy_device
> *phydev)
> ret = vsc8514_config_host_serdes(phydev);
> if (ret)
> goto err;
> + vsc85xx_coma_mode_release(phydev);
> }
>
> phy_unlock_mdio_bus(phydev);
>
Re: [PATCH net-next v2 2/3] net: phy: mscc: improved serdes calibration applied to VSC8514
On 15.02.2021 17:57, Bjarni Jonasson wrote:
> The current IB serdes calibration algorithm (performed by the onboard 8051)
> has proven to be unstable for the VSC8514 QSGMII phy.
> A new algorithm has been developed based on
> 'Frequency-offset Jittered-Injection' or 'FoJi' method which solves
> all known issues. This patch disables the 8051 algorithm and
> replaces it with the new FoJi algorithm.
> The calibration is now performed in the drive.
>
> Signed-off-by: Steen Hegelund
> Signed-off-by: Bjarni Jonasson
> Tested-by: Vladimir Oltean
> Fixes: e4f9ba642f0b ("net: phy: mscc: add support for VSC8514 PHY.")
> ---
> v1 -> v2:
> Preserved reversed christmas tree
> Changed net to net-next
>
> drivers/net/phy/mscc/mscc.h | 23 +
> drivers/net/phy/mscc/mscc_main.c | 930 +--
> 2 files changed, 795 insertions(+), 158 deletions(-)
This is a massive workaround. Would it make sense to place it in a
separate source code file instead of polluting mscc_main.c?
This may improve maintainability.
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index c2023f93c0b2..9d8ee387739e 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -113,6 +113,9 @@ enum rgmii_clock_delay {
> #define PHY_S6G_PLL_STATUS 0x31
> #define PHY_S6G_IB_STATUS0 0x2f
>
> +#define PHY_S6G_PLL5G_CFG2_GAIN_MASK GENMASK(9, 5)
> +#define PHY_S6G_PLL5G_CFG2_ENA_GAIN 1
> +
> #define PHY_S6G_SYS_RST_POS31
> #define PHY_S6G_ENA_LANE_POS 18
> #define PHY_S6G_ENA_LOOP_POS 8
> @@ -125,6 +128,21 @@ enum rgmii_clock_delay {
> #define PHY_S6G_CFG2_FSM_DIS 1
> #define PHY_S6G_CFG2_FSM_CLK_BP 23
>
> +#define PHY_S6G_DES_PHY_CTRL_POS 13
> +#define PHY_S6G_DES_MBTR_CTRL_POS 10
> +#define PHY_S6G_DES_CPMD_SEL_POS 8
> +#define PHY_S6G_DES_BW_HYST_POS5
> +#define PHY_S6G_DES_BW_ANA_POS 1
> +#define PHY_S6G_DES_CFG0x21
> +#define PHY_S6G_IB_CFG00x22
> +#define PHY_S6G_IB_CFG10x23
> +#define PHY_S6G_IB_CFG20x24
> +#define PHY_S6G_IB_CFG30x25
> +#define PHY_S6G_IB_CFG40x26
> +#define PHY_S6G_GP_CFG 0x2E
> +#define PHY_S6G_DFT_CFG0 0x35
> +#define PHY_S6G_IB_DFT_CFG20x37
> +
> #define MSCC_EXT_PAGE_ACCESS 31
> #define MSCC_PHY_PAGE_STANDARD 0x /* Standard registers
> */
> #define MSCC_PHY_PAGE_EXTENDED 0x0001 /* Extended registers
> */
> @@ -140,6 +158,7 @@ enum rgmii_clock_delay {
> #define MSCC_PHY_PAGE_1588 0x1588 /* PTP (1588) */
> #define MSCC_PHY_PAGE_TEST 0x2a30 /* Test reg */
> #define MSCC_PHY_PAGE_TR 0x52b5 /* Token ring registers */
> +#define MSCC_PHY_GPIO_CONTROL_214
>
> /* Extended Page 1 Registers */
> #define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT18
> @@ -339,6 +358,10 @@ enum rgmii_clock_delay {
> #define VSC8584_REVB 0x0001
> #define MSCC_DEV_REV_MASKGENMASK(3, 0)
>
> +#define MSCC_ROM_TRAP_SERDES_6G_CFG 0x1E48
> +#define MSCC_RAM_TRAP_SERDES_6G_CFG 0x1E4F
> +#define PATCH_VEC_ZERO_EN0x0100
> +
> struct reg_val {
> u16 reg;
> u32 val;
> diff --git a/drivers/net/phy/mscc/mscc_main.c
> b/drivers/net/phy/mscc/mscc_main.c
> index 09650c3340a1..03181542bcb7 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -1738,6 +1738,738 @@ static int vsc85xx_config_init(struct phy_device
> *phydev)
> return 0;
> }
>
> +static int __phy_write_mcb_s6g(struct phy_device *phydev, u32 reg, u8 mcb,
> +u32 op)
> +{
> + unsigned long deadline;
> + u32 val;
> + int ret;
> +
> + ret = vsc85xx_csr_write(phydev, PHY_MCB_TARGET, reg,
> + op | (1 << mcb));
> + if (ret)
> + return -EINVAL;
> +
> + deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
> + do {
> + usleep_range(500, 1000);
> + val = vsc85xx_csr_read(phydev, PHY_MCB_TARGET, reg);
> +
> + if (val == 0x)
> + return -EIO;
> +
> + } while (time_before(jiffies, deadline) && (val & op));
> +
> + if (val & op)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +/* Trigger a read to the specified MCB */
> +static int phy_update_mcb_s6g(struct phy_device *phydev, u32 reg, u8 mcb)
> +{
> + return __phy_write_mcb_s6g(phydev, reg, mcb, PHY_MCB_S6G_READ);
> +}
> +
> +/* Trigger a write to the specified MCB */
> +static int phy_commit_mcb_s6g(struct phy_device *phydev, u32 reg, u8 mcb)
> +{
> + return __phy_write_mcb_s6g(phydev,
Re: [PATCH net-next v2 1/3] net: phy: mscc: adding LCPLL reset to VSC8514
On 15.02.2021 17:57, Bjarni Jonasson wrote:
> At Power-On Reset, transients may cause the LCPLL to lock onto a
> clock that is momentarily unstable. This is normally seen in QSGMII
> setups where the higher speed 6G SerDes is being used.
> This patch adds an initial LCPLL Reset to the PHY (first instance)
> to avoid this issue.
>
> Signed-off-by: Steen Hegelund
> Signed-off-by: Bjarni Jonasson
> Fixes: e4f9ba642f0b ("net: phy: mscc: add support for VSC8514 PHY.")
Cover letter is missing, and Fixes should be first.
> ---
> v1 -> v2:
> Preserved reversed christmas tree
> Removed forward definitions
> Fixed build issues
> Changed net to net-next
>
> drivers/net/phy/mscc/mscc.h | 8 +
> drivers/net/phy/mscc/mscc_main.c | 354 ---
> 2 files changed, 240 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index 9481bce94c2e..c2023f93c0b2 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -102,6 +102,7 @@ enum rgmii_clock_delay {
> #define PHY_MCB_S6G_READ BIT(30)
>
> #define PHY_S6G_PLL5G_CFG0 0x06
> +#define PHY_S6G_PLL5G_CFG2 0x08
> #define PHY_S6G_LCPLL_CFG 0x11
> #define PHY_S6G_PLL_CFG0x2b
> #define PHY_S6G_COMMON_CFG 0x2c
> @@ -121,6 +122,9 @@ enum rgmii_clock_delay {
> #define PHY_S6G_PLL_FSM_CTRL_DATA_POS 8
> #define PHY_S6G_PLL_FSM_ENA_POS7
>
> +#define PHY_S6G_CFG2_FSM_DIS 1
> +#define PHY_S6G_CFG2_FSM_CLK_BP 23
> +
> #define MSCC_EXT_PAGE_ACCESS 31
> #define MSCC_PHY_PAGE_STANDARD 0x /* Standard registers
> */
> #define MSCC_PHY_PAGE_EXTENDED 0x0001 /* Extended registers
> */
> @@ -412,6 +416,10 @@ struct vsc8531_edge_rate_table {
> };
> #endif /* CONFIG_OF_MDIO */
>
> +enum csr_target {
> + MACRO_CTRL = 0x07,
> +};
> +
> #if IS_ENABLED(CONFIG_MACSEC)
> int vsc8584_macsec_init(struct phy_device *phydev);
> void vsc8584_handle_macsec_interrupt(struct phy_device *phydev);
> diff --git a/drivers/net/phy/mscc/mscc_main.c
> b/drivers/net/phy/mscc/mscc_main.c
> index 2f2157e3deab..09650c3340a1 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -710,6 +710,113 @@ static int phy_base_read(struct phy_device *phydev, u32
> regnum)
> return __phy_package_read(phydev, regnum);
> }
>
> +static u32 vsc85xx_csr_read(struct phy_device *phydev,
> + enum csr_target target, u32 reg)
> +{
> + unsigned long deadline;
> + u32 val, val_l, val_h;
> +
> + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_CSR_CNTL);
> +
> + /* CSR registers are grouped under different Target IDs.
> + * 6-bit Target_ID is split between MSCC_EXT_PAGE_CSR_CNTL_20 and
> + * MSCC_EXT_PAGE_CSR_CNTL_19 registers.
> + * Target_ID[5:2] maps to bits[3:0] of MSCC_EXT_PAGE_CSR_CNTL_20
> + * and Target_ID[1:0] maps to bits[13:12] of MSCC_EXT_PAGE_CSR_CNTL_19.
> + */
> +
> + /* Setup the Target ID */
> + phy_base_write(phydev, MSCC_EXT_PAGE_CSR_CNTL_20,
> +MSCC_PHY_CSR_CNTL_20_TARGET(target >> 2));
> +
> + if ((target >> 2 == 0x1) || (target >> 2 == 0x3))
> + /* non-MACsec access */
> + target &= 0x3;
> + else
> + target = 0;
> +
> + /* Trigger CSR Action - Read into the CSR's */
> + phy_base_write(phydev, MSCC_EXT_PAGE_CSR_CNTL_19,
> +MSCC_PHY_CSR_CNTL_19_CMD | MSCC_PHY_CSR_CNTL_19_READ |
> +MSCC_PHY_CSR_CNTL_19_REG_ADDR(reg) |
> +MSCC_PHY_CSR_CNTL_19_TARGET(target));
> +
> + /* Wait for register access*/
> + deadline = jiffies + msecs_to_jiffies(PROC_CMD_NCOMPLETED_TIMEOUT_MS);
> + do {
> + usleep_range(500, 1000);
> + val = phy_base_read(phydev, MSCC_EXT_PAGE_CSR_CNTL_19);
> + } while (time_before(jiffies, deadline) &&
> + !(val & MSCC_PHY_CSR_CNTL_19_CMD));
> +
> + if (!(val & MSCC_PHY_CSR_CNTL_19_CMD))
> + return 0x;
> +
> + /* Read the Least Significant Word (LSW) (17) */
> + val_l = phy_base_read(phydev, MSCC_EXT_PAGE_CSR_CNTL_17);
> +
> + /* Read the Most Significant Word (MSW) (18) */
> + val_h = phy_base_read(phydev, MSCC_EXT_PAGE_CSR_CNTL_18);
> +
> + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
> +MSCC_PHY_PAGE_STANDARD);
> +
> + return (val_h << 16) | val_l;
> +}
> +
> +static int vsc85xx_csr_write(struct phy_device *phydev,
> + enum csr_target target, u32 reg, u32 val)
> +{
> + unsigned long deadline;
> +
> + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_CSR_CNTL);
> +
> + /* CSR registers are grouped under different Target IDs.
> + * 6-bit Target_ID is split
Re: [PATCH v2 net-next 2/5] net: ipa: don't report EPROBE_DEFER error
On 11.02.2021 22:19, Alex Elder wrote:
> When initializing the IPA core clock and interconnects, it's
> possible we'll get an EPROBE_DEFER error. This isn't really an
> error, it's just means we need to be re-probed later.
>
> Check the return code when initializing these, and if it's
> EPROBE_DEFER, skip printing the error message.
>
> Signed-off-by: Alex Elder
> ---
> drivers/net/ipa/ipa_clock.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
> index 354675a643db5..238a713f6b604 100644
> --- a/drivers/net/ipa/ipa_clock.c
> +++ b/drivers/net/ipa/ipa_clock.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> - * Copyright (C) 2018-2020 Linaro Ltd.
> + * Copyright (C) 2018-2021 Linaro Ltd.
> */
>
> #include
> @@ -68,8 +68,9 @@ static int ipa_interconnect_init_one(struct device *dev,
> if (IS_ERR(path)) {
> int ret = PTR_ERR(path);
>
> - dev_err(dev, "error %d getting %s interconnect\n", ret,
> - data->name);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "error %d getting %s interconnect\n", ret,
> + data->name);
>
You may want to use dev_err_probe() here.
> return ret;
> }
> @@ -281,7 +282,10 @@ ipa_clock_init(struct device *dev, const struct
> ipa_clock_data *data)
>
> clk = clk_get(dev, "core");
> if (IS_ERR(clk)) {
> - dev_err(dev, "error %ld getting core clock\n", PTR_ERR(clk));
> + ret = PTR_ERR(clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "error %d getting core clock\n", ret);
> +
> return ERR_CAST(clk);
> }
>
>
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 11.02.2021 13:17, Pavel Machek wrote: > On Thu 2021-01-14 12:05:21, Heiner Kallweit wrote: >> On 14.01.2021 11:41, claudiu.bez...@microchip.com wrote: >>> >>> >>> On 14.01.2021 12:25, Russell King - ARM Linux admin wrote: >>>> >>>> As I've said, if phylib/PHY driver is not restoring the state of the >>>> PHY on resume from suspend-to-ram, then that's an issue with phylib >>>> and/or the phy driver. >>> >>> In the patch I proposed in this thread the restoring is done in PHY driver. >>> Do you think I should continue the investigation and check if something >>> should be done from the phylib itself? >>> >> It was the right move to approach the PM maintainers to clarify whether >> the resume PM callback has to assume that power had been cut off and >> it has to completely reconfigure the device. If they confirm this >> understanding, then: > > Power to some devices can be cut during s2ram, yes. > Thanks for the confirmation. >> - the general question remains why there's separate resume and restore >> callbacks, and what restore is supposed to do that resume doesn't >> have to do > > You'll often have same implementation, yes. > If resume and restore both have to assume that power was cut off, then they have to fully re-initialize the device. Therefore it's still not clear to me when you would have differing implementations for both callbacks. >> - it should be sufficient to use mdio_bus_phy_restore also as resume >> callback (instead of changing each and every PHY driver's resume), >> because we can expect that somebody cutting off power to the PHY >> properly suspends the MDIO bus before > > If restore works with power cut and power not cut then yes, you should > get away with that. > > Best regards, > Pavel > Heiner
Re: phy_attach_direct()'s use of device_bind_driver()
On 11.02.2021 09:57, Saravana Kannan wrote: > On Wed, Feb 10, 2021 at 11:31 PM Heiner Kallweit wrote: >> >> On 11.02.2021 00:29, Saravana Kannan wrote: >>> On Wed, Feb 10, 2021 at 2:52 PM Andrew Lunn wrote: >>>> >>>> On Wed, Feb 10, 2021 at 02:13:48PM -0800, Saravana Kannan wrote: >>>>> Hi, >>>>> >>>>> This email was triggered by this other email[1]. >>>>> >>>>> Why is phy_attach_direct() directly calling device_bind_driver() >>>>> instead of using bus_probe_device()? >>>> >>>> Hi Saravana >>>> >>>> So this is to do with the generic PHY, which is a special case. >>>> >>>> First the normal case. The MDIO bus driver registers an MDIO bus using >>>> mdiobus_register(). This will enumerate the bus, finding PHYs on >>>> it. Each PHY device is registered with the device core, using the >>>> usual device_add(). The core will go through the registered PHY >>>> drivers and see if one can drive this hardware, based on the ID >>>> registers the PHY has at address 2 and 3. If a match is found, the >>>> driver probes the device, all in the usual way. >>>> >>>> Sometime later, the MAC driver wants to make use of the PHY >>>> device. This is often in the open() call of the MAC driver, when the >>>> interface is configured up. The MAC driver asks phylib to associate a >>>> PHY devices to the MAC device. In the normal case, the PHY has been >>>> probed, and everything is good to go. >>>> >>>> However, sometimes, there is no driver for the PHY. There is no driver >>>> for that hardware. Or the driver has not been built, or it is not on >>>> the disk, etc. So the device core has not been able to probe >>>> it. However, IEEE 802.3 clause 22 defines a minimum set of registers a >>>> PHY should support. And most PHY devices have this minimum. So there >>>> is a fall back driver, the generic PHY driver. It assumes the minimum >>>> registers are available, and does its best to drive the hardware. It >>>> often works, but not always. So if the MAC asks phylib to connect to a >>>> PHY which does not have a driver, we forcefully bind the generic >>>> driver to the device, and hope for the best. >>> >>> Thanks for the detailed answer Andrew! I think it gives me enough >>> info/context to come up with a proper fix. >>> >>>> We don't actually recommend using the generic driver. Use the specific >>>> driver for the hardware. But the generic driver can at least get you >>>> going, allow you to scp the correct driver onto the system, etc. >>> >>> I'm not sure if I can control what driver they use. If I can fix this >>> warning, I'll probably try to do that. >>> >> The genphy driver is a last resort, at least they lose functionality like >> downshift detection and control. Therefore they should go with the >> dedicated Marvell PHY driver. >> >> But right, this avoids the warning, but the underlying issue (probably >> in device_bind_driver()) still exists. Would be good if you can fix it. > > Yeah, I plan to fix this. So I have a few more questions. In the > example I gave, what should happen if the gpios listed in the phy's DT > node aren't ready yet? The generic phy driver itself probably isn't > using any GPIO? But will the phy work without the GPIO hardware being > initialized? The reason I'm asking this question is, if the phy is > linked to a supplier and the supplier is not ready, should the > device_bind_driver() succeed or not? > There may be situations where the gpio is used for the PHY reset and default state is "reset assigned". If we can't control the reset signal then PHY isn't usable. Therefore I'm inclined to say we should not succeed. Let's see which opinions Andrew and Russell have. However I have a little bit of a hard time to imagine how this scenario can happen. device_bind_driver(), as part of phy_attach_direct(), is typically called from ndo_open() of the net device, like in your stmmac case. Means user space would open the network interface before the gpio controller has even been probed. > -Saravana >
Re: phy_attach_direct()'s use of device_bind_driver()
On 11.02.2021 00:29, Saravana Kannan wrote: > On Wed, Feb 10, 2021 at 2:52 PM Andrew Lunn wrote: >> >> On Wed, Feb 10, 2021 at 02:13:48PM -0800, Saravana Kannan wrote: >>> Hi, >>> >>> This email was triggered by this other email[1]. >>> >>> Why is phy_attach_direct() directly calling device_bind_driver() >>> instead of using bus_probe_device()? >> >> Hi Saravana >> >> So this is to do with the generic PHY, which is a special case. >> >> First the normal case. The MDIO bus driver registers an MDIO bus using >> mdiobus_register(). This will enumerate the bus, finding PHYs on >> it. Each PHY device is registered with the device core, using the >> usual device_add(). The core will go through the registered PHY >> drivers and see if one can drive this hardware, based on the ID >> registers the PHY has at address 2 and 3. If a match is found, the >> driver probes the device, all in the usual way. >> >> Sometime later, the MAC driver wants to make use of the PHY >> device. This is often in the open() call of the MAC driver, when the >> interface is configured up. The MAC driver asks phylib to associate a >> PHY devices to the MAC device. In the normal case, the PHY has been >> probed, and everything is good to go. >> >> However, sometimes, there is no driver for the PHY. There is no driver >> for that hardware. Or the driver has not been built, or it is not on >> the disk, etc. So the device core has not been able to probe >> it. However, IEEE 802.3 clause 22 defines a minimum set of registers a >> PHY should support. And most PHY devices have this minimum. So there >> is a fall back driver, the generic PHY driver. It assumes the minimum >> registers are available, and does its best to drive the hardware. It >> often works, but not always. So if the MAC asks phylib to connect to a >> PHY which does not have a driver, we forcefully bind the generic >> driver to the device, and hope for the best. > > Thanks for the detailed answer Andrew! I think it gives me enough > info/context to come up with a proper fix. > >> We don't actually recommend using the generic driver. Use the specific >> driver for the hardware. But the generic driver can at least get you >> going, allow you to scp the correct driver onto the system, etc. > > I'm not sure if I can control what driver they use. If I can fix this > warning, I'll probably try to do that. > The genphy driver is a last resort, at least they lose functionality like downshift detection and control. Therefore they should go with the dedicated Marvell PHY driver. But right, this avoids the warning, but the underlying issue (probably in device_bind_driver()) still exists. Would be good if you can fix it. > -Saravana > Heiner
Re: phy_attach_direct()'s use of device_bind_driver()
On 10.02.2021 23:13, Saravana Kannan wrote: > Hi, > > This email was triggered by this other email[1]. > > Why is phy_attach_direct() directly calling device_bind_driver() > instead of using bus_probe_device()? I'm asking because this is > causing device links status to not get updated correctly and causes > this[2] warning. > The genphy driver is a fallback if no dedicated PHY driver matches the PHY device. It doesn't match any device, therefore it needs to be explicitly bound. > We can fix the device links issue with something like this[3], but > want to understand the reason for the current implementation of > phy_attach_direct() before we go ahead and put in that fix. > > Thanks, > Saravana > > [1] - > https://lore.kernel.org/lkml/e11bc6a2-ec9d-ea3b-71f7-13c9f764b...@nvidia.com/#t > [2] - > https://lore.kernel.org/lkml/56f7d032-ba5a-a8c7-23de-2969d98c5...@nvidia.com/ > [3] - > https://lore.kernel.org/lkml/6a43e209-1d2d-b10a-4564-0289d5413...@nvidia.com/ >
Re: [PATCH net-next v2 5/9] net: phy: icplus: split IP101A/G driver
On 10.02.2021 17:47, Michael Walle wrote:
> Unfortunately, the IP101A and IP101G share the same PHY identifier.
> While most of the functions are somewhat backwards compatible, there is
> for example the APS_EN bit on the IP101A but on the IP101G this bit
> reserved. Also, the IP101G has many more functionalities.
>
> Deduce the model by accessing the page select register which - according
> to the datasheet - is not available on the IP101A. If this register is
> writable, assume we have an IP101G.
>
> Split the combined IP101A/G driver into two separate drivers.
>
> Signed-off-by: Michael Walle
> ---
> Changes since v1:
> - use match_phy_device() as suggested by Heiner
>
> Andrew, I've dropped your Reviewed-by because of this.
>
> drivers/net/phy/icplus.c | 69 ++--
> 1 file changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index 036bac628b11..1bc9baa9048f 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL");
> #define IP101A_G_IRQ_DUPLEX_CHANGE BIT(1)
> #define IP101A_G_IRQ_LINK_CHANGE BIT(0)
>
> +#define IP101G_PAGE_CONTROL 0x14
> +#define IP101G_PAGE_CONTROL_MASK GENMASK(4, 0)
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> @@ -301,6 +303,58 @@ static irqreturn_t ip101a_g_handle_interrupt(struct
> phy_device *phydev)
> return IRQ_HANDLED;
> }
>
> +static int ip101a_g_has_page_register(struct phy_device *phydev)
> +{
> + int oldval, val, ret;
> +
> + oldval = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (oldval < 0)
> + return oldval;
> +
> + ret = phy_write(phydev, IP101G_PAGE_CONTROL, 0x);
> + if (ret)
> + return ret;
> +
> + val = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (val < 0)
> + return val;
> +
> + ret = phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
> + if (ret)
> + return ret;
> +
> + return val == IP101G_PAGE_CONTROL_MASK;
> +}
> +
> +static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
> +{
> + int ret;
> +
> + if (phydev->phy_id != IP101A_PHY_ID)
> + return 0;
> +
> + /* The IP101A and the IP101G share the same PHY identifier.The IP101G
> + * seems to be a successor of the IP101A and implements more functions.
> + * Amongst other things there is a page select register, which is not
> + * available on the IP101A. Use this to distinguish these two.
> + */
> + ret = ip101a_g_has_page_register(phydev);
> + if (ret < 0)
> + return ret;
> +
> + return (ip101a) ? !ret : ret;
Simpler would be: return ip101a == !ret;
> +}
> +
> +static int ip101a_match_phy_device(struct phy_device *phydev)
> +{
> + return ip101a_g_match_phy_device(phydev, true);
> +}
> +
> +static int ip101g_match_phy_device(struct phy_device *phydev)
> +{
> + return ip101a_g_match_phy_device(phydev, false);
> +}
> +
> static struct phy_driver icplus_driver[] = {
> {
> PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
> @@ -320,8 +374,19 @@ static struct phy_driver icplus_driver[] = {
> .suspend= genphy_suspend,
> .resume = genphy_resume,
> }, {
> - PHY_ID_MATCH_EXACT(IP101A_PHY_ID),
> - .name = "ICPlus IP101A/G",
> + .name = "ICPlus IP101A",
> + .match_phy_device = ip101a_match_phy_device,
> + /* PHY_BASIC_FEATURES */
These comments have once been automatically added as part of
a change in phy_driver structure. For new drivers they don't
make too much sense.
> + .probe = ip101a_g_probe,
> + .config_intr= ip101a_g_config_intr,
> + .handle_interrupt = ip101a_g_handle_interrupt,
> + .config_init= ip101a_g_config_init,
> + .soft_reset = genphy_soft_reset,
> + .suspend= genphy_suspend,
> + .resume = genphy_resume,
> +}, {
> + .name = "ICPlus IP101G",
> + .match_phy_device = ip101g_match_phy_device,
> /* PHY_BASIC_FEATURES */
> .probe = ip101a_g_probe,
> .config_intr= ip101a_g_config_intr,
>
Re: [PATCH v2 net-next 3/4] net: phy: Add Qualcomm QCA807x driver
On 10.02.2021 13:55, Robert Marko wrote: > This adds driver for the Qualcomm QCA8072 and QCA8075 PHY-s. > > They are 2 or 5 port IEEE 802.3 clause 22 compliant > 10BASE-Te, 100BASE-TX and 1000BASE-T PHY-s. > > They feature 2 SerDes, one for PSGMII or QSGMII connection with MAC, > while second one is SGMII for connection to MAC or fiber. > > Both models have a combo port that supports 1000BASE-X and 100BASE-FX > fiber. > > Each PHY inside of QCA807x series has 2 digitally controlled output only > pins that natively drive LED-s. > But some vendors used these to driver generic LED-s controlled by > user space, so lets enable registering each PHY as GPIO controller and > add driver for it. > > This also adds the ability to specify DT properties so that 1000 Base-T > LED will also be lit up for 100 and 10 Base connections. > > This is usually done by U-boot, but boards running mainline U-boot are > not configuring this yet. > > These PHY-s are commonly used in Qualcomm IPQ40xx, IPQ60xx and IPQ807x > boards. > > Signed-off-by: Robert Marko > Cc: Luka Perkov > --- > Changes in v2: > * Drop LED related code > * Fix ordering in KConfig and Makefile > * Add SFP module validation upon instert > * Rework IRQ code > * Convert values for PSGMII/QSGMII SerDes driver > into register values instead of using defines in dt-bindings > * Fill phydev->port with correct port type > > drivers/net/phy/Kconfig | 10 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/qca807x.c | 855 ++ > 3 files changed, 866 insertions(+) > create mode 100644 drivers/net/phy/qca807x.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 92164e7b7f60..f0725d8acd4a 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -259,6 +259,16 @@ config AT803X_PHY > help > Currently supports the AR8030, AR8031, AR8033 and AR8035 model > > +config QCA807X_PHY > + tristate "Qualcomm QCA807X PHYs" > + depends on OF_MDIO > + help > + Adds support for the Qualcomm QCA807x PHYs. > + These are 802.3 Clause 22 compliant PHYs supporting gigabit > + ethernet as well as 100Base-FX and 1000Base-X fibre. > + > + Currently supports the QCA8072 and QCA8075 models. > + > config QSEMI_PHY > tristate "Quality Semiconductor PHYs" > help > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index ca0a313423b9..25530057950e 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -73,6 +73,7 @@ obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o > obj-$(CONFIG_MICROSEMI_PHY) += mscc/ > obj-$(CONFIG_NATIONAL_PHY) += national.o > obj-$(CONFIG_NXP_TJA11XX_PHY)+= nxp-tja11xx.o > +obj-$(CONFIG_QCA807X_PHY)+= qca807x.o > obj-$(CONFIG_QSEMI_PHY) += qsemi.o > obj-$(CONFIG_REALTEK_PHY)+= realtek.o > obj-$(CONFIG_RENESAS_PHY)+= uPD60620.o > diff --git a/drivers/net/phy/qca807x.c b/drivers/net/phy/qca807x.c > new file mode 100644 > index ..9126035853e4 > --- /dev/null > +++ b/drivers/net/phy/qca807x.c > @@ -0,0 +1,855 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020 Sartura Ltd. > + * > + * Author: Robert Marko > + * > + * Qualcomm QCA8072 and QCA8075 PHY driver > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define PHY_ID_QCA8072 0x004dd0b2 > +#define PHY_ID_QCA8075 0x004dd0b1 > +#define PHY_ID_QCA807X_PSGMII0x06820805 > + > +/* Downshift */ > +#define QCA807X_SMARTSPEED_ENBIT(5) > +#define QCA807X_SMARTSPEED_RETRY_LIMIT_MASK GENMASK(4, 2) > +#define QCA807X_SMARTSPEED_RETRY_LIMIT_DEFAULT 5 > +#define QCA807X_SMARTSPEED_RETRY_LIMIT_MIN 2 > +#define QCA807X_SMARTSPEED_RETRY_LIMIT_MAX 9 > + > +/* Cable diagnostic test (CDT) */ > +#define QCA807X_CDT 0x16 > +#define QCA807X_CDT_ENABLE BIT(15) > +#define QCA807X_CDT_ENABLE_INTER_PAIR_SHORT BIT(13) > +#define QCA807X_CDT_STATUS BIT(11) > +#define QCA807X_CDT_MMD3_STATUS 0x8064 > +#define QCA807X_CDT_MDI0_STATUS_MASK GENMASK(15, 12) > +#define QCA807X_CDT_MDI1_STATUS_MASK GENMASK(11, 8) > +#define QCA807X_CDT_MDI2_STATUS_MASK GENMASK(7, 4) > +#define QCA807X_CDT_MDI3_STATUS_MASK GENMASK(3, 0) > +#define QCA807X_CDT_RESULTS_INVALID 0x0 > +#define QCA807X_CDT_RESULTS_OK 0x1 > +#define QCA807X_CDT_RESULTS_OPEN 0x2 > +#define QCA807X_CDT_RESULTS_SAME_SHORT 0x3 > +#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK0x4 > +#define
Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register
On 10.02.2021 13:17, Michael Walle wrote: > Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin: >> On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote: >>> Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin: >>> The PHY doesn't support fiber and register 0..15 are always available >>> regardless of the selected page for the IP101G. >>> >>> genphy_() stuff will work, but the IP101G PHY driver specific functions, >>> like interrupt and mdix will break if someone is messing with the page >>> register from userspace. >>> >>> So Heiner's point was, that there are other PHY drivers which >>> also break when a user changes registers from userspace and no one >>> seemed to cared about that for now. >>> >>> I guess it boils down to: how hard should we try to get the driver >>> behave correctly if the user is changing registers. Or can we >>> just make the assumption that if the PHY driver sets the page >>> selection to its default, all the other callbacks will work >>> on this page. >> >> Provided the PHY driver uses the paged accessors for all paged >> registers, userspace can't break the PHY driver because we have proper >> locking in the paged accessors (I wrote them.) Userspace can access the >> paged registers too, but there will be no locking other than on each >> individual access. This can't be fixed without augmenting the kernel/ >> user API, and in any case is not a matter for the PHY driver. >> >> So, let's stop worrying about the userspace paged access problem for >> driver reviews; that's a core phylib and userspace API issue. >> >> The paged accessor API is designed to allow the driver author to access >> registers in the most efficient manner. There are two parts to it. >> >> 1) the phy_*_paged() accessors switch the page before accessing the >> register, and restore it afterwards. If you need to access a lot >> of paged registers, this can be inefficient. >> >> 2) phy_save_page()..phy_restore_page() allows wrapping of __phy_* >> accessors to access paged registers. >> >> 3) phy_select_page()..phy_restore_page() also allows wrapping of >> __phy_* accessors to access paged registers. >> >> phy_save_page() and phy_select_page() must /always/ be paired with >> a call to phy_restore_page(), since the former pair takes the bus lock >> and the latter releases it. >> >> (2) and (3) allow multiple accesses to either a single page without >> constantly saving and restoring the page, and can also be used to >> select other pages without the save/restore and locking steps. We >> could export __phy_read_page() and __phy_write_page() if required. >> >> While the bus lock is taken, userspace can't access any PHY on the bus. > > That was how the v1 of this series was written. But Heiner's review > comment was, what if we just set the default page and don't > use phy_save_page()..phy_restore_page() for the registers which are > behind the default page. And in this case, userspace _can_ break > access to the paged registers. > The comment was assuming that paging also applies to register 0..15, like it is the case for Realtek PHY's. That's not the case for your PHY, therefore the situation is slightly different. > -michael
Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register
On 10.02.2021 09:25, Michael Walle wrote:
> Hi,
>
> Am 2021-02-10 08:03, schrieb Heiner Kallweit:
>> On 09.02.2021 17:40, Michael Walle wrote:
>>> Registers >= 16 are paged. Be sure to set the page. It seems this was
>>> working for now, because the default is correct for the registers used
>>> in the driver at the moment. But this will also assume, nobody will
>>> change the page select register before linux is started. The page select
>>> register is _not_ reset with a soft reset of the PHY.
>>>
>>> Add read_page()/write_page() support for the IP101G and use it
>>> accordingly.
>>>
>>> Signed-off-by: Michael Walle
>>> ---
>>> drivers/net/phy/icplus.c | 50 +++-
>>> 1 file changed, 39 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
>>> index a6e1c7611f15..858b9326a72d 100644
>>> --- a/drivers/net/phy/icplus.c
>>> +++ b/drivers/net/phy/icplus.c
>>> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
>>> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
>>> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>>>
>>> +#define IP101G_DEFAULT_PAGE 16
>>> +
>>> #define IP175C_PHY_ID 0x02430d80
>>> #define IP1001_PHY_ID 0x02430d90
>>> #define IP101A_PHY_ID 0x02430c54
>>> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
>>> static int ip101a_g_config_init(struct phy_device *phydev)
>>> {
>>> struct ip101a_g_phy_priv *priv = phydev->priv;
>>> - int err;
>>> + int oldpage, err;
>>> +
>>> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
>>>
>>> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
>>> switch (priv->sel_intr32) {
>>> case IP101GR_SEL_INTR32_RXER:
>>> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
>>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
>>> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
>>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
>>> if (err < 0)
>>> - return err;
>>> + goto out;
>>> break;
>>>
>>> case IP101GR_SEL_INTR32_INTR:
>>> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
>>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
>>> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
>>> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
>>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
>>> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
>>> if (err < 0)
>>> - return err;
>>> + goto out;
>>> break;
>>>
>>> default:
>>> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device
>>> *phydev)
>>> * reserved as 'write-one'.
>>> */
>>> if (priv->model == IP101A) {
>>> - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
>>> IP101A_G_APS_ON);
>>> + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
>>> + IP101A_G_APS_ON);
>>> if (err)
>>> - return err;
>>> + goto out;
>>> }
>>>
>>> - return 0;
>>> +out:
>>> + return phy_restore_page(phydev, oldpage, err);
>>
>> If a random page was set before entering config_init, do we actually want
>> to restore it? Or wouldn't it be better to set the default page as part
>> of initialization?
>
> First, I want to convert this to the match_phy_device() and while at it,
> I noticed that there is this one "problem". Short summary: the IP101A isn't
> paged, the IP101G has serveral and if page 16 is selected it is more or
> less compatible with the IP101A. My problem here is now how to share the
> functions for both PHYs without duplicating all the code. Eg. the IP101A
> will phy_read/phy_write/phy_modify(), that is, all the locked versions.
> For the IP101G I'd either need the _paged() versions or the __phy ones
> which don't take the mdio_bus lock.
>
> Here is what I came up with:
> (1) provide a common function which uses the __phy ones, then the
> callback for the A version will take the mdio_bus lock and calls
&g
Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register
On 09.02.2021 17:40, Michael Walle wrote:
> Registers >= 16 are paged. Be sure to set the page. It seems this was
> working for now, because the default is correct for the registers used
> in the driver at the moment. But this will also assume, nobody will
> change the page select register before linux is started. The page select
> register is _not_ reset with a soft reset of the PHY.
>
> Add read_page()/write_page() support for the IP101G and use it
> accordingly.
>
> Signed-off-by: Michael Walle
> ---
> drivers/net/phy/icplus.c | 50 +++-
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index a6e1c7611f15..858b9326a72d 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> +#define IP101G_DEFAULT_PAGE 16
> +
> #define IP175C_PHY_ID 0x02430d80
> #define IP1001_PHY_ID 0x02430d90
> #define IP101A_PHY_ID 0x02430c54
> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
> static int ip101a_g_config_init(struct phy_device *phydev)
> {
> struct ip101a_g_phy_priv *priv = phydev->priv;
> - int err;
> + int oldpage, err;
> +
> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
>
> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
> switch (priv->sel_intr32) {
> case IP101GR_SEL_INTR32_RXER:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> case IP101GR_SEL_INTR32_INTR:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> +IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> default:
> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device
> *phydev)
>* reserved as 'write-one'.
>*/
> if (priv->model == IP101A) {
> - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
> IP101A_G_APS_ON);
> + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
> + IP101A_G_APS_ON);
> if (err)
> - return err;
> + goto out;
> }
>
> - return 0;
> +out:
> + return phy_restore_page(phydev, oldpage, err);
If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?
> }
>
> static int ip101a_g_ack_interrupt(struct phy_device *phydev)
> @@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct
> phy_device *phydev)
> return IRQ_HANDLED;
> }
>
> +static int ip101a_g_read_page(struct phy_device *phydev)
> +{
> + struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> + if (priv->model == IP101A)
> + return 0;
> +
> + return __phy_read(phydev, IP101G_PAGE_CONTROL);
> +}
> +
> +static int ip101a_g_write_page(struct phy_device *phydev, int page)
> +{
> + struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> + if (priv->model == IP101A)
> + return 0;
> +
> + return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
> +}
> +
> static struct phy_driver icplus_driver[] = {
> {
> PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
> @@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = {
> .config_intr= ip101a_g_config_intr,
> .handle_interrupt = ip101a_g_handle_interrupt,
> .config_init= ip101a_g_config_init,
> + .read_page = ip101a_g_read_page,
> + .write_page = ip101a_g_write_page,
> .soft_reset = genphy_soft_reset,
> .suspend= genphy_suspend,
> .resume = genphy_resume,
>
Re: [PATCH net-next 5/9] net: phy: icplus: add IP101A/IP101G model detection
On 09.02.2021 17:40, Michael Walle wrote:
> Unfortunately, the IP101A and IP101G share the same PHY identifier.
> While most of the functions are somewhat backwards compatible, there is
> for example the APS_EN bit on the IP101A but on the IP101G this bit
> reserved. Also, the IP101G has many more functionalities.
>
> Deduce the model by accessing the page select register which - according
> to the datasheet - is not available on the IP101A. If this register is
> writable, assume we have an IP101G.
>
> Signed-off-by: Michael Walle
> ---
> drivers/net/phy/icplus.c | 43 +++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index 036bac628b11..189a9a34ed5f 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL");
> #define IP101A_G_IRQ_DUPLEX_CHANGE BIT(1)
> #define IP101A_G_IRQ_LINK_CHANGE BIT(0)
>
> +#define IP101G_PAGE_CONTROL 0x14
> +#define IP101G_PAGE_CONTROL_MASK GENMASK(4, 0)
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> @@ -61,8 +63,14 @@ enum ip101gr_sel_intr32 {
> IP101GR_SEL_INTR32_RXER,
> };
>
> +enum ip101_model {
> + IP101A,
> + IP101G,
> +};
> +
> struct ip101a_g_phy_priv {
> enum ip101gr_sel_intr32 sel_intr32;
> + enum ip101_model model;
> };
>
> static int ip175c_config_init(struct phy_device *phydev)
> @@ -175,6 +183,39 @@ static int ip175c_config_aneg(struct phy_device *phydev)
> return 0;
> }
>
> +/* The IP101A and the IP101G share the same PHY identifier.The IP101G seems
> to
> + * be a successor of the IP101A and implements more functions. Amongst other
> + * things a page select register, which is not available on the IP101. Use
> this
> + * to distinguish these two.
> + */
> +static int ip101a_g_detect_model(struct phy_device *phydev)
> +{
> + struct ip101a_g_phy_priv *priv = phydev->priv;
> + int oldval, ret;
> +
> + oldval = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (oldval < 0)
> + return oldval;
> +
> + ret = phy_write(phydev, IP101G_PAGE_CONTROL, 0x);
> + if (ret)
> + return ret;
> +
> + ret = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (ret < 0)
> + return ret;
> +
> + if (ret == IP101G_PAGE_CONTROL_MASK)
> + priv->model = IP101G;
> + else
> + priv->model = IP101A;
> +
> + phydev_dbg(phydev, "Detected %s\n",
> +priv->model == IP101G ? "IP101G" : "IP101A");
> +
> + return phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
> +}
> +
> static int ip101a_g_probe(struct phy_device *phydev)
> {
> struct device *dev = >mdio.dev;
> @@ -203,7 +244,7 @@ static int ip101a_g_probe(struct phy_device *phydev)
>
> phydev->priv = priv;
>
> - return 0;
> + return ip101a_g_detect_model(phydev);
> }
>
> static int ip101a_g_config_init(struct phy_device *phydev)
>
You could also implement the match_phy_device callback. Then you can
have separate PHY drivers for IP101A/IP101G. Would be cleaner I think.
See the Realtek PHY driver for an example.
Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
On 09.02.2021 11:15, Serge Semin wrote:
> On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
>> On 08.02.2021 15:03, Serge Semin wrote:
>>> It has been noticed that RTL8211E PHY stops detecting and reporting events
>>> when EEE is successfully advertised and RXC stopping in LPI is enabled.
>>> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
>>> register) is set. At the same time LED2 stops blinking as if EEE mode has
>>> been disabled. Notably the network traffic still flows through the PHY
>>> with no obvious problem. Anyway if any MDIO read procedure is performed
>>> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
>>> starts blinking and PHY interrupts happens again. The problem has been
>>> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
>>> reporting its event via a dedicated IRQ signal. (Obviously the problem has
>>> been unnoticed in the polling mode, since it gets naturally fixed by the
>>> periodic MDIO read procedure from the PHY status register - BMSR.)
>>>
>>> In order to fix that problem we suggest to locally re-implement the MMD
>>> write method for RTL8211E PHY and perform a dummy read right after the
>>> PC1R register is accessed to enable the RXC stopping in LPI mode.
>>>
>>> Signed-off-by: Serge Semin
>>> ---
>>> drivers/net/phy/realtek.c | 37 +
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index 99ecd6c4c15a..cbb86c257aae 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device
>>> *phydev, int devnum, u16 regnum,
>>> return ret;
>>> }
>>>
>>> +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16
>>> regnum,
>>> + u16 val)
>>> +{
>>> + int ret;
>>> +
>>> + /* Write to the MMD registers by using the standard control/data pair.
>>> +* The only difference is that we need to perform a dummy read after
>>> +* the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
>>> +* of a partial core freeze so LED2 stops blinking in EEE mode, PHY
>>> +* stops detecting the link change and raising IRQs until any read from
>>> +* its registers performed. That happens only if and right after the PHY
>>> +* is enabled to stop RXC in LPI mode.
>>> +*/
>>> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = __phy_write(phydev, MII_MMD_DATA, regnum);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
>>> + if (ret)
>>> + return ret;
>>> +
>>
>
>> Nice analysis. Alternatively to duplicating this code piece we could
>> export mmd_phy_indirect(). But up to you.
>
> I also considered creating a generic method to access the MMD
> registers of a generic PHY, something like phy_read()/phy_write(), but
> for MMD (alas just exporting mmd_phy_indirect() would not be enough).
> But as I see it such methods need to be created only after we get to
> have at least several places with duplicating direct MMD-read/write
> patterns. Doing that just for a single place seems redundant. Anyway it's
> up to maintainers to decide whether they want to see a generic part
> of the phy_read_mmd()/phy_write_mmd() methods being detached and
> exported as something like genphy_{read,write}_mmd() methods. I can do
> that in v2 if you ask me to.
>
Right, adding something like a genphy_{read,write}_mmd() doesn't make
too much sense for now. What I meant is just exporting mmd_phy_indirect().
Then you don't have to open-code the first three steps of a mmd read/write.
And it requires no additional code in phylib.
But that's not at all a showstopper here.
> -Sergey
>
>>
>>> + ret = __phy_write(phydev, MII_MMD_DATA, val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
>>> + val & MDIO_PCS_CTRL1_CLKSTOP_EN)
>>> + ret = __phy_read(phydev, MII_MMD_DATA);
>>> +
>>> + return ret < 0 ? ret : 0;
>>> +}
>>> +
>>> static int rtl822x_get_features(struct phy_device *phydev)
>>> {
>>> int val;
>>> @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
>>> .resume = genphy_resume,
>>> .read_page = rtl821x_read_page,
>>> .write_page = rtl821x_write_page,
>>> + .write_mmd = rtl8211e_write_mmd,
>>> }, {
>>> PHY_ID_MATCH_EXACT(0x001cc916),
>>> .name = "RTL8211F Gigabit Ethernet",
>>>
>>
Re: [PATCH net-next v2] net: phy: broadcom: remove BCM5482 1000Base-BX support
On 09.02.2021 02:30, Andrew Lunn wrote: > On Tue, Feb 09, 2021 at 12:17:06AM +0100, Michael Walle wrote: >> It is nowhere used in the kernel. It also seems to be lacking the >> proper fiber advertise flags. Remove it. > > Maybe also remove the #define for PHY_BCM_FLAGS_MODE_1000BX? Maybe > there is an out of tree driver using this? By removing the #define, it > will fail at compile time, making it obvious the support has been > removed? > AFAICS this flag is still used in BCM54616S PHY driver code.
Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
On 08.02.2021 15:03, Serge Semin wrote:
> It has been noticed that RTL8211E PHY stops detecting and reporting events
> when EEE is successfully advertised and RXC stopping in LPI is enabled.
> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
> register) is set. At the same time LED2 stops blinking as if EEE mode has
> been disabled. Notably the network traffic still flows through the PHY
> with no obvious problem. Anyway if any MDIO read procedure is performed
> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
> starts blinking and PHY interrupts happens again. The problem has been
> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
> reporting its event via a dedicated IRQ signal. (Obviously the problem has
> been unnoticed in the polling mode, since it gets naturally fixed by the
> periodic MDIO read procedure from the PHY status register - BMSR.)
>
> In order to fix that problem we suggest to locally re-implement the MMD
> write method for RTL8211E PHY and perform a dummy read right after the
> PC1R register is accessed to enable the RXC stopping in LPI mode.
>
> Signed-off-by: Serge Semin
> ---
> drivers/net/phy/realtek.c | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 99ecd6c4c15a..cbb86c257aae 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev,
> int devnum, u16 regnum,
> return ret;
> }
>
> +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16
> regnum,
> + u16 val)
> +{
> + int ret;
> +
> + /* Write to the MMD registers by using the standard control/data pair.
> + * The only difference is that we need to perform a dummy read after
> + * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
> + * of a partial core freeze so LED2 stops blinking in EEE mode, PHY
> + * stops detecting the link change and raising IRQs until any read from
> + * its registers performed. That happens only if and right after the PHY
> + * is enabled to stop RXC in LPI mode.
> + */
> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
> + if (ret)
> + return ret;
> +
> + ret = __phy_write(phydev, MII_MMD_DATA, regnum);
> + if (ret)
> + return ret;
> +
> + ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
> + if (ret)
> + return ret;
> +
Nice analysis. Alternatively to duplicating this code piece we could
export mmd_phy_indirect(). But up to you.
> + ret = __phy_write(phydev, MII_MMD_DATA, val);
> + if (ret)
> + return ret;
> +
> + if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
> + val & MDIO_PCS_CTRL1_CLKSTOP_EN)
> + ret = __phy_read(phydev, MII_MMD_DATA);
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> static int rtl822x_get_features(struct phy_device *phydev)
> {
> int val;
> @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
> .resume = genphy_resume,
> .read_page = rtl821x_read_page,
> .write_page = rtl821x_write_page,
> + .write_mmd = rtl8211e_write_mmd,
> }, {
> PHY_ID_MATCH_EXACT(0x001cc916),
> .name = "RTL8211F Gigabit Ethernet",
>
Re: [PATCH v2] r8169: Add support for another RTL8168FP
On 02.02.2021 05:48, Kai-Heng Feng wrote: > According to the vendor driver, the new chip with XID 0x54b is > essentially the same as the one with XID 0x54a, but it doesn't need the > firmware. > > So add support accordingly. > > Signed-off-by: Kai-Heng Feng > --- > v2: > - Add phy support. > - Rebase on net-next. > > drivers/net/ethernet/realtek/r8169.h| 1 + > drivers/net/ethernet/realtek/r8169_main.c | 17 +++-- > drivers/net/ethernet/realtek/r8169_phy_config.c | 1 + > 3 files changed, 13 insertions(+), 6 deletions(-) > for net-next Reviewed-by: Heiner Kallweit
Re: [PATCH] r8169: Add support for another RTL8168FP
On 01.02.2021 17:47, Kai-Heng Feng wrote:
> According to the vendor driver, the new chip with XID 0x54b is
> essentially the same as the one with XID 0x54a, but it doesn't need the
> firmware.
>
> So add support accordingly.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/net/ethernet/realtek/r8169.h | 1 +
> drivers/net/ethernet/realtek/r8169_main.c | 21 +
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.h
> b/drivers/net/ethernet/realtek/r8169.h
> index 7be86ef5a584..2728df46ec41 100644
> --- a/drivers/net/ethernet/realtek/r8169.h
> +++ b/drivers/net/ethernet/realtek/r8169.h
> @@ -63,6 +63,7 @@ enum mac_version {
> RTL_GIGA_MAC_VER_50,
> RTL_GIGA_MAC_VER_51,
> RTL_GIGA_MAC_VER_52,
> + RTL_GIGA_MAC_VER_53,
> RTL_GIGA_MAC_VER_60,
> RTL_GIGA_MAC_VER_61,
> RTL_GIGA_MAC_VER_63,
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index a569abe7f5ef..ee1f72a9d453 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -145,6 +145,7 @@ static const struct {
> [RTL_GIGA_MAC_VER_50] = {"RTL8168ep/8111ep" },
> [RTL_GIGA_MAC_VER_51] = {"RTL8168ep/8111ep" },
> [RTL_GIGA_MAC_VER_52] = {"RTL8168fp/RTL8117", FIRMWARE_8168FP_3},
> + [RTL_GIGA_MAC_VER_53] = {"RTL8168fp/RTL8117", },
> [RTL_GIGA_MAC_VER_60] = {"RTL8125A" },
> [RTL_GIGA_MAC_VER_61] = {"RTL8125A",FIRMWARE_8125A_3},
> /* reserve 62 for CFG_METHOD_4 in the vendor driver */
> @@ -682,7 +683,7 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp)
> {
> return tp->mac_version >= RTL_GIGA_MAC_VER_34 &&
> tp->mac_version != RTL_GIGA_MAC_VER_39 &&
> -tp->mac_version <= RTL_GIGA_MAC_VER_52;
> +tp->mac_version <= RTL_GIGA_MAC_VER_53;
> }
>
> static bool rtl_supports_eee(struct rtl8169_private *tp)
> @@ -1012,7 +1013,9 @@ static u16 rtl_ephy_read(struct rtl8169_private *tp,
> int reg_addr)
> static void r8168fp_adjust_ocp_cmd(struct rtl8169_private *tp, u32 *cmd, int
> type)
> {
> /* based on RTL8168FP_OOBMAC_BASE in vendor driver */
> - if (tp->mac_version == RTL_GIGA_MAC_VER_52 && type == ERIAR_OOB)
> + if (type == ERIAR_OOB &&
> + (tp->mac_version == RTL_GIGA_MAC_VER_52 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_53))
> *cmd |= 0x7f0 << 18;
> }
>
> @@ -1164,7 +1167,7 @@ static void rtl8168_driver_start(struct rtl8169_private
> *tp)
> case RTL_GIGA_MAC_VER_31:
> rtl8168dp_driver_start(tp);
> break;
> - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> rtl8168ep_driver_start(tp);
> break;
> default:
> @@ -1195,7 +1198,7 @@ static void rtl8168_driver_stop(struct rtl8169_private
> *tp)
> case RTL_GIGA_MAC_VER_31:
> rtl8168dp_driver_stop(tp);
> break;
> - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> rtl8168ep_driver_stop(tp);
> break;
> default:
> @@ -1223,7 +1226,7 @@ static bool r8168_check_dash(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_28:
> case RTL_GIGA_MAC_VER_31:
> return r8168dp_check_dash(tp);
> - case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_52:
> + case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_53:
> return r8168ep_check_dash(tp);
> default:
> return false;
> @@ -1930,6 +1933,7 @@ static enum mac_version rtl8169_get_mac_version(u16
> xid, bool gmii)
> { 0x7c8, 0x608, RTL_GIGA_MAC_VER_61 },
>
> /* RTL8117 */
> + { 0x7cf, 0x54b, RTL_GIGA_MAC_VER_53 },
> { 0x7cf, 0x54a, RTL_GIGA_MAC_VER_52 },
>
> /* 8168EP family. */
> @@ -2274,7 +2278,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_38:
> RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN |
> RX_DMA_BURST);
> break;
> - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST
> | RX_EARLY_OFF);
> break;
> case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> @@ -2449,7 +2453,7 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond_2)
> static void rtl_wait_txrx_fifo_empty(struct rtl8169_private *tp)
> {
> switch (tp->mac_version) {
> - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_52:
> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> rtl_loop_wait_high(tp, _txcfg_empty_cond, 100, 42);
>
Re: [PATCH net 0/1] net: phy: Fix interrupt mask loss on resume from hibernation
On 22.01.2021 15:35, Laurent Badel wrote: > Some PHYs such as SMSC LAN87xx clear the interrupt mask register on > software reset. Since mdio_bus_phy_restore() calls phy_init_hw() which > does a software reset of the PHY, these PHYs will lose their interrupt > mask configuration on resuming from hibernation. The (optional) software reset is done via soft_reset callback. So if the PHY in question needs special treatment after a soft reset, why not add it to the soft_reset callback? > > I initially reconfigured only the PHY interrupt mask using > phydev->config_intr(), which worked fine with PM_DEBUG/test_resume, but > there seems to be an issue when resuming from a real hibernation, by which > the interrupt type is not set appropriately (in this case > IRQ_TYPE_LEVEL_LOW). Calling irq_set_irq_type() directly from sysfs This sounds to me like a lower level driver (e.g. for GPIO / interrupt controller) not resuming properly from hibernation. Supposedly things like edge/level high/low/both are stored per interrupt line in a register of the interrupt controller, and the controller would have to restore the register value on resume from hibernation. You may want to have a look at that driver. > restored the PHY functionality immediately suggesting that everything is > otherwise well configured. Therefore this patch suggests freeing and > re-requesting the interrupt, to guarantee proper interrupt configuration. > > Laurent Badel (1): > net: phy: Reconfigure PHY interrupt in mdio_bus_phy_restore() > > drivers/net/phy/phy_device.c | 9 + > 1 file changed, 9 insertions(+) >
Missing param value! Expected 'rw=...value...'
Since 60b7cab23e5e ("proc_sysctl: fix oops caused by incorrect command
parameters.")
I'm getting the following warning: Missing param value! Expected
'rw=...value...'
AFAIK param rw doesn't require a value. That's what my command line looks like:
Command line: initrd=\intel-ucode.img initrd=\initramfs-linux.img
root=/dev/sda2 rw
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 14.01.2021 11:41, claudiu.bez...@microchip.com wrote: > > > On 14.01.2021 12:25, Russell King - ARM Linux admin wrote: >> >> As I've said, if phylib/PHY driver is not restoring the state of the >> PHY on resume from suspend-to-ram, then that's an issue with phylib >> and/or the phy driver. > > In the patch I proposed in this thread the restoring is done in PHY driver. > Do you think I should continue the investigation and check if something > should be done from the phylib itself? > It was the right move to approach the PM maintainers to clarify whether the resume PM callback has to assume that power had been cut off and it has to completely reconfigure the device. If they confirm this understanding, then: - the general question remains why there's separate resume and restore callbacks, and what restore is supposed to do that resume doesn't have to do - it should be sufficient to use mdio_bus_phy_restore also as resume callback (instead of changing each and every PHY driver's resume), because we can expect that somebody cutting off power to the PHY properly suspends the MDIO bus before > Thank you, > Claudiu Beznea > >> >> -- >> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ >> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 13.01.2021 23:01, Russell King - ARM Linux admin wrote: > On Wed, Jan 13, 2021 at 10:34:53PM +0100, Heiner Kallweit wrote: >> On 13.01.2021 13:36, claudiu.bez...@microchip.com wrote: >>> On 13.01.2021 13:09, Heiner Kallweit wrote: >>>> On 13.01.2021 10:29, claudiu.bez...@microchip.com wrote: >>>>> It could enter in this mode based on request for standby or >>>>> suspend-to-mem: >>>>> echo mem > /sys/power/state >>>>> echo standby > /sys/power/state > > This is a standard way to enter S2R - I've used it many times in the > past on platforms that support it. > >> I'm not a Linux PM expert, to me it seems your use case is somewhere in the >> middle between s2r and hibernation. I *think* the assumption with s2r is >> that one component shouldn't simply cut the power to another component, >> and the kernel has no idea about it. > > When entering S2R, power can (and probably will) be cut to all system > components, certainly all components that do not support wakeup. If > the system doesn't support WoL, then that will include the ethernet > PHY. > I'm with you if we talk about a driver's suspend callback cutting power to the component it controls, or at least putting it to a power-saving state. However a driver shouldn't have to expect that during S2R somebody else cuts the power. If this would be the case, then I think we wouldn't need separate resume and restore pm callbacks in general. > When resuming, the responsibility is of the kernel and each driver's > .resume function to ensure that the hardware state is restored. Only > each device driver that knows the device itself can restore the state > of that device. In the case of an ethernet PHY, that is phylib and > its associated PHY driver. > Also in phylib we have separate functions mdio_bus_phy_resume() and mdio_bus_phy_restore(), with the first one not fully reconfiguring the PHY. > One has to be a tad careful with phylib and PHYs compared to their > MAC devices in terms of the resume order - it has not been unheard > of over the years for a MAC device to be resumed before its connected > PHY has been. > Right.
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 13.01.2021 13:36, claudiu.bez...@microchip.com wrote: > > > On 13.01.2021 13:09, Heiner Kallweit wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> On 13.01.2021 10:29, claudiu.bez...@microchip.com wrote: >>> Hi Heiner, >>> >>> On 08.01.2021 18:31, Heiner Kallweit wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>>> content is safe >>>> >>>> On 08.01.2021 16:45, Claudiu Beznea wrote: >>>>> KSZ9131 is used in setups with SAMA7G5. SAMA7G5 supports a special >>>>> power saving mode (backup mode) that cuts the power for almost all >>>>> parts of the SoC. The rail powering the ethernet PHY is also cut off. >>>>> When resuming, in case the PHY has been configured on probe with >>>>> slew rate or DLL settings these needs to be restored thus call >>>>> driver's config_init() on resume. >>>>> >>>> When would the SoC enter this backup mode? >>> >>> It could enter in this mode based on request for standby or suspend-to-mem: >>> echo mem > /sys/power/state >>> echo standby > /sys/power/state >>> >>> What I didn't mentioned previously is that the RAM remains in self-refresh >>> while the rest of the SoC is powered down. >>> >> >> This leaves the question which driver sets backup mode in the SoC. > > From Linux point of view the backup mode is a standard suspend-to-mem PM > mode. The only difference is in SoC specific PM code > (arch/arm/mach-at91/pm_suspend.S) where the SoC shutdown command is > executed at the end and the fact that we save the address in RAM of > cpu_resume() function in a powered memory. Then, the resume is done with > the help of bootloader (it configures necessary clocks) and jump the > execution to the previously saved address, resuming Linux. > >> Whatever/whoever wakes the SoC later would have to take care that basically >> everything that was switched off is reconfigured (incl. calling >> phy_init_hw()). > > For this the bootloader should know the PHY settings passed via DT (skew > settings or DLL settings). The bootloader runs from a little SRAM which, at > the moment doesn't know to parse DT bindings and the DT parsing lib might > be big enough that the final bootloader size will cross the SRAM size. > >> So it' more or less the same as waking up from hibernation. Therefore I think >> the .restore of all subsystems would have to be executed, incl. .restore of >> the MDIO bus. > > I see your point. I think it has been implemented like a standard > suspend-to-mem PM mode because the RAM remains in self-refresh whereas in > hibernation it is shut of (as far as I know). > >> Having said that I don't think that change belongs into the >> PHY driver. >> Just imagine tomorrow another PHY type is used in a SAMA7G5 setup. >> Then you would have to do same change in another PHY driver. > > I understand this. At the moment the PM code for drivers in SAMA7G5 are > saving/restoring in/from RAM the registers content in suspend/resume() > functions of each drivers and I think it has been chosen like this as the > RAM remains in self-refresh. Mapping this mode to hibernation will involve > saving the content of RAM to a non-volatile support which is not wanted as > this increases the suspend/resume time and it wasn't intended. > >> >> >>>> And would it suspend the >>>> MDIO bus before cutting power to the PHY? >>> >>> SAMA7G5 embeds Cadence macb driver which has a integrated MDIO bus. Inside >>> macb driver the bus is registered with of_mdiobus_register() or >>> mdiobus_register() based on the PHY devices present in DT or not. On macb >>> suspend()/resume() functions there are calls to >>> phylink_stop()/phylink_start() before cutting/after enabling the power to >>> the PHY. >>> >>>> I'm asking because in mdio_bus_phy_restore() we call phy_init_hw() >>>> already (that calls the driver's config_init). >>> >>> As far as I can see from documentation the .restore API of dev_pm_ops is >>> hibernation specific (please correct me if I'm wrong). On transitions to >>> backup mode the suspend()/resume() PM APIs are called on the drivers. >>> I'm not a Linux PM expert, to me it seems your use case is somewhere in the middle between s2r and hibernation. I *think* the assumption with s2r is that one component shouldn't simply cut the power to another component, and the kernel has
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 13.01.2021 10:29, claudiu.bez...@microchip.com wrote: > Hi Heiner, > > On 08.01.2021 18:31, Heiner Kallweit wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> On 08.01.2021 16:45, Claudiu Beznea wrote: >>> KSZ9131 is used in setups with SAMA7G5. SAMA7G5 supports a special >>> power saving mode (backup mode) that cuts the power for almost all >>> parts of the SoC. The rail powering the ethernet PHY is also cut off. >>> When resuming, in case the PHY has been configured on probe with >>> slew rate or DLL settings these needs to be restored thus call >>> driver's config_init() on resume. >>> >> When would the SoC enter this backup mode? > > It could enter in this mode based on request for standby or suspend-to-mem: > echo mem > /sys/power/state > echo standby > /sys/power/state > > What I didn't mentioned previously is that the RAM remains in self-refresh > while the rest of the SoC is powered down. > This leaves the question which driver sets backup mode in the SoC. Whatever/whoever wakes the SoC later would have to take care that basically everything that was switched off is reconfigured (incl. calling phy_init_hw()). So it' more or less the same as waking up from hibernation. Therefore I think the .restore of all subsystems would have to be executed, incl. .restore of the MDIO bus. Having said that I don't think that change belongs into the PHY driver. Just imagine tomorrow another PHY type is used in a SAMA7G5 setup. Then you would have to do same change in another PHY driver. >> And would it suspend the >> MDIO bus before cutting power to the PHY? > > SAMA7G5 embeds Cadence macb driver which has a integrated MDIO bus. Inside > macb driver the bus is registered with of_mdiobus_register() or > mdiobus_register() based on the PHY devices present in DT or not. On macb > suspend()/resume() functions there are calls to > phylink_stop()/phylink_start() before cutting/after enabling the power to > the PHY. > >> I'm asking because in mdio_bus_phy_restore() we call phy_init_hw() >> already (that calls the driver's config_init). > > As far as I can see from documentation the .restore API of dev_pm_ops is > hibernation specific (please correct me if I'm wrong). On transitions to > backup mode the suspend()/resume() PM APIs are called on the drivers. > > Thank you, > Claudiu Beznea > >> >>> Signed-off-by: Claudiu Beznea >>> --- >>> drivers/net/phy/micrel.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >>> index 3fe552675dd2..52d3a0480158 100644 >>> --- a/drivers/net/phy/micrel.c >>> +++ b/drivers/net/phy/micrel.c >>> @@ -1077,7 +1077,7 @@ static int kszphy_resume(struct phy_device *phydev) >>>*/ >>> usleep_range(1000, 2000); >>> >>> - ret = kszphy_config_reset(phydev); >>> + ret = phydev->drv->config_init(phydev); >>> if (ret) >>> return ret; >>> >>>
Re: net: macb: can macb use __napi_schedule_irqoff() instead of __napi_schedule()
On 11.01.2021 20:45, Paul Thomas wrote:
> Hello, recently I was doing a lot of tracing/profiling to understand
> an issue we were having. Anyway, during this I ran across
> __napi_schedule_irqoff() where the comment in dev.c says "Variant of
> __napi_schedule() assuming hard irqs are masked".
>
> It looks like the queue_writel(queue, IDR, bp->rx_intr_mask); call
> just before the __napi_schedule() call in macb_main.c is doing this
> hard irq masking? So could it change to be like this?
>
It's unsafe under forced irq threading. There has been a number of
discussions about this topic in the past.
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1616,7 +1623,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>
> if (napi_schedule_prep(>napi)) {
> netdev_vdbg(bp->dev, "scheduling RX
> softirq\n");
> - __napi_schedule(>napi);
> + __napi_schedule_irqoff(>napi);
> }
> }
>
> -Paul
>
Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
On 14.12.2020 14:01, Robin Murphy wrote:
> On 2020-12-13 16:32, Heiner Kallweit wrote:
>> Zillions of drivers use the unlikely() hint when checking the result of
>> dma_mapping_error(). This is an inline function anyway, so we can move
>> the hint into this function and remove it from drivers.
>
> Reviewed-by: Robin Murphy
>
> FWIW I consider this case similar to the same hint in WARN_ON() and friends -
> it's a pretty severe condition that should never be expected to be hit in
> normal operation, so it's entirely logical for it to be implicitly unlikely.
> I struggle to imagine any case that would specifically *not* want that (or
> worse, want to hint it as likely). Some DMA API backends may spend
> considerable time trying as hard as possible to make a mapping work before
> eventually admitting defeat, so the idea of ever trying to optimise at the
> driver level for failure in hot paths just makes no sense.
>
> Thanks,
> Robin.
>
>> Signed-off-by: Heiner Kallweit
>> ---
>> v2:
>> Split the big patch into the change for dma-mapping.h and follow-up
>> patches per subsystem that will go through the trees of the respective
>> maintainers.
>> ---
>> include/linux/dma-mapping.h | 2 +-
>> kernel/dma/map_benchmark.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 2e49996a8..6177e20b5 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -95,7 +95,7 @@ static inline int dma_mapping_error(struct device *dev,
>> dma_addr_t dma_addr)
>> {
>> debug_dma_mapping_error(dev, dma_addr);
>> - if (dma_addr == DMA_MAPPING_ERROR)
>> + if (unlikely(dma_addr == DMA_MAPPING_ERROR))
>> return -ENOMEM;
>> return 0;
>> }
>> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
>> index b1496e744..901420a5d 100644
>> --- a/kernel/dma/map_benchmark.c
>> +++ b/kernel/dma/map_benchmark.c
>> @@ -78,7 +78,7 @@ static int map_benchmark_thread(void *data)
>> map_stime = ktime_get();
>> dma_addr = dma_map_single(map->dev, buf, PAGE_SIZE, map->dir);
>> - if (unlikely(dma_mapping_error(map->dev, dma_addr))) {
>> + if (dma_mapping_error(map->dev, dma_addr)) {
>> pr_err("dma_map_single failed on %s\n",
>> dev_name(map->dev));
>> ret = -ENOMEM;
>>
Is this patch going to make it to linux-next?
Re: [PATCH] net: phy: micrel: reconfigure the phy on resume
On 08.01.2021 16:45, Claudiu Beznea wrote: > KSZ9131 is used in setups with SAMA7G5. SAMA7G5 supports a special > power saving mode (backup mode) that cuts the power for almost all > parts of the SoC. The rail powering the ethernet PHY is also cut off. > When resuming, in case the PHY has been configured on probe with > slew rate or DLL settings these needs to be restored thus call > driver's config_init() on resume. > When would the SoC enter this backup mode? And would it suspend the MDIO bus before cutting power to the PHY? I'm asking because in mdio_bus_phy_restore() we call phy_init_hw() already (that calls the driver's config_init). > Signed-off-by: Claudiu Beznea > --- > drivers/net/phy/micrel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 3fe552675dd2..52d3a0480158 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1077,7 +1077,7 @@ static int kszphy_resume(struct phy_device *phydev) >*/ > usleep_range(1000, 2000); > > - ret = kszphy_config_reset(phydev); > + ret = phydev->drv->config_init(phydev); > if (ret) > return ret; > >
Re: [Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address
On 04.01.2021 18:28, Hongwei Zhang wrote:
>
>> From: Jakub Kicinski
>> Sent: Monday, December 28, 2020 5:01 PM
>>
>> On Tue, 22 Dec 2020 22:00:34 +0100 Andrew Lunn wrote:
>>> On Tue, Dec 22, 2020 at 09:46:52PM +0100, Heiner Kallweit wrote:
>>>> On 22.12.2020 21:14, Hongwei Zhang wrote:
>>>>> Dear Reviewer,
>>>>>
>>>>> Use native MAC address is preferred over other choices, thus change the
>>>>> order
>>>>> of reading MAC address, try to read it from MAC chip first, if it's not
>>>>> availabe, then try to read it from device tree.
>>>>>
>>>>> Hi Heiner,
>>>>>
>>>>>> From: Heiner Kallweit
>>>>>> Sent: Monday, December 21, 2020 4:37 PM
>>>>>>> Change the order of reading MAC address, try to read it from MAC chip
>>>>>>> first, if it's not availabe, then try to read it from device tree.
>>>>>>>
>>>>>> This commit message leaves a number of questions. It seems the change
>>>>>> isn't related at all to the
>>>>>> change that it's supposed to fix.
>>>>>>
>>>>>> - What is the issue that you're trying to fix?
>>>>>> - And what is wrong with the original change?
>>>>>
>>>>> There is no bug or something wrong with the original code. This patch is
>>>>> for
>>>>> improving the code. We thought if the native MAC address is available,
>>>>> then
>>>>> it's preferred over MAC address from dts (assuming both sources are
>>>>> available).
>>>>>
>>>>> One possible scenario, a MAC address is set in dts and the BMC image is
>>>>> compiled and loaded into more than one platform, then the platforms will
>>>>> have network issue due to the same MAC address they read.
>>>>>
>>>>
>>>> Typically the DTS MAC address is overwritten by the boot loader, e.g.
>>>> uboot.
>>>> And the boot loader can read it from chip registers. There are more drivers
>>>> trying to read the MAC address from DTS first. Eventually, I think, the
>>>> code
>>>> here will read the same MAC address from chip registers as uboot did
>>>> before.
>
> Thanks for your review, Heiner,
>
> I am working on a platform and want to use the method you said, reading from
> DTS
> is easy, but overwrite the MAC in DTS with chip MAC address, it will change
> the
> checksum of the image. Would you please provide an implementation example?
>
One example is the igb driver. That's the relevant code snippet:
if (eth_platform_get_mac_address(>dev, hw->mac.addr)) {
/* copy the MAC address out of the NVM */
if (hw->mac.ops.read_mac_addr(hw))
dev_err(>dev, "NVM Read Error\n");
}
And I'm not sure the image checksum is relevant here. The boot loader
dynamically replaces the MAC address before handing over the DTS to
Linux kernel. At that time an image checksum shouldn't be relevant.
Who would be supposed to check it?
> Thanks!
>>>
>>> Do we need to worry about, the chip contains random junk, which passes
>>> the validitiy test? Before this patch the value from DT would be used,
>>> and the random junk is ignored. Is this change possibly going to cause
>>> a regression?
>
> Hi Andrew,
>
> Thanks for your review. Yes, yours is a good point, as my change relies on
> the driver's ability to read correct MAC from the chip, or the check of
> is_valid_ether_addr(), which only checking for zeros and multicasting MAC.
> On the other hand, your concern is still true if no MAC is defined in DTS
> file.
>
> Thanks!
>>
>> Hongwei, please address Andrew's questions.
>>
>> Once the discussion is over please repost the patches as
>> git-format-patch would generate them. The patch 2/2 of this
>> series is not really a patch, which confuses all patch handling
>> systems.
>>
>> It also appears that 35c54922dc97 ("ARM: dts: tacoma: Add reserved
>> memory for ramoops") does not exist upstream.
>>
>
> Hi Jakub,
>
> Thanks for your review; I am quite new to the contribution process. I will
> resubmit my
> patch with the SHA value issue fixed. Please see my response at above.
>
> --Hongwei
>
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 04.01.2021 18:39, Lukas Wunner wrote: > On Thu, Dec 31, 2020 at 10:38:12AM +0100, Heiner Kallweit wrote: >> On 31.12.2020 05:07, Lukas Wunner wrote: >>> FWIW, if platform_pci_power_manageable() returns true, it can probably >>> be assumed that allowing runtime PM by default is okay. So as a first >>> step, you may want to call that instead of adding a new callback. >> >> I don't think that's sufficient. Most likely all the broken old systems >> return true for platform_pci_power_manageable(). > > platform_pci_power_manageable() is not a global flag, but rather > a per-device flag whether the platform is capable of power-managing > that device. E.g. for the ACPI platform, it indicates that objects > such as _PS0 or _PS3 are present in the device's namespace. > > My point is that if the platform can power-manage a device, > then it ought to be safe to enable runtime PM by default for it. > Not sure about that. Just that the BIOS claims it can power-manage the device, doesn't rule out that it's broken and fails to do so. > If you insist on a "big hammer" approach by turning on runtime PM > by default for everything, you risk regressions. You can avoid > that by going for a smart approach which enables runtime PM in > cases when it's safe. > > Thanks, > > Lukas > Heiner
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 31.12.2020 10:38, Heiner Kallweit wrote:
> On 31.12.2020 05:07, Lukas Wunner wrote:
>> On Wed, Dec 30, 2020 at 11:56:04PM +0100, Heiner Kallweit wrote:
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3024,7 +3024,9 @@ void pci_pm_init(struct pci_dev *dev)
>>> u16 status;
>>> u16 pmc;
>>>
>>> - pm_runtime_forbid(>dev);
>>> + if (pci_acpi_forbid_runtime_pm())
>>> + pm_runtime_forbid(>dev);
>>> +
>>
>> Generic PCI code usually does not call ACPI-specific functions directly,
>> but rather through a pci_platform_pm_ops callback.
>>
>> FWIW, if platform_pci_power_manageable() returns true, it can probably
>> be assumed that allowing runtime PM by default is okay. So as a first
>> step, you may want to call that instead of adding a new callback.
>>
> I don't think that's sufficient. Most likely all the broken old systems
> return true for platform_pci_power_manageable(). So yes, most likely
> we'd need a new callback if we want to have the platform ops abstraction.
> But it could be an optional callback, something like: forbid_runtime_pm
> The question is just: is it worth it?
>
> By the way: pci_set_platform_pm() returns an error if a callback isn't
> set, but no existing caller bothers to check the return code.
>
>> Thanks,
>>
>> Lukas
>>
> Heiner
>
This would be the version with the abstraction.
---
drivers/pci/pci-acpi.c | 6 ++
drivers/pci/pci.c | 9 -
drivers/pci/pci.h | 5 -
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 53502a751..b57a79af7 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1108,6 +1108,11 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
return !!adev->power.flags.dsw_present;
}
+static bool acpi_pci_forbid_runtime_pm(void)
+{
+ return acpi_gbl_FADT.header.revision < 6;
+}
+
static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
.bridge_d3 = acpi_pci_bridge_d3,
.is_manageable = acpi_pci_power_manageable,
@@ -1117,6 +1122,7 @@ static const struct pci_platform_pm_ops
acpi_pci_platform_pm = {
.choose_state = acpi_pci_choose_state,
.set_wakeup = acpi_pci_wakeup,
.need_resume = acpi_pci_need_resume,
+ .forbid_runtime_pm = acpi_pci_forbid_runtime_pm,
};
void acpi_pci_add_bus(struct pci_bus *bus)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d..3af832b7f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -982,6 +982,12 @@ static inline bool platform_pci_need_resume(struct pci_dev
*dev)
return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
}
+static inline bool platform_pci_forbid_runtime_pm(void)
+{
+ return pci_platform_pm && pci_platform_pm->forbid_runtime_pm ?
+ pci_platform_pm->forbid_runtime_pm() : false;
+}
+
static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
{
if (pci_platform_pm && pci_platform_pm->bridge_d3)
@@ -3024,7 +3030,8 @@ void pci_pm_init(struct pci_dev *dev)
u16 status;
u16 pmc;
- pm_runtime_forbid(>dev);
+ if (platform_pci_forbid_runtime_pm())
+ pm_runtime_forbid(>dev);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
device_enable_async_suspend(>dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5c5936509..d2d406ee4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -71,8 +71,10 @@ int pci_bus_error_reset(struct pci_dev *dev);
* suspended) needs to be resumed to be configured for system
* wakeup.
*
+ * @forbid_runtime_pm: returns true in case of known issues breaking runtime pm
+ *
* If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
+ * these callbacks are mandatory (except forbid_runtime_pm).
*/
struct pci_platform_pm_ops {
bool (*bridge_d3)(struct pci_dev *dev);
@@ -83,6 +85,7 @@ struct pci_platform_pm_ops {
pci_power_t (*choose_state)(struct pci_dev *dev);
int (*set_wakeup)(struct pci_dev *dev, bool enable);
bool (*need_resume)(struct pci_dev *dev);
+ bool (*forbid_runtime_pm)(void);
};
int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
--
2.29.2
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 31.12.2020 05:07, Lukas Wunner wrote: > On Wed, Dec 30, 2020 at 11:56:04PM +0100, Heiner Kallweit wrote: >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3024,7 +3024,9 @@ void pci_pm_init(struct pci_dev *dev) >> u16 status; >> u16 pmc; >> >> -pm_runtime_forbid(>dev); >> +if (pci_acpi_forbid_runtime_pm()) >> +pm_runtime_forbid(>dev); >> + > > Generic PCI code usually does not call ACPI-specific functions directly, > but rather through a pci_platform_pm_ops callback. > > FWIW, if platform_pci_power_manageable() returns true, it can probably > be assumed that allowing runtime PM by default is okay. So as a first > step, you may want to call that instead of adding a new callback. > I don't think that's sufficient. Most likely all the broken old systems return true for platform_pci_power_manageable(). So yes, most likely we'd need a new callback if we want to have the platform ops abstraction. But it could be an optional callback, something like: forbid_runtime_pm The question is just: is it worth it? By the way: pci_set_platform_pm() returns an error if a callback isn't set, but no existing caller bothers to check the return code. > Thanks, > > Lukas > Heiner
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas wrote:
>>
>> [+to Rafael, author of the commit you mentioned,
>> +cc Mika, Kai Heng, Lukas, linux-pm, linux-kernel]
>>
>> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
>>> More than 10 yrs ago Runtime PM was disabled per default by bb910a7040
>>> ("PCI/PM Runtime: Make runtime PM of PCI devices inactive by default").
>>>
>>> Reason given: "avoid breakage on systems where ACPI-based wake-up is
>>> known to fail for some devices"
>>> Unfortunately the commit message doesn't mention any affected devices
>>> or systems.
>
> Even if it did that, it wouldn't have been a full list almost for sure.
>
> We had received multiple problem reports related to that, most likely
> because the ACPI PM in BIOSes at that time was tailored for
> system-wide PM transitions only.
>
>>> With Runtime PM disabled e.g. the PHY on network devices may remain
>>> powered up even with no cable plugged in, affecting battery lifetime
>>> on mobile devices. Currently we have to rely on the respective distro
>>> or user to enable Runtime PM via sysfs (echo auto > power/control).
>>> Some devices work around this restriction by calling pm_runtime_allow
>>> in their probe routine, even though that's not recommended by
>>> https://www.kernel.org/doc/Documentation/power/pci.txt
>>>
>>> Disabling Runtime PM per default seems to be a big hammer, a quirk
>>> for affected devices / systems may had been better. And we still
>>> have the option to disable Runtime PM for selected devices via sysfs.
>>>
>>> So, to cut a long story short: Wouldn't it be time to remove this
>>> restriction?
>>
>> I don't know the history of this, but maybe Rafael or the others can
>> shed some light on it.
>
> The systems that had those problems 10 years ago would still have
> them, but I expect there to be more systems where runtime PM can be
> enabled by default for PCI devices without issues.
>
As a proposal, maybe we can use the ACPI revision as an indicator for
whether ACPI implementation is new enough to not be affected by the old
problems. With the following simple patch runtime pm won't be disabled
per default for ACPI versions >= 6.0. AFAIK ACPI 6.0 was published in 2015.
On a side note:
It seems the sole motivation to disable runtime pm per default is ACPI
problems. So why do we disable runtime pm also on non-ACPI systems?
With the proposed patch runtime pm is enabled per default if
CONFIG_ACPI isn't defined.
---
drivers/pci/pci-acpi.c | 5 +
drivers/pci/pci.c | 4 +++-
drivers/pci/pci.h | 5 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 53502a751..265f5d2c4 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -27,6 +27,11 @@ const guid_t pci_acpi_dsm_guid =
GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
+bool pci_acpi_forbid_runtime_pm(void)
+{
+ return acpi_gbl_FADT.header.revision < 6;
+}
+
#if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
{
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d..83b5a7e63 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3024,7 +3024,9 @@ void pci_pm_init(struct pci_dev *dev)
u16 status;
u16 pmc;
- pm_runtime_forbid(>dev);
+ if (pci_acpi_forbid_runtime_pm())
+ pm_runtime_forbid(>dev);
+
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
device_enable_async_suspend(>dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5c5936509..c1d521fb2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -701,11 +701,16 @@ static inline int pci_aer_raw_clear_status(struct pci_dev
*dev) { return -EINVAL
#ifdef CONFIG_ACPI
int pci_acpi_program_hp_params(struct pci_dev *dev);
+bool pci_acpi_forbid_runtime_pm(void);
#else
static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
{
return -ENODEV;
}
+static inline bool pci_acpi_forbid_runtime_pm(void)
+{
+ return false;
+}
#endif
#ifdef CONFIG_PCIEASPM
--
2.29.2
[tip: x86/misc] x86/reboot: Add Zotac ZBOX CI327 nano PCI reboot quirk
The following commit has been merged into the x86/misc branch of tip:
Commit-ID: 4b2d8ca9208be636b30e924b1cbcb267b0740c93
Gitweb:
https://git.kernel.org/tip/4b2d8ca9208be636b30e924b1cbcb267b0740c93
Author:Heiner Kallweit
AuthorDate:Tue, 01 Dec 2020 12:39:57 +01:00
Committer: Borislav Petkov
CommitterDate: Wed, 30 Dec 2020 18:38:39 +01:00
x86/reboot: Add Zotac ZBOX CI327 nano PCI reboot quirk
On this system the M.2 PCIe WiFi card isn't detected after reboot, only
after cold boot. reboot=pci fixes this behavior. In [0] the same issue
is described, although on another system and with another Intel WiFi
card. In case it's relevant, both systems have Celeron CPUs.
Add a PCI reboot quirk on affected systems until a more generic fix is
available.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=202399
[ bp: Massage commit message. ]
Signed-off-by: Heiner Kallweit
Signed-off-by: Borislav Petkov
Link: https://lkml.kernel.org/r/1524eafd-f89c-cfa4-ed70-0bde9e45e...@gmail.com
---
arch/x86/kernel/reboot.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index db11594..9991c59 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -477,6 +477,15 @@ static const struct dmi_system_id reboot_dmi_table[]
__initconst = {
},
},
+ { /* PCIe Wifi card isn't detected after reboot otherwise */
+ .callback = set_pci_reboot,
+ .ident = "Zotac ZBOX CI327 nano",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "NA"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "ZBOX-CI327NANO-GS-01"),
+ },
+ },
+
/* Sony */
{ /* Handle problems with rebooting on Sony VGN-Z540N */
.callback = set_bios_reboot,
Re: Registering IRQ for MT7530 internal PHYs
On 30.12.2020 17:15, Florian Fainelli wrote: > > > On 12/30/2020 1:12 AM, Heiner Kallweit wrote: >> On 30.12.2020 10:07, DENG Qingfang wrote: >>> Hi Heiner, >>> Thanks for your reply. >>> >>> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit >>> wrote: >>>> I don't think that's the best option. >>> >>> I'm well aware of that. >>> >>>> You may want to add a PHY driver for your chip. Supposedly it >>>> supports at least PHY suspend/resume. You can use the RTL8366RB >>>> PHY driver as template. >>> >>> There's no MediaTek PHY driver yet. Do we really need a new one just >>> for the interrupts? >>> >> Not only for the interrupts. The genphy driver e.g. doesn't support >> PHY suspend/resume. And the PHY driver needs basically no code, >> just set the proper callbacks. > > That statement about not supporting suspend/resume is not exactly true, > the generic "1g" PHY driver only implements suspend/resume through the > use of the standard BMCR power down bit, but not anything more > complicated than that. > Oh, right. Somehow I had in the back of my mind that the genphy driver has no suspend/resume callbacks set. > Interrupt handling within the PHY itself is not defined by the existing > standard registers and will typically not reside in a standard register > space either, so just for that reason you do need a custom PHY driver. > There are other advantages if you need to expose additional PHY features > down the road like PHY counters, energy detection, automatic power down etc. > > I don't believe we will see discrete/standalone Mediatek PHY chips, but > if that happens, then you would already have a framework for supporting > them. >
Re: Registering IRQ for MT7530 internal PHYs
On 30.12.2020 10:07, DENG Qingfang wrote:
> Hi Heiner,
> Thanks for your reply.
>
> On Wed, Dec 30, 2020 at 3:39 PM Heiner Kallweit wrote:
>> I don't think that's the best option.
>
> I'm well aware of that.
>
>> You may want to add a PHY driver for your chip. Supposedly it
>> supports at least PHY suspend/resume. You can use the RTL8366RB
>> PHY driver as template.
>
> There's no MediaTek PHY driver yet. Do we really need a new one just
> for the interrupts?
>
Not only for the interrupts. The genphy driver e.g. doesn't support
PHY suspend/resume. And the PHY driver needs basically no code,
just set the proper callbacks.
>>> + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
>>> + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n",
>>> mt7530_read(priv, MT7530_SYS_INT_EN));
>>> +
>> This is debug code to be removed in the final version?
>
> Yes.
>
>>> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
>>> + if (val & BIT(phy)) {
>>> + unsigned int child_irq;
>>> +
>>> + child_irq = irq_find_mapping(priv->irq_domain, phy);
>>> + handle_nested_irq(child_irq);
>>> + handled = true;
>>> + }
>>> + }
>>> +
>>> + return handled ? IRQ_HANDLED : IRQ_NONE;
>>
>> IRQ_RETVAL() could be used here.
>
> Good to know :)
>
>>
>>> +}
>>> +
>>> +static void mt7530_irq_mask(struct irq_data *d)
>>> +{
>>> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
>>> +
>>> + priv->irq_enable &= ~BIT(d->hwirq);
>>
>> Here you don't actually do something. HW doesn't support masking
>> interrupt generation for a port?
>
> priv->irq_enable will be written to MT7530_SYS_INT_EN in
> mt7530_irq_bus_sync_unlock. You can think of it as an inverted mask.
>
Re: Registering IRQ for MT7530 internal PHYs
On 30.12.2020 05:22, DENG Qingfang wrote:
> Hi,
>
> I added MT7530 IRQ support and registered its internal PHYs to IRQ.
> It works but my patch used two hacks.
>
> 1. Removed phy_drv_supports_irq check, because config_intr and
> handle_interrupt are not set for Generic PHY.
>
I don't think that's the best option. First, did you check which
consequences this has? The check is there for a reason.
You may want to add a PHY driver for your chip. Supposedly it
supports at least PHY suspend/resume. You can use the RTL8366RB
PHY driver as template.
> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
> we cannot call dsa_slave_mii_bus_init which is private.
>
> Any better ideas?
>
> Regards,
> Qingfang
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a67cac15a724..d59a8c50ede3 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port,
> }
> }
>
> +static irqreturn_t
> +mt7530_irq(int irq, void *data)
> +{
> + struct mt7530_priv *priv = data;
> + bool handled = false;
> + int phy;
> + u32 val;
> +
> + val = mt7530_read(priv, MT7530_SYS_INT_STS);
> + mt7530_write(priv, MT7530_SYS_INT_STS, val);
> +
> + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
> + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n",
> mt7530_read(priv, MT7530_SYS_INT_EN));
> +
This is debug code to be removed in the final version?
> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> + if (val & BIT(phy)) {
> + unsigned int child_irq;
> +
> + child_irq = irq_find_mapping(priv->irq_domain, phy);
> + handle_nested_irq(child_irq);
> + handled = true;
> + }
> + }
> +
> + return handled ? IRQ_HANDLED : IRQ_NONE;
IRQ_RETVAL() could be used here.
> +}
> +
> +static void mt7530_irq_mask(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + priv->irq_enable &= ~BIT(d->hwirq);
Here you don't actually do something. HW doesn't support masking
interrupt generation for a port?
> +}
> +
> +static void mt7530_irq_unmask(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + priv->irq_enable |= BIT(d->hwirq);
> +}
> +
> +static void mt7530_irq_bus_lock(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + mutex_lock(>reg_mutex);
> +}
> +
> +static void mt7530_irq_bus_sync_unlock(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
> + mutex_unlock(>reg_mutex);
> +}
> +
> +static struct irq_chip mt7530_irq_chip = {
> + .name = MT7530_NAME,
> + .irq_mask = mt7530_irq_mask,
> + .irq_unmask = mt7530_irq_unmask,
> + .irq_bus_lock = mt7530_irq_bus_lock,
> + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock,
> +};
> +
> +static int
> +mt7530_irq_map(struct irq_domain *domain, unsigned int irq,
> +irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(irq, domain->host_data);
> + irq_set_chip_and_handler(irq, _irq_chip, handle_simple_irq);
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops mt7530_irq_domain_ops = {
> + .map = mt7530_irq_map,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +
> +static void
> +mt7530_irq_init(struct mt7530_priv *priv)
> +{
> + struct mii_bus *bus = priv->ds->slave_mii_bus;
> + struct device *dev = priv->dev;
> + struct device_node *np = dev->of_node;
> + int parent_irq;
> + int phy, ret;
> +
> + parent_irq = of_irq_get(np, 0);
> + if (parent_irq <= 0) {
> + dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
> + return;
> + }
> +
> + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> + ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
> + IRQF_ONESHOT, MT7530_NAME, priv);
> + if (ret) {
> + dev_err(dev, "failed to request IRQ: %d\n", ret);
> + return;
> + }
> +
> + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> + _irq_domain_ops, priv);
> + if (!priv->irq_domain) {
> + dev_err(dev, "failed to create IRQ domain\n");
> + return;
> + }
> +
> + /* IRQ for internal PHYs */
> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> + unsigned int irq = irq_create_mapping(priv->irq_domain, phy);
> +
> + irq_set_parent(irq, parent_irq);
> + bus->irq[phy] = irq;
> +
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 29.12.2020 12:56, Kai-Heng Feng wrote:
> On Sat, Dec 26, 2020 at 11:26 PM Heiner Kallweit wrote:
>>
>> On 17.11.2020 17:57, Rafael J. Wysocki wrote:
>>> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas wrote:
>>>>
>>>> [+to Rafael, author of the commit you mentioned,
>>>> +cc Mika, Kai Heng, Lukas, linux-pm, linux-kernel]
>>>>
>>>> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
>>>>> More than 10 yrs ago Runtime PM was disabled per default by bb910a7040
>>>>> ("PCI/PM Runtime: Make runtime PM of PCI devices inactive by default").
>>>>>
>>>>> Reason given: "avoid breakage on systems where ACPI-based wake-up is
>>>>> known to fail for some devices"
>>>>> Unfortunately the commit message doesn't mention any affected devices
>>>>> or systems.
>>>
>>> Even if it did that, it wouldn't have been a full list almost for sure.
>>>
>>> We had received multiple problem reports related to that, most likely
>>> because the ACPI PM in BIOSes at that time was tailored for
>>> system-wide PM transitions only.
>>>
>>
>> To follow up on this discussion:
>> We could call pm_runtime_forbid() conditionally, e.g. with the following
>> condition. This would enable runtime pm per default for all non-ACPI
>> systems, and it uses the BIOS date as an indicator for a hopefully
>> not that broken ACPI implementation. However I could understand the
>> argument that this looks a little hacky ..
>>
>> if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)
>
> dmi_get_bios_year() may not be a good indicator. Last time I used it
> caused regression, because the value changed after BIOS update.
> For example, my MacBook Pro is manufactured in 2011, but
> dmi_get_bios_year() returns 2018 with latest BIOS.
>
Right, it's a weak indicator, and I'm aware of it. My thought is:
A massively broken ACPI implementation would have been fixed over
time if there are years between manufacturing date and last BIOS
update. Of course this doesn't have to be true ..
I just had a look at the ACPI spec and maybe we could use the ACPI
version information (major.minor) in the FADT table. E.g. we could
enable runtime pm from version 6.0. That should be reasonable new.
I'm open for any other proposals ..
> Kai-Heng
>
>>
>>
>>
>>>>> With Runtime PM disabled e.g. the PHY on network devices may remain
>>>>> powered up even with no cable plugged in, affecting battery lifetime
>>>>> on mobile devices. Currently we have to rely on the respective distro
>>>>> or user to enable Runtime PM via sysfs (echo auto > power/control).
>>>>> Some devices work around this restriction by calling pm_runtime_allow
>>>>> in their probe routine, even though that's not recommended by
>>>>> https://www.kernel.org/doc/Documentation/power/pci.txt
>>>>>
>>>>> Disabling Runtime PM per default seems to be a big hammer, a quirk
>>>>> for affected devices / systems may had been better. And we still
>>>>> have the option to disable Runtime PM for selected devices via sysfs.
>>>>>
>>>>> So, to cut a long story short: Wouldn't it be time to remove this
>>>>> restriction?
>>>>
>>>> I don't know the history of this, but maybe Rafael or the others can
>>>> shed some light on it.
>>>
>>> The systems that had those problems 10 years ago would still have
>>> them, but I expect there to be more systems where runtime PM can be
>>> enabled by default for PCI devices without issues.
>>>
>>
Re: Time to re-enable Runtime PM per default for PCI devcies?
On 17.11.2020 17:57, Rafael J. Wysocki wrote:
> On Tue, Nov 17, 2020 at 5:38 PM Bjorn Helgaas wrote:
>>
>> [+to Rafael, author of the commit you mentioned,
>> +cc Mika, Kai Heng, Lukas, linux-pm, linux-kernel]
>>
>> On Tue, Nov 17, 2020 at 04:56:09PM +0100, Heiner Kallweit wrote:
>>> More than 10 yrs ago Runtime PM was disabled per default by bb910a7040
>>> ("PCI/PM Runtime: Make runtime PM of PCI devices inactive by default").
>>>
>>> Reason given: "avoid breakage on systems where ACPI-based wake-up is
>>> known to fail for some devices"
>>> Unfortunately the commit message doesn't mention any affected devices
>>> or systems.
>
> Even if it did that, it wouldn't have been a full list almost for sure.
>
> We had received multiple problem reports related to that, most likely
> because the ACPI PM in BIOSes at that time was tailored for
> system-wide PM transitions only.
>
To follow up on this discussion:
We could call pm_runtime_forbid() conditionally, e.g. with the following
condition. This would enable runtime pm per default for all non-ACPI
systems, and it uses the BIOS date as an indicator for a hopefully
not that broken ACPI implementation. However I could understand the
argument that this looks a little hacky ..
if (IS_ENABLED(CONFIG_ACPI) && dmi_get_bios_year() <= 2016)
>>> With Runtime PM disabled e.g. the PHY on network devices may remain
>>> powered up even with no cable plugged in, affecting battery lifetime
>>> on mobile devices. Currently we have to rely on the respective distro
>>> or user to enable Runtime PM via sysfs (echo auto > power/control).
>>> Some devices work around this restriction by calling pm_runtime_allow
>>> in their probe routine, even though that's not recommended by
>>> https://www.kernel.org/doc/Documentation/power/pci.txt
>>>
>>> Disabling Runtime PM per default seems to be a big hammer, a quirk
>>> for affected devices / systems may had been better. And we still
>>> have the option to disable Runtime PM for selected devices via sysfs.
>>>
>>> So, to cut a long story short: Wouldn't it be time to remove this
>>> restriction?
>>
>> I don't know the history of this, but maybe Rafael or the others can
>> shed some light on it.
>
> The systems that had those problems 10 years ago would still have
> them, but I expect there to be more systems where runtime PM can be
> enabled by default for PCI devices without issues.
>
Re: [PATCH -next] intel/iwlwifi: use DEFINE_MUTEX (and mutex_init() had been too late)
On 23.12.2020 15:11, Zheng Yongjun wrote:
> Signed-off-by: Zheng Yongjun
> ---
> drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index 9dcd2e990c9c..71119044382f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> @@ -133,7 +133,7 @@ enum {
> };
>
> /* Protects the table contents, i.e. the ops pointer & drv list */
> -static struct mutex iwlwifi_opmode_table_mtx;
> +static DEFINE_MUTEX(iwlwifi_opmode_table_mtx);
> static struct iwlwifi_opmode_table {
> const char *name; /* name: iwldvm, iwlmvm, etc */
> const struct iwl_op_mode_ops *ops; /* pointer to op_mode ops */
> @@ -1786,8 +1786,6 @@ static int __init iwl_drv_init(void)
> {
> int i, err;
>
> - mutex_init(_opmode_table_mtx);
> -
> for (i = 0; i < ARRAY_SIZE(iwlwifi_opmode_table); i++)
> INIT_LIST_HEAD(_opmode_table[i].drv);
>
>
The change itself is ok, but:
- commit message is missing (did you run checkpatch?)
- Why should the current mutex_init() call be too late?
Re: [Aspeed, v2 2/2] net: ftgmac100: Change the order of getting MAC address
On 22.12.2020 21:14, Hongwei Zhang wrote:
> Dear Reviewer,
>
> Use native MAC address is preferred over other choices, thus change the order
> of reading MAC address, try to read it from MAC chip first, if it's not
> availabe, then try to read it from device tree.
>
>
> Hi Heiner,
>
>> From:Heiner Kallweit
>> Sent:Monday, December 21, 2020 4:37 PM
>>> Change the order of reading MAC address, try to read it from MAC chip
>>> first, if it's not availabe, then try to read it from device tree.
>>>
>> This commit message leaves a number of questions. It seems the change isn't
>> related at all to the
>> change that it's supposed to fix.
>>
>> - What is the issue that you're trying to fix?
>> - And what is wrong with the original change?
>
> There is no bug or something wrong with the original code. This patch is for
> improving the code. We thought if the native MAC address is available, then
> it's preferred over MAC address from dts (assuming both sources are
> available).
>
> One possible scenario, a MAC address is set in dts and the BMC image is
> compiled and loaded into more than one platform, then the platforms will
> have network issue due to the same MAC address they read.
>
Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
And the boot loader can read it from chip registers. There are more drivers
trying to read the MAC address from DTS first. Eventually, I think, the code
here will read the same MAC address from chip registers as uboot did before.
> Thanks for your review, I've update the patch to fix the comments.
>>
>>> Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for
>>> ramoops")
>>> Signed-off-by: Hongwei Zhang
>>> ---
>>> drivers/net/ethernet/faraday/ftgmac100.c | 22 +-
>>> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> --Hongwei
>
Re: [Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address
Am 21.12.2020 um 21:51 schrieb Hongwei Zhang:
> Change the order of reading MAC address, try to read it from MAC chip
> first, if it's not availabe, then try to read it from device tree.
>
This commit message leaves a number of questions. It seems the change
isn't related at all to the change that it's supposed to fix.
- What is the issue that you're trying to fix?
- And what is wrong with the original change?
> Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for ramoops")
> Signed-off-by: Hongwei Zhang
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 22 +-
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 65cd25372020..9be69cbdab96 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -184,14 +184,7 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
> unsigned int l;
> void *addr;
>
> - addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
> - if (addr) {
> - ether_addr_copy(priv->netdev->dev_addr, mac);
> - dev_info(priv->dev, "Read MAC address %pM from device tree\n",
> - mac);
> - return;
> - }
> -
> + /* Read from Chip if not from chip */
?!?
> m = ioread32(priv->base + FTGMAC100_OFFSET_MAC_MADR);
> l = ioread32(priv->base + FTGMAC100_OFFSET_MAC_LADR);
>
> @@ -205,7 +198,18 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
> if (is_valid_ether_addr(mac)) {
> ether_addr_copy(priv->netdev->dev_addr, mac);
> dev_info(priv->dev, "Read MAC address %pM from chip\n", mac);
> - } else {
> + return;
> + }
> +
> + /* Read from Chip if not from device tree */
Isn't this how it works now?
> + addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
> + if (addr) {
> + ether_addr_copy(priv->netdev->dev_addr, mac);
> + dev_info(priv->dev, "Read MAC address %pM from device tree\n",
> + mac);
> + return;
> + }
> + else {
> eth_hw_addr_random(priv->netdev);
> dev_info(priv->dev, "Generated random MAC address %pM\n",
>priv->netdev->dev_addr);
>
Re: [PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
Am 13.12.2020 um 22:27 schrieb Song Bao Hua (Barry Song):
>
>
>> -Original Message-----
>> From: Heiner Kallweit [mailto:hkallwe...@gmail.com]
>> Sent: Monday, December 14, 2020 5:33 AM
>> To: Christoph Hellwig ; Marek Szyprowski
>> ; Robin Murphy ; Song Bao Hua
>> (Barry Song)
>> Cc: open list:AMD IOMMU (AMD-VI) ; Linux
>> Kernel Mailing List
>> Subject: [PATCH v2] dma-mapping: add unlikely hint for error path in
>> dma_mapping_error
>>
>> Zillions of drivers use the unlikely() hint when checking the result of
>> dma_mapping_error(). This is an inline function anyway, so we can move
>> the hint into this function and remove it from drivers.
>>
>> Signed-off-by: Heiner Kallweit
>
> not sure if this is really necessary. It seems the original code
> is more readable. Readers can more easily understand we are
> predicting the branch based on the return value of
> dma_mapping_error().
>
I basically see two points promoting the proposed change:
1. Driver authors shouldn't have to think (as far as possible) about
whether a branch prediction hint could make sense for a standard
core API call. I saw quite some past discussions about when
something is unlikely enough so that an unlikely() makes sense.
If the core can hide some more complexity from drivers, then
I think it's a good thing.
2. If we ever want or have to change the use of unlikely with
dma_mapping_error(), then we have to do it in just one place.
> Anyway, I don't object to this one. if other people like it, I am
> also ok with it.
>
>> ---
>> v2:
>> Split the big patch into the change for dma-mapping.h and follow-up
>> patches per subsystem that will go through the trees of the respective
>> maintainers.
>> ---
>> include/linux/dma-mapping.h | 2 +-
>> kernel/dma/map_benchmark.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 2e49996a8..6177e20b5 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -95,7 +95,7 @@ static inline int dma_mapping_error(struct device *dev,
>> dma_addr_t dma_addr)
>> {
>> debug_dma_mapping_error(dev, dma_addr);
>>
>> -if (dma_addr == DMA_MAPPING_ERROR)
>> +if (unlikely(dma_addr == DMA_MAPPING_ERROR))
>> return -ENOMEM;
>> return 0;
>> }
>> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
>> index b1496e744..901420a5d 100644
>> --- a/kernel/dma/map_benchmark.c
>> +++ b/kernel/dma/map_benchmark.c
>> @@ -78,7 +78,7 @@ static int map_benchmark_thread(void *data)
>>
>> map_stime = ktime_get();
>> dma_addr = dma_map_single(map->dev, buf, PAGE_SIZE, map->dir);
>> -if (unlikely(dma_mapping_error(map->dev, dma_addr))) {
>> +if (dma_mapping_error(map->dev, dma_addr)) {
>> pr_err("dma_map_single failed on %s\n",
>> dev_name(map->dev));
>> ret = -ENOMEM;
>> --
>> 2.29.2
>
> Thanks
> Barry
>
[PATCH v2] dma-mapping: add unlikely hint for error path in dma_mapping_error
Zillions of drivers use the unlikely() hint when checking the result of
dma_mapping_error(). This is an inline function anyway, so we can move
the hint into this function and remove it from drivers.
Signed-off-by: Heiner Kallweit
---
v2:
Split the big patch into the change for dma-mapping.h and follow-up
patches per subsystem that will go through the trees of the respective
maintainers.
---
include/linux/dma-mapping.h | 2 +-
kernel/dma/map_benchmark.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 2e49996a8..6177e20b5 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -95,7 +95,7 @@ static inline int dma_mapping_error(struct device *dev,
dma_addr_t dma_addr)
{
debug_dma_mapping_error(dev, dma_addr);
- if (dma_addr == DMA_MAPPING_ERROR)
+ if (unlikely(dma_addr == DMA_MAPPING_ERROR))
return -ENOMEM;
return 0;
}
diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
index b1496e744..901420a5d 100644
--- a/kernel/dma/map_benchmark.c
+++ b/kernel/dma/map_benchmark.c
@@ -78,7 +78,7 @@ static int map_benchmark_thread(void *data)
map_stime = ktime_get();
dma_addr = dma_map_single(map->dev, buf, PAGE_SIZE, map->dir);
- if (unlikely(dma_mapping_error(map->dev, dma_addr))) {
+ if (dma_mapping_error(map->dev, dma_addr)) {
pr_err("dma_map_single failed on %s\n",
dev_name(map->dev));
ret = -ENOMEM;
--
2.29.2
Re: [PATCH net v2] lan743x: fix rx_napi_poll/interrupt ping-pong
Am 11.12.2020 um 13:43 schrieb Sven Van Asbroeck: > Hi Heiner, > > On Thu, Dec 10, 2020 at 2:32 AM Heiner Kallweit wrote: >> >> >> In addition you could play with sysfs attributes >> /sys/class/net//gro_flush_timeout >> /sys/class/net//napi_defer_hard_irqs > > Interesting, I will look into that. > I run a 1Gbit chip with gro_flush_timeout = 2 and napi_defer_hard_irqs = 1. This helped to reduce interrupt load significantly under iperf3 (w/o interrupt coalescing at chip level) >>> @@ -2407,7 +2409,7 @@ static int lan743x_rx_open(struct lan743x_rx *rx) >>> >>> netif_napi_add(adapter->netdev, >>> >napi, lan743x_rx_napi_poll, >>> -rx->ring_size - 1); >>> +64); >> >> This value isn't completely arbitrary. >> Better use constant NAPI_POLL_WEIGHT. >> > > Thank you, I will change it in the next patch version. >
Re: [PATCH] x86/reboot/quirks: Add Zotac ZBOX CI327 nano PCI reboot quirk
Am 10.12.2020 um 20:04 schrieb Borislav Petkov: > On Tue, Dec 01, 2020 at 12:39:57PM +0100, Heiner Kallweit wrote: >> On this system the M.2 PCIe WiFi card isn't detected after reboot, >> only after cold boot. reboot=pci fixes this behavior. >> In [0] the same issue is described, although on another system and >> with another Intel WiFi card. In case it's relevant, both systems >> have Celeron CPU's. >> The dicussion in [0] involved the PCI maintainer, and proposal was >> to go with the PCI reboot quirk on affected systems until a more >> generic fix is available. >> >> [0] https://bugzilla.kernel.org/show_bug.cgi?id=202399 > > But this quirk is for your system only - the one in the bugzilla entry > would need another one? Or? > Right, as Bjorn wrote in comment 14: ".., and there may be many systems with this issue and we may be adding such quirks frequently. But maybe that's the only option, since we don't know any other way to fix this." I'd prefer that the autor of a quirk also has the hw to test it on. Therefore I just added the quirk for my system as a template to the bug report. > Thx. > Heiner
[PATCH] dma-mapping: move hint unlikely for dma_mapping_error from drivers to core
Zillions of drivers use the unlikely() hint when checking the result of dma_mapping_error(). This is an inline function anyway, so we can move the hint into the function and remove it from drivers. >From time to time discussions pop up how effective unlikely() is, and that it should be used only if something is really very unlikely. I think that's the case here. Patch was created with some help from coccinelle. @@ expression dev, dma_addr; @@ - unlikely(dma_mapping_error(dev, dma_addr)) + dma_mapping_error(dev, dma_addr) Signed-off-by: Heiner Kallweit --- If ok, then tbd through which tree this is supposed to go. Patch is based on linux-next-20201210. --- drivers/crypto/cavium/cpt/cptvf_reqmanager.c | 3 +-- drivers/crypto/hisilicon/hpre/hpre_crypto.c | 2 +- .../marvell/octeontx/otx_cptvf_reqmgr.c | 5 ++-- drivers/crypto/mediatek/mtk-aes.c | 2 +- drivers/crypto/mediatek/mtk-sha.c | 6 ++--- drivers/crypto/qat/qat_common/qat_algs.c | 8 +++--- drivers/crypto/qat/qat_common/qat_asym_algs.c | 25 +-- drivers/i2c/busses/i2c-amd-mp2-plat.c | 2 +- drivers/infiniband/hw/hfi1/sdma.c | 2 +- drivers/net/ethernet/aeroflex/greth.c | 4 +-- drivers/net/ethernet/amazon/ena/ena_netdev.c | 8 +++--- .../net/ethernet/apm/xgene/xgene_enet_main.c | 2 +- .../net/ethernet/aquantia/atlantic/aq_nic.c | 5 ++-- .../net/ethernet/aquantia/atlantic/aq_ring.c | 2 +- drivers/net/ethernet/arc/emac_main.c | 2 +- .../net/ethernet/atheros/atl1c/atl1c_main.c | 6 ++--- drivers/net/ethernet/broadcom/bgmac.c | 4 +-- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 10 .../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 2 +- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 +-- drivers/net/ethernet/chelsio/cxgb4/sge.c | 4 +-- drivers/net/ethernet/chelsio/cxgb4vf/sge.c| 4 +-- drivers/net/ethernet/faraday/ftgmac100.c | 2 +- drivers/net/ethernet/faraday/ftmac100.c | 4 +-- .../net/ethernet/freescale/dpaa/dpaa_eth.c| 13 +- .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 12 - drivers/net/ethernet/freescale/enetc/enetc.c | 4 +-- drivers/net/ethernet/freescale/gianfar.c | 6 ++--- drivers/net/ethernet/google/gve/gve_tx.c | 4 +-- drivers/net/ethernet/hisilicon/hisi_femac.c | 2 +- drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 4 +-- .../net/ethernet/hisilicon/hns3/hns3_enet.c | 4 +-- drivers/net/ethernet/lantiq_xrx200.c | 5 ++-- drivers/net/ethernet/marvell/mv643xx_eth.c| 3 +-- drivers/net/ethernet/marvell/mvneta.c | 9 +++ drivers/net/ethernet/marvell/mvneta_bm.c | 2 +- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 +++--- .../marvell/octeontx2/nic/otx2_common.c | 2 +- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 10 drivers/net/ethernet/mellanox/mlx4/en_rx.c| 2 +- .../mellanox/mlx5/core/diag/rsc_dump.c| 2 +- .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 2 +- .../mellanox/mlx5/core/en_accel/ktls_rx.c | 2 +- .../mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +- .../net/ethernet/mellanox/mlx5/core/en_rx.c | 2 +- .../net/ethernet/mellanox/mlx5/core/en_tx.c | 6 ++--- .../net/ethernet/neterion/vxge/vxge-config.c | 6 ++--- .../net/ethernet/neterion/vxge/vxge-main.c| 6 ++--- drivers/net/ethernet/nvidia/forcedeth.c | 21 ++-- .../net/ethernet/pensando/ionic/ionic_txrx.c | 2 +- drivers/net/ethernet/qlogic/qed/qed_ll2.c | 4 +-- .../net/ethernet/qlogic/qede/qede_ethtool.c | 2 +- drivers/net/ethernet/qlogic/qede/qede_fp.c| 8 +++--- drivers/net/ethernet/realtek/r8169_main.c | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 2 +- drivers/net/ethernet/sfc/falcon/rx.c | 3 +-- drivers/net/ethernet/sfc/falcon/tx.c | 4 +-- drivers/net/ethernet/sfc/rx_common.c | 3 +-- drivers/net/ethernet/sfc/tx_common.c | 4 +-- drivers/net/ethernet/sfc/tx_tso.c | 2 +- drivers/net/ethernet/sis/sis900.c | 24 -- drivers/net/ethernet/socionext/sni_ave.c | 2 +- drivers/net/ethernet/sun/sunhme.c | 8 +++--- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 6 ++--- drivers/net/ethernet/ti/netcp_core.c | 4 +-- drivers/net/ethernet/via/via-rhine.c | 2 +- .../net/ethernet/xilinx/xilinx_axienet_main.c | 6 ++--- drivers/net/wireless/ath/ath10k/htt_rx.c | 2 +- drivers/net/wireless/ath/ath10k/pci.c | 2 +- drivers/net/wireless/ath/ath10k/snoc.c| 2 +- drivers/net/wireless/ath/ath11k/ce.c | 2 +- drivers/net/wireless/ath/ath11k/dp_rx.c | 2 +- drivers/net/wireless/ath/ath5k/base.c | 2 +- drivers/net/wireless/ath/ath9k/beacon.c | 2 +- drivers/net/wireless/ath/ath9k/recv.c | 21 +++- drivers/net/wireless/ath/ath9k/xmi
Re: [PATCH net v2] lan743x: fix rx_napi_poll/interrupt ping-pong
Am 10.12.2020 um 04:55 schrieb Sven Van Asbroeck:
> From: Sven Van Asbroeck
>
> Even if there is more rx data waiting on the chip, the rx napi poll fn
> will never run more than once - it will always read a few buffers, then
> bail out and re-arm interrupts. Which results in ping-pong between napi
> and interrupt.
>
> This defeats the purpose of napi, and is bad for performance.
>
> Fix by making the rx napi poll behave identically to other ethernet
> drivers:
> 1. initialize rx napi polling with an arbitrary budget (64).
> 2. in the polling fn, return full weight if rx queue is not depleted,
>this tells the napi core to "keep polling".
> 3. update the rx tail ("ring the doorbell") once for every 8 processed
>rx ring buffers.
>
> Thanks to Jakub Kicinski, Eric Dumazet and Andrew Lunn for their expert
> opinions and suggestions.
>
> Tested with 20 seconds of full bandwidth receive (iperf3):
> rx irqs softirqs(NET_RX)
> -
> before 2382733620
> after 129 4081
>
In addition you could play with sysfs attributes
/sys/class/net//gro_flush_timeout
/sys/class/net//napi_defer_hard_irqs
> Tested-by: Sven Van Asbroeck # lan7430
> Fixes: 23f0703c125be ("lan743x: Add main source files for new lan743x driver")
> Signed-off-by: Sven Van Asbroeck
> ---
>
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git #
> b7e4ba9a91df
>
> To: Bryan Whitehead
> To: Microchip Linux Driver Support
> To: "David S. Miller"
> To: Jakub Kicinski
> Cc: Andrew Lunn
> Cc: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> drivers/net/ethernet/microchip/lan743x_main.c | 44 ++-
> 1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 87b6c59a1e03..30ec308b9a4c 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -1964,6 +1964,14 @@ static struct sk_buff *lan743x_rx_allocate_skb(struct
> lan743x_rx *rx)
> length, GFP_ATOMIC | GFP_DMA);
> }
>
> +static void lan743x_rx_update_tail(struct lan743x_rx *rx, int index)
> +{
> + /* update the tail once per 8 descriptors */
> + if ((index & 7) == 7)
> + lan743x_csr_write(rx->adapter, RX_TAIL(rx->channel_number),
> + index);
> +}
> +
> static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index,
> struct sk_buff *skb)
> {
> @@ -1994,6 +2002,7 @@ static int lan743x_rx_init_ring_element(struct
> lan743x_rx *rx, int index,
> descriptor->data0 = (RX_DESC_DATA0_OWN_ |
> (length & RX_DESC_DATA0_BUF_LENGTH_MASK_));
> skb_reserve(buffer_info->skb, RX_HEAD_PADDING);
> + lan743x_rx_update_tail(rx, index);
>
> return 0;
> }
> @@ -2012,6 +2021,7 @@ static void lan743x_rx_reuse_ring_element(struct
> lan743x_rx *rx, int index)
> descriptor->data0 = (RX_DESC_DATA0_OWN_ |
> ((buffer_info->buffer_length) &
> RX_DESC_DATA0_BUF_LENGTH_MASK_));
> + lan743x_rx_update_tail(rx, index);
> }
>
> static void lan743x_rx_release_ring_element(struct lan743x_rx *rx, int index)
> @@ -2223,34 +2233,26 @@ static int lan743x_rx_napi_poll(struct napi_struct
> *napi, int weight)
> struct lan743x_rx *rx = container_of(napi, struct lan743x_rx, napi);
> struct lan743x_adapter *adapter = rx->adapter;
> u32 rx_tail_flags = 0;
> - int count;
> + int count, result;
>
> if (rx->vector_flags & LAN743X_VECTOR_FLAG_SOURCE_STATUS_W2C) {
> /* clear int status bit before reading packet */
> lan743x_csr_write(adapter, DMAC_INT_STS,
> DMAC_INT_BIT_RXFRM_(rx->channel_number));
> }
> - count = 0;
> - while (count < weight) {
> - int rx_process_result = lan743x_rx_process_packet(rx);
> -
> - if (rx_process_result == RX_PROCESS_RESULT_PACKET_RECEIVED) {
> - count++;
> - } else if (rx_process_result ==
> - RX_PROCESS_RESULT_NOTHING_TO_DO) {
> + for (count = 0; count < weight; count++) {
> + result = lan743x_rx_process_packet(rx);
> + if (result == RX_PROCESS_RESULT_NOTHING_TO_DO)
> break;
> - } else if (rx_process_result ==
> - RX_PROCESS_RESULT_PACKET_DROPPED) {
> - continue;
> - }
> }
> rx->frame_count += count;
> - if (count == weight)
> - goto done;
> + if (count == weight || result == RX_PROCESS_RESULT_PACKET_RECEIVED)
> + return weight;
>
> if (!napi_complete_done(napi, count))
> - goto done;
> + return count;
>
>
[PATCH] PCI: Remove pci_try_set_mwi
pci_set_mwi() and pci_try_set_mwi() do exactly the same, just that the former one is declared as __must_check. However also some callers of pci_set_mwi() have a comment that it's an optional feature. I don't think there's much sense in this separation and the use of __must_check. Therefore remove pci_try_set_mwi() and remove the __must_check attribute from pci_set_mwi(). I don't expect either function to be used in new code anyway. Signed-off-by: Heiner Kallweit --- patch applies on top of pci/misc for v5.11 --- Documentation/PCI/pci.rst | 5 + drivers/ata/pata_cs5530.c | 2 +- drivers/ata/sata_mv.c | 2 +- drivers/dma/dw/pci.c | 2 +- drivers/dma/hsu/pci.c | 2 +- drivers/ide/cs5530.c | 2 +- drivers/mfd/intel-lpss-pci.c | 2 +- drivers/net/ethernet/adaptec/starfire.c | 2 +- drivers/net/ethernet/alacritech/slicoss.c | 2 +- drivers/net/ethernet/dec/tulip/tulip_core.c | 5 + drivers/net/ethernet/sun/cassini.c| 4 ++-- drivers/net/wireless/intersil/p54/p54pci.c| 2 +- .../intersil/prism54/islpci_hotplug.c | 3 +-- .../wireless/realtek/rtl818x/rtl8180/dev.c| 2 +- drivers/pci/pci.c | 19 --- drivers/scsi/3w-9xxx.c| 4 ++-- drivers/scsi/3w-sas.c | 4 ++-- drivers/scsi/csiostor/csio_init.c | 2 +- drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/qla2xxx/qla_init.c | 8 drivers/scsi/qla2xxx/qla_mr.c | 2 +- drivers/tty/serial/8250/8250_lpss.c | 2 +- drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- drivers/usb/gadget/udc/net2280.c | 2 +- drivers/usb/gadget/udc/pch_udc.c | 2 +- include/linux/pci.h | 5 ++--- 27 files changed, 33 insertions(+), 60 deletions(-) diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst index 814b40f83..120362cc9 100644 --- a/Documentation/PCI/pci.rst +++ b/Documentation/PCI/pci.rst @@ -226,10 +226,7 @@ If the PCI device can use the PCI Memory-Write-Invalidate transaction, call pci_set_mwi(). This enables the PCI_COMMAND bit for Mem-Wr-Inval and also ensures that the cache line size register is set correctly. Check the return value of pci_set_mwi() as not all architectures -or chip-sets may support Memory-Write-Invalidate. Alternatively, -if Mem-Wr-Inval would be nice to have but is not required, call -pci_try_set_mwi() to have the system do its best effort at enabling -Mem-Wr-Inval. +or chip-sets may support Memory-Write-Invalidate. Request MMIO/IOP resources diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c index ad75d02b6..8654b3ae1 100644 --- a/drivers/ata/pata_cs5530.c +++ b/drivers/ata/pata_cs5530.c @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) } pci_set_master(cs5530_0); - pci_try_set_mwi(cs5530_0); + pci_set_mwi(cs5530_0); /* * Set PCI CacheLineSize to 16-bytes: diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c index 664ef658a..ee37755ea 100644 --- a/drivers/ata/sata_mv.c +++ b/drivers/ata/sata_mv.c @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, mv_print_info(host); pci_set_master(pdev); - pci_try_set_mwi(pdev); + pci_set_mwi(pdev); return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED, IS_GEN_I(hpriv) ? _sht : _sht); } diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index 1142aa6f8..1c20b7485 100644 --- a/drivers/dma/dw/pci.c +++ b/drivers/dma/dw/pci.c @@ -30,7 +30,7 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid) } pci_set_master(pdev); - pci_try_set_mwi(pdev); + pci_set_mwi(pdev); ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); if (ret) diff --git a/drivers/dma/hsu/pci.c b/drivers/dma/hsu/pci.c index 07cc7320a..420dd3706 100644 --- a/drivers/dma/hsu/pci.c +++ b/drivers/dma/hsu/pci.c @@ -73,7 +73,7 @@ static int hsu_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) } pci_set_master(pdev); - pci_try_set_mwi(pdev); + pci_set_mwi(pdev); ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); if (ret) diff --git a/drivers/ide/cs5530.c b/drivers/ide/cs5530.c index 5bb46e713..5d2c421ab 100644 --- a/drivers/ide/cs5530.c +++ b/drivers/ide/cs5530.c @@ -168,7 +168,7 @@ static int init_chipset_cs5530(struct pci_dev *dev) */ pci_set_master(cs5530_0); - pci_try_set_mwi(cs5530_0); + pci_set_mwi(cs5530_0); /* * Set PCI CacheLineSize to 16-bytes: diff --git a/drivers/mfd/in
Re: [PATCH 2/2] PCI/ASPM: Use capability to override ASPM via sysfs
Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng: > If we are to use sysfs to change ASPM settings, we may want to override > the default ASPM policy. > > So use ASPM capability, instead of default policy, to be able to use all > possible ASPM states. > > Signed-off-by: Kai-Heng Feng > --- > drivers/pci/pcie/aspm.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 2ea9fddadfad..326da7bbc84d 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -1239,8 +1239,7 @@ static ssize_t aspm_attr_store_common(struct device > *dev, > > link->aspm_disable |= state; > } > - > - pcie_config_aspm_link(link, policy_to_aspm_state(link)); > + pcie_config_aspm_link(link, link->aspm_capable); > I like the idea, because the policy can be changed by the user anyway. Therefore I don't see it as a hard system limit. However I think this change is not sufficient. Each call to pcie_config_aspm_link(link, policy_to_aspm_state(link)), e.g. in path pcie_aspm_pm_state_change -> pcie_config_aspm_path will reset the enabled states to the policy. > mutex_unlock(_lock); > up_read(_bus_sem); >
Re: [PATCH 1/2] PCI/ASPM: Store disabled ASPM states
Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng:
> If we use sysfs to disable L1 ASPM, then enable one L1 ASPM substate
> again, all other substates will also be enabled too:
>
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:1
> l1_1_pcipm:1
> l1_2_aspm:1
> l1_2_pcipm:1
> l1_aspm:1
>
> link# echo 0 > l1_aspm
>
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:0
> l1_1_pcipm:0
> l1_2_aspm:0
> l1_2_pcipm:0
> l1_aspm:0
>
> link# echo 1 > l1_2_aspm
>
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:1
> l1_1_pcipm:1
> l1_2_aspm:1
> l1_2_pcipm:1
> l1_aspm:1
>
> This is because disabled ASPM states weren't saved, so enable any of the
> substate will also enable others.
>
> So store the disabled ASPM states for consistency.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/pci/pcie/aspm.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index ac0557a305af..2ea9fddadfad 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> *link, int blacklist)
> /* Setup initial capable state. Will be updated later */
> link->aspm_capable = link->aspm_support;
>
> + link->aspm_disable = link->aspm_capable & ~link->aspm_default;
> +
This makes sense only in combination with patch 2. However I think patch 1
should be independent of patch 2. Especially if we consider patch 1 a fix
that is applied to stable whilst patch 2 is an improvement for next.
> /* Get and check endpoint acceptable latencies */
> list_for_each_entry(child, >devices, bus_list) {
> u32 reg32, encoding;
> @@ -1226,11 +1228,15 @@ static ssize_t aspm_attr_store_common(struct device
> *dev,
> mutex_lock(_lock);
>
> if (state_enable) {
> - link->aspm_disable &= ~state;
> /* need to enable L1 for substates */
> if (state & ASPM_STATE_L1SS)
> - link->aspm_disable &= ~ASPM_STATE_L1;
> + state |= ASPM_STATE_L1;
> +
> + link->aspm_disable &= ~state;
I don't see what this part of the patch changes. Can you elaborate on why
this is needed?
> } else {
> + if (state == ASPM_STATE_L1)
> + state |= ASPM_STATE_L1SS;
> +
I think this part should be sufficient to fix the behavior. because what
I think currently happens:
1. original status: policy powersupersave, nothing disabled -> L1 + L1SS active
2. disable L1: L1 disabled, pcie_config_aspm_link() disabled L1 and L1SS
w/o adding L1SS to link-> aspm_disabled
3. enable one L1SS state: aspm_attr_store_common() removes L1 from
link->aspm_disabled -> link->aspm_disabled is empty, resulting in
L1 + L1SS being active
> link->aspm_disable |= state;
> }
>
>
[PATCH] x86/reboot/quirks: Add Zotac ZBOX CI327 nano PCI reboot quirk
On this system the M.2 PCIe WiFi card isn't detected after reboot,
only after cold boot. reboot=pci fixes this behavior.
In [0] the same issue is described, although on another system and
with another Intel WiFi card. In case it's relevant, both systems
have Celeron CPU's.
The dicussion in [0] involved the PCI maintainer, and proposal was
to go with the PCI reboot quirk on affected systems until a more
generic fix is available.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=202399
Signed-off-by: Heiner Kallweit
---
arch/x86/kernel/reboot.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index db115943e..9991c5920 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -477,6 +477,15 @@ static const struct dmi_system_id reboot_dmi_table[]
__initconst = {
},
},
+ { /* PCIe Wifi card isn't detected after reboot otherwise */
+ .callback = set_pci_reboot,
+ .ident = "Zotac ZBOX CI327 nano",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "NA"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "ZBOX-CI327NANO-GS-01"),
+ },
+ },
+
/* Sony */
{ /* Handle problems with rebooting on Sony VGN-Z540N */
.callback = set_bios_reboot,
--
2.29.2
Re: [PATCH] mlxsw: switch from 'pci_' to 'dma_' API
Am 29.11.2020 um 22:17 schrieb Christophe JAILLET:
> he wrappers in include/linux/pci-dma-compat.h should go away.
>
> The patch has been generated with the coccinelle script below and has been
> hand modified to replace GFP_ with a correct flag.
> It has been compile tested.
>
> When memory is allocated in 'mlxsw_pci_queue_init()' and
> 'mlxsw_pci_fw_area_init()' GFP_KERNEL can be used because this flag is
> already used in the same function.
>
> When memory is allocated in 'mlxsw_pci_mbox_alloc()' GFP_KERNEL can be
> used because it is only called from a probe function. The call chain is:
> --> mlxsw_pci_probe
> --> mlxsw_pci_cmd_init
> --> mlxsw_pci_mbox_alloc
>
> @@
> @@
> -PCI_DMA_BIDIRECTIONAL
> +DMA_BIDIRECTIONAL
>
> @@
> @@
> -PCI_DMA_TODEVICE
> +DMA_TO_DEVICE
>
> @@
> @@
> -PCI_DMA_FROMDEVICE
> +DMA_FROM_DEVICE
>
> @@
> @@
> -PCI_DMA_NONE
> +DMA_NONE
>
> @@
> expression e1, e2, e3;
> @@
> -pci_alloc_consistent(e1, e2, e3)
> +dma_alloc_coherent(>dev, e2, e3, GFP_)
>
> @@
> expression e1, e2, e3;
> @@
> -pci_zalloc_consistent(e1, e2, e3)
> +dma_alloc_coherent(>dev, e2, e3, GFP_)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_free_consistent(e1, e2, e3, e4)
> +dma_free_coherent(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_map_single(e1, e2, e3, e4)
> +dma_map_single(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_unmap_single(e1, e2, e3, e4)
> +dma_unmap_single(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4, e5;
> @@
> -pci_map_page(e1, e2, e3, e4, e5)
> +dma_map_page(>dev, e2, e3, e4, e5)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_unmap_page(e1, e2, e3, e4)
> +dma_unmap_page(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_map_sg(e1, e2, e3, e4)
> +dma_map_sg(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_unmap_sg(e1, e2, e3, e4)
> +dma_unmap_sg(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_dma_sync_single_for_cpu(e1, e2, e3, e4)
> +dma_sync_single_for_cpu(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_dma_sync_single_for_device(e1, e2, e3, e4)
> +dma_sync_single_for_device(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_dma_sync_sg_for_cpu(e1, e2, e3, e4)
> +dma_sync_sg_for_cpu(>dev, e2, e3, e4)
>
> @@
> expression e1, e2, e3, e4;
> @@
> -pci_dma_sync_sg_for_device(e1, e2, e3, e4)
> +dma_sync_sg_for_device(>dev, e2, e3, e4)
>
> @@
> expression e1, e2;
> @@
> -pci_dma_mapping_error(e1, e2)
> +dma_mapping_error(>dev, e2)
>
> @@
> expression e1, e2;
> @@
> -pci_set_dma_mask(e1, e2)
> +dma_set_mask(>dev, e2)
>
> @@
> expression e1, e2;
> @@
> -pci_set_consistent_dma_mask(e1, e2)
> +dma_set_coherent_mask(>dev, e2)
>
> Signed-off-by: Christophe JAILLET
> ---
> If needed, see post from Christoph Hellwig on the kernel-janitors ML:
>https://marc.info/?l=kernel-janitors=158745678307186=4
> ---
> drivers/net/ethernet/mellanox/mlxsw/pci.c | 52 +++
> 1 file changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c
> b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> index 641cdd81882b..7519d3b6934e 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> @@ -323,8 +323,8 @@ static int mlxsw_pci_wqe_frag_map(struct mlxsw_pci
> *mlxsw_pci, char *wqe,
> struct pci_dev *pdev = mlxsw_pci->pdev;
> dma_addr_t mapaddr;
>
> - mapaddr = pci_map_single(pdev, frag_data, frag_len, direction);
> - if (unlikely(pci_dma_mapping_error(pdev, mapaddr))) {
> + mapaddr = dma_map_single(>dev, frag_data, frag_len, direction);
> + if (unlikely(dma_mapping_error(>dev, mapaddr))) {
> dev_err_ratelimited(>dev, "failed to dma map tx frag\n");
> return -EIO;
> }
> @@ -342,7 +342,7 @@ static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci
> *mlxsw_pci, char *wqe,
>
> if (!frag_len)
> return;
> - pci_unmap_single(pdev, mapaddr, frag_len, direction);
> + dma_unmap_single(>dev, mapaddr, frag_len, direction);
> }
>
> static int mlxsw_pci_rdq_skb_alloc(struct mlxsw_pci *mlxsw_pci,
> @@ -858,9 +858,9 @@ static int mlxsw_pci_queue_init(struct mlxsw_pci
> *mlxsw_pci, char *mbox,
> tasklet_setup(>tasklet, q_ops->tasklet);
>
> mem_item->size = MLXSW_PCI_AQ_SIZE;
> - mem_item->buf = pci_alloc_consistent(mlxsw_pci->pdev,
> - mem_item->size,
> - _item->mapaddr);
> + mem_item->buf = dma_alloc_coherent(_pci->pdev->dev,
> +mem_item->size, _item->mapaddr,
> +GFP_KERNEL);
> if (!mem_item->buf)
>
Re: [PATCH v2] net: phy: realtek: read actual speed on rtl8211f to detect downshift
Am 24.11.2020 um 23:33 schrieb Antonio Borneo:
> On Tue, 2020-11-24 at 23:22 +0100, Heiner Kallweit wrote:
>> Am 24.11.2020 um 22:59 schrieb Antonio Borneo:
>>> The rtl8211f supports downshift and before commit 5502b218e001
>>> ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
>>> the read-back of register MII_CTRL1000 was used to detect the
>>> negotiated link speed.
>>> The code added in commit d445dff2df60 ("net: phy: realtek: read
>>> actual speed to detect downshift") is working fine also for this
>>> phy and it's trivial re-using it to restore the downshift
>>> detection on rtl8211f.
>>>
>>> Add the phy specific read_status() pointing to the existing
>>> function rtlgen_read_status().
>>>
>>> Signed-off-by: Antonio Borneo
>>> Link:
>>> https://lore.kernel.org/r/478f871a-583d-01f1-9cc5-2eea56d8c...@huawei.com
>>> ---
>>> To: Andrew Lunn
>>> To: Heiner Kallweit
>>> To: Russell King
>>> To: "David S. Miller"
>>> To: Jakub Kicinski
>>> To: net...@vger.kernel.org
>>> To: Yonglong Liu
>>> To: Willy Liu
>>> Cc: linux...@huawei.com
>>> Cc: Salil Mehta
>>> Cc: linux-st...@st-md-mailman.stormreply.com
>>> Cc: linux-kernel@vger.kernel.org
>>> In-Reply-To: <20201124143848.874894-1-antonio.bor...@st.com>
>>>
>>> V1 => V2
>>> move from a generic implementation affecting every phy
>>> to a rtl8211f specific implementation
>>> ---
>>> drivers/net/phy/realtek.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index 575580d3ffe0..8ff8a4edc173 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -621,6 +621,7 @@ static struct phy_driver realtek_drvs[] = {
>>> PHY_ID_MATCH_EXACT(0x001cc916),
>>> .name = "RTL8211F Gigabit Ethernet",
>>> .config_init= _config_init,
>>> + .read_status= rtlgen_read_status,
>>> .ack_interrupt = _ack_interrupt,
>>> .config_intr= _config_intr,
>>> .suspend= genphy_suspend,
>>>
>>> base-commit: 9bd2702d292cb7b565b09e949d30288ab7a26d51
>>>
>>
>> Pefect would be to make this a fix for 5502b218e001,
>> but rtlgen_read_status() was added one year after this change.
>> Marking the change that added rtlgen_read_status() as "Fixes"
>> would be technically ok, but as it's not actually broken not
>> everybody may be happy with this.
>> Having said that I'd be fine with treating this as an improvement,
>> downshift should be a rare case.
>
> Correct! Being the commit that adds rtlgen_read_status() an improvement,
> should not be backported, so this patch is not marked anymore as a fix!
> Plus, this does not fix 5502b218e001 in the general case, but limited to
> one specific phy, making the 'fixes' label less relevant.
> Anyway, the commit message reports all the ingredients for a backport.
>
> By the way, I have incorrectly sent this based on netdev, but it's not a
> fix anymore! Should I rebase it on netdev-next and resend?
>
For this small change it shouldn't make a difference whether it's based
on net or net-next. I don't think anything has changed here. But better
check whether patch applies cleanly on net-next. Patch should have been
annotated as [PATCH net-next], but I think a re-send isn't needed as
Jakub can see it based on this communication.
> Antonio
>
Re: [PATCH v2] net: phy: realtek: read actual speed on rtl8211f to detect downshift
Am 24.11.2020 um 22:59 schrieb Antonio Borneo:
> The rtl8211f supports downshift and before commit 5502b218e001
> ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
> the read-back of register MII_CTRL1000 was used to detect the
> negotiated link speed.
> The code added in commit d445dff2df60 ("net: phy: realtek: read
> actual speed to detect downshift") is working fine also for this
> phy and it's trivial re-using it to restore the downshift
> detection on rtl8211f.
>
> Add the phy specific read_status() pointing to the existing
> function rtlgen_read_status().
>
> Signed-off-by: Antonio Borneo
> Link:
> https://lore.kernel.org/r/478f871a-583d-01f1-9cc5-2eea56d8c...@huawei.com
> ---
> To: Andrew Lunn
> To: Heiner Kallweit
> To: Russell King
> To: "David S. Miller"
> To: Jakub Kicinski
> To: net...@vger.kernel.org
> To: Yonglong Liu
> To: Willy Liu
> Cc: linux...@huawei.com
> Cc: Salil Mehta
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-kernel@vger.kernel.org
> In-Reply-To: <20201124143848.874894-1-antonio.bor...@st.com>
>
> V1 => V2
> move from a generic implementation affecting every phy
> to a rtl8211f specific implementation
> ---
> drivers/net/phy/realtek.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 575580d3ffe0..8ff8a4edc173 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -621,6 +621,7 @@ static struct phy_driver realtek_drvs[] = {
> PHY_ID_MATCH_EXACT(0x001cc916),
> .name = "RTL8211F Gigabit Ethernet",
> .config_init= _config_init,
> + .read_status= rtlgen_read_status,
> .ack_interrupt = _ack_interrupt,
> .config_intr= _config_intr,
> .suspend= genphy_suspend,
>
> base-commit: 9bd2702d292cb7b565b09e949d30288ab7a26d51
>
Pefect would be to make this a fix for 5502b218e001,
but rtlgen_read_status() was added one year after this change.
Marking the change that added rtlgen_read_status() as "Fixes"
would be technically ok, but as it's not actually broken not
everybody may be happy with this.
Having said that I'd be fine with treating this as an improvement,
downshift should be a rare case.
Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'
Am 24.11.2020 um 16:17 schrieb Antonio Borneo:
> On Tue, 2020-11-24 at 14:56 +, Russell King - ARM Linux admin wrote:
>> On Tue, Nov 24, 2020 at 03:38:48PM +0100, Antonio Borneo wrote:
>>> If the auto-negotiation fails to establish a gigabit link, the phy
>>> can try to 'down-shift': it resets the bits in MII_CTRL1000 to
>>> stop advertising 1Gbps and retries the negotiation at 100Mbps.
>>>
>>> From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
>>> in genphy_read_status") the content of MII_CTRL1000 is not checked
>>> anymore at the end of the negotiation, preventing the detection of
>>> phy 'down-shift'.
>>> In case of 'down-shift' phydev->advertising gets out-of-sync wrt
>>> MII_CTRL1000 and still includes modes that the phy have already
>>> dropped. The link partner could still advertise higher speeds,
>>> while the link is established at one of the common lower speeds.
>>> The logic 'and' in phy_resolve_aneg_linkmode() between
>>> phydev->advertising and phydev->lp_advertising will report an
>>> incorrect mode.
>>>
>>> Issue detected with a local phy rtl8211f connected with a gigabit
>>> capable router through a two-pairs network cable.
>>>
>>> After auto-negotiation, read back MII_CTRL1000 and mask-out from
>>> phydev->advertising the modes that have been eventually discarded
>>> due to the 'down-shift'.
>>
>> Sorry, but no. While your solution will appear to work, in
>> introduces unexpected changes to the user visible APIs.
>>
>>> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>>> + if (phydev->is_gigabit_capable) {
>>> + adv = phy_read(phydev, MII_CTRL1000);
>>> + if (adv < 0)
>>> + return adv;
>>> + /* update advertising in case of 'down-shift' */
>>> + mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
>>> + adv);
>>
>> If a down-shift occurs, this will cause the configured advertising
>> mask to lose the 1G speed, which will be visible to userspace.
>
> You are right, it gets propagated to user that 1Gbps is not advertised
>
>> Userspace doesn't expect the advertising mask to change beneath it.
>> Since updates from userspace are done using a read-modify-write of
>> the ksettings, this can have the undesired effect of removing 1G
>> from the configured advertising mask.
>>
>> We've had other PHYs have this behaviour; the correct solution is for
>> the PHY driver to implement reading the resolution from the PHY rather
>> than relying on the generic implementation if it can down-shift
>
> If it's already upstream, could you please point to one of the phy driver
> that already implements this properly?
>
See e.g. aqr107_read_rate(), used by aqr107_read_status().
> Thanks
> Antonio
>
Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'
Am 24.11.2020 um 15:38 schrieb Antonio Borneo:
> If the auto-negotiation fails to establish a gigabit link, the phy
> can try to 'down-shift': it resets the bits in MII_CTRL1000 to
> stop advertising 1Gbps and retries the negotiation at 100Mbps.
>
I see that Russell answered already. My 2cts:
Are you sure all PHY's supporting downshift adjust the
advertisement bits? IIRC an Aquantia PHY I dealt with does not.
And if a PHY does so I'd consider this problematic:
Let's say you have a broken cable and the PHY downshifts to
100Mbps. If you change the cable then the PHY would still negotiate
100Mbps only.
Also I think phydev->advertising reflects what the user wants to
advertise, as mentioned by Russell before.
>>From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
> in genphy_read_status") the content of MII_CTRL1000 is not checked
> anymore at the end of the negotiation, preventing the detection of
> phy 'down-shift'.
> In case of 'down-shift' phydev->advertising gets out-of-sync wrt
> MII_CTRL1000 and still includes modes that the phy have already
> dropped. The link partner could still advertise higher speeds,
> while the link is established at one of the common lower speeds.
> The logic 'and' in phy_resolve_aneg_linkmode() between
> phydev->advertising and phydev->lp_advertising will report an
> incorrect mode.
>
> Issue detected with a local phy rtl8211f connected with a gigabit
> capable router through a two-pairs network cable.
>
> After auto-negotiation, read back MII_CTRL1000 and mask-out from
> phydev->advertising the modes that have been eventually discarded
> due to the 'down-shift'.
>
> Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
> genphy_read_status")
> Cc: sta...@vger.kernel.org # v5.1+
> Signed-off-by: Antonio Borneo
> Link:
> https://lore.kernel.org/r/478f871a-583d-01f1-9cc5-2eea56d8c...@huawei.com
> ---
> To: Andrew Lunn
> To: Heiner Kallweit
> To: Russell King
> To: "David S. Miller"
> To: Jakub Kicinski
> To: net...@vger.kernel.org
> To: Yonglong Liu
> Cc: linux...@huawei.com
> Cc: Salil Mehta
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-kernel@vger.kernel.org
> Cc: Antonio Borneo
>
> drivers/net/phy/phy_device.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5dab6be6fc38..5d1060aa1b25 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2331,7 +2331,7 @@ EXPORT_SYMBOL(genphy_read_status_fixed);
> */
> int genphy_read_status(struct phy_device *phydev)
> {
> - int err, old_link = phydev->link;
> + int adv, err, old_link = phydev->link;
>
> /* Update the link, but return if there was an error */
> err = genphy_update_link(phydev);
> @@ -2356,6 +2356,14 @@ int genphy_read_status(struct phy_device *phydev)
> return err;
>
> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> + if (phydev->is_gigabit_capable) {
> + adv = phy_read(phydev, MII_CTRL1000);
> + if (adv < 0)
> + return adv;
> + /* update advertising in case of 'down-shift' */
> + mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
> + adv);
> + }
> phy_resolve_aneg_linkmode(phydev);
> } else if (phydev->autoneg == AUTONEG_DISABLE) {
> err = genphy_read_status_fixed(phydev);
>
> base-commit: d549699048b4b5c22dd710455bcdb76966e55aa3
>
[PATCH net-next] net: warn if gso_type isn't set for a GSO SKB
In bug report [0] a warning in r8169 driver was reported that was
caused by an invalid GSO SKB (gso_type was 0). See [1] for a discussion
about this issue. Still the origin of the invalid GSO SKB isn't clear.
It shouldn't be a network drivers task to check for invalid GSO SKB's.
Also, even if issue [0] can be fixed, we can't be sure that a
similar issue doesn't pop up again at another place.
Therefore let gso_features_check() check for such invalid GSO SKB's.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=209423
[1] https://www.spinics.net/lists/netdev/msg690794.html
Signed-off-by: Heiner Kallweit
---
net/core/dev.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 4bfdcd6b2..3c3070d9d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3495,6 +3495,11 @@ static netdev_features_t gso_features_check(const struct
sk_buff *skb,
if (gso_segs > dev->gso_max_segs)
return features & ~NETIF_F_GSO_MASK;
+ if (!skb_shinfo(skb)->gso_type) {
+ skb_warn_bad_offload(skb);
+ return features & ~NETIF_F_GSO_MASK;
+ }
+
/* Support for GSO partial features requires software
* intervention before we can actually process the packets
* so we need to strip support for any partial features now
--
2.29.2
Re: [PATCH v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER
Am 19.11.2020 um 22:41 schrieb Andrew Lunn: Doesn't checkpatch complain about line length > 80 here? >>> >>> :) >>> >>> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144 >>> Author: Joe Perches >>> Date: Fri May 29 16:12:21 2020 -0700 >>> >>> checkpatch/coding-style: deprecate 80-column warning >>> >> >> Ah, again something learnt. Thanks for the reference. > > But it then got revoked for netdev. Or at least it was planned to > re-impose 80 for netdev. I don't know if checkpatch got patched yet. > > Andrew > At a first glance it sounds strange that subsystems may define own rules for such basic things. But supposedly there has been a longer emotional disucssion about this already ..
Re: [PATCH v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER
Am 19.11.2020 um 22:17 schrieb Grygorii Strashko:
>
>
> On 19/11/2020 23:11, Heiner Kallweit wrote:
>> Am 19.11.2020 um 21:34 schrieb Grygorii Strashko:
>>> The mdio_bus may have dependencies from GPIO controller and so got
>>> deferred. Now it will print error message every time -EPROBE_DEFER is
>>> returned which from:
>>> __mdiobus_register()
>>> |-devm_gpiod_get_optional()
>>> without actually identifying error code.
>>>
>>> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"
>>>
>>> Hence, suppress error message for devm_gpiod_get_optional() returning
>>> -EPROBE_DEFER case by using dev_err_probe().
>>>
>>> Signed-off-by: Grygorii Strashko
>>> ---
>>> drivers/net/phy/mdio_bus.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>>> index 757e950fb745..83cd61c3dd01 100644
>>> --- a/drivers/net/phy/mdio_bus.c
>>> +++ b/drivers/net/phy/mdio_bus.c
>>> @@ -546,10 +546,10 @@ int __mdiobus_register(struct mii_bus *bus, struct
>>> module *owner)
>>> /* de-assert bus level PHY GPIO reset */
>>> gpiod = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
>>> if (IS_ERR(gpiod)) {
>>> - dev_err(>dev, "mii_bus %s couldn't get reset GPIO\n",
>>> - bus->id);
>>> + err = dev_err_probe(>dev, PTR_ERR(gpiod),
>>> + "mii_bus %s couldn't get reset GPIO\n", bus->id);
>>
>> Doesn't checkpatch complain about line length > 80 here?
>
> :)
>
> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> Author: Joe Perches
> Date: Fri May 29 16:12:21 2020 -0700
>
> checkpatch/coding-style: deprecate 80-column warning
>
Ah, again something learnt. Thanks for the reference.
>>
>>> device_del(>dev);
>>> - return PTR_ERR(gpiod);
>>> + return err;
>>> } else if (gpiod) {
>>> bus->reset_gpiod = gpiod;
>>>
>>
>> Last but not least the net or net-next patch annotation is missing.
>> I'd be fine with treating the change as an improvement (net-next).
>>
>> Apart from that change looks good to me.
>>
>
Re: [PATCH v2] mdio_bus: suppress err message for reset gpio EPROBE_DEFER
Am 19.11.2020 um 21:34 schrieb Grygorii Strashko:
> The mdio_bus may have dependencies from GPIO controller and so got
> deferred. Now it will print error message every time -EPROBE_DEFER is
> returned which from:
> __mdiobus_register()
> |-devm_gpiod_get_optional()
> without actually identifying error code.
>
> "mdio_bus 4a101000.mdio: mii_bus 4a101000.mdio couldn't get reset GPIO"
>
> Hence, suppress error message for devm_gpiod_get_optional() returning
> -EPROBE_DEFER case by using dev_err_probe().
>
> Signed-off-by: Grygorii Strashko
> ---
> drivers/net/phy/mdio_bus.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 757e950fb745..83cd61c3dd01 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -546,10 +546,10 @@ int __mdiobus_register(struct mii_bus *bus, struct
> module *owner)
> /* de-assert bus level PHY GPIO reset */
> gpiod = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(gpiod)) {
> - dev_err(>dev, "mii_bus %s couldn't get reset GPIO\n",
> - bus->id);
> + err = dev_err_probe(>dev, PTR_ERR(gpiod),
> + "mii_bus %s couldn't get reset GPIO\n",
> bus->id);
Doesn't checkpatch complain about line length > 80 here?
> device_del(>dev);
> - return PTR_ERR(gpiod);
> + return err;
> } else if (gpiod) {
> bus->reset_gpiod = gpiod;
>
>
Last but not least the net or net-next patch annotation is missing.
I'd be fine with treating the change as an improvement (net-next).
Apart from that change looks good to me.
