* [PATCH net 4/7] tcp: add small random increments to the source port
From: Willy Tarreau @ 2022-04-27 6:52 UTC (permalink / raw)
To: netdev
Cc: David Miller, Jakub Kicinski, Eric Dumazet, Moshe Kol,
Yossi Gilad, Amit Klein, linux-kernel, Willy Tarreau
In-Reply-To: <20220427065233.2075-1-w@1wt.eu>
Here we're randomly adding between 0 and 7 random increments to the
selected source port in order to add some noise in the source port
selection that will make the next port less predictable.
With the default port range of 32768-60999 this means a worst case
reuse scenario of 14116/8=1764 connections between two consecutive
uses of the same port, with an average of 14116/4.5=3137. This code
was stressed at more than 800000 connections per second to a fixed
target with all connections closed by the client using RSTs (worst
condition) and only 2 connections failed among 13 billion, despite
the hash being reseeded every 10 seconds, indicating a perfectly
safe situation.
Cc: Moshe Kol <moshe.kol@mail.huji.ac.il>
Cc: Yossi Gilad <yossi.gilad@mail.huji.ac.il>
Cc: Amit Klein <aksecurity@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
net/ipv4/inet_hashtables.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 747f272da25b..f58c5caf3130 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -831,11 +831,12 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
return -EADDRNOTAVAIL;
ok:
- /* If our first attempt found a candidate, skip next candidate
- * in 1/16 of cases to add some noise.
+ /* Here we want to add a little bit of randomness to the next source
+ * port that will be chosen. We use a max() with a random here so that
+ * on low contention the randomness is maximal and on high contention
+ * it may be inexistent.
*/
- if (!i && !(prandom_u32() % 16))
- i = 2;
+ i = max_t(int, i, (prandom_u32() & 7) * 2);
WRITE_ONCE(table_perturb[index], READ_ONCE(table_perturb[index]) + i + 2);
/* Head lock still held and bh's disabled */
--
2.17.5
^ permalink raw reply related
* [PATCH net 2/7] tcp: use different parts of the port_offset for index and offset
From: Willy Tarreau @ 2022-04-27 6:52 UTC (permalink / raw)
To: netdev
Cc: David Miller, Jakub Kicinski, Eric Dumazet, Moshe Kol,
Yossi Gilad, Amit Klein, linux-kernel, Willy Tarreau,
Jason A . Donenfeld
In-Reply-To: <20220427065233.2075-1-w@1wt.eu>
Amit Klein suggests that we use different parts of port_offset for the
table's index and the port offset so that there is no direct relation
between them.
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Moshe Kol <moshe.kol@mail.huji.ac.il>
Cc: Yossi Gilad <yossi.gilad@mail.huji.ac.il>
Cc: Amit Klein <aksecurity@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
net/ipv4/inet_hashtables.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 09cbad0488ca..747f272da25b 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -777,7 +777,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
net_get_random_once(table_perturb, sizeof(table_perturb));
index = hash_32(port_offset, INET_TABLE_PERTURB_SHIFT);
- offset = (READ_ONCE(table_perturb[index]) + port_offset) % remaining;
+ offset = (READ_ONCE(table_perturb[index]) + (port_offset >> 32)) % remaining;
/* In first pass we try ports of @low parity.
* inet_csk_get_port() does the opposite choice.
*/
--
2.17.5
^ permalink raw reply related
* [PATCH net 1/7] secure_seq: return the full 64-bit of the siphash
From: Willy Tarreau @ 2022-04-27 6:52 UTC (permalink / raw)
To: netdev
Cc: David Miller, Jakub Kicinski, Eric Dumazet, Moshe Kol,
Yossi Gilad, Amit Klein, linux-kernel, Willy Tarreau,
Jason A . Donenfeld
In-Reply-To: <20220427065233.2075-1-w@1wt.eu>
SipHash replaced MD5 in secure_ipv4_port_ephemeral() via commit
7cd23e5300c1 ("secure_seq: use SipHash in place of MD5"), but the
output remained truncated to 32-bit only. In order to exploit more
bits from the hash, let's make the function return the full 64-bit
of siphash_3u32().
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Moshe Kol <moshe.kol@mail.huji.ac.il>
Cc: Yossi Gilad <yossi.gilad@mail.huji.ac.il>
Cc: Amit Klein <aksecurity@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
include/net/inet_hashtables.h | 2 +-
include/net/secure_seq.h | 2 +-
net/core/secure_seq.c | 2 +-
net/ipv4/inet_hashtables.c | 6 +++---
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index f72ec113ae56..98e1ec1a14f0 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -425,7 +425,7 @@ static inline void sk_rcv_saddr_set(struct sock *sk, __be32 addr)
}
int __inet_hash_connect(struct inet_timewait_death_row *death_row,
- struct sock *sk, u32 port_offset,
+ struct sock *sk, u64 port_offset,
int (*check_established)(struct inet_timewait_death_row *,
struct sock *, __u16,
struct inet_timewait_sock **));
diff --git a/include/net/secure_seq.h b/include/net/secure_seq.h
index d7d2495f83c2..5cea9ed9c773 100644
--- a/include/net/secure_seq.h
+++ b/include/net/secure_seq.h
@@ -4,7 +4,7 @@
#include <linux/types.h>
-u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport);
+u64 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport);
u32 secure_ipv6_port_ephemeral(const __be32 *saddr, const __be32 *daddr,
__be16 dport);
u32 secure_tcp_seq(__be32 saddr, __be32 daddr,
diff --git a/net/core/secure_seq.c b/net/core/secure_seq.c
index 9b8443774449..2cdd43a63f64 100644
--- a/net/core/secure_seq.c
+++ b/net/core/secure_seq.c
@@ -142,7 +142,7 @@ u32 secure_tcp_seq(__be32 saddr, __be32 daddr,
}
EXPORT_SYMBOL_GPL(secure_tcp_seq);
-u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport)
+u64 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport)
{
net_secret_init();
return siphash_3u32((__force u32)saddr, (__force u32)daddr,
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 17440840a791..09cbad0488ca 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -504,7 +504,7 @@ static int __inet_check_established(struct inet_timewait_death_row *death_row,
return -EADDRNOTAVAIL;
}
-static u32 inet_sk_port_offset(const struct sock *sk)
+static u64 inet_sk_port_offset(const struct sock *sk)
{
const struct inet_sock *inet = inet_sk(sk);
@@ -734,7 +734,7 @@ EXPORT_SYMBOL_GPL(inet_unhash);
static u32 table_perturb[1 << INET_TABLE_PERTURB_SHIFT];
int __inet_hash_connect(struct inet_timewait_death_row *death_row,
- struct sock *sk, u32 port_offset,
+ struct sock *sk, u64 port_offset,
int (*check_established)(struct inet_timewait_death_row *,
struct sock *, __u16, struct inet_timewait_sock **))
{
@@ -859,7 +859,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
int inet_hash_connect(struct inet_timewait_death_row *death_row,
struct sock *sk)
{
- u32 port_offset = 0;
+ u64 port_offset = 0;
if (!inet_sk(sk)->inet_num)
port_offset = inet_sk_port_offset(sk);
--
2.17.5
^ permalink raw reply related
* [PATCH net 0/7] insufficient TCP source port randomness
From: Willy Tarreau @ 2022-04-27 6:52 UTC (permalink / raw)
To: netdev
Cc: David Miller, Jakub Kicinski, Eric Dumazet, Moshe Kol,
Yossi Gilad, Amit Klein, linux-kernel, Willy Tarreau
Hi,
In a not-yet published paper, Moshe Kol, Amit Klein, and Yossi Gilad
report being able to accurately identify a client by forcing it to emit
only 40 times more connections than the number of entries in the
table_perturb[] table, which is indexed by hashing the connection tuple.
The current 2^8 setting allows them to perform that attack with only 10k
connections, which is not hard to achieve in a few seconds.
Eric, Amit and I have been working on this for a few weeks now imagining,
testing and eliminating a number of approaches that Amit and his team were
still able to break or that were found to be too risky or too expensive,
and ended up with the simple improvements in this series that resists to
the attack, doesn't degrade the performance, and preserves a reliable port
selection algorithm to avoid connection failures, including the odd/even
port selection preference that allows bind() to always find a port quickly
even under strong connect() stress.
The approach relies on several factors:
- resalting the hash secret that's used to choose the table_perturb[]
entry every 10 seconds to eliminate slow attacks and force the
attacker to forget everything that was learned after this delay.
This already eliminates most of the problem because if a client
stays silent for more than 10 seconds there's no link between the
previous and the next patterns, and 10s isn't yet frequent enough
to cause too frequent repetition of a same port that may induce a
connection failure ;
- adding small random increments to the source port. Previously, a
random 0 or 1 was added every 16 ports. Now a random 0 to 7 is
added after each port. This means that with the default 32768-60999
range, a worst case rollover happens after 1764 connections, and
an average of 3137. This doesn't stop statistical attacks but
requires significantly more iterations of the same attack to
confirm a guess.
- increasing the table_perturb[] size from 2^8 to 2^16, which Amit
says will require 2.6 million connections to be attacked with the
changes above, making it pointless to get a fingerprint that will
only last 10 seconds. Due to the size, the table was made dynamic.
- a few minor improvements on the bits used from the hash, to eliminate
some unfortunate correlations that may possibly have been exploited
to design future attack models.
These changes were tested under the most extreme conditions, up to
1.1 million connections per second to one and a few targets, showing no
performance regression, and only 2 connection failures within 13 billion,
which is less than 2^-32 and perfectly within usual values.
The series is split into small reviewable changes and was already reviewed
by Amit and Eric.
Regards,
Willy
---
Eric Dumazet (1):
tcp: resalt the secret every 10 seconds
Willy Tarreau (6):
secure_seq: return the full 64-bit of the siphash
tcp: use different parts of the port_offset for index and offset
tcp: add small random increments to the source port
tcp: dynamically allocate the perturb table used by source ports
tcp: increase source port perturb table to 2^16
tcp: drop the hash_32() part from the index calculation
include/net/inet_hashtables.h | 2 +-
include/net/secure_seq.h | 2 +-
net/core/secure_seq.c | 14 ++++++++----
net/ipv4/inet_hashtables.c | 40 ++++++++++++++++++++++-------------
4 files changed, 37 insertions(+), 21 deletions(-)
--
2.17.5
^ permalink raw reply
* [PATCH net 3/7] tcp: resalt the secret every 10 seconds
From: Willy Tarreau @ 2022-04-27 6:52 UTC (permalink / raw)
To: netdev
Cc: David Miller, Jakub Kicinski, Eric Dumazet, Moshe Kol,
Yossi Gilad, Amit Klein, linux-kernel
In-Reply-To: <20220427065233.2075-1-w@1wt.eu>
From: Eric Dumazet <edumazet@google.com>
In order to limit the ability for an observer to recognize the source
ports sequence used to contact a set of destinations, we should
periodically shuffle the secret. 10 seconds looks effective enough
without causing particular issues.
Cc: Moshe Kol <moshe.kol@mail.huji.ac.il>
Cc: Yossi Gilad <yossi.gilad@mail.huji.ac.il>
Cc: Amit Klein <aksecurity@gmail.com>
Tested-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/core/secure_seq.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/net/core/secure_seq.c b/net/core/secure_seq.c
index 2cdd43a63f64..200ab4686275 100644
--- a/net/core/secure_seq.c
+++ b/net/core/secure_seq.c
@@ -22,6 +22,8 @@
static siphash_aligned_key_t net_secret;
static siphash_aligned_key_t ts_secret;
+#define EPHEMERAL_PORT_SHUFFLE_PERIOD (10 * HZ)
+
static __always_inline void net_secret_init(void)
{
net_get_random_once(&net_secret, sizeof(net_secret));
@@ -101,10 +103,12 @@ u32 secure_ipv6_port_ephemeral(const __be32 *saddr, const __be32 *daddr,
struct in6_addr saddr;
struct in6_addr daddr;
__be16 dport;
+ unsigned int timeseed;
} __aligned(SIPHASH_ALIGNMENT) combined = {
.saddr = *(struct in6_addr *)saddr,
.daddr = *(struct in6_addr *)daddr,
- .dport = dport
+ .dport = dport,
+ .timeseed = jiffies / EPHEMERAL_PORT_SHUFFLE_PERIOD,
};
net_secret_init();
return siphash(&combined, offsetofend(typeof(combined), dport),
@@ -145,8 +149,10 @@ EXPORT_SYMBOL_GPL(secure_tcp_seq);
u64 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport)
{
net_secret_init();
- return siphash_3u32((__force u32)saddr, (__force u32)daddr,
- (__force u16)dport, &net_secret);
+ return siphash_4u32((__force u32)saddr, (__force u32)daddr,
+ (__force u16)dport,
+ jiffies / EPHEMERAL_PORT_SHUFFLE_PERIOD,
+ &net_secret);
}
EXPORT_SYMBOL_GPL(secure_ipv4_port_ephemeral);
#endif
--
2.17.5
^ permalink raw reply related
* [PATCH net-next v2 4/5] net: lan966x: Add support for PTP_PF_PEROUT
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
In-Reply-To: <20220427065127.3765659-1-horatiu.vultur@microchip.com>
Lan966x has 8 PTP programmable pins, where the last pins is hardcoded to
be used by PHC0, which does the frame timestamping. All the rest of the
PTP pins can be shared between the PHCs and can have different functions
like perout or extts. For now add support for PTP_FS_PEROUT.
The HW is not able to support absolute start time but can use the nsec
for phase adjustment when generating PPS.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
.../ethernet/microchip/lan966x/lan966x_main.h | 2 +
.../ethernet/microchip/lan966x/lan966x_ptp.c | 167 ++++++++++++++++++
2 files changed, 169 insertions(+)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 5213263c4e87..76255e2a86f3 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -56,6 +56,7 @@
#define LAN966X_PHC_COUNT 3
#define LAN966X_PHC_PORT 0
+#define LAN966X_PHC_PINS_NUM 7
#define IFH_REW_OP_NOOP 0x0
#define IFH_REW_OP_ONE_STEP_PTP 0x3
@@ -177,6 +178,7 @@ struct lan966x_stat_layout {
struct lan966x_phc {
struct ptp_clock *clock;
struct ptp_clock_info info;
+ struct ptp_pin_desc pins[LAN966X_PHC_PINS_NUM];
struct hwtstamp_config hwtstamp_config;
struct lan966x *lan966x;
u8 index;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 3e455a3fad08..3199a266ed3d 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -493,6 +493,158 @@ static int lan966x_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
return 0;
}
+static int lan966x_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ struct lan966x_phc *phc = container_of(ptp, struct lan966x_phc, info);
+ struct lan966x *lan966x = phc->lan966x;
+ struct ptp_clock_info *info;
+ int i;
+
+ /* Currently support only 1 channel */
+ if (chan != 0)
+ return -1;
+
+ switch (func) {
+ case PTP_PF_NONE:
+ case PTP_PF_PEROUT:
+ break;
+ default:
+ return -1;
+ }
+
+ /* The PTP pins are shared by all the PHC. So it is required to see if
+ * the pin is connected to another PHC. The pin is connected to another
+ * PHC if that pin already has a function on that PHC.
+ */
+ for (i = 0; i < LAN966X_PHC_COUNT; ++i) {
+ info = &lan966x->phc[i].info;
+
+ /* Ignore the check with ourself */
+ if (ptp == info)
+ continue;
+
+ if (info->pin_config[pin].func == PTP_PF_PEROUT)
+ return -1;
+ }
+
+ return 0;
+}
+
+static int lan966x_ptp_perout(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ struct lan966x_phc *phc = container_of(ptp, struct lan966x_phc, info);
+ struct lan966x *lan966x = phc->lan966x;
+ struct timespec64 ts_phase, ts_period;
+ unsigned long flags;
+ s64 wf_high, wf_low;
+ bool pps = false;
+ int pin;
+
+ if (rq->perout.flags & ~(PTP_PEROUT_DUTY_CYCLE |
+ PTP_PEROUT_PHASE))
+ return -EOPNOTSUPP;
+
+ pin = ptp_find_pin(phc->clock, PTP_PF_PEROUT, rq->perout.index);
+ if (pin == -1 || pin >= LAN966X_PHC_PINS_NUM)
+ return -EINVAL;
+
+ if (!on) {
+ spin_lock_irqsave(&lan966x->ptp_clock_lock, flags);
+ lan_rmw(PTP_PIN_CFG_PIN_ACTION_SET(PTP_PIN_ACTION_IDLE) |
+ PTP_PIN_CFG_PIN_DOM_SET(phc->index) |
+ PTP_PIN_CFG_PIN_SYNC_SET(0),
+ PTP_PIN_CFG_PIN_ACTION |
+ PTP_PIN_CFG_PIN_DOM |
+ PTP_PIN_CFG_PIN_SYNC,
+ lan966x, PTP_PIN_CFG(pin));
+ spin_unlock_irqrestore(&lan966x->ptp_clock_lock, flags);
+ return 0;
+ }
+
+ if (rq->perout.period.sec == 1 &&
+ rq->perout.period.nsec == 0)
+ pps = true;
+
+ if (rq->perout.flags & PTP_PEROUT_PHASE) {
+ ts_phase.tv_sec = rq->perout.phase.sec;
+ ts_phase.tv_nsec = rq->perout.phase.nsec;
+ } else {
+ ts_phase.tv_sec = rq->perout.start.sec;
+ ts_phase.tv_nsec = rq->perout.start.nsec;
+ }
+
+ if (ts_phase.tv_sec || (ts_phase.tv_nsec && !pps)) {
+ dev_warn(lan966x->dev,
+ "Absolute time not supported!\n");
+ return -EINVAL;
+ }
+
+ if (rq->perout.flags & PTP_PEROUT_DUTY_CYCLE) {
+ struct timespec64 ts_on;
+
+ ts_on.tv_sec = rq->perout.on.sec;
+ ts_on.tv_nsec = rq->perout.on.nsec;
+
+ wf_high = timespec64_to_ns(&ts_on);
+ } else {
+ wf_high = 5000;
+ }
+
+ if (pps) {
+ spin_lock_irqsave(&lan966x->ptp_clock_lock, flags);
+ lan_wr(PTP_WF_LOW_PERIOD_PIN_WFL(ts_phase.tv_nsec),
+ lan966x, PTP_WF_LOW_PERIOD(pin));
+ lan_wr(PTP_WF_HIGH_PERIOD_PIN_WFH(wf_high),
+ lan966x, PTP_WF_HIGH_PERIOD(pin));
+ lan_rmw(PTP_PIN_CFG_PIN_ACTION_SET(PTP_PIN_ACTION_CLOCK) |
+ PTP_PIN_CFG_PIN_DOM_SET(phc->index) |
+ PTP_PIN_CFG_PIN_SYNC_SET(3),
+ PTP_PIN_CFG_PIN_ACTION |
+ PTP_PIN_CFG_PIN_DOM |
+ PTP_PIN_CFG_PIN_SYNC,
+ lan966x, PTP_PIN_CFG(pin));
+ spin_unlock_irqrestore(&lan966x->ptp_clock_lock, flags);
+ return 0;
+ }
+
+ ts_period.tv_sec = rq->perout.period.sec;
+ ts_period.tv_nsec = rq->perout.period.nsec;
+
+ wf_low = timespec64_to_ns(&ts_period);
+ wf_low -= wf_high;
+
+ spin_lock_irqsave(&lan966x->ptp_clock_lock, flags);
+ lan_wr(PTP_WF_LOW_PERIOD_PIN_WFL(wf_low),
+ lan966x, PTP_WF_LOW_PERIOD(pin));
+ lan_wr(PTP_WF_HIGH_PERIOD_PIN_WFH(wf_high),
+ lan966x, PTP_WF_HIGH_PERIOD(pin));
+ lan_rmw(PTP_PIN_CFG_PIN_ACTION_SET(PTP_PIN_ACTION_CLOCK) |
+ PTP_PIN_CFG_PIN_DOM_SET(phc->index) |
+ PTP_PIN_CFG_PIN_SYNC_SET(0),
+ PTP_PIN_CFG_PIN_ACTION |
+ PTP_PIN_CFG_PIN_DOM |
+ PTP_PIN_CFG_PIN_SYNC,
+ lan966x, PTP_PIN_CFG(pin));
+ spin_unlock_irqrestore(&lan966x->ptp_clock_lock, flags);
+
+ return 0;
+}
+
+static int lan966x_ptp_enable(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ switch (rq->type) {
+ case PTP_CLK_REQ_PEROUT:
+ return lan966x_ptp_perout(ptp, rq, on);
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
static struct ptp_clock_info lan966x_ptp_clock_info = {
.owner = THIS_MODULE,
.name = "lan966x ptp",
@@ -501,6 +653,10 @@ static struct ptp_clock_info lan966x_ptp_clock_info = {
.settime64 = lan966x_ptp_settime64,
.adjtime = lan966x_ptp_adjtime,
.adjfine = lan966x_ptp_adjfine,
+ .verify = lan966x_ptp_verify,
+ .enable = lan966x_ptp_enable,
+ .n_per_out = LAN966X_PHC_PINS_NUM,
+ .n_pins = LAN966X_PHC_PINS_NUM,
};
static int lan966x_ptp_phc_init(struct lan966x *lan966x,
@@ -508,8 +664,19 @@ static int lan966x_ptp_phc_init(struct lan966x *lan966x,
struct ptp_clock_info *clock_info)
{
struct lan966x_phc *phc = &lan966x->phc[index];
+ struct ptp_pin_desc *p;
+ int i;
+
+ for (i = 0; i < LAN966X_PHC_PINS_NUM; i++) {
+ p = &phc->pins[i];
+
+ snprintf(p->name, sizeof(p->name), "pin%d", i);
+ p->index = i;
+ p->func = PTP_PF_NONE;
+ }
phc->info = *clock_info;
+ phc->info.pin_config = &phc->pins[0];
phc->clock = ptp_clock_register(&phc->info, lan966x->dev);
if (IS_ERR(phc->clock))
return PTR_ERR(phc->clock);
--
2.33.0
^ permalink raw reply related
* [PATCH net-next v2 5/5] net: lan966x: Add support for PTP_PF_EXTTS
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
In-Reply-To: <20220427065127.3765659-1-horatiu.vultur@microchip.com>
Extend the PTP programmable pins to implement also PTP_PF_EXTTS
function. The PTP pin can be configured to capture only on the rising
edge of the PPS signal. And once an event is seen then an interrupt is
generated and the local time counter is saved.
The interrupt is shared between all the pins.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
.../ethernet/microchip/lan966x/lan966x_main.c | 17 +++
.../ethernet/microchip/lan966x/lan966x_main.h | 2 +
.../ethernet/microchip/lan966x/lan966x_ptp.c | 109 +++++++++++++++++-
3 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index f072ae674740..5a503f3991d9 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -692,6 +692,9 @@ static void lan966x_cleanup_ports(struct lan966x *lan966x)
if (lan966x->ptp_irq)
devm_free_irq(lan966x->dev, lan966x->ptp_irq, lan966x);
+
+ if (lan966x->ptp_ext_irq)
+ devm_free_irq(lan966x->dev, lan966x->ptp_ext_irq, lan966x);
}
static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
@@ -1058,6 +1061,20 @@ static int lan966x_probe(struct platform_device *pdev)
lan966x->fdma = true;
}
+ if (lan966x->ptp) {
+ lan966x->ptp_ext_irq = platform_get_irq_byname(pdev, "ptp-ext");
+ if (lan966x->ptp_ext_irq > 0) {
+ err = devm_request_threaded_irq(&pdev->dev,
+ lan966x->ptp_ext_irq, NULL,
+ lan966x_ptp_ext_irq_handler,
+ IRQF_ONESHOT,
+ "ptp-ext irq", lan966x);
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "Unable to use ptp-ext irq");
+ }
+ }
+
/* init switch */
lan966x_init(lan966x);
lan966x_stats_init(lan966x);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 76255e2a86f3..3b86ddddc756 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -233,6 +233,7 @@ struct lan966x {
int ana_irq;
int ptp_irq;
int fdma_irq;
+ int ptp_ext_irq;
/* worqueue for fdb */
struct workqueue_struct *fdb_work;
@@ -394,6 +395,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
void lan966x_ptp_txtstamp_release(struct lan966x_port *port,
struct sk_buff *skb);
irqreturn_t lan966x_ptp_irq_handler(int irq, void *args);
+irqreturn_t lan966x_ptp_ext_irq_handler(int irq, void *args);
int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev);
int lan966x_fdma_change_mtu(struct lan966x *lan966x);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 3199a266ed3d..3a621c5165bc 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -321,6 +321,63 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
return IRQ_HANDLED;
}
+irqreturn_t lan966x_ptp_ext_irq_handler(int irq, void *args)
+{
+ struct lan966x *lan966x = args;
+ struct lan966x_phc *phc;
+ unsigned long flags;
+ u64 time = 0;
+ time64_t s;
+ int pin, i;
+ s64 ns;
+
+ if (!(lan_rd(lan966x, PTP_PIN_INTR)))
+ return IRQ_NONE;
+
+ /* Go through all domains and see which pin generated the interrupt */
+ for (i = 0; i < LAN966X_PHC_COUNT; ++i) {
+ struct ptp_clock_event ptp_event = {0};
+
+ phc = &lan966x->phc[i];
+ pin = ptp_find_pin_unlocked(phc->clock, PTP_PF_EXTTS, 0);
+ if (pin == -1)
+ continue;
+
+ if (!(lan_rd(lan966x, PTP_PIN_INTR) & BIT(pin)))
+ continue;
+
+ spin_lock_irqsave(&lan966x->ptp_clock_lock, flags);
+
+ /* Enable to get the new interrupt.
+ * By writing 1 it clears the bit
+ */
+ lan_wr(BIT(pin), lan966x, PTP_PIN_INTR);
+
+ /* Get current time */
+ s = lan_rd(lan966x, PTP_TOD_SEC_MSB(pin));
+ s <<= 32;
+ s |= lan_rd(lan966x, PTP_TOD_SEC_LSB(pin));
+ ns = lan_rd(lan966x, PTP_TOD_NSEC(pin));
+ ns &= PTP_TOD_NSEC_TOD_NSEC;
+
+ spin_unlock_irqrestore(&lan966x->ptp_clock_lock, flags);
+
+ if ((ns & 0xFFFFFFF0) == 0x3FFFFFF0) {
+ s--;
+ ns &= 0xf;
+ ns += 999999984;
+ }
+ time = ktime_set(s, ns);
+
+ ptp_event.index = pin;
+ ptp_event.timestamp = time;
+ ptp_event.type = PTP_CLOCK_EXTTS;
+ ptp_clock_event(phc->clock, &ptp_event);
+ }
+
+ return IRQ_HANDLED;
+}
+
static int lan966x_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
{
struct lan966x_phc *phc = container_of(ptp, struct lan966x_phc, info);
@@ -508,6 +565,7 @@ static int lan966x_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
switch (func) {
case PTP_PF_NONE:
case PTP_PF_PEROUT:
+ case PTP_PF_EXTTS:
break;
default:
return -1;
@@ -524,7 +582,8 @@ static int lan966x_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
if (ptp == info)
continue;
- if (info->pin_config[pin].func == PTP_PF_PEROUT)
+ if (info->pin_config[pin].func == PTP_PF_PEROUT ||
+ info->pin_config[pin].func == PTP_PF_EXTTS)
return -1;
}
@@ -632,12 +691,59 @@ static int lan966x_ptp_perout(struct ptp_clock_info *ptp,
return 0;
}
+static int lan966x_ptp_extts(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ struct lan966x_phc *phc = container_of(ptp, struct lan966x_phc, info);
+ struct lan966x *lan966x = phc->lan966x;
+ unsigned long flags;
+ int pin;
+ u32 val;
+
+ if (lan966x->ptp_ext_irq <= 0)
+ return -EOPNOTSUPP;
+
+ /* Reject requests with unsupported flags */
+ if (rq->extts.flags & ~(PTP_ENABLE_FEATURE |
+ PTP_RISING_EDGE |
+ PTP_STRICT_FLAGS))
+ return -EOPNOTSUPP;
+
+ pin = ptp_find_pin(phc->clock, PTP_PF_EXTTS, rq->extts.index);
+ if (pin == -1 || pin >= LAN966X_PHC_PINS_NUM)
+ return -EINVAL;
+
+ spin_lock_irqsave(&lan966x->ptp_clock_lock, flags);
+ lan_rmw(PTP_PIN_CFG_PIN_ACTION_SET(PTP_PIN_ACTION_SAVE) |
+ PTP_PIN_CFG_PIN_SYNC_SET(on ? 3 : 0) |
+ PTP_PIN_CFG_PIN_DOM_SET(phc->index) |
+ PTP_PIN_CFG_PIN_SELECT_SET(pin),
+ PTP_PIN_CFG_PIN_ACTION |
+ PTP_PIN_CFG_PIN_SYNC |
+ PTP_PIN_CFG_PIN_DOM |
+ PTP_PIN_CFG_PIN_SELECT,
+ lan966x, PTP_PIN_CFG(pin));
+
+ val = lan_rd(lan966x, PTP_PIN_INTR_ENA);
+ if (on)
+ val |= BIT(pin);
+ else
+ val &= ~BIT(pin);
+ lan_wr(val, lan966x, PTP_PIN_INTR_ENA);
+
+ spin_unlock_irqrestore(&lan966x->ptp_clock_lock, flags);
+
+ return 0;
+}
+
static int lan966x_ptp_enable(struct ptp_clock_info *ptp,
struct ptp_clock_request *rq, int on)
{
switch (rq->type) {
case PTP_CLK_REQ_PEROUT:
return lan966x_ptp_perout(ptp, rq, on);
+ case PTP_CLK_REQ_EXTTS:
+ return lan966x_ptp_extts(ptp, rq, on);
default:
return -EOPNOTSUPP;
}
@@ -656,6 +762,7 @@ static struct ptp_clock_info lan966x_ptp_clock_info = {
.verify = lan966x_ptp_verify,
.enable = lan966x_ptp_enable,
.n_per_out = LAN966X_PHC_PINS_NUM,
+ .n_ext_ts = LAN966X_PHC_PINS_NUM,
.n_pins = LAN966X_PHC_PINS_NUM,
};
--
2.33.0
^ permalink raw reply related
* [PATCH net-next v2 3/5] net: lan966x: Add registers used to configure the PTP pin
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
In-Reply-To: <20220427065127.3765659-1-horatiu.vultur@microchip.com>
Add registers that are used to configure the PTP pins. These registers
are used to enable the interrupts per PTP pin and to set the waveform
generated by the pin.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
.../ethernet/microchip/lan966x/lan966x_regs.h | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 2f59285bef29..8265ad89f0bc 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -684,6 +684,24 @@ enum lan966x_target {
/* FDMA:FDMA:FDMA_ERRORS */
#define FDMA_ERRORS __REG(TARGET_FDMA, 0, 1, 8, 0, 1, 428, 412, 0, 1, 4)
+/* PTP:PTP_CFG:PTP_PIN_INTR */
+#define PTP_PIN_INTR __REG(TARGET_PTP, 0, 1, 512, 0, 1, 16, 0, 0, 1, 4)
+
+#define PTP_PIN_INTR_INTR_PTP GENMASK(7, 0)
+#define PTP_PIN_INTR_INTR_PTP_SET(x)\
+ FIELD_PREP(PTP_PIN_INTR_INTR_PTP, x)
+#define PTP_PIN_INTR_INTR_PTP_GET(x)\
+ FIELD_GET(PTP_PIN_INTR_INTR_PTP, x)
+
+/* PTP:PTP_CFG:PTP_PIN_INTR_ENA */
+#define PTP_PIN_INTR_ENA __REG(TARGET_PTP, 0, 1, 512, 0, 1, 16, 4, 0, 1, 4)
+
+#define PTP_PIN_INTR_ENA_INTR_ENA GENMASK(7, 0)
+#define PTP_PIN_INTR_ENA_INTR_ENA_SET(x)\
+ FIELD_PREP(PTP_PIN_INTR_ENA_INTR_ENA, x)
+#define PTP_PIN_INTR_ENA_INTR_ENA_GET(x)\
+ FIELD_GET(PTP_PIN_INTR_ENA_INTR_ENA, x)
+
/* PTP:PTP_CFG:PTP_DOM_CFG */
#define PTP_DOM_CFG __REG(TARGET_PTP, 0, 1, 512, 0, 1, 16, 12, 0, 1, 4)
@@ -717,6 +735,12 @@ enum lan966x_target {
#define PTP_PIN_CFG_PIN_SYNC_GET(x)\
FIELD_GET(PTP_PIN_CFG_PIN_SYNC, x)
+#define PTP_PIN_CFG_PIN_SELECT GENMASK(23, 21)
+#define PTP_PIN_CFG_PIN_SELECT_SET(x)\
+ FIELD_PREP(PTP_PIN_CFG_PIN_SELECT, x)
+#define PTP_PIN_CFG_PIN_SELECT_GET(x)\
+ FIELD_GET(PTP_PIN_CFG_PIN_SELECT, x)
+
#define PTP_PIN_CFG_PIN_DOM GENMASK(17, 16)
#define PTP_PIN_CFG_PIN_DOM_SET(x)\
FIELD_PREP(PTP_PIN_CFG_PIN_DOM, x)
@@ -744,6 +768,22 @@ enum lan966x_target {
#define PTP_TOD_NSEC_TOD_NSEC_GET(x)\
FIELD_GET(PTP_TOD_NSEC_TOD_NSEC, x)
+/* PTP:PTP_PINS:WF_HIGH_PERIOD */
+#define PTP_WF_HIGH_PERIOD(g) __REG(TARGET_PTP,\
+ 0, 1, 0, g, 8, 64, 24, 0, 1, 4)
+
+#define PTP_WF_HIGH_PERIOD_PIN_WFH(x) ((x) & GENMASK(29, 0))
+#define PTP_WF_HIGH_PERIOD_PIN_WFH_M GENMASK(29, 0)
+#define PTP_WF_HIGH_PERIOD_PIN_WFH_X(x) ((x) & GENMASK(29, 0))
+
+/* PTP:PTP_PINS:WF_LOW_PERIOD */
+#define PTP_WF_LOW_PERIOD(g) __REG(TARGET_PTP,\
+ 0, 1, 0, g, 8, 64, 28, 0, 1, 4)
+
+#define PTP_WF_LOW_PERIOD_PIN_WFL(x) ((x) & GENMASK(29, 0))
+#define PTP_WF_LOW_PERIOD_PIN_WFL_M GENMASK(29, 0)
+#define PTP_WF_LOW_PERIOD_PIN_WFL_X(x) ((x) & GENMASK(29, 0))
+
/* PTP:PTP_TS_FIFO:PTP_TWOSTEP_CTRL */
#define PTP_TWOSTEP_CTRL __REG(TARGET_PTP, 0, 1, 612, 0, 1, 12, 0, 0, 1, 4)
--
2.33.0
^ permalink raw reply related
* [PATCH net-next v2 2/5] net: lan966x: Change the PTP pin used to read/write the PHC.
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
In-Reply-To: <20220427065127.3765659-1-horatiu.vultur@microchip.com>
To read/write a value to a PHC, it is required to use a PTP pin.
Currently it is used pin 5, but change to pin 7 as is the last pin.
All the other pins will have different functions.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 0a1041da4384..3e455a3fad08 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -16,7 +16,7 @@
*/
#define LAN966X_1PPB_FORMAT 3480517749LL
-#define TOD_ACC_PIN 0x5
+#define TOD_ACC_PIN 0x7
enum {
PTP_PIN_ACTION_IDLE = 0,
--
2.33.0
^ permalink raw reply related
* [PATCH net-next v2 1/5] dt-bindings: net: lan966x: Extend with the ptp external interrupt.
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
In-Reply-To: <20220427065127.3765659-1-horatiu.vultur@microchip.com>
Extend dt-bindings for lan966x with ptp external interrupt. This is
generated when an external 1pps signal is received on the ptp pin.
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
.../devicetree/bindings/net/microchip,lan966x-switch.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
index 13812768b923..131dc5a652de 100644
--- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
+++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
@@ -39,6 +39,7 @@ properties:
- description: frame dma based extraction
- description: analyzer interrupt
- description: ptp interrupt
+ - description: ptp external interrupt
interrupt-names:
minItems: 1
@@ -47,6 +48,7 @@ properties:
- const: fdma
- const: ana
- const: ptp
+ - const: ptp-ext
resets:
items:
--
2.33.0
^ permalink raw reply related
* [PATCH net-next v2 0/5] net: lan966x: Add support for PTP programmable pins
From: Horatiu Vultur @ 2022-04-27 6:51 UTC (permalink / raw)
To: netdev, devicetree, linux-kernel
Cc: davem, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
UNGLinuxDriver, richardcochran, Horatiu Vultur
Lan966x has 8 PTP programmable pins. The last pin is hardcoded to be used
by PHC0 and all the rest are shareable between the PHCs. The PTP pins can
implement both extts and perout functions.
v1->v2:
- use ptp_find_pin_unlocked instead of ptp_find_pin inside the irq handler.
Horatiu Vultur (5):
dt-bindings: net: lan966x: Extend with the ptp external interrupt.
net: lan966x: Change the PTP pin used to read/write the PHC.
net: lan966x: Add registers used to configure the PTP pin
net: lan966x: Add support for PTP_PF_PEROUT
net: lan966x: Add support for PTP_PF_EXTTS
.../net/microchip,lan966x-switch.yaml | 2 +
.../ethernet/microchip/lan966x/lan966x_main.c | 17 ++
.../ethernet/microchip/lan966x/lan966x_main.h | 4 +
.../ethernet/microchip/lan966x/lan966x_ptp.c | 276 +++++++++++++++++-
.../ethernet/microchip/lan966x/lan966x_regs.h | 40 +++
5 files changed, 338 insertions(+), 1 deletion(-)
--
2.33.0
^ permalink raw reply
* Re: [linux-next:master] BUILD REGRESSION e7d6987e09a328d4a949701db40ef63fbb970670
From: Jiri Pirko @ 2022-04-27 6:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: kernel test robot, Andrew Morton, netdev, Ido Schimmel
In-Reply-To: <20220426103537.4d0f43b7@kernel.org>
Tue, Apr 26, 2022 at 07:35:37PM CEST, kuba@kernel.org wrote:
>On Tue, 26 Apr 2022 16:59:07 +0200 Jiri Pirko wrote:
>> >>is this one on your radar?
>> >
>> >Will send a fix for this, thanks.
>>
>> Can't find the line. I don't see
>> e7d6987e09a328d4a949701db40ef63fbb970670 in linux-next :/
>
>Eh, no idea which tree it came from, but FWIW I do have that one in my
>local tree. So here it is:
>
> 844 devlink_linecard = devlink_linecard_create(priv_to_devlink(mlxsw_core),
> 845 slot_index, &mlxsw_linecard_ops,
> 846 linecard);
> 847 if (IS_ERR(devlink_linecard)) {
> 848 err = PTR_ERR(devlink_linecard);
> 849 goto err_devlink_linecard_create;
> 850 }
> 851 linecard->devlink_linecard = devlink_linecard;
> 852 INIT_DELAYED_WORK(&linecard->status_event_to_dw,
> 853 &mlxsw_linecard_status_event_to_work);
>
>Unless I'm missing something looks like a false positive :S
Yeah, that is where I ended up as well, came into conclusion I have to
be looking at a different code.
^ permalink raw reply
* Re: [PATCH v2 net-next 07/10] net: dsa: request drivers to perform FDB isolation
From: Hans Schultz @ 2022-04-27 6:45 UTC (permalink / raw)
To: Andrew Lunn, Hans Schultz
Cc: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller,
Florian Fainelli, Vivien Didelot, Vladimir Oltean,
Kurt Kanzenbach, Hauke Mehrtens, Woojung Huh, UNGLinuxDriver,
Sean Wang, Landen Chao, DENG Qingfang, Claudiu Manoil,
Alexandre Belloni, Linus Walleij, Alvin Šipraga,
George McCollister
In-Reply-To: <YmgaX4On/2j3lJf/@lunn.ch>
On tis, apr 26, 2022 at 18:14, Andrew Lunn <andrew@lunn.ch> wrote:
>> > @@ -941,23 +965,29 @@ struct dsa_switch_ops {
>> > * Forwarding database
>> > */
>> > int (*port_fdb_add)(struct dsa_switch *ds, int port,
>> > - const unsigned char *addr, u16 vid);
>> > + const unsigned char *addr, u16 vid,
>> > + struct dsa_db db);
>>
>> Hi! Wouldn't it be better to have a struct that has all the functions
>> parameters in one instead of adding further parameters to these
>> functions?
>>
>> I am asking because I am also needing to add a parameter to
>> port_fdb_add(), and it would be more future oriented to have a single
>> function parameter as a struct, so that it is easier to add parameters
>> to these functions without havíng to change the prototype of the
>> function every time.
>
> Hi Hans
>
> Please trim the text to only what is relevant when replying. It is
> easy to miss comments when having to Page Down, Page Down, Page down,
> to find comments.
>
> Andrew
Hi Andrew,
ahh yes, my client collapses those lines, but thanks for letting me
know. I will trim going forward.
Hans
^ permalink raw reply
* [PATCH net] usbnet: smsc95xx: Fix deadlock on runtime resume
From: Lukas Wunner @ 2022-04-27 6:41 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
smsc95xx_resume() to call phy_init_hw(). That function waits for the
device to runtime resume even though it is placed in the runtime resume
path, causing a deadlock.
The problem is that phy_init_hw() calls down to smsc95xx_mdiobus_read(),
which never uses the _nopm variant of usbnet_read_cmd(). Amend it to
autosense that it's called from the runtime resume/suspend path and use
the _nopm variant if so.
Stacktrace for posterity:
INFO: task kworker/2:1:49 blocked for more than 122 seconds.
Workqueue: usb_hub_wq hub_event
schedule
rpm_resume
__pm_runtime_resume
usb_autopm_get_interface
usbnet_read_cmd
__smsc95xx_read_reg
__smsc95xx_phy_wait_not_busy
__smsc95xx_mdio_read
smsc95xx_mdiobus_read
__mdiobus_read
mdiobus_read
smsc_phy_reset
phy_init_hw
smsc95xx_resume
usb_resume_interface
usb_resume_both
usb_runtime_resume
__rpm_callback
rpm_callback
rpm_resume
__pm_runtime_resume
usb_autoresume_device
hub_event
process_one_work
Fixes: 05b35e7eb9a1 ("smsc95xx: add phylib support")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v5.10+
Cc: Andre Edich <andre.edich@microchip.com>
---
drivers/net/usb/smsc95xx.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 4ef61f6b85df..82b8feaa5162 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -285,11 +285,21 @@ static void smsc95xx_mdio_write_nopm(struct usbnet *dev, int idx, int regval)
__smsc95xx_mdio_write(dev, pdata->phydev->mdio.addr, idx, regval, 1);
}
+static bool smsc95xx_in_pm(struct usbnet *dev)
+{
+#ifdef CONFIG_PM
+ return dev->udev->dev.power.runtime_status == RPM_RESUMING ||
+ dev->udev->dev.power.runtime_status == RPM_SUSPENDING;
+#else
+ return false;
+#endif
+}
+
static int smsc95xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
{
struct usbnet *dev = bus->priv;
- return __smsc95xx_mdio_read(dev, phy_id, idx, 0);
+ return __smsc95xx_mdio_read(dev, phy_id, idx, smsc95xx_in_pm(dev));
}
static int smsc95xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
@@ -297,7 +307,7 @@ static int smsc95xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
{
struct usbnet *dev = bus->priv;
- __smsc95xx_mdio_write(dev, phy_id, idx, regval, 0);
+ __smsc95xx_mdio_write(dev, phy_id, idx, regval, smsc95xx_in_pm(dev));
return 0;
}
--
2.35.2
^ permalink raw reply related
* [PATCH net-next] net: phy: Deduplicate interrupt disablement on PHY attach
From: Lukas Wunner @ 2022-04-27 6:30 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski, Paolo Abeni, Heiner Kallweit,
Andrew Lunn, Russell King
Cc: netdev
phy_attach_direct() first calls phy_init_hw() (which restores interrupt
settings through ->config_intr()), then calls phy_disable_interrupts().
So if phydev->interrupts was previously set to 1, interrupts are briefly
enabled, then disabled, which seems nonsensical.
If it was previously set to 0, interrupts are disabled twice, which is
equally nonsensical.
Deduplicate interrupt disablement.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/net/phy/phy_device.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def..f867042b2eb4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1449,6 +1449,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
phydev->state = PHY_READY;
+ phydev->interrupts = PHY_INTERRUPT_DISABLED;
+
/* Port is set to PORT_TP by default and the actual PHY driver will set
* it to different value depending on the PHY configuration. If we have
* the generic PHY driver we can't figure it out, thus set the old
@@ -1471,10 +1473,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
if (err)
goto error;
- err = phy_disable_interrupts(phydev);
- if (err)
- return err;
-
phy_resume(phydev);
phy_led_triggers_register(phydev);
--
2.35.2
^ permalink raw reply related
* [PATCH v2,bpf-next] samples/bpf: detach xdp prog when program exits unexpectedly in xdp_rxq_info_user
From: Zhengchao Shao @ 2022-04-27 6:23 UTC (permalink / raw)
To: bpf, netdev, linux-kernel, ast, daniel, davem, kuba, hawk,
john.fastabend, andrii, kafai, songliubraving, yhs, kpsingh
Cc: weiyongjun1, shaozhengchao, yuehaibing
When xdp_rxq_info_user program exits unexpectedly, it doesn't detach xdp
prog of device, and other xdp prog can't be attached to the device. So
call init_exit() to detach xdp prog when program exits unexpectedly.
Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
samples/bpf/xdp_rxq_info_user.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index f2d90cba5164..9f6de6508713 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -18,7 +18,7 @@ static const char *__doc__ = " XDP RX-queue info extract example\n\n"
#include <getopt.h>
#include <net/if.h>
#include <time.h>
-
+#include <limits.h>
#include <arpa/inet.h>
#include <linux/if_link.h>
@@ -44,6 +44,9 @@ static struct bpf_map *rx_queue_index_map;
#define EXIT_FAIL_BPF 4
#define EXIT_FAIL_MEM 5
+#define FAIL_MEM_SIG INT_MAX
+#define FAIL_STAT_SIG (INT_MAX - 1)
+
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
{"dev", required_argument, NULL, 'd' },
@@ -77,6 +80,12 @@ static void int_exit(int sig)
printf("program on interface changed, not removing\n");
}
}
+
+ if (sig == FAIL_MEM_SIG)
+ exit(EXIT_FAIL_MEM);
+ else if (sig == FAIL_STAT_SIG)
+ exit(EXIT_FAIL);
+
exit(EXIT_OK);
}
@@ -141,7 +150,8 @@ static char* options2str(enum cfg_options_flags flag)
if (flag & READ_MEM)
return "read";
fprintf(stderr, "ERR: Unknown config option flags");
- exit(EXIT_FAIL);
+ int_exit(FAIL_STAT_SIG);
+ return "unknown";
}
static void usage(char *argv[])
@@ -174,7 +184,7 @@ static __u64 gettime(void)
res = clock_gettime(CLOCK_MONOTONIC, &t);
if (res < 0) {
fprintf(stderr, "Error with gettimeofday! (%i)\n", res);
- exit(EXIT_FAIL);
+ int_exit(FAIL_STAT_SIG);
}
return (__u64) t.tv_sec * NANOSEC_PER_SEC + t.tv_nsec;
}
@@ -202,7 +212,7 @@ static struct datarec *alloc_record_per_cpu(void)
array = calloc(nr_cpus, sizeof(struct datarec));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
- exit(EXIT_FAIL_MEM);
+ int_exit(FAIL_MEM_SIG);
}
return array;
}
@@ -215,7 +225,7 @@ static struct record *alloc_record_per_rxq(void)
array = calloc(nr_rxqs, sizeof(struct record));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
- exit(EXIT_FAIL_MEM);
+ int_exit(FAIL_MEM_SIG);
}
return array;
}
@@ -229,7 +239,7 @@ static struct stats_record *alloc_stats_record(void)
rec = calloc(1, sizeof(struct stats_record));
if (!rec) {
fprintf(stderr, "Mem alloc error\n");
- exit(EXIT_FAIL_MEM);
+ int_exit(FAIL_MEM_SIG);
}
rec->rxq = alloc_record_per_rxq();
for (i = 0; i < nr_rxqs; i++)
--
2.33.0
^ permalink raw reply related
* Re: [PATCH V2] vDPA/ifcvf: allow userspace to suspend a queue
From: Zhu, Lingshan @ 2022-04-27 6:16 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: virtualization, netdev
In-Reply-To: <d36fbb4e-c848-3a06-6a81-8cd1b219a6d4@redhat.com>
On 4/27/2022 1:56 PM, Jason Wang wrote:
>
> 在 2022/4/24 19:33, Zhu Lingshan 写道:
>> Formerly, ifcvf driver has implemented a lazy-initialization mechanism
>> for the virtqueues, it would store all virtqueue config fields that
>> passed down from the userspace, then load them to the virtqueues and
>> enable the queues upon DRIVER_OK.
>>
>> To allow the userspace to suspend a virtqueue,
>> this commit passes queue_enable to the virtqueue directly through
>> set_vq_ready().
>>
>> This feature requires and this commits implementing all virtqueue
>> ops(set_vq_addr, set_vq_num and set_vq_ready) to take immediate
>> actions than lazy-initialization, so ifcvf_hw_enable() is retired.
>>
>> set_features() should take immediate actions as well.
>>
>> ifcvf_add_status() is retierd because we should not add
>> status like FEATURES_OK by ifcvf's decision, this driver should
>> only set device status upon vdpa_ops.set_status()
>>
>> To avoid losing virtqueue configurations caused by multiple
>> rounds of reset(), this commit also refactors thed evice reset
>> routine, now it simply reset the config handler and the virtqueues,
>> and only once device-reset().
>
>
> It looks like the patch tries to do too many things at one run. I'd
> suggest to split them:
>
>
> 1) on-the-fly set via set_vq_ready(), but I don't see a reason why we
> need to change other lazy stuffs, since setting queue_enable to 1
> before DRIVER_OK won't start the virtqueue anyhow
> 2) if necessary, converting the lazy stuffs
> 3) the synchornize_irq() fixes
> 4) other stuffs
Thanks! I will try!
>
> Thanks
>
>
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_base.c | 150 +++++++++++++++++++-------------
>> drivers/vdpa/ifcvf/ifcvf_base.h | 16 ++--
>> drivers/vdpa/ifcvf/ifcvf_main.c | 81 +++--------------
>> 3 files changed, 111 insertions(+), 136 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 48c4dadb0c7c..bbc9007a6f34 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -179,20 +179,7 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8
>> status)
>> void ifcvf_reset(struct ifcvf_hw *hw)
>> {
>> - hw->config_cb.callback = NULL;
>> - hw->config_cb.private = NULL;
>> -
>> ifcvf_set_status(hw, 0);
>> - /* flush set_status, make sure VF is stopped, reset */
>> - ifcvf_get_status(hw);
>> -}
>> -
>> -static void ifcvf_add_status(struct ifcvf_hw *hw, u8 status)
>> -{
>> - if (status != 0)
>> - status |= ifcvf_get_status(hw);
>> -
>> - ifcvf_set_status(hw, status);
>> ifcvf_get_status(hw);
>> }
>> @@ -213,7 +200,7 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
>> return features;
>> }
>> -u64 ifcvf_get_features(struct ifcvf_hw *hw)
>> +u64 ifcvf_get_device_features(struct ifcvf_hw *hw)
>> {
>> return hw->hw_features;
>> }
>> @@ -280,7 +267,7 @@ void ifcvf_write_dev_config(struct ifcvf_hw *hw,
>> u64 offset,
>> vp_iowrite8(*p++, hw->dev_cfg + offset + i);
>> }
>> -static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
>> +void ifcvf_set_features(struct ifcvf_hw *hw, u64 features)
>> {
>> struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> @@ -289,22 +276,22 @@ static void ifcvf_set_features(struct
>> ifcvf_hw *hw, u64 features)
>> vp_iowrite32(1, &cfg->guest_feature_select);
>> vp_iowrite32(features >> 32, &cfg->guest_feature);
>> +
>> + vp_ioread32(&cfg->guest_feature);
>> }
>> -static int ifcvf_config_features(struct ifcvf_hw *hw)
>> +u64 ifcvf_get_features(struct ifcvf_hw *hw)
>> {
>> - struct ifcvf_adapter *ifcvf;
>> + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> + u64 features;
>> - ifcvf = vf_to_adapter(hw);
>> - ifcvf_set_features(hw, hw->req_features);
>> - ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK);
>> + vp_iowrite32(0, &cfg->device_feature_select);
>> + features = vp_ioread32(&cfg->device_feature);
>> - if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) {
>> - IFCVF_ERR(ifcvf->pdev, "Failed to set FEATURES_OK status\n");
>> - return -EIO;
>> - }
>> + vp_iowrite32(1, &cfg->device_feature_select);
>> + features |= ((u64)vp_ioread32(&cfg->guest_feature) << 32);
>> - return 0;
>> + return features;
>> }
>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>> @@ -331,68 +318,111 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw,
>> u16 qid, u16 num)
>> ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg;
>> q_pair_id = qid / hw->nr_vring;
>> avail_idx_addr =
>> &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2];
>> - hw->vring[qid].last_avail_idx = num;
>> vp_iowrite16(num, avail_idx_addr);
>> return 0;
>> }
>> -static int ifcvf_hw_enable(struct ifcvf_hw *hw)
>> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
>> {
>> - struct virtio_pci_common_cfg __iomem *cfg;
>> - u32 i;
>> + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> - cfg = hw->common_cfg;
>> - for (i = 0; i < hw->nr_vring; i++) {
>> - if (!hw->vring[i].ready)
>> - break;
>> + vp_iowrite16(qid, &cfg->queue_select);
>> + vp_iowrite16(num, &cfg->queue_size);
>> +}
>> - vp_iowrite16(i, &cfg->queue_select);
>> - vp_iowrite64_twopart(hw->vring[i].desc, &cfg->queue_desc_lo,
>> - &cfg->queue_desc_hi);
>> - vp_iowrite64_twopart(hw->vring[i].avail, &cfg->queue_avail_lo,
>> - &cfg->queue_avail_hi);
>> - vp_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
>> - &cfg->queue_used_hi);
>> - vp_iowrite16(hw->vring[i].size, &cfg->queue_size);
>> - ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
>> - vp_iowrite16(1, &cfg->queue_enable);
>> - }
>> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
>> + u64 driver_area, u64 device_area)
>> +{
>> + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> +
>> + vp_iowrite16(qid, &cfg->queue_select);
>> + vp_iowrite64_twopart(desc_area, &cfg->queue_desc_lo,
>> + &cfg->queue_desc_hi);
>> + vp_iowrite64_twopart(driver_area, &cfg->queue_avail_lo,
>> + &cfg->queue_avail_hi);
>> + vp_iowrite64_twopart(device_area, &cfg->queue_used_lo,
>> + &cfg->queue_used_hi);
>> return 0;
>> }
>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
>> {
>> - u32 i;
>> + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> +
>> + vp_iowrite16(qid, &cfg->queue_select);
>> + /* write 0 to queue_enable will suspend a queue*/
>> + vp_iowrite16(ready, &cfg->queue_enable);
>> +}
>> +
>> +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid)
>> +{
>> + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> + bool queue_enable;
>> +
>> + vp_iowrite16(qid, &cfg->queue_select);
>> + queue_enable = vp_ioread16(&cfg->queue_enable);
>> +
>> + return (bool)queue_enable;
>> +}
>> +
>> +static void synchronize_per_vq_irq(struct ifcvf_hw *hw)
>> +{
>> + int i;
>> - ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>> for (i = 0; i < hw->nr_vring; i++) {
>> - ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>> + if (hw->vring[i].irq != -EINVAL)
>> + synchronize_irq(hw->vring[i].irq);
>> }
>> }
>> -int ifcvf_start_hw(struct ifcvf_hw *hw)
>> +static void synchronize_vqs_reused_irq(struct ifcvf_hw *hw)
>> {
>> - ifcvf_reset(hw);
>> - ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> - ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
>> + if (hw->vqs_reused_irq != -EINVAL)
>> + synchronize_irq(hw->vqs_reused_irq);
>> +}
>> - if (ifcvf_config_features(hw) < 0)
>> - return -EINVAL;
>> +static void synchronize_vq_irq(struct ifcvf_hw *hw)
>> +{
>> + u8 status = hw->msix_vector_status;
>> - if (ifcvf_hw_enable(hw) < 0)
>> - return -EINVAL;
>> + if (status == MSIX_VECTOR_PER_VQ_AND_CONFIG)
>> + synchronize_per_vq_irq(hw);
>> + else
>> + synchronize_vqs_reused_irq(hw);
>> +}
>> - ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
>> +static void synchronize_config_irq(struct ifcvf_hw *hw)
>> +{
>> + if (hw->config_irq != -EINVAL)
>> + synchronize_irq(hw->config_irq);
>> +}
>> - return 0;
>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < hw->nr_vring; i++) {
>> + synchronize_vq_irq(hw);
>> + hw->vring[i].cb.callback = NULL;
>> + hw->vring[i].cb.private = NULL;
>> + ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
>> + }
>> +}
>> +
>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
>> +{
>> + synchronize_config_irq(hw);
>> + hw->config_cb.callback = NULL;
>> + hw->config_cb.private = NULL;
>> + ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
>> }
>> void ifcvf_stop_hw(struct ifcvf_hw *hw)
>> {
>> - ifcvf_hw_disable(hw);
>> - ifcvf_reset(hw);
>> + ifcvf_reset_vring(hw);
>> + ifcvf_reset_config_handler(hw);
>> }
>> void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 115b61f4924b..f3dce0d795cb 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -49,12 +49,6 @@
>> #define MSIX_VECTOR_DEV_SHARED 3
>> struct vring_info {
>> - u64 desc;
>> - u64 avail;
>> - u64 used;
>> - u16 size;
>> - u16 last_avail_idx;
>> - bool ready;
>> void __iomem *notify_addr;
>> phys_addr_t notify_pa;
>> u32 irq;
>> @@ -76,7 +70,6 @@ struct ifcvf_hw {
>> phys_addr_t notify_base_pa;
>> u32 notify_off_multiplier;
>> u32 dev_type;
>> - u64 req_features;
>> u64 hw_features;
>> struct virtio_pci_common_cfg __iomem *common_cfg;
>> void __iomem *dev_cfg;
>> @@ -123,7 +116,7 @@ u8 ifcvf_get_status(struct ifcvf_hw *hw);
>> void ifcvf_set_status(struct ifcvf_hw *hw, u8 status);
>> void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>> void ifcvf_reset(struct ifcvf_hw *hw);
>> -u64 ifcvf_get_features(struct ifcvf_hw *hw);
>> +u64 ifcvf_get_device_features(struct ifcvf_hw *hw);
>> u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>> int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features);
>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>> @@ -131,6 +124,13 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16
>> qid, u16 num);
>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>> int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
>> u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
>> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
>> + u64 driver_area, u64 device_area);
>> u16 ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
>> u16 ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
>> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num);
>> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
>> +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
>> +void ifcvf_set_features(struct ifcvf_hw *hw, u64 features);
>> +u64 ifcvf_get_features(struct ifcvf_hw *hw);
>> #endif /* _IFCVF_H_ */
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 4366320fb68d..0257ba98cffe 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -358,53 +358,6 @@ static int ifcvf_request_irq(struct
>> ifcvf_adapter *adapter)
>> return 0;
>> }
>> -static int ifcvf_start_datapath(void *private)
>> -{
>> - struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
>> - u8 status;
>> - int ret;
>> -
>> - ret = ifcvf_start_hw(vf);
>> - if (ret < 0) {
>> - status = ifcvf_get_status(vf);
>> - status |= VIRTIO_CONFIG_S_FAILED;
>> - ifcvf_set_status(vf, status);
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -static int ifcvf_stop_datapath(void *private)
>> -{
>> - struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
>> - int i;
>> -
>> - for (i = 0; i < vf->nr_vring; i++)
>> - vf->vring[i].cb.callback = NULL;
>> -
>> - ifcvf_stop_hw(vf);
>> -
>> - return 0;
>> -}
>> -
>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
>> -{
>> - struct ifcvf_hw *vf = ifcvf_private_to_vf(adapter);
>> - int i;
>> -
>> - for (i = 0; i < vf->nr_vring; i++) {
>> - vf->vring[i].last_avail_idx = 0;
>> - vf->vring[i].desc = 0;
>> - vf->vring[i].avail = 0;
>> - vf->vring[i].used = 0;
>> - vf->vring[i].ready = 0;
>> - vf->vring[i].cb.callback = NULL;
>> - vf->vring[i].cb.private = NULL;
>> - }
>> -
>> - ifcvf_reset(vf);
>> -}
>> -
>> static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device
>> *vdpa_dev)
>> {
>> return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
>> @@ -426,7 +379,7 @@ static u64 ifcvf_vdpa_get_device_features(struct
>> vdpa_device *vdpa_dev)
>> u64 features;
>> if (type == VIRTIO_ID_NET || type == VIRTIO_ID_BLOCK)
>> - features = ifcvf_get_features(vf);
>> + features = ifcvf_get_device_features(vf);
>> else {
>> features = 0;
>> IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
>> @@ -444,7 +397,7 @@ static int ifcvf_vdpa_set_driver_features(struct
>> vdpa_device *vdpa_dev, u64 feat
>> if (ret)
>> return ret;
>> - vf->req_features = features;
>> + ifcvf_set_features(vf, features);
>> return 0;
>> }
>> @@ -453,7 +406,7 @@ static u64 ifcvf_vdpa_get_driver_features(struct
>> vdpa_device *vdpa_dev)
>> {
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> - return vf->req_features;
>> + return ifcvf_get_features(vf);
>> }
>> static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)
>> @@ -486,11 +439,6 @@ static void ifcvf_vdpa_set_status(struct
>> vdpa_device *vdpa_dev, u8 status)
>> ifcvf_set_status(vf, status);
>> return;
>> }
>> -
>> - if (ifcvf_start_datapath(adapter) < 0)
>> - IFCVF_ERR(adapter->pdev,
>> - "Failed to set ifcvf vdpa status %u\n",
>> - status);
>> }
>> ifcvf_set_status(vf, status);
>> @@ -509,12 +457,10 @@ static int ifcvf_vdpa_reset(struct vdpa_device
>> *vdpa_dev)
>> if (status_old == 0)
>> return 0;
>> - if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
>> - ifcvf_stop_datapath(adapter);
>> - ifcvf_free_irq(adapter);
>> - }
>> + ifcvf_stop_hw(vf);
>> + ifcvf_free_irq(adapter);
>> - ifcvf_reset_vring(adapter);
>> + ifcvf_reset(vf);
>> return 0;
>> }
>> @@ -554,14 +500,17 @@ static void ifcvf_vdpa_set_vq_ready(struct
>> vdpa_device *vdpa_dev,
>> {
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> - vf->vring[qid].ready = ready;
>> + ifcvf_set_vq_ready(vf, qid, ready);
>> }
>> static bool ifcvf_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev,
>> u16 qid)
>> {
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> + bool ready;
>> +
>> + ready = ifcvf_get_vq_ready(vf, qid);
>> - return vf->vring[qid].ready;
>> + return ready;
>> }
>> static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev,
>> u16 qid,
>> @@ -569,7 +518,7 @@ static void ifcvf_vdpa_set_vq_num(struct
>> vdpa_device *vdpa_dev, u16 qid,
>> {
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> - vf->vring[qid].size = num;
>> + ifcvf_set_vq_num(vf, qid, num);
>> }
>> static int ifcvf_vdpa_set_vq_address(struct vdpa_device
>> *vdpa_dev, u16 qid,
>> @@ -578,11 +527,7 @@ static int ifcvf_vdpa_set_vq_address(struct
>> vdpa_device *vdpa_dev, u16 qid,
>> {
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> - vf->vring[qid].desc = desc_area;
>> - vf->vring[qid].avail = driver_area;
>> - vf->vring[qid].used = device_area;
>> -
>> - return 0;
>> + return ifcvf_set_vq_address(vf, qid, desc_area, driver_area,
>> device_area);
>> }
>> static void ifcvf_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16
>> qid)
>
^ permalink raw reply
* [PATCH net-next 7/7] net: phy: smsc: Cope with hot-removal in interrupt handler
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
If reading the Interrupt Source Flag register fails with -ENODEV, then
the PHY has been hot-removed and the correct response is to bail out
instead of throwing a WARN splat and attempting to suspend the PHY.
The PHY should be stopped in due course anyway as the kernel
asynchronously tears down the device.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/net/phy/smsc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a521d48b22a7..35bff7fd234c 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -91,7 +91,9 @@ static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
irq_status = phy_read(phydev, MII_LAN83C185_ISF);
if (irq_status < 0) {
- phy_error(phydev);
+ if (irq_status != -ENODEV)
+ phy_error(phydev);
+
return IRQ_NONE;
}
--
2.35.2
^ permalink raw reply related
* [PATCH net-next 6/7] net: phy: smsc: Cache interrupt mask
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
Cache the interrupt mask to avoid re-reading it from the PHY upon every
interrupt. The PHY may be located on a USB device, so the additional
read may unnecessarily increase interrupt overhead and latency.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/net/phy/smsc.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index d8cac02a79b9..a521d48b22a7 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -44,6 +44,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = {
};
struct smsc_phy_priv {
+ u16 intmask;
bool energy_enable;
struct clk *refclk;
};
@@ -58,7 +59,6 @@ static int smsc_phy_ack_interrupt(struct phy_device *phydev)
static int smsc_phy_config_intr(struct phy_device *phydev)
{
struct smsc_phy_priv *priv = phydev->priv;
- u16 intmask = 0;
int rc;
if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
@@ -66,12 +66,15 @@ static int smsc_phy_config_intr(struct phy_device *phydev)
if (rc)
return rc;
- intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
+ priv->intmask = MII_LAN83C185_ISF_INT4 | MII_LAN83C185_ISF_INT6;
if (priv->energy_enable)
- intmask |= MII_LAN83C185_ISF_INT7;
- rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+ priv->intmask |= MII_LAN83C185_ISF_INT7;
+
+ rc = phy_write(phydev, MII_LAN83C185_IM, priv->intmask);
} else {
- rc = phy_write(phydev, MII_LAN83C185_IM, intmask);
+ priv->intmask = 0;
+
+ rc = phy_write(phydev, MII_LAN83C185_IM, 0);
if (rc)
return rc;
@@ -83,13 +86,8 @@ static int smsc_phy_config_intr(struct phy_device *phydev)
static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
{
- int irq_status, irq_enabled;
-
- irq_enabled = phy_read(phydev, MII_LAN83C185_IM);
- if (irq_enabled < 0) {
- phy_error(phydev);
- return IRQ_NONE;
- }
+ struct smsc_phy_priv *priv = phydev->priv;
+ int irq_status;
irq_status = phy_read(phydev, MII_LAN83C185_ISF);
if (irq_status < 0) {
@@ -97,7 +95,7 @@ static irqreturn_t smsc_phy_handle_interrupt(struct phy_device *phydev)
return IRQ_NONE;
}
- if (!(irq_status & irq_enabled))
+ if (!(irq_status & priv->intmask))
return IRQ_NONE;
phy_trigger_machine(phydev);
--
2.35.2
^ permalink raw reply related
* [PATCH net-next 4/7] usbnet: smsc95xx: Avoid link settings race on interrupt reception
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
When a PHY interrupt is signaled, the SMSC LAN95xx driver updates the
MAC full duplex mode and PHY flow control registers based on cached data
in struct phy_device:
smsc95xx_status() # raises EVENT_LINK_RESET
usbnet_deferred_kevent()
smsc95xx_link_reset() # uses cached data in phydev
Simultaneously, phylib polls link status once per second and updates
that cached data:
phy_state_machine()
phy_check_link_status()
phy_read_status()
lan87xx_read_status()
genphy_read_status() # updates cached data in phydev
If smsc95xx_link_reset() wins the race against genphy_read_status(),
the registers may be updated based on stale data.
E.g. if the link was previously down, phydev->duplex is set to
DUPLEX_UNKNOWN and that's what smsc95xx_link_reset() will use, even
though genphy_read_status() may update it to DUPLEX_FULL afterwards.
PHY interrupts are currently only enabled on suspend to trigger wakeup,
so the impact of the race is limited, but we're about to enable them
perpetually.
Avoid the race by delaying execution of smsc95xx_link_reset() until
phy_state_machine() has done its job and calls back via
smsc95xx_handle_link_change().
Signaling EVENT_LINK_RESET on wakeup is not necessary because phylib
picks up link status changes through polling. So drop the declaration
of a ->link_reset() callback.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/net/usb/smsc95xx.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 6c37c7adde1b..36abfaeae3c6 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -566,7 +566,7 @@ static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev)
return smsc95xx_write_reg(dev, AFC_CFG, afc_cfg);
}
-static int smsc95xx_link_reset(struct usbnet *dev)
+static void smsc95xx_mac_update_fullduplex(struct usbnet *dev)
{
struct smsc95xx_priv *pdata = dev->driver_priv;
unsigned long flags;
@@ -583,14 +583,14 @@ static int smsc95xx_link_reset(struct usbnet *dev)
spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
- if (ret < 0)
- return ret;
+ if (ret < 0) {
+ netdev_warn(dev->net, "Error updating MAC full duplex mode\n");
+ return;
+ }
ret = smsc95xx_phy_update_flowcontrol(dev);
if (ret < 0)
netdev_warn(dev->net, "Error updating PHY flow control\n");
-
- return ret;
}
static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
@@ -607,7 +607,7 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
if (intdata & INT_ENP_PHY_INT_)
- usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+ ;
else
netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
intdata);
@@ -1070,6 +1070,7 @@ static void smsc95xx_handle_link_change(struct net_device *net)
struct usbnet *dev = netdev_priv(net);
phy_print_status(net->phydev);
+ smsc95xx_mac_update_fullduplex(dev);
usbnet_defer_kevent(dev, EVENT_LINK_CHANGE);
}
@@ -1975,7 +1976,6 @@ static const struct driver_info smsc95xx_info = {
.description = "smsc95xx USB 2.0 Ethernet",
.bind = smsc95xx_bind,
.unbind = smsc95xx_unbind,
- .link_reset = smsc95xx_link_reset,
.reset = smsc95xx_reset,
.check_connect = smsc95xx_start_phy,
.stop = smsc95xx_stop,
--
2.35.2
^ permalink raw reply related
* [PATCH net-next 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
Link status of SMSC LAN95xx chips is polled once per second, even though
they're capable of signaling PHY interrupts through the MAC layer.
Forward those interrupts to the PHY driver to avoid polling. Benefits
are reduced bus traffic, reduced CPU overhead and quicker interface
bringup.
Polling was introduced in 2016 by commit d69d16949346 ("usbnet:
smsc95xx: fix link detection for disabled autonegotiation").
Back then, the LAN95xx driver neglected to enable the ENERGYON interrupt,
hence couldn't detect link-up events when auto-negotiation was disabled.
The proper solution would have been to enable the ENERGYON interrupt
instead of polling.
Since then, PHY handling was moved from the LAN95xx driver to the SMSC
PHY driver with commit 05b35e7eb9a1 ("smsc95xx: add phylib support").
That PHY driver is capable of link detection with auto-negotiation
disabled because it enables the ENERGYON interrupt.
Note that signaling interrupts through the MAC layer not only works with
the integrated PHY, but also with an external PHY, provided its
interrupt pin is attached to LAN95xx's nPHY_INT pin.
In the unlikely event that the interrupt pin of an external PHY is
attached to a GPIO of the SoC (or not connected at all), the driver can
be amended to retrieve the irq from the PHY's of_node.
To forward PHY interrupts to phylib, it is not sufficient to call
phy_mac_interrupt(). Instead, the PHY's interrupt handler needs to run
so that PHY interrupts are cleared. That's because according to page
119 of the LAN950x datasheet, "The source of this interrupt is a level.
The interrupt persists until it is cleared in the PHY."
https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf
Therefore, create an IRQ domain with a single IRQ for the PHY. In the
future, the IRQ domain may be extended to support the 11 GPIOs on the
LAN95xx.
Normally the PHY interrupt should be masked until the PHY driver has
cleared it. However masking requires a (sleeping) USB transaction and
interrupts are received in (non-sleepable) softirq context. I decided
not to mask the interrupt at all (by using the dummy_irq_chip's noop
->irq_mask() callback): The USB interrupt endpoint is polled in 1 msec
intervals and normally that's sufficient to wake the PHY driver's IRQ
thread and have it clear the interrupt. If it does take longer, worst
thing that can happen is the IRQ thread is woken again. No big deal.
Because PHY interrupts are now perpetually enabled, there's no need to
selectively enable them on suspend. So remove all invocations of
smsc95xx_enable_phy_wakeup_interrupts().
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Andre Edich <andre.edich@microchip.com>
---
drivers/net/usb/smsc95xx.c | 118 +++++++++++++++++++++----------------
1 file changed, 66 insertions(+), 52 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 36abfaeae3c6..1d376bad13e5 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -18,6 +18,8 @@
#include <linux/usb/usbnet.h>
#include <linux/slab.h>
#include <linux/of_net.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/mdio.h>
#include <linux/phy.h>
#include <net/selftests.h>
@@ -53,6 +55,9 @@
#define SUSPEND_ALLMODES (SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
+#define SMSC95XX_NR_IRQS (1) /* raise to 12 for GPIOs */
+#define PHY_HWIRQ (SMSC95XX_NR_IRQS - 1)
+
struct smsc95xx_priv {
u32 mac_cr;
u32 hash_hi;
@@ -61,6 +66,9 @@ struct smsc95xx_priv {
spinlock_t mac_cr_lock;
u8 features;
u8 suspend_flags;
+ struct irq_chip irqchip;
+ struct irq_domain *irqdomain;
+ struct fwnode_handle *irqfwnode;
struct mii_bus *mdiobus;
struct phy_device *phydev;
};
@@ -595,6 +603,8 @@ static void smsc95xx_mac_update_fullduplex(struct usbnet *dev)
static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
{
+ struct smsc95xx_priv *pdata = dev->driver_priv;
+ unsigned long flags;
u32 intdata;
if (urb->actual_length != 4) {
@@ -606,11 +616,20 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
intdata = get_unaligned_le32(urb->transfer_buffer);
netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata);
+ /* USB interrupts are received in softirq (tasklet) context.
+ * Switch to hardirq context to make genirq code happy.
+ */
+ local_irq_save(flags);
+ __irq_enter_raw();
+
if (intdata & INT_ENP_PHY_INT_)
- ;
+ generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
else
netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n",
intdata);
+
+ __irq_exit_raw();
+ local_irq_restore(flags);
}
/* Enable or disable Tx & Rx checksum offload engines */
@@ -1078,8 +1097,9 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
{
struct smsc95xx_priv *pdata;
bool is_internal_phy;
+ char usb_path[64];
+ int ret, phy_irq;
u32 val;
- int ret;
printk(KERN_INFO SMSC_CHIPNAME " v" SMSC_DRIVER_VERSION "\n");
@@ -1119,10 +1139,38 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
if (ret)
goto free_pdata;
+ /* create irq domain for use by PHY driver and GPIO consumers */
+ usb_make_path(dev->udev, usb_path, sizeof(usb_path));
+ pdata->irqfwnode = irq_domain_alloc_named_fwnode(usb_path);
+ if (!pdata->irqfwnode) {
+ ret = -ENOMEM;
+ goto free_pdata;
+ }
+
+ pdata->irqdomain = irq_domain_create_linear(pdata->irqfwnode,
+ SMSC95XX_NR_IRQS,
+ &irq_domain_simple_ops,
+ pdata);
+ if (!pdata->irqdomain) {
+ ret = -ENOMEM;
+ goto free_irqfwnode;
+ }
+
+ phy_irq = irq_create_mapping(pdata->irqdomain, PHY_HWIRQ);
+ if (!phy_irq) {
+ ret = -ENOENT;
+ goto remove_irqdomain;
+ }
+
+ pdata->irqchip = dummy_irq_chip;
+ pdata->irqchip.name = SMSC_CHIPNAME;
+ irq_set_chip_and_handler_name(phy_irq, &pdata->irqchip,
+ handle_simple_irq, "phy");
+
pdata->mdiobus = mdiobus_alloc();
if (!pdata->mdiobus) {
ret = -ENOMEM;
- goto free_pdata;
+ goto dispose_irq;
}
ret = smsc95xx_read_reg(dev, HW_CFG, &val);
@@ -1155,6 +1203,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
goto unregister_mdio;
}
+ pdata->phydev->irq = phy_irq;
pdata->phydev->is_internal = is_internal_phy;
/* detect device revision as different features may be available */
@@ -1197,6 +1246,15 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
free_mdio:
mdiobus_free(pdata->mdiobus);
+dispose_irq:
+ irq_dispose_mapping(phy_irq);
+
+remove_irqdomain:
+ irq_domain_remove(pdata->irqdomain);
+
+free_irqfwnode:
+ irq_domain_free_fwnode(pdata->irqfwnode);
+
free_pdata:
kfree(pdata);
return ret;
@@ -1209,6 +1267,9 @@ static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
phy_disconnect(dev->net->phydev);
mdiobus_unregister(pdata->mdiobus);
mdiobus_free(pdata->mdiobus);
+ irq_dispose_mapping(irq_find_mapping(pdata->irqdomain, PHY_HWIRQ));
+ irq_domain_remove(pdata->irqdomain);
+ irq_domain_free_fwnode(pdata->irqfwnode);
netif_dbg(dev, ifdown, dev->net, "free pdata\n");
kfree(pdata);
}
@@ -1233,29 +1294,6 @@ static u32 smsc_crc(const u8 *buffer, size_t len, int filter)
return crc << ((filter % 2) * 16);
}
-static int smsc95xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
-{
- int ret;
-
- netdev_dbg(dev->net, "enabling PHY wakeup interrupts\n");
-
- /* read to clear */
- ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_SRC);
- if (ret < 0)
- return ret;
-
- /* enable interrupt source */
- ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_MASK);
- if (ret < 0)
- return ret;
-
- ret |= mask;
-
- smsc95xx_mdio_write_nopm(dev, PHY_INT_MASK, ret);
-
- return 0;
-}
-
static int smsc95xx_link_ok_nopm(struct usbnet *dev)
{
int ret;
@@ -1422,7 +1460,6 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev)
static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
{
struct smsc95xx_priv *pdata = dev->driver_priv;
- int ret;
if (!netif_running(dev->net)) {
/* interface is ifconfig down so fully power down hw */
@@ -1441,27 +1478,10 @@ static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
}
netdev_dbg(dev->net, "autosuspend entering SUSPEND1\n");
-
- /* enable PHY wakeup events for if cable is attached */
- ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
- PHY_INT_MASK_ANEG_COMP_);
- if (ret < 0) {
- netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
- return ret;
- }
-
netdev_info(dev->net, "entering SUSPEND1 mode\n");
return smsc95xx_enter_suspend1(dev);
}
- /* enable PHY wakeup events so we remote wakeup if cable is pulled */
- ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
- PHY_INT_MASK_LINK_DOWN_);
- if (ret < 0) {
- netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
- return ret;
- }
-
netdev_dbg(dev->net, "autosuspend entering SUSPEND3\n");
return smsc95xx_enter_suspend3(dev);
}
@@ -1527,13 +1547,6 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
}
if (pdata->wolopts & WAKE_PHY) {
- ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
- (PHY_INT_MASK_ANEG_COMP_ | PHY_INT_MASK_LINK_DOWN_));
- if (ret < 0) {
- netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
- goto done;
- }
-
/* if link is down then configure EDPD and enter SUSPEND1,
* otherwise enter SUSPEND0 below
*/
@@ -1767,11 +1780,12 @@ static int smsc95xx_resume(struct usb_interface *intf)
return ret;
}
+ phy_init_hw(pdata->phydev);
+
ret = usbnet_resume(intf);
if (ret < 0)
netdev_warn(dev->net, "usbnet_resume error\n");
- phy_init_hw(pdata->phydev);
return ret;
}
--
2.35.2
^ permalink raw reply related
* Re: [PATCH v2] ath10k: skip ath10k_halt during suspend for driver state RESTARTING
From: Kalle Valo @ 2022-04-27 6:08 UTC (permalink / raw)
To: Abhishek Kumar
Cc: quic_wgong, briannorris, linux-kernel, linux-wireless, ath10k,
netdev, David S. Miller, Jakub Kicinski, Paolo Abeni
In-Reply-To: <20220426221859.v2.1.I650b809482e1af8d0156ed88b5dc2677a0711d46@changeid>
Abhishek Kumar <kuabhs@chromium.org> writes:
> Double free crash is observed when FW recovery(caused by wmi
> timeout/crash) is followed by immediate suspend event. The FW recovery
> is triggered by ath10k_core_restart() which calls driver clean up via
> ath10k_halt(). When the suspend event occurs between the FW recovery,
> the restart worker thread is put into frozen state until suspend completes.
> The suspend event triggers ath10k_stop() which again triggers ath10k_halt()
> The double invocation of ath10k_halt() causes ath10k_htt_rx_free() to be
> called twice(Note: ath10k_htt_rx_alloc was not called by restart worker
> thread because of its frozen state), causing the crash.
>
> To fix this, during the suspend flow, skip call to ath10k_halt() in
> ath10k_stop() when the current driver state is ATH10K_STATE_RESTARTING.
> Also, for driver state ATH10K_STATE_RESTARTING, call
> ath10k_wait_for_suspend() in ath10k_stop(). This is because call to
> ath10k_wait_for_suspend() is skipped later in
> [ath10k_halt() > ath10k_core_stop()] for the driver state
> ATH10K_STATE_RESTARTING.
>
> The frozen restart worker thread will be cancelled during resume when the
> device comes out of suspend.
>
> Below is the crash stack for reference:
>
> [ 428.469167] ------------[ cut here ]------------
> [ 428.469180] kernel BUG at mm/slub.c:4150!
> [ 428.469193] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [ 428.469219] Workqueue: events_unbound async_run_entry_fn
> [ 428.469230] RIP: 0010:kfree+0x319/0x31b
> [ 428.469241] RSP: 0018:ffffa1fac015fc30 EFLAGS: 00010246
> [ 428.469247] RAX: ffffedb10419d108 RBX: ffff8c05262b0000
> [ 428.469252] RDX: ffff8c04a8c07000 RSI: 0000000000000000
> [ 428.469256] RBP: ffffa1fac015fc78 R08: 0000000000000000
> [ 428.469276] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 428.469285] Call Trace:
> [ 428.469295] ? dma_free_attrs+0x5f/0x7d
> [ 428.469320] ath10k_core_stop+0x5b/0x6f
> [ 428.469336] ath10k_halt+0x126/0x177
> [ 428.469352] ath10k_stop+0x41/0x7e
> [ 428.469387] drv_stop+0x88/0x10e
> [ 428.469410] __ieee80211_suspend+0x297/0x411
> [ 428.469441] rdev_suspend+0x6e/0xd0
> [ 428.469462] wiphy_suspend+0xb1/0x105
> [ 428.469483] ? name_show+0x2d/0x2d
> [ 428.469490] dpm_run_callback+0x8c/0x126
> [ 428.469511] ? name_show+0x2d/0x2d
> [ 428.469517] __device_suspend+0x2e7/0x41b
> [ 428.469523] async_suspend+0x1f/0x93
> [ 428.469529] async_run_entry_fn+0x3d/0xd1
> [ 428.469535] process_one_work+0x1b1/0x329
> [ 428.469541] worker_thread+0x213/0x372
> [ 428.469547] kthread+0x150/0x15f
> [ 428.469552] ? pr_cont_work+0x58/0x58
> [ 428.469558] ? kthread_blkcg+0x31/0x31
>
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00288-QCARMSWPZ-1
> Co-developed-by: Wen Gong <quic_wgong@quicinc.com>
> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> ---
>
> Changes in v2:
> - Fixed typo, replaced ath11k by ath10k in the comments.
> - Adjusted the position of my S-O-B tag.
> - Added the Tested-on tag.
>
> drivers/net/wireless/ath/ath10k/mac.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index d804e19a742a..e9c1f11fef0a 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -5345,8 +5345,22 @@ static void ath10k_stop(struct ieee80211_hw *hw)
>
> mutex_lock(&ar->conf_mutex);
> if (ar->state != ATH10K_STATE_OFF) {
> - if (!ar->hw_rfkill_on)
> - ath10k_halt(ar);
> + if (!ar->hw_rfkill_on) {
> + /* If the current driver state is RESTARTING but not yet
> + * fully RESTARTED because of incoming suspend event,
> + * then ath10k_halt is already called via
> + * ath10k_core_restart and should not be called here.
> + */
> + if (ar->state != ATH10K_STATE_RESTARTING)
> + ath10k_halt(ar);
> + else
> + /* Suspending here, because when in RESTARTING
> + * state, ath10k_core_stop skips
> + * ath10k_wait_for_suspend.
> + */
> + ath10k_wait_for_suspend(ar,
> + WMI_PDEV_SUSPEND_AND_DISABLE_INTR);
> + }
I'm nitpicking but I prefer to use parenthesis with function names, so I
changed the comments. Also there was one ath10k-check warning:
drivers/net/wireless/ath/ath10k/mac.c:5360: line length of 91 exceeds 90 columns
In the pending branch I changed it to:
if (ar->state != ATH10K_STATE_RESTARTING) {
ath10k_halt(ar);
} else {
/* Suspending here, because when in RESTARTING
* state, ath10k_core_stop() skips
* ath10k_wait_for_suspend().
*/
opt = WMI_PDEV_SUSPEND_AND_DISABLE_INTR;
ath10k_wait_for_suspend(ar, opt);
}
Not really pretty but I prefer to keep ath10k warning free.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* [PATCH net-next 3/7] usbnet: smsc95xx: Don't reset PHY behind PHY driver's back
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
smsc95xx_reset() resets the PHY behind the PHY driver's back, which
seems like a bad idea generally. Remove that portion of the function.
We're about to use PHY interrupts instead of polling to detect link
changes on SMSC LAN95xx chips. Because smsc95xx_reset() is called from
usbnet_open(), PHY interrupt settings are lost whenever the net_device
is brought up.
There are two other callers of smsc95xx_reset(), namely smsc95xx_bind()
and smsc95xx_reset_resume(), and both may indeed benefit from a PHY
reset. However they already perform one through their calls to
phy_connect_direct() and phy_init_hw().
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Martyn Welch <martyn.welch@collabora.com>
Cc: Gabriel Hojda <ghojda@yo2urs.ro>
---
drivers/net/usb/smsc95xx.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 2cb44d65bbc3..6c37c7adde1b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -887,24 +887,6 @@ static int smsc95xx_reset(struct usbnet *dev)
return ret;
}
- ret = smsc95xx_write_reg(dev, PM_CTRL, PM_CTL_PHY_RST_);
- if (ret < 0)
- return ret;
-
- timeout = 0;
- do {
- msleep(10);
- ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
- if (ret < 0)
- return ret;
- timeout++;
- } while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
-
- if (timeout >= 100) {
- netdev_warn(dev->net, "timeout waiting for PHY Reset\n");
- return ret;
- }
-
ret = smsc95xx_set_mac_address(dev);
if (ret < 0)
return ret;
--
2.35.2
^ permalink raw reply related
* [PATCH net-next 2/7] usbnet: smsc95xx: Don't clear read-only PHY interrupt
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
Upon receiving data from the Interrupt Endpoint, the SMSC LAN95xx driver
attempts to clear the signaled interrupts by writing "all ones" to the
Interrupt Status Register.
However the driver only ever enables a single type of interrupt, namely
the PHY Interrupt. And according to page 119 of the LAN950x datasheet,
its bit in the Interrupt Status Register is read-only. There's no other
way to clear it than in a separate PHY register:
https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf
Consequently, writing "all ones" to the Interrupt Status Register is
pointless and can be dropped.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/net/usb/smsc95xx.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index edf0492ad489..2cb44d65bbc3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -572,10 +572,6 @@ static int smsc95xx_link_reset(struct usbnet *dev)
unsigned long flags;
int ret;
- ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
- if (ret < 0)
- return ret;
-
spin_lock_irqsave(&pdata->mac_cr_lock, flags);
if (pdata->phydev->duplex != DUPLEX_FULL) {
pdata->mac_cr &= ~MAC_CR_FDPX_;
--
2.35.2
^ permalink raw reply related
* [PATCH net-next 1/7] usbnet: Run unregister_netdev() before unbind() again
From: Lukas Wunner @ 2022-04-27 5:48 UTC (permalink / raw)
To: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-usb, Andre Edich, Oleksij Rempel, Martyn Welch,
Gabriel Hojda, Christoph Fritz, Lino Sanfilippo,
Philipp Rosenberger, Heiner Kallweit, Andrew Lunn, Russell King
In-Reply-To: <cover.1651037513.git.lukas@wunner.de>
Commit 2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()")
sought to fix a use-after-free on disconnect of USB Ethernet adapters.
It turns out that a different fix is necessary to address the issue:
https://lore.kernel.org/netdev/18b3541e5372bc9b9fc733d422f4e698c089077c.1650177997.git.lukas@wunner.de/
So the commit was not necessary.
The commit made binding and unbinding of USB Ethernet asymmetrical:
Before, usbnet_probe() first invoked the ->bind() callback and then
register_netdev(). usbnet_disconnect() mirrored that by first invoking
unregister_netdev() and then ->unbind().
Since the commit, the order in usbnet_disconnect() is reversed and no
longer mirrors usbnet_probe().
One consequence is that a PHY disconnected (and stopped) in ->unbind()
is afterwards stopped once more by unregister_netdev() as it closes the
netdev before unregistering. That necessitates a contortion in ->stop()
because the PHY may only be stopped if it hasn't already been
disconnected.
Reverting the commit allows making the call to phy_stop() unconditional
in ->stop().
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Martyn Welch <martyn.welch@collabora.com>
Cc: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/usb/asix_devices.c | 6 +-----
drivers/net/usb/smsc95xx.c | 3 +--
drivers/net/usb/usbnet.c | 6 +++---
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 38e47a93fb83..5b5eb630c4b7 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -795,11 +795,7 @@ static int ax88772_stop(struct usbnet *dev)
{
struct asix_common_private *priv = dev->driver_priv;
- /* On unplugged USB, we will get MDIO communication errors and the
- * PHY will be set in to PHY_HALTED state.
- */
- if (priv->phydev->state != PHY_HALTED)
- phy_stop(priv->phydev);
+ phy_stop(priv->phydev);
return 0;
}
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 4ef61f6b85df..edf0492ad489 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1243,8 +1243,7 @@ static int smsc95xx_start_phy(struct usbnet *dev)
static int smsc95xx_stop(struct usbnet *dev)
{
- if (dev->net->phydev)
- phy_stop(dev->net->phydev);
+ phy_stop(dev->net->phydev);
return 0;
}
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9a6450f796dc..36b24ec11650 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1616,9 +1616,6 @@ void usbnet_disconnect (struct usb_interface *intf)
xdev->bus->bus_name, xdev->devpath,
dev->driver_info->description);
- if (dev->driver_info->unbind)
- dev->driver_info->unbind(dev, intf);
-
net = dev->net;
unregister_netdev (net);
@@ -1626,6 +1623,9 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_scuttle_anchored_urbs(&dev->deferred);
+ if (dev->driver_info->unbind)
+ dev->driver_info->unbind(dev, intf);
+
usb_kill_urb(dev->interrupt);
usb_free_urb(dev->interrupt);
kfree(dev->padding_pkt);
--
2.35.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox