* [PATCH bpf-next v4 1/3] sock: move sock_valbool_flag to header
From: Dmitry Yakunin @ 2020-06-17 11:02 UTC (permalink / raw)
To: daniel, alexei.starovoitov
Cc: davem, brakmo, eric.dumazet, kafai, bpf, netdev
This is preparation for usage in bpf_setsockopt.
Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
include/net/sock.h | 9 +++++++++
net/core/sock.c | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index c53cc42..8ba438b 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -879,6 +879,15 @@ static inline void sock_reset_flag(struct sock *sk, enum sock_flags flag)
__clear_bit(flag, &sk->sk_flags);
}
+static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
+ int valbool)
+{
+ if (valbool)
+ sock_set_flag(sk, bit);
+ else
+ sock_reset_flag(sk, bit);
+}
+
static inline bool sock_flag(const struct sock *sk, enum sock_flags flag)
{
return test_bit(flag, &sk->sk_flags);
diff --git a/net/core/sock.c b/net/core/sock.c
index 6c4acf1..5ba4753 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -695,15 +695,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
return ret;
}
-static inline void sock_valbool_flag(struct sock *sk, enum sock_flags bit,
- int valbool)
-{
- if (valbool)
- sock_set_flag(sk, bit);
- else
- sock_reset_flag(sk, bit);
-}
-
bool sk_mc_loop(struct sock *sk)
{
if (dev_recursion_level())
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next v4 2/3] tcp: expose tcp_sock_set_keepidle_locked
From: Dmitry Yakunin @ 2020-06-17 11:02 UTC (permalink / raw)
To: daniel, alexei.starovoitov
Cc: davem, brakmo, eric.dumazet, kafai, bpf, netdev
In-Reply-To: <20200617110217.35669-1-zeil@yandex-team.ru>
This is preparation for usage in bpf_setsockopt.
v2:
- remove redundant EXPORT_SYMBOL (Alexei Starovoitov)
Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
include/linux/tcp.h | 1 +
net/ipv4/tcp.c | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 9aac824..3bdec31 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -499,6 +499,7 @@ int tcp_skb_shift(struct sk_buff *to, struct sk_buff *from, int pcount,
void tcp_sock_set_cork(struct sock *sk, bool on);
int tcp_sock_set_keepcnt(struct sock *sk, int val);
+int tcp_sock_set_keepidle_locked(struct sock *sk, int val);
int tcp_sock_set_keepidle(struct sock *sk, int val);
int tcp_sock_set_keepintvl(struct sock *sk, int val);
void tcp_sock_set_nodelay(struct sock *sk);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 15d47d5..18f8d54 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2901,7 +2901,7 @@ void tcp_sock_set_user_timeout(struct sock *sk, u32 val)
}
EXPORT_SYMBOL(tcp_sock_set_user_timeout);
-static int __tcp_sock_set_keepidle(struct sock *sk, int val)
+int tcp_sock_set_keepidle_locked(struct sock *sk, int val)
{
struct tcp_sock *tp = tcp_sk(sk);
@@ -2928,7 +2928,7 @@ int tcp_sock_set_keepidle(struct sock *sk, int val)
int err;
lock_sock(sk);
- err = __tcp_sock_set_keepidle(sk, val);
+ err = tcp_sock_set_keepidle_locked(sk, val);
release_sock(sk);
return err;
}
@@ -3127,7 +3127,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
break;
case TCP_KEEPIDLE:
- err = __tcp_sock_set_keepidle(sk, val);
+ err = tcp_sock_set_keepidle_locked(sk, val);
break;
case TCP_KEEPINTVL:
if (val < 1 || val > MAX_TCP_KEEPINTVL)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] net: macb: reject unsupported rgmii delays
From: Russell King - ARM Linux admin @ 2020-06-17 10:55 UTC (permalink / raw)
To: Helmut Grohne
Cc: Nicolas Ferre, David S. Miller, Jakub Kicinski, Palmer Dabbelt,
Paul Walmsley, netdev
In-Reply-To: <20200616074955.GA9092@laureti-dev>
On Tue, Jun 16, 2020 at 09:49:56AM +0200, Helmut Grohne wrote:
> The macb driver does not support configuring rgmii delays. At least for
> the Zynq GEM, delays are not supported by the hardware at all. However,
> the driver happily accepts and ignores any such delays.
>
> When operating in a mac to phy connection, the delay setting applies to
> the phy. Since the MAC does not support delays, the phy must provide
> them and the only supported mode is rgmii-id. However, in a fixed mac
> to mac connection, the delay applies to the mac itself. Therefore the
> only supported rgmii mode is rgmii.
This seems incorrect - see the phy documentation in
Documentation/networking/phy.rst:
* PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
internal delay by itself, it assumes that either the Ethernet MAC (if capable
or the PCB traces) insert the correct 1.5-2ns delay
* PHY_INTERFACE_MODE_RGMII_TXID: the PHY should insert an internal delay
for the transmit data lines (TXD[3:0]) processed by the PHY device
* PHY_INTERFACE_MODE_RGMII_RXID: the PHY should insert an internal delay
for the receive data lines (RXD[3:0]) processed by the PHY device
* PHY_INTERFACE_MODE_RGMII_ID: the PHY should insert internal delays for
both transmit AND receive data lines from/to the PHY device
Note that PHY_INTERFACE_MODE_RGMII, the delay can be added by _either_
the MAC or by PCB trace routing.
The individual RGMII delay modes are more about what the PHY itself is
asked to do with respect to inserting delays, so I don't think your
patch makes sense.
In any case...
> Link: https://lore.kernel.org/netdev/20200610081236.GA31659@laureti-dev/
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5b9d7c60eebc..bee5bf65e8b3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config,
> state->interface != PHY_INTERFACE_MODE_RMII &&
> state->interface != PHY_INTERFACE_MODE_GMII &&
> state->interface != PHY_INTERFACE_MODE_SGMII &&
> - !phy_interface_mode_is_rgmii(state->interface)) {
> + state->interface != PHY_INTERFACE_MODE_RGMII_ID) {
Here you reject everything except PHY_INTERFACE_MODE_RGMII_ID.
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> }
> @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp)
> struct phy_device *phydev;
> int ret;
>
> + if (of_phy_is_fixed_link(dn) &&
> + phy_interface_mode_is_rgmii(bp->phy_interface) &&
> + bp->phy_interface != PHY_INTERFACE_MODE_RGMII) {
but here you reject everything except PHY_INTERFACE_MODE_RGMII. These
can't both be right. If you start with PHY_INTERFACE_MODE_RGMII, and
have a fixed link, you'll have PHY_INTERFACE_MODE_RGMII passed into
the validate function, which will then fail.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH v2] bluetooth: Adding a configurable autoconnect timeout
From: Marcel Holtmann @ 2020-06-17 10:43 UTC (permalink / raw)
To: Alain Michaud
Cc: Bluetooth Kernel Mailing List, David S. Miller, Jakub Kicinski,
Johan Hedberg, linux-kernel, netdev
In-Reply-To: <20200615210638.132889-1-alainm@chromium.org>
Hi Alain,
> This patch adds a configurable LE autoconnect timeout.
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
> Changes in v1:
> Fixing longer than 80 char line.
>
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 1 +
> net/bluetooth/hci_event.c | 2 +-
> net/bluetooth/hci_request.c | 4 ++--
> net/bluetooth/mgmt_config.c | 13 +++++++++++++
> 5 files changed, 18 insertions(+), 3 deletions(-)
I created a local tree where I merged all pending patches together and then I send that out to the mailing list for review. This patch doesn’t apply cleanly anymore and thus I need you to resend it once we have that pending series merged into bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2] Bluetooth: Terminate the link if pairing is cancelled
From: Marcel Holtmann @ 2020-06-17 10:33 UTC (permalink / raw)
To: Manish Mandlik
Cc: Luiz Augusto von Dentz, Bluetooth Kernel Mailing List,
ChromeOS Bluetooth Upstreaming, Alain Michaud, David S. Miller,
Johan Hedberg, netdev, linux-kernel
In-Reply-To: <20200616092341.v2.1.I9dd050ead919f2cc3ef83d4e866de537c7799cf3@changeid>
Hi Manish,
> If user decides to cancel the ongoing pairing process (e.g. by clicking
> the cancel button on pairing/passkey window), abort any ongoing pairing
> and then terminate the link if it was created because of the pair
> device action.
>
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> ---
>
> Changes in v2:
> - Added code to track if the connection was triggered because of the pair
> device action and then only terminate the link on pairing cancel.
>
> include/net/bluetooth/hci_core.h | 14 ++++++++++++--
> net/bluetooth/hci_conn.c | 11 ++++++++---
> net/bluetooth/l2cap_core.c | 6 ++++--
> net/bluetooth/mgmt.c | 22 ++++++++++++++++++----
> 4 files changed, 42 insertions(+), 11 deletions(-)
patch has been added to my local tree. I will send an update with all pending patches in a bit.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 1/7] Bluetooth: Add definitions for advertisement monitor features
From: Marcel Holtmann @ 2020-06-17 10:31 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Alain Michaud, Yoni Shavit, Michael Sun, David S. Miller,
Jakub Kicinski, Johan Hedberg, open list, netdev
In-Reply-To: <20200615172440.v5.1.I636f906bf8122855dfd2ba636352bbdcb50c35ed@changeid>
Hi Miao-chen,
> This adds support for Advertisement Monitor API. Here are the commands
> and events added.
> - Read Advertisement Monitor Feature command
> - Add Advertisement Pattern Monitor command
> - Remove Advertisement Monitor command
> - Advertisement Monitor Added event
> - Advertisement Monitor Removed event
>
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3:
> - Update command/event opcodes.
> - Correct data types.
>
> Changes in v2: None
>
> include/net/bluetooth/mgmt.h | 49 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
I have added all 7 patches to my local tree. I added minor style modifications and merged it together with the device flags support.
Regards
Marcel
^ permalink raw reply
* [PATCH net 1/2] mptcp: cache msk on MP_JOIN init_req
From: Paolo Abeni @ 2020-06-17 10:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, mptcp
In-Reply-To: <cover.1592388398.git.pabeni@redhat.com>
The msk ownership is transferred to the child socket at
3rd ack time, so that we avoid more lookups later. If the
request does not reach the 3rd ack, the MSK reference is
dropped at request sock release time.
As a side effect, fallback is now tracked by a NULL msk
reference instead of zeroed 'mp_join' field. This will
simplify the next patch.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 39 +++++++++++++++++----------------------
2 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index db56535dfc29..c6eeaf3e8dcb 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -249,6 +249,7 @@ struct mptcp_subflow_request_sock {
u64 thmac;
u32 local_nonce;
u32 remote_nonce;
+ struct mptcp_sock *msk;
};
static inline struct mptcp_subflow_request_sock *
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index bbdb74b8bc3c..4068bdb2523b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -69,6 +69,9 @@ static void subflow_req_destructor(struct request_sock *req)
pr_debug("subflow_req=%p", subflow_req);
+ if (subflow_req->msk)
+ sock_put((struct sock *)subflow_req->msk);
+
if (subflow_req->mp_capable)
mptcp_token_destroy_request(subflow_req->token);
tcp_request_sock_ops.destructor(req);
@@ -86,8 +89,8 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
}
/* validate received token and create truncated hmac and nonce for SYN-ACK */
-static bool subflow_token_join_request(struct request_sock *req,
- const struct sk_buff *skb)
+static struct mptcp_sock *subflow_token_join_request(struct request_sock *req,
+ const struct sk_buff *skb)
{
struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
u8 hmac[SHA256_DIGEST_SIZE];
@@ -97,13 +100,13 @@ static bool subflow_token_join_request(struct request_sock *req,
msk = mptcp_token_get_sock(subflow_req->token);
if (!msk) {
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINNOTOKEN);
- return false;
+ return NULL;
}
local_id = mptcp_pm_get_local_id(msk, (struct sock_common *)req);
if (local_id < 0) {
sock_put((struct sock *)msk);
- return false;
+ return NULL;
}
subflow_req->local_id = local_id;
@@ -114,9 +117,7 @@ static bool subflow_token_join_request(struct request_sock *req,
subflow_req->remote_nonce, hmac);
subflow_req->thmac = get_unaligned_be64(hmac);
-
- sock_put((struct sock *)msk);
- return true;
+ return msk;
}
static void subflow_init_req(struct request_sock *req,
@@ -133,6 +134,7 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->mp_capable = 0;
subflow_req->mp_join = 0;
+ subflow_req->msk = NULL;
#ifdef CONFIG_TCP_MD5SIG
/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
@@ -166,12 +168,9 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->remote_id = mp_opt.join_id;
subflow_req->token = mp_opt.token;
subflow_req->remote_nonce = mp_opt.nonce;
- pr_debug("token=%u, remote_nonce=%u", subflow_req->token,
- subflow_req->remote_nonce);
- if (!subflow_token_join_request(req, skb)) {
- subflow_req->mp_join = 0;
- // @@ need to trigger RST
- }
+ subflow_req->msk = subflow_token_join_request(req, skb);
+ pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
+ subflow_req->remote_nonce, subflow_req->msk);
}
}
@@ -354,10 +353,9 @@ static bool subflow_hmac_valid(const struct request_sock *req,
const struct mptcp_subflow_request_sock *subflow_req;
u8 hmac[SHA256_DIGEST_SIZE];
struct mptcp_sock *msk;
- bool ret;
subflow_req = mptcp_subflow_rsk(req);
- msk = mptcp_token_get_sock(subflow_req->token);
+ msk = subflow_req->msk;
if (!msk)
return false;
@@ -365,12 +363,7 @@ static bool subflow_hmac_valid(const struct request_sock *req,
subflow_req->remote_nonce,
subflow_req->local_nonce, hmac);
- ret = true;
- if (crypto_memneq(hmac, mp_opt->hmac, MPTCPOPT_HMAC_LEN))
- ret = false;
-
- sock_put((struct sock *)msk);
- return ret;
+ return !crypto_memneq(hmac, mp_opt->hmac, MPTCPOPT_HMAC_LEN);
}
static void mptcp_sock_destruct(struct sock *sk)
@@ -522,10 +515,12 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
} else if (ctx->mp_join) {
struct mptcp_sock *owner;
- owner = mptcp_token_get_sock(ctx->token);
+ owner = subflow_req->msk;
if (!owner)
goto dispose_child;
+ /* move the msk reference ownership to the subflow */
+ subflow_req->msk = NULL;
ctx->conn = (struct sock *)owner;
if (!mptcp_finish_join(child))
goto dispose_child;
--
2.26.2
^ permalink raw reply related
* [PATCH net 2/2] mptcp: drop MP_JOIN request sock on syn cookies
From: Paolo Abeni @ 2020-06-17 10:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, mptcp
In-Reply-To: <cover.1592388398.git.pabeni@redhat.com>
Currently any MPTCP socket using syn cookies will fallback to
TCP at 3rd ack time. In case of MP_JOIN requests, the RFC mandate
closing the child and sockets, but the existing error paths
do not handle the syncookie scenario correctly.
Address the issue always forcing the child shutdown in case of
MP_JOIN fallback.
Fixes: ae2dd7164943 ("mptcp: handle tcp fallback when using syn cookies")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/subflow.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 4068bdb2523b..3838a0b3a21f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -431,22 +431,25 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
struct mptcp_subflow_context *listener = mptcp_subflow_ctx(sk);
struct mptcp_subflow_request_sock *subflow_req;
struct mptcp_options_received mp_opt;
- bool fallback_is_fatal = false;
+ bool fallback, fallback_is_fatal;
struct sock *new_msk = NULL;
- bool fallback = false;
struct sock *child;
pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
- /* we need later a valid 'mp_capable' value even when options are not
- * parsed
+ /* After child creation we must look for 'mp_capable' even when options
+ * are not parsed
*/
mp_opt.mp_capable = 0;
- if (tcp_rsk(req)->is_mptcp == 0)
+
+ /* hopefully temporary handling for MP_JOIN+syncookie */
+ subflow_req = mptcp_subflow_rsk(req);
+ fallback_is_fatal = subflow_req->mp_join;
+ fallback = !tcp_rsk(req)->is_mptcp;
+ if (fallback)
goto create_child;
/* if the sk is MP_CAPABLE, we try to fetch the client key */
- subflow_req = mptcp_subflow_rsk(req);
if (subflow_req->mp_capable) {
if (TCP_SKB_CB(skb)->seq != subflow_req->ssn_offset + 1) {
/* here we can receive and accept an in-window,
@@ -467,12 +470,11 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
if (!new_msk)
fallback = true;
} else if (subflow_req->mp_join) {
- fallback_is_fatal = true;
mptcp_get_options(skb, &mp_opt);
if (!mp_opt.mp_join ||
!subflow_hmac_valid(req, &mp_opt)) {
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
- return NULL;
+ fallback = true;
}
}
--
2.26.2
^ permalink raw reply related
* [PATCH net 0/2] mptcp: cope with syncookie on MP_JOINs
From: Paolo Abeni @ 2020-06-17 10:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, mptcp
Currently syncookies on MP_JOIN connections are not handled correctly: the
connections fallback to TCP and are kept alive instead of resetting them at
fallback time.
The first patch propagates the required information up to syn_recv_sock time,
and the 2nd patch addresses the unifying the error path for all MP_JOIN
requests.
Paolo Abeni (2):
mptcp: cache msk on MP_JOIN init_req
mptcp: drop MP_JOIN request sock on syn cookies
net/mptcp/protocol.h | 1 +
net/mptcp/subflow.c | 57 +++++++++++++++++++++-----------------------
2 files changed, 28 insertions(+), 30 deletions(-)
--
2.26.2
^ permalink raw reply
* Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block_cb
From: wenxu @ 2020-06-17 10:09 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Simon Horman, netdev, davem, vladbu
In-Reply-To: <20200617083817.GA1744@salvia>
On 6/17/2020 4:38 PM, Pablo Neira Ayuso wrote:
> On Wed, Jun 17, 2020 at 11:36:19AM +0800, wenxu wrote:
>> On 6/17/2020 4:38 AM, Pablo Neira Ayuso wrote:
>>> On Tue, Jun 16, 2020 at 05:47:17PM +0200, Simon Horman wrote:
>>>> On Tue, Jun 16, 2020 at 11:18:16PM +0800, wenxu wrote:
>>>>> 在 2020/6/16 22:34, Simon Horman 写道:
>>>>>> On Tue, Jun 16, 2020 at 10:20:46PM +0800, wenxu wrote:
>>>>>>> 在 2020/6/16 18:51, Simon Horman 写道:
>>>>>>>> On Tue, Jun 16, 2020 at 11:19:38AM +0800, wenxu@ucloud.cn wrote:
>>>>>>>>> From: wenxu <wenxu@ucloud.cn>
>>>>>>>>>
>>>>>>>>> In the function __flow_block_indr_cleanup, The match stataments
>>>>>>>>> this->cb_priv == cb_priv is always false, the flow_block_cb->cb_priv
>>>>>>>>> is totally different data with the flow_indr_dev->cb_priv.
>>>>>>>>>
>>>>>>>>> Store the representor cb_priv to the flow_block_cb->indr.cb_priv in
>>>>>>>>> the driver.
>>>>>>>>>
>>>>>>>>> Fixes: 1fac52da5942 ("net: flow_offload: consolidate indirect flow_block infrastructure")
>>>>>>>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>>>>>>>> Hi Wenxu,
>>>>>>>>
>>>>>>>> I wonder if this can be resolved by using the cb_ident field of struct
>>>>>>>> flow_block_cb.
>>>>>>>>
>>>>>>>> I observe that mlx5e_rep_indr_setup_block() seems to be the only call-site
>>>>>>>> where the value of the cb_ident parameter of flow_block_cb_alloc() is
>>>>>>>> per-block rather than per-device. So part of my proposal is to change
>>>>>>>> that.
>>>>>>> I check all the xxdriver_indr_setup_block. It seems all the cb_ident parameter of
>>>>>>>
>>>>>>> flow_block_cb_alloc is per-block. Both in the nfp_flower_setup_indr_tc_block
>>>>>>>
>>>>>>> and bnxt_tc_setup_indr_block.
>>>>>>>
>>>>>>>
>>>>>>> nfp_flower_setup_indr_tc_block:
>>>>>>>
>>>>>>> struct nfp_flower_indr_block_cb_priv *cb_priv;
>>>>>>>
>>>>>>> block_cb = flow_block_cb_alloc(nfp_flower_setup_indr_block_cb,
>>>>>>> cb_priv, cb_priv,
>>>>>>> nfp_flower_setup_indr_tc_release);
>>>>>>>
>>>>>>>
>>>>>>> bnxt_tc_setup_indr_block:
>>>>>>>
>>>>>>> struct bnxt_flower_indr_block_cb_priv *cb_priv;
>>>>>>>
>>>>>>> block_cb = flow_block_cb_alloc(bnxt_tc_setup_indr_block_cb,
>>>>>>> cb_priv, cb_priv,
>>>>>>> bnxt_tc_setup_indr_rel);
>>>>>>>
>>>>>>>
>>>>>>> And the function flow_block_cb_is_busy called in most place. Pass the
>>>>>>>
>>>>>>> parameter as cb_priv but not cb_indent .
>>>>>> Thanks, I see that now. But I still think it would be useful to understand
>>>>>> the purpose of cb_ident. It feels like it would lead to a clean solution
>>>>>> to the problem you have highlighted.
>>>>> I think The cb_ident means identify. It is used to identify the each flow block cb.
>>>>>
>>>>> In the both flow_block_cb_is_busy and flow_block_cb_lookup function check
>>>>>
>>>>> the block_cb->cb_ident == cb_ident.
>>>> Thanks, I think that I now see what you mean about the different scope of
>>>> cb_ident and your proposal to allow cleanup by flow_indr_dev_unregister().
>>>>
>>>> I do, however, still wonder if there is a nicer way than reaching into
>>>> the structure and manually setting block_cb->indr.cb_priv
>>>> at each call-site.
>>>>
>>>> Perhaps a variant of flow_block_cb_alloc() for indirect blocks
>>>> would be nicer?
>>> A follow up patch to add this new variant would be good. Probably
>>> __flow_block_indr_binding() can go away with this new variant to set
>>> up the indirect flow block.
>>
>> Maybe __flow_block_indr_binding() can't go away. The data and cleanup callback which should
>> init the flow_block_indr is only in the flow_indr_dev_setup_offload. This can't be gotten in
>> flow_indr_block_cb_alloc.
> Probably flow_indr_block_bind_cb_t can be updated to include the data
> and the cleanup callback.
Yes this can setup the indr_block info in the flow_indr_block_cb_alloc.
it also needs a flow_indr_block_cb_remove to handle the UNBIND setup.
>
^ permalink raw reply
* Re: [PATCH v2 01/02] net: phy: marvell: Add Marvell 88E1340 support
From: Maxim Kochetkov @ 2020-06-17 9:59 UTC (permalink / raw)
To: Russell King - ARM Linux admin; +Cc: netdev, andrew, f.fainelli, hkallweit1
In-Reply-To: <20200617084729.GN1551@shell.armlinux.org.uk>
I just copied this part from another marvell PHY description.
I can remove &-style reference for all marvell PHY's at next patch.
17.06.2020 11:47, Russell King - ARM Linux admin wrote:
> On Wed, Jun 17, 2020 at 07:52:45AM +0300, Maxim Kochetkov wrote:
>> Add Marvell 88E1340 support
>> Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
>> ---
>> drivers/net/phy/marvell.c | 23 +++++++++++++++++++++++
>> include/linux/marvell_phy.h | 1 +
>> 2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index 7fc8e10c5f33..4cc4e25fed2d 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -2459,6 +2459,28 @@ static struct phy_driver marvell_drivers[] = {
>> .get_tunable = m88e1540_get_tunable,
>> .set_tunable = m88e1540_set_tunable,
>> },
>> + {
>> + .phy_id = MARVELL_PHY_ID_88E1340S,
>> + .phy_id_mask = MARVELL_PHY_ID_MASK,
>> + .name = "Marvell 88E1340S",
>> + .probe = m88e1510_probe,
>> + /* PHY_GBIT_FEATURES */
>> + .config_init = &marvell_config_init,
>> + .config_aneg = &m88e1510_config_aneg,
>> + .read_status = &marvell_read_status,
>> + .ack_interrupt = &marvell_ack_interrupt,
>> + .config_intr = &marvell_config_intr,
>> + .did_interrupt = &m88e1121_did_interrupt,
>> + .resume = &genphy_resume,
>> + .suspend = &genphy_suspend,
>> + .read_page = marvell_read_page,
>> + .write_page = marvell_write_page,
>> + .get_sset_count = marvell_get_sset_count,
>> + .get_strings = marvell_get_strings,
>> + .get_stats = marvell_get_stats,
>> + .get_tunable = m88e1540_get_tunable,
>> + .set_tunable = m88e1540_set_tunable,
>
> Can we use a single style for referencing functions please? The kernel
> in general does not use &func, it's more typing than is necessary. The
> C99 standard says:
>
> 6.3.2.1 Lvalues, arrays, and function designators
>
> 4 A function designator is an expression that has function type.
> Except when it is the operand of the sizeof operator or the unary
> & operator, a function designator with type ``function returning
> type'' is converted to an expression that has type ``pointer to
> function returning type''.
>
> Hence,
>
> .resume = &genphy_resume
>
> and
>
> .resume = genphy_resume
>
> are equivalent but sizeof(genphy_resume) and sizeof(&genphy_resume) are
> not.
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH 4/4] Bluetooth: Add get/set device flags mgmt op
From: Marcel Holtmann @ 2020-06-17 9:58 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: Bluez mailing list, Alain Michaud, ChromeOS Bluetooth Upstreaming,
David S. Miller, Johan Hedberg, netdev, open list, Jakub Kicinski
In-Reply-To: <20200616210008.4.If379101eba01fd9f0903e04cc817eb2c8e7f7d96@changeid>
Hi Abhishek,
> Add the get device flags and set device flags mgmt ops and the device
> flags changed event. Their behavior is described in detail in
> mgmt-api.txt in bluez.
>
> Sample btmon trace when a HID device is added (trimmed to 75 chars):
>
> @ MGMT Command: Unknown (0x0050) plen 11 {0x0001} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 ...........
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0004} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0003} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0002} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Command Compl.. (0x0001) plen 10 {0x0001} [hci0] 18:06:14.98
> Unknown (0x0050) plen 7
> Status: Success (0x00)
> 90 c5 13 cd f3 cd 02 .......
> @ MGMT Command: Add Device (0x0033) plen 8 {0x0001} [hci0] 18:06:14.98
> LE Address: CD:F3:CD:13:C5:90 (Static)
> Action: Auto-connect remote device (0x02)
> @ MGMT Event: Device Added (0x001a) plen 8 {0x0004} [hci0] 18:06:14.98
> LE Address: CD:F3:CD:13:C5:90 (Static)
> Action: Auto-connect remote device (0x02)
> @ MGMT Event: Device Added (0x001a) plen 8 {0x0003} [hci0] 18:06:14.98
> LE Address: CD:F3:CD:13:C5:90 (Static)
> Action: Auto-connect remote device (0x02)
> @ MGMT Event: Device Added (0x001a) plen 8 {0x0002} [hci0] 18:06:14.98
> LE Address: CD:F3:CD:13:C5:90 (Static)
> Action: Auto-connect remote device (0x02)
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0004} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0003} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0002} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
> @ MGMT Event: Unknown (0x002a) plen 15 {0x0001} [hci0] 18:06:14.98
> 90 c5 13 cd f3 cd 02 01 00 00 00 01 00 00 00 ...............
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> ---
>
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/mgmt.h | 28 ++++++++
> net/bluetooth/hci_sock.c | 1 +
> net/bluetooth/mgmt.c | 134 +++++++++++++++++++++++++++++++++++
> 4 files changed, 164 insertions(+)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 16ab6ce8788341..5e03aac76ad47f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -259,6 +259,7 @@ enum {
> HCI_MGMT_LOCAL_NAME_EVENTS,
> HCI_MGMT_OOB_DATA_EVENTS,
> HCI_MGMT_EXP_FEATURE_EVENTS,
> + HCI_MGMT_DEVICE_FLAGS_EVENTS,
this part is not needed. We are doing this for commands where a client has to initiate a read command first before things get enabled. In this case the triggering command is Add Device and that has been there for a while. So no need to extra guard this.
> };
>
> /*
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index e515288f328f47..8e47b0c5fe52bb 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -720,6 +720,27 @@ struct mgmt_rp_set_exp_feature {
> #define MGMT_OP_SET_DEF_RUNTIME_CONFIG 0x004e
> #define MGMT_SET_DEF_RUNTIME_CONFIG_SIZE 0
>
> +#define MGMT_OP_GET_DEVICE_FLAGS 0x004F
> +#define MGMT_GET_DEVICE_FLAGS_SIZE 7
> +struct mgmt_cp_get_device_flags {
> + struct mgmt_addr_info addr;
> +} __packed;
> +struct mgmt_rp_get_device_flags {
> + struct mgmt_addr_info addr;
> + __le32 supported_flags;
> + __le32 current_flags;
> +} __packed;
> +
> +#define MGMT_OP_SET_DEVICE_FLAGS 0x0050
> +#define MGMT_SET_DEVICE_FLAGS_SIZE 11
> +struct mgmt_cp_set_device_flags {
> + struct mgmt_addr_info addr;
> + __le32 current_flags;
> +} __packed;
> +struct mgmt_rp_set_device_flags {
> + struct mgmt_addr_info addr;
> +} __packed;
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
> @@ -951,3 +972,10 @@ struct mgmt_ev_exp_feature_changed {
> __u8 uuid[16];
> __le32 flags;
> } __packed;
> +
> +#define MGMT_EV_DEVICE_FLAGS_CHANGED 0x002a
> +struct mgmt_ev_device_flags_changed {
> + struct mgmt_addr_info addr;
> + __le32 supported_flags;
> + __le32 current_flags;
> +} __packed;
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index d5627967fc254f..a7903b6206620c 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1354,6 +1354,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
> hci_sock_set_flag(sk, HCI_MGMT_SETTING_EVENTS);
> hci_sock_set_flag(sk, HCI_MGMT_DEV_CLASS_EVENTS);
> hci_sock_set_flag(sk, HCI_MGMT_LOCAL_NAME_EVENTS);
> + hci_sock_set_flag(sk, HCI_MGMT_DEVICE_FLAGS_EVENTS);
This is actually wrong. The other flags are there for event where you have multiple versions of the same event. If we ever introduce an Add Extended Device command, then yes, we need to guard things here. Right now, we don’t.
> }
> break;
> }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6d996e5e5bcc2d..2805f662d85695 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -114,6 +114,8 @@ static const u16 mgmt_commands[] = {
> MGMT_OP_SET_EXP_FEATURE,
> MGMT_OP_READ_DEF_SYSTEM_CONFIG,
> MGMT_OP_SET_DEF_SYSTEM_CONFIG,
> + MGMT_OP_GET_DEVICE_FLAGS,
> + MGMT_OP_SET_DEVICE_FLAGS,
> };
>
> static const u16 mgmt_events[] = {
> @@ -154,6 +156,7 @@ static const u16 mgmt_events[] = {
> MGMT_EV_EXT_INFO_CHANGED,
> MGMT_EV_PHY_CONFIGURATION_CHANGED,
> MGMT_EV_EXP_FEATURE_CHANGED,
> + MGMT_EV_DEVICE_FLAGS_CHANGED,
> };
>
> static const u16 mgmt_untrusted_commands[] = {
> @@ -3853,6 +3856,122 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> MGMT_STATUS_NOT_SUPPORTED);
> }
>
> +#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
> +
> +static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 data_len)
> +{
> + struct mgmt_cp_get_device_flags *cp = data;
> + struct mgmt_rp_get_device_flags rp;
> + struct bdaddr_list_with_flags *br_params;
> + struct hci_conn_params *params;
> + u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> + u32 current_flags = 0;
> + u8 status = MGMT_STATUS_INVALID_PARAMS;
> +
> + bt_dev_dbg(hdev, "Get device flags %pMR (type 0x%x)\n",
> + &cp->addr.bdaddr, cp->addr.type);
> +
> + if (cp->addr.type == BDADDR_BREDR) {
> + br_params = hci_bdaddr_list_lookup_with_flags(&hdev->whitelist,
> + &cp->addr.bdaddr,
> + cp->addr.type);
> + if (!br_params)
> + goto done;
> +
> + current_flags = br_params->current_flags;
> + } else {
> + params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> + le_addr_type(cp->addr.type));
> +
> + if (!params)
> + goto done;
> +
> + current_flags = params->current_flags;
> + }
> +
> + bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
> + rp.addr.type = cp->addr.type;
> + rp.supported_flags = cpu_to_le32(supported_flags);
> + rp.current_flags = cpu_to_le32(current_flags);
> +
> + status = MGMT_STATUS_SUCCESS;
> +
> +done:
> + return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_GET_DEVICE_FLAGS, status,
> + &rp, sizeof(rp));
> +}
> +
> +static int device_flags_changed(struct hci_dev *hdev, bdaddr_t *bdaddr,
> + u8 bdaddr_type, u32 supported_flags,
> + u32 current_flags, struct sock *skip)
> +{
> + struct mgmt_ev_device_flags_changed ev;
> +
> + bacpy(&ev.addr.bdaddr, bdaddr);
> + ev.addr.type = bdaddr_type;
> + ev.supported_flags = cpu_to_le32(supported_flags);
> + ev.current_flags = cpu_to_le32(current_flags);
> +
> + return mgmt_limited_event(MGMT_EV_DEVICE_FLAGS_CHANGED, hdev, &ev,
> + sizeof(ev), HCI_MGMT_DEVICE_FLAGS_EVENTS,
> + skip);
> +}
> +
> +static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 len)
> +{
> + struct mgmt_cp_set_device_flags *cp = data;
> + struct bdaddr_list_with_flags *br_params;
> + struct hci_conn_params *params;
> + u8 status = MGMT_STATUS_INVALID_PARAMS;
> + u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> + u32 current_flags = __le32_to_cpu(cp->current_flags);
> +
> + bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
> + &cp->addr.bdaddr, cp->addr.type,
> + __le32_to_cpu(current_flags));
> +
> + if ((supported_flags | current_flags) != supported_flags) {
> + bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)",
> + current_flags, supported_flags);
> + goto done;
> + }
> +
> + if (cp->addr.type == BDADDR_BREDR) {
> + br_params = hci_bdaddr_list_lookup_with_flags(&hdev->whitelist,
> + &cp->addr.bdaddr,
> + cp->addr.type);
> +
> + if (br_params) {
> + br_params->current_flags = current_flags;
> + status = MGMT_STATUS_SUCCESS;
> + } else {
> + bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)",
> + &cp->addr.bdaddr, cp->addr.type);
> + }
> + } else {
> + params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> + le_addr_type(cp->addr.type));
> + if (params) {
> + params->current_flags = current_flags;
> + status = MGMT_STATUS_SUCCESS;
> + } else {
> + bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
> + &cp->addr.bdaddr,
> + le_addr_type(cp->addr.type));
> + }
> + }
> +
> +done:
> + if (status == MGMT_STATUS_SUCCESS)
> + device_flags_changed(hdev, &cp->addr.bdaddr, cp->addr.type,
> + supported_flags, current_flags, sk);
> +
> + return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_FLAGS, status,
> + &cp->addr, sizeof(cp->addr));
> +}
> +
> static void read_local_oob_data_complete(struct hci_dev *hdev, u8 status,
> u16 opcode, struct sk_buff *skb)
> {
> @@ -5970,7 +6089,9 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> {
> struct mgmt_cp_add_device *cp = data;
> u8 auto_conn, addr_type;
> + struct hci_conn_params *params;
> int err;
> + u32 current_flags = 0;
>
> bt_dev_dbg(hdev, "sock %p", sk);
>
> @@ -6038,12 +6159,19 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> MGMT_STATUS_FAILED, &cp->addr,
> sizeof(cp->addr));
> goto unlock;
> + } else {
> + params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> + addr_type);
> + if (params)
> + current_flags = params->current_flags;
> }
>
> hci_update_background_scan(hdev);
>
> added:
> device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> + device_flags_changed(hdev, &cp->addr.bdaddr, cp->addr.type,
> + SUPPORTED_DEVICE_FLAGS(), current_flags, NULL);
>
> err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
> MGMT_STATUS_SUCCESS, &cp->addr,
> @@ -7306,6 +7434,12 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
> HCI_MGMT_UNTRUSTED },
> { set_def_system_config, MGMT_SET_DEF_SYSTEM_CONFIG_SIZE,
> HCI_MGMT_VAR_LEN },
> +
> + { NULL }, /* Read default runtime config */
> + { NULL }, /* Set default runtime config */
> +
> + { get_device_flags, MGMT_GET_DEVICE_FLAGS_SIZE },
> + { set_device_flags, MGMT_SET_DEVICE_FLAGS_SIZE },
> };
I have create a local tree that has the read/set runtime config commands already in there. I added your patches 1-3 to the tree already. I might just remove the HCI_MGMT_DEVICE_FLAGS_EVENTS and add this patch as well. Everything else looks good.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] net: macb: reject unsupported rgmii delays
From: Vladimir Oltean @ 2020-06-17 9:57 UTC (permalink / raw)
To: Helmut Grohne
Cc: Nicolas Ferre, David S. Miller, Jakub Kicinski, Russell King,
Palmer Dabbelt, Paul Walmsley, netdev
In-Reply-To: <20200616074955.GA9092@laureti-dev>
Hi Helmut,
On Tue, 16 Jun 2020 at 11:00, Helmut Grohne <helmut.grohne@intenta.de> wrote:
>
> The macb driver does not support configuring rgmii delays. At least for
> the Zynq GEM, delays are not supported by the hardware at all. However,
> the driver happily accepts and ignores any such delays.
>
> When operating in a mac to phy connection, the delay setting applies to
> the phy. Since the MAC does not support delays, the phy must provide
> them and the only supported mode is rgmii-id. However, in a fixed mac
> to mac connection, the delay applies to the mac itself. Therefore the
> only supported rgmii mode is rgmii.
>
> Link: https://lore.kernel.org/netdev/20200610081236.GA31659@laureti-dev/
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5b9d7c60eebc..bee5bf65e8b3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config,
> state->interface != PHY_INTERFACE_MODE_RMII &&
> state->interface != PHY_INTERFACE_MODE_GMII &&
> state->interface != PHY_INTERFACE_MODE_SGMII &&
> - !phy_interface_mode_is_rgmii(state->interface)) {
> + state->interface != PHY_INTERFACE_MODE_RGMII_ID) {
I don't think this change is correct though?
What if there were PCB traces in place, for whatever reason? Then the
driver would need to accept a phy with rgmii-txid, rgmii-rxid or
rgmii.
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> }
> @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp)
> struct phy_device *phydev;
> int ret;
>
> + if (of_phy_is_fixed_link(dn) &&
> + phy_interface_mode_is_rgmii(bp->phy_interface) &&
> + bp->phy_interface != PHY_INTERFACE_MODE_RGMII) {
> + netdev_err(dev, "RGMII delays are not supported\n");
> + return -EINVAL;
> + }
> +
Have you checked that this doesn't break any existing in-tree users?
> if (dn)
> ret = phylink_of_phy_connect(bp->phylink, dn, 0);
>
> --
> 2.20.1
>
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH] net: macb: reject unsupported rgmii delays
From: Nicolas Ferre @ 2020-06-17 9:24 UTC (permalink / raw)
To: Helmut Grohne, David S. Miller, Jakub Kicinski, Russell King,
Palmer Dabbelt, Paul Walmsley, netdev, Claudiu Beznea
In-Reply-To: <20200616074955.GA9092@laureti-dev>
On 16/06/2020 at 09:49, Helmut Grohne wrote:
> The macb driver does not support configuring rgmii delays. At least for
> the Zynq GEM, delays are not supported by the hardware at all. However,
> the driver happily accepts and ignores any such delays.
>
> When operating in a mac to phy connection, the delay setting applies to
> the phy. Since the MAC does not support delays, the phy must provide
> them and the only supported mode is rgmii-id. However, in a fixed mac
> to mac connection, the delay applies to the mac itself. Therefore the
> only supported rgmii mode is rgmii.
>
> Link: https://lore.kernel.org/netdev/20200610081236.GA31659@laureti-dev/
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5b9d7c60eebc..bee5bf65e8b3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config,
> state->interface != PHY_INTERFACE_MODE_RMII &&
> state->interface != PHY_INTERFACE_MODE_GMII &&
> state->interface != PHY_INTERFACE_MODE_SGMII &&
> - !phy_interface_mode_is_rgmii(state->interface)) {
> + state->interface != PHY_INTERFACE_MODE_RGMII_ID) {
Nitpicking: there's a comment just above, might be interesting to make
it more precisely matching this change. It mustn't delay addition though.
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> }
> @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp)
> struct phy_device *phydev;
> int ret;
>
> + if (of_phy_is_fixed_link(dn) &&
> + phy_interface_mode_is_rgmii(bp->phy_interface) &&
> + bp->phy_interface != PHY_INTERFACE_MODE_RGMII) {
> + netdev_err(dev, "RGMII delays are not supported\n");
> + return -EINVAL;
> + }
> +
Otherwise, it looks good to me after reading the associated discussion
link in your commit message: thanks for that!
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> if (dn)
> ret = phylink_of_phy_connect(bp->phylink, dn, 0);
>
> --
> 2.20.1
>
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
From: Jisheng Zhang @ 2020-06-17 9:09 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, Andrew Lunn, Russell King, David S. Miller,
netdev, linux-kernel
In-Reply-To: <e81ad573-ba30-a449-4529-d9a578ce0ee7@gmail.com>
On Fri, 15 May 2020 19:30:38 +0200 Heiner Kallweit wrote:
>
>
> On 15.05.2020 18:18, Florian Fainelli wrote:
> >
> >
> > On 5/15/2020 12:41 AM, Jisheng Zhang wrote:
> >> On Thu, 14 May 2020 21:50:53 +0200 Heiner Kallweit wrote:
> >>
> >>>
> >>>
> >>> On 14.05.2020 08:25, Jisheng Zhang wrote:
> >>>> On Wed, 13 May 2020 20:45:13 +0200 Heiner Kallweit wrote:
> >>>>
> >>>>>
> >>>>> On 13.05.2020 08:51, Jisheng Zhang wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Tue, 12 May 2020 20:43:40 +0200 Heiner Kallweit wrote:
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 12.05.2020 12:46, Jisheng Zhang wrote:
> >>>>>>>> The PHY Register Accessible Interrupt is enabled by default, so
> >>>>>>>> there's such an interrupt during init. In PHY POLL mode case, the
> >>>>>>>> INTB/PMEB pin is alway active, it is not good. Clear the interrupt by
> >>>>>>>> calling rtl8211f_ack_interrupt().
> >>>>>>>
> >>>>>>> As you say "it's not good" w/o elaborating a little bit more on it:
> >>>>>>> Do you face any actual issue? Or do you just think that it's not nice?
> >>>>>>
> >>>>>>
> >>>>>> The INTB/PMEB pin can be used in two different modes:
> >>>>>> INTB: used for interrupt
> >>>>>> PMEB: special mode for Wake-on-LAN
> >>>>>>
> >>>>>> The PHY Register Accessible Interrupt is enabled by
> >>>>>> default, there's always such an interrupt during the init. In PHY POLL mode
> >>>>>> case, the pin is always active. If platforms plans to use the INTB/PMEB pin
> >>>>>> as WOL, then the platform will see WOL active. It's not good.
> >>>>>>
> >>>>> The platform should listen to this pin only once WOL has been configured and
> >>>>> the pin has been switched to PMEB function. For the latter you first would
> >>>>> have to implement the set_wol callback in the PHY driver.
> >>>>> Or where in which code do you plan to switch the pin function to PMEB?
> >>>>
> >>>> I think it's better to switch the pin function in set_wol callback. But this
> >>>> is another story. No matter WOL has been configured or not, keeping the
> >>>> INTB/PMEB pin active is not good. what do you think?
> >>>>
> >>>
> >>> It shouldn't hurt (at least it didn't hurt for the last years), because no
> >>> listener should listen to the pin w/o having it configured before.
> >>> So better extend the PHY driver first (set_wol, ..), and then do the follow-up
> >>> platform changes (e.g. DT config of a connected GPIO).
> >>
> >> There are two sides involved here: the listener, it should not listen to the pin
> >> as you pointed out; the phy side, this patch tries to make the phy side
> >> behave normally -- not keep the INTB/PMEB pin always active. The listener
> >> side behaves correctly doesn't mean the phy side could keep the pin active.
> >>
> >> When .set_wol isn't implemented, this patch could make the system suspend/resume
> >> work properly.
> >>
> >> PS: even with set_wol implemented as configure the pin mode, I think we
> >> still need to clear the interrupt for phy poll mode either in set_wol
> >> or as this patch does.
> >
> > I agree with Jisheng here, Heiner, is there a reason you are pushing
> > back on the change? Acknowledging prior interrupts while configuring the
> > PHY is a common and established practice.
> >
> First it's about the justification of the change as such, and second about the
> question whether the change should be in the driver or in phylib.
>
> Acking interrupts we do already if the PHY is configured for interrupt mode,
> we call phy_clear_interrupt() at the beginning of phy_enable_interrupts()
> and at the end of phy_disable_interrupts().
> When using polling mode there is no strict need to ack interrupts.
> If we say however that interrupts should be acked in general, then I think
> it's not specific to RTL8211F, but it's something for phylib. Most likely
> we would have to add a call to phy_clear_interrupt() to phy_init_hw().
it's specific to RTL8211F from the following two PoV:
1. the PIN is shared between INTB and PMEB.
2. the PHY Register Accessible Interrupt is enabled by default
I didn't see such behaviors with other PHYs.
Thanks
^ permalink raw reply
* Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block_cb
From: Pablo Neira Ayuso @ 2020-06-17 9:03 UTC (permalink / raw)
To: wenxu; +Cc: netdev, davem, vladbu
In-Reply-To: <7d21a0b5-9f90-7f66-ae7a-80b0d9bbf2a1@ucloud.cn>
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On Wed, Jun 17, 2020 at 10:42:19AM +0800, wenxu wrote:
[...]
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> index ef7f6bc..042c285 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
> >> @@ -1918,6 +1918,7 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
> >>
> >> flow_block_cb_add(block_cb, f);
> >> list_add_tail(&block_cb->driver_list, &bnxt_block_cb_list);
> >> + block_cb->indr.cb_priv = bp;
> > cb_indent ?
> >
> > Why are you splitting the fix in multiple patches? This makes it
> > harder to review.
> >
> > I think patch 1/4, 2/4 and 4/4 belong to the same logical change?
> > Collapse them.
>
> I think patch 1/4, 2/4, 4/4 are different bugs， Although they are all in the new indirect
> flow_block infrastructure.
>
> patch1 fix the miss cleanup for flow_block_cb of flowtable
>
> patch2 fix the incorrect check the cb_priv of flow_block_cb
>
> patch4 fix the miss driver_list del in the cleanup callback
>
> So maybe make them alone is better?
Maybe.
I'm attaching the collapsed patch for preview.
10 files changed, 21 insertions(+), 15 deletions(-)
This one single collapsed patch is small.
And it looks like it is part of the same logic fix for the .cleanup
path? You had to update three aspects to make the cleanup path work.
3/4 is a different code path, leaving it standalone makes sense to me,
on top of this one probably.
This is just a proposal.
Thank you.
[-- Attachment #2: 0001-net-flow_offload-fix-flow_indr_dev_unregister-path.patch --]
[-- Type: text/x-diff, Size: 9820 bytes --]
From 8a17ca6fa7852e359aa5fad01ecdda4f5ae33eeb Mon Sep 17 00:00:00 2001
From: wenxu <wenxu@ucloud.cn>
Date: Sat, 13 Jun 2020 17:25:59 +0800
Subject: [PATCH RFC] net: flow_offload: fix flow_indr_dev_unregister path
If the representor is removed, then identify the indirect flow_blocks
that need to be removed by the release callback and the port representor
structure. To identify the port representor structure, a new
indr.cb_priv field needs to be introduced. The flow_block also needs to
be removed from the driver list from the cleanup path.
Fixes: 1fac52da5942 ("net: flow_offload: consolidate indirect flow_block infrastructure")
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 3 ++-
drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c | 4 ++--
drivers/net/ethernet/netronome/nfp/flower/main.c | 2 +-
drivers/net/ethernet/netronome/nfp/flower/main.h | 3 +--
drivers/net/ethernet/netronome/nfp/flower/offload.c | 7 ++++---
include/net/flow_offload.h | 3 ++-
net/core/flow_offload.c | 11 ++++++-----
net/netfilter/nf_flow_table_offload.c | 1 +
net/netfilter/nf_tables_offload.c | 1 +
net/sched/cls_api.c | 1 +
10 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 0eef4f5e4a46..042c2850fcff 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1918,6 +1918,7 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
flow_block_cb_add(block_cb, f);
list_add_tail(&block_cb->driver_list, &bnxt_block_cb_list);
+ block_cb->indr.cb_priv = bp;
break;
case FLOW_BLOCK_UNBIND:
cb_priv = bnxt_tc_indr_block_cb_lookup(bp, netdev);
@@ -2074,7 +2075,7 @@ void bnxt_shutdown_tc(struct bnxt *bp)
return;
flow_indr_dev_unregister(bnxt_tc_setup_indr_cb, bp,
- bnxt_tc_setup_indr_block_cb);
+ bnxt_tc_setup_indr_rel);
rhashtable_destroy(&tc_info->flow_table);
rhashtable_destroy(&tc_info->l2_table);
rhashtable_destroy(&tc_info->decap_l2_table);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
index 80713123de5c..187f84c2ec23 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
@@ -447,7 +447,7 @@ mlx5e_rep_indr_setup_block(struct net_device *netdev,
}
flow_block_cb_add(block_cb, f);
list_add_tail(&block_cb->driver_list, &mlx5e_block_cb_list);
-
+ block_cb->indr.cb_priv = rpriv;
return 0;
case FLOW_BLOCK_UNBIND:
indr_priv = mlx5e_rep_indr_block_priv_lookup(rpriv, netdev);
@@ -496,7 +496,7 @@ int mlx5e_rep_tc_netdevice_event_register(struct mlx5e_rep_priv *rpriv)
void mlx5e_rep_tc_netdevice_event_unregister(struct mlx5e_rep_priv *rpriv)
{
flow_indr_dev_unregister(mlx5e_rep_indr_setup_cb, rpriv,
- mlx5e_rep_indr_setup_tc_cb);
+ mlx5e_rep_indr_block_unbind);
}
#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index c39327677a7d..bb448c82cdc2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -861,7 +861,7 @@ static void nfp_flower_clean(struct nfp_app *app)
flush_work(&app_priv->cmsg_work);
flow_indr_dev_unregister(nfp_flower_indr_setup_tc_cb, app,
- nfp_flower_setup_indr_block_cb);
+ nfp_flower_setup_indr_tc_release);
if (app_priv->flower_ext_feats & NFP_FL_FEATS_VF_RLIM)
nfp_flower_qos_cleanup(app);
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 6c3dc3baf387..c98333799f89 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -460,8 +460,7 @@ int nfp_flower_setup_qos_offload(struct nfp_app *app, struct net_device *netdev,
void nfp_flower_stats_rlim_reply(struct nfp_app *app, struct sk_buff *skb);
int nfp_flower_indr_setup_tc_cb(struct net_device *netdev, void *cb_priv,
enum tc_setup_type type, void *type_data);
-int nfp_flower_setup_indr_block_cb(enum tc_setup_type type, void *type_data,
- void *cb_priv);
+void nfp_flower_setup_indr_tc_release(void *cb_priv);
void
__nfp_flower_non_repr_priv_get(struct nfp_flower_non_repr_priv *non_repr_priv);
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 695d24b9dd92..ca2f01a3418d 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -1619,8 +1619,8 @@ nfp_flower_indr_block_cb_priv_lookup(struct nfp_app *app,
return NULL;
}
-int nfp_flower_setup_indr_block_cb(enum tc_setup_type type,
- void *type_data, void *cb_priv)
+static int nfp_flower_setup_indr_block_cb(enum tc_setup_type type,
+ void *type_data, void *cb_priv)
{
struct nfp_flower_indr_block_cb_priv *priv = cb_priv;
struct flow_cls_offload *flower = type_data;
@@ -1637,7 +1637,7 @@ int nfp_flower_setup_indr_block_cb(enum tc_setup_type type,
}
}
-static void nfp_flower_setup_indr_tc_release(void *cb_priv)
+void nfp_flower_setup_indr_tc_release(void *cb_priv)
{
struct nfp_flower_indr_block_cb_priv *priv = cb_priv;
@@ -1687,6 +1687,7 @@ nfp_flower_setup_indr_tc_block(struct net_device *netdev, struct nfp_app *app,
flow_block_cb_add(block_cb, f);
list_add_tail(&block_cb->driver_list, &nfp_block_cb_list);
+ block_cb->indr.cb_priv = app;
return 0;
case FLOW_BLOCK_UNBIND:
cb_priv = nfp_flower_indr_block_cb_priv_lookup(app, netdev);
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index f2c8311a0433..ef4d8b0e304c 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -450,6 +450,7 @@ struct flow_block_indr {
struct net_device *dev;
enum flow_block_binder_type binder_type;
void *data;
+ void *cb_priv;
void (*cleanup)(struct flow_block_cb *block_cb);
};
@@ -536,7 +537,7 @@ typedef int flow_indr_block_bind_cb_t(struct net_device *dev, void *cb_priv,
int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv);
void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
- flow_setup_cb_t *setup_cb);
+ void (*release)(void *cb_priv));
int flow_indr_dev_setup_offload(struct net_device *dev,
enum tc_setup_type type, void *data,
struct flow_block_offload *bo,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 0cfc35e6be28..66143518f1d4 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -372,14 +372,15 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
}
EXPORT_SYMBOL(flow_indr_dev_register);
-static void __flow_block_indr_cleanup(flow_setup_cb_t *setup_cb, void *cb_priv,
+static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
+ void *cb_priv,
struct list_head *cleanup_list)
{
struct flow_block_cb *this, *next;
list_for_each_entry_safe(this, next, &flow_block_indr_list, indr.list) {
- if (this->cb == setup_cb &&
- this->cb_priv == cb_priv) {
+ if (this->release == release &&
+ this->indr.cb_priv == cb_priv) {
list_move(&this->indr.list, cleanup_list);
return;
}
@@ -397,7 +398,7 @@ static void flow_block_indr_notify(struct list_head *cleanup_list)
}
void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
- flow_setup_cb_t *setup_cb)
+ void (*release)(void *cb_priv))
{
struct flow_indr_dev *this, *next, *indr_dev = NULL;
LIST_HEAD(cleanup_list);
@@ -418,7 +419,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
return;
}
- __flow_block_indr_cleanup(setup_cb, cb_priv, &cleanup_list);
+ __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
mutex_unlock(&flow_indr_block_lock);
flow_block_indr_notify(&cleanup_list);
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 62651e6683f6..5fff1e040168 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -950,6 +950,7 @@ static void nf_flow_table_indr_cleanup(struct flow_block_cb *block_cb)
nf_flow_table_gc_cleanup(flowtable, dev);
down_write(&flowtable->flow_block_lock);
list_del(&block_cb->list);
+ list_del(&block_cb->driver_list);
flow_block_cb_free(block_cb);
up_write(&flowtable->flow_block_lock);
}
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 185fc82c99aa..c7cf1cde46de 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -296,6 +296,7 @@ static void nft_indr_block_cleanup(struct flow_block_cb *block_cb)
nft_flow_block_offload_init(&bo, dev_net(dev), FLOW_BLOCK_UNBIND,
basechain, &extack);
mutex_lock(&net->nft.commit_mutex);
+ list_del(&block_cb->driver_list);
list_move(&block_cb->list, &bo.cb_list);
nft_flow_offload_unbind(&bo, basechain);
mutex_unlock(&net->nft.commit_mutex);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a00a203b2ef5..f8028d73edf1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -652,6 +652,7 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
&block->flow_block, tcf_block_shared(block),
&extack);
down_write(&block->cb_lock);
+ list_del(&block_cb->driver_list);
list_move(&block_cb->list, &bo.cb_list);
up_write(&block->cb_lock);
rtnl_lock();
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 01/02] net: phy: marvell: Add Marvell 88E1340 support
From: Russell King - ARM Linux admin @ 2020-06-17 8:47 UTC (permalink / raw)
To: Maxim Kochetkov; +Cc: netdev, andrew, f.fainelli, hkallweit1
In-Reply-To: <05f6912b-d529-ae7d-183e-efa6951e94b7@inbox.ru>
On Wed, Jun 17, 2020 at 07:52:45AM +0300, Maxim Kochetkov wrote:
> Add Marvell 88E1340 support
> Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
> ---
> drivers/net/phy/marvell.c | 23 +++++++++++++++++++++++
> include/linux/marvell_phy.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 7fc8e10c5f33..4cc4e25fed2d 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2459,6 +2459,28 @@ static struct phy_driver marvell_drivers[] = {
> .get_tunable = m88e1540_get_tunable,
> .set_tunable = m88e1540_set_tunable,
> },
> + {
> + .phy_id = MARVELL_PHY_ID_88E1340S,
> + .phy_id_mask = MARVELL_PHY_ID_MASK,
> + .name = "Marvell 88E1340S",
> + .probe = m88e1510_probe,
> + /* PHY_GBIT_FEATURES */
> + .config_init = &marvell_config_init,
> + .config_aneg = &m88e1510_config_aneg,
> + .read_status = &marvell_read_status,
> + .ack_interrupt = &marvell_ack_interrupt,
> + .config_intr = &marvell_config_intr,
> + .did_interrupt = &m88e1121_did_interrupt,
> + .resume = &genphy_resume,
> + .suspend = &genphy_suspend,
> + .read_page = marvell_read_page,
> + .write_page = marvell_write_page,
> + .get_sset_count = marvell_get_sset_count,
> + .get_strings = marvell_get_strings,
> + .get_stats = marvell_get_stats,
> + .get_tunable = m88e1540_get_tunable,
> + .set_tunable = m88e1540_set_tunable,
Can we use a single style for referencing functions please? The kernel
in general does not use &func, it's more typing than is necessary. The
C99 standard says:
6.3.2.1 Lvalues, arrays, and function designators
4 A function designator is an expression that has function type.
Except when it is the operand of the sizeof operator or the unary
& operator, a function designator with type ``function returning
type'' is converted to an expression that has type ``pointer to
function returning type''.
Hence,
.resume = &genphy_resume
and
.resume = genphy_resume
are equivalent but sizeof(genphy_resume) and sizeof(&genphy_resume) are
not.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH 06/12] xen-blkfront: add callbacks for PM suspend and hibernation]
From: Roger Pau Monné @ 2020-06-17 8:38 UTC (permalink / raw)
To: Anchal Agarwal
Cc: Boris Ostrovsky, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, x86@kernel.org, jgross@suse.com,
linux-pm@vger.kernel.org, linux-mm@kvack.org, Kamata, Munehisa,
sstabellini@kernel.org, konrad.wilk@oracle.com, axboe@kernel.dk,
davem@davemloft.net, rjw@rjwysocki.net, len.brown@intel.com,
pavel@ucw.cz, peterz@infradead.org, Valentin, Eduardo,
Singh, Balbir, xen-devel@lists.xenproject.org,
vkuznets@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Woodhouse, David,
benh@kernel.crashing.org
In-Reply-To: <20200616223003.GA28769@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>
On Tue, Jun 16, 2020 at 10:30:03PM +0000, Anchal Agarwal wrote:
> On Tue, Jun 16, 2020 at 09:49:25PM +0000, Anchal Agarwal wrote:
> > On Thu, Jun 04, 2020 at 09:05:48AM +0200, Roger Pau Monné wrote:
> > > On Wed, Jun 03, 2020 at 11:33:52PM +0000, Agarwal, Anchal wrote:
> > > > On Tue, May 19, 2020 at 11:27:50PM +0000, Anchal Agarwal wrote:
> > > > > From: Munehisa Kamata <kamatam@amazon.com>
> > > > > + xenbus_dev_error(dev, err, "Freezing timed out;"
> > > > > + "the device may become inconsistent state");
> > > >
> > > > Leaving the device in this state is quite bad, as it's in a closed
> > > > state and with the queues frozen. You should make an attempt to
> > > > restore things to a working state.
> > > >
> > > > You mean if backend closed after timeout? Is there a way to know that? I understand it's not good to
> > > > leave it in this state however, I am still trying to find if there is a good way to know if backend is still connected after timeout.
> > > > Hence the message " the device may become inconsistent state". I didn't see a timeout not even once on my end so that's why
> > > > I may be looking for an alternate perspective here. may be need to thaw everything back intentionally is one thing I could think of.
> > >
> > > You can manually force this state, and then check that it will behave
> > > correctly. I would expect that on a failure to disconnect from the
> > > backend you should switch the frontend to the 'Init' state in order to
> > > try to reconnect to the backend when possible.
> > >
> > From what I understand forcing manually is, failing the freeze without
> > disconnect and try to revive the connection by unfreezing the
> > queues->reconnecting to backend [which never got diconnected]. May be even
> > tearing down things manually because I am not sure what state will frontend
> > see if backend fails to to disconnect at any point in time. I assumed connected.
> > Then again if its "CONNECTED" I may not need to tear down everything and start
> > from Initialising state because that may not work.
> >
> > So I am not so sure about backend's state so much, lets say if xen_blkif_disconnect fail,
> > I don't see it getting handled in the backend then what will be backend's state?
> > Will it still switch xenbus state to 'Closed'? If not what will frontend see,
> > if it tries to read backend's state through xenbus_read_driver_state ?
> >
> > So the flow be like:
> > Front end marks XenbusStateClosing
> > Backend marks its state as XenbusStateClosing
> > Frontend marks XenbusStateClosed
> > Backend disconnects calls xen_blkif_disconnect
> > Backend fails to disconnect, the above function returns EBUSY
> > What will be state of backend here?
> > Frontend did not tear down the rings if backend does not switches the
> > state to 'Closed' in case of failure.
> >
> > If backend stays in CONNECTED state, then even if we mark it Initialised in frontend, backend
> > won't be calling connect(). {From reading code in frontend_changed}
> > IMU, Initialising will fail since backend dev->state != XenbusStateClosed plus
> > we did not tear down anything so calling talk_to_blkback may not be needed
> >
> > Does that sound correct?
> Send that too quickly, I also meant to add XenBusIntialised state should be ok
> only if we expect backend will stay in "Connected" state. Also, I experimented
> with that notion. I am little worried about the correctness here.
> Can the backend come to an Unknown state somehow?
Not really, there's no such thing as an Unknown state.
There are no guarantees about what a backend can do really, so it
could indeed switch to a not recognized state, but that would be a
bug in the backend.
Roger.
^ permalink raw reply
* Re: [PATCH net v3 2/4] flow_offload: fix incorrect cb_priv check for flow_block_cb
From: Pablo Neira Ayuso @ 2020-06-17 8:38 UTC (permalink / raw)
To: wenxu; +Cc: Simon Horman, netdev, davem, vladbu
In-Reply-To: <d53fe351-6761-693c-7421-d489876eb3ad@ucloud.cn>
On Wed, Jun 17, 2020 at 11:36:19AM +0800, wenxu wrote:
>
> On 6/17/2020 4:38 AM, Pablo Neira Ayuso wrote:
> > On Tue, Jun 16, 2020 at 05:47:17PM +0200, Simon Horman wrote:
> >> On Tue, Jun 16, 2020 at 11:18:16PM +0800, wenxu wrote:
> >>> 在 2020/6/16 22:34, Simon Horman 写道:
> >>>> On Tue, Jun 16, 2020 at 10:20:46PM +0800, wenxu wrote:
> >>>>> 在 2020/6/16 18:51, Simon Horman 写道:
> >>>>>> On Tue, Jun 16, 2020 at 11:19:38AM +0800, wenxu@ucloud.cn wrote:
> >>>>>>> From: wenxu <wenxu@ucloud.cn>
> >>>>>>>
> >>>>>>> In the function __flow_block_indr_cleanup, The match stataments
> >>>>>>> this->cb_priv == cb_priv is always false, the flow_block_cb->cb_priv
> >>>>>>> is totally different data with the flow_indr_dev->cb_priv.
> >>>>>>>
> >>>>>>> Store the representor cb_priv to the flow_block_cb->indr.cb_priv in
> >>>>>>> the driver.
> >>>>>>>
> >>>>>>> Fixes: 1fac52da5942 ("net: flow_offload: consolidate indirect flow_block infrastructure")
> >>>>>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
> >>>>>> Hi Wenxu,
> >>>>>>
> >>>>>> I wonder if this can be resolved by using the cb_ident field of struct
> >>>>>> flow_block_cb.
> >>>>>>
> >>>>>> I observe that mlx5e_rep_indr_setup_block() seems to be the only call-site
> >>>>>> where the value of the cb_ident parameter of flow_block_cb_alloc() is
> >>>>>> per-block rather than per-device. So part of my proposal is to change
> >>>>>> that.
> >>>>> I check all the xxdriver_indr_setup_block. It seems all the cb_ident parameter of
> >>>>>
> >>>>> flow_block_cb_alloc is per-block. Both in the nfp_flower_setup_indr_tc_block
> >>>>>
> >>>>> and bnxt_tc_setup_indr_block.
> >>>>>
> >>>>>
> >>>>> nfp_flower_setup_indr_tc_block:
> >>>>>
> >>>>> struct nfp_flower_indr_block_cb_priv *cb_priv;
> >>>>>
> >>>>> block_cb = flow_block_cb_alloc(nfp_flower_setup_indr_block_cb,
> >>>>> cb_priv, cb_priv,
> >>>>> nfp_flower_setup_indr_tc_release);
> >>>>>
> >>>>>
> >>>>> bnxt_tc_setup_indr_block:
> >>>>>
> >>>>> struct bnxt_flower_indr_block_cb_priv *cb_priv;
> >>>>>
> >>>>> block_cb = flow_block_cb_alloc(bnxt_tc_setup_indr_block_cb,
> >>>>> cb_priv, cb_priv,
> >>>>> bnxt_tc_setup_indr_rel);
> >>>>>
> >>>>>
> >>>>> And the function flow_block_cb_is_busy called in most place. Pass the
> >>>>>
> >>>>> parameter as cb_priv but not cb_indent .
> >>>> Thanks, I see that now. But I still think it would be useful to understand
> >>>> the purpose of cb_ident. It feels like it would lead to a clean solution
> >>>> to the problem you have highlighted.
> >>> I think The cb_ident means identify. It is used to identify the each flow block cb.
> >>>
> >>> In the both flow_block_cb_is_busy and flow_block_cb_lookup function check
> >>>
> >>> the block_cb->cb_ident == cb_ident.
> >> Thanks, I think that I now see what you mean about the different scope of
> >> cb_ident and your proposal to allow cleanup by flow_indr_dev_unregister().
> >>
> >> I do, however, still wonder if there is a nicer way than reaching into
> >> the structure and manually setting block_cb->indr.cb_priv
> >> at each call-site.
> >>
> >> Perhaps a variant of flow_block_cb_alloc() for indirect blocks
> >> would be nicer?
> > A follow up patch to add this new variant would be good. Probably
> > __flow_block_indr_binding() can go away with this new variant to set
> > up the indirect flow block.
>
>
> Maybe __flow_block_indr_binding() can't go away. The data and cleanup callback which should
> init the flow_block_indr is only in the flow_indr_dev_setup_offload. This can't be gotten in
> flow_indr_block_cb_alloc.
Probably flow_indr_block_bind_cb_t can be updated to include the data
and the cleanup callback.
^ permalink raw reply
* Re: [PATCH 06/12] xen-blkfront: add callbacks for PM suspend and hibernation]
From: Roger Pau Monné @ 2020-06-17 8:35 UTC (permalink / raw)
To: Anchal Agarwal
Cc: Boris Ostrovsky, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, x86@kernel.org, jgross@suse.com,
linux-pm@vger.kernel.org, linux-mm@kvack.org, Kamata, Munehisa,
sstabellini@kernel.org, konrad.wilk@oracle.com, axboe@kernel.dk,
davem@davemloft.net, rjw@rjwysocki.net, len.brown@intel.com,
pavel@ucw.cz, peterz@infradead.org, Valentin, Eduardo,
Singh, Balbir, xen-devel@lists.xenproject.org,
vkuznets@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Woodhouse, David,
benh@kernel.crashing.org
In-Reply-To: <20200616214925.GA21684@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>
On Tue, Jun 16, 2020 at 09:49:25PM +0000, Anchal Agarwal wrote:
> On Thu, Jun 04, 2020 at 09:05:48AM +0200, Roger Pau Monné wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > On Wed, Jun 03, 2020 at 11:33:52PM +0000, Agarwal, Anchal wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > + xenbus_dev_error(dev, err, "Freezing timed out;"
> > > > + "the device may become inconsistent state");
> > >
> > > Leaving the device in this state is quite bad, as it's in a closed
> > > state and with the queues frozen. You should make an attempt to
> > > restore things to a working state.
> > >
> > > You mean if backend closed after timeout? Is there a way to know that? I understand it's not good to
> > > leave it in this state however, I am still trying to find if there is a good way to know if backend is still connected after timeout.
> > > Hence the message " the device may become inconsistent state". I didn't see a timeout not even once on my end so that's why
> > > I may be looking for an alternate perspective here. may be need to thaw everything back intentionally is one thing I could think of.
> >
> > You can manually force this state, and then check that it will behave
> > correctly. I would expect that on a failure to disconnect from the
> > backend you should switch the frontend to the 'Init' state in order to
> > try to reconnect to the backend when possible.
> >
> From what I understand forcing manually is, failing the freeze without
> disconnect and try to revive the connection by unfreezing the
> queues->reconnecting to backend [which never got diconnected]. May be even
> tearing down things manually because I am not sure what state will frontend
> see if backend fails to to disconnect at any point in time. I assumed connected.
> Then again if its "CONNECTED" I may not need to tear down everything and start
> from Initialising state because that may not work.
>
> So I am not so sure about backend's state so much, lets say if xen_blkif_disconnect fail,
> I don't see it getting handled in the backend then what will be backend's state?
> Will it still switch xenbus state to 'Closed'? If not what will frontend see,
> if it tries to read backend's state through xenbus_read_driver_state ?
>
> So the flow be like:
> Front end marks XenbusStateClosing
> Backend marks its state as XenbusStateClosing
> Frontend marks XenbusStateClosed
> Backend disconnects calls xen_blkif_disconnect
> Backend fails to disconnect, the above function returns EBUSY
> What will be state of backend here?
Backend should stay in state 'Closing' then, until it can finish
tearing down.
> Frontend did not tear down the rings if backend does not switches the
> state to 'Closed' in case of failure.
>
> If backend stays in CONNECTED state, then even if we mark it Initialised in frontend, backend
Backend will stay in state 'Closing' I think.
> won't be calling connect(). {From reading code in frontend_changed}
> IMU, Initialising will fail since backend dev->state != XenbusStateClosed plus
> we did not tear down anything so calling talk_to_blkback may not be needed
>
> Does that sound correct?
I think switching to the initial state in order to try to attempt a
reconnection would be our best bet here.
Thanks, Roger.
^ permalink raw reply
* [net-next PATCH 4/5 v2] net: dsa: rtl8366: VLAN 0 as disable tagging
From: Linus Walleij @ 2020-06-17 8:31 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
In-Reply-To: <20200617083132.1847234-1-linus.walleij@linaro.org>
The code in net/8021q/vlan.c, vlan_device_event() sets
VLAN 0 for a VLAN-capable ethernet device when it
comes up.
Since the RTL8366 DSA switches must have a VLAN and
PVID set up for any packets to come through we have
already set up default VLAN for each port as part of
bringing the switch online.
Make sure that setting VLAN 0 has the same effect
and does not try to actually tell the hardware to use
VLAN 0 on the port because that will not work.
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.8-rc1 and other changes.
---
drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------
1 file changed, 52 insertions(+), 13 deletions(-)
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 66bd1241204c..7f0691a6da13 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
struct realtek_smi *smi = ds->priv;
+ u16 vid_begin = vlan->vid_begin;
+ u16 vid_end = vlan->vid_end;
u16 vid;
int ret;
- for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
+ if (vid_begin == 0) {
+ dev_info(smi->dev, "prepare VLAN 0 - ignored\n");
+ if (vid_end == 0)
+ return 0;
+ /* Skip VLAN 0 and start with VLAN 1 */
+ vid_begin = 1;
+ }
+
+ for (vid = vid_begin; vid < vid_end; vid++)
if (!smi->ops->is_vlan_valid(smi, vid))
return -EINVAL;
dev_info(smi->dev, "prepare VLANs %04x..%04x\n",
- vlan->vid_begin, vlan->vid_end);
+ vid_begin, vid_end);
/* Enable VLAN in the hardware
* FIXME: what's with this 4k business?
@@ -383,27 +393,46 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
bool untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
struct realtek_smi *smi = ds->priv;
+ u16 vid_begin = vlan->vid_begin;
+ u16 vid_end = vlan->vid_end;
u32 member = 0;
u32 untag = 0;
u16 vid;
int ret;
- for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
- if (!smi->ops->is_vlan_valid(smi, vid))
+ if (vid_begin == 0) {
+ dev_info(smi->dev, "set VLAN 0 on port %d = default VLAN\n",
+ port);
+ /* Set up default tagging */
+ ret = rtl8366_set_default_vlan_and_pvid(smi, port);
+ if (ret) {
+ dev_err(smi->dev,
+ "error setting default VLAN on port %d\n",
+ port);
return;
+ }
+ if (vid_end == 0)
+ return;
+ /* Skip VLAN 0 and start with VLAN 1 */
+ vid_begin = 1;
+ }
- dev_info(smi->dev, "add VLAN on port %d, %s, %s\n",
- port,
- untagged ? "untagged" : "tagged",
- pvid ? " PVID" : "no PVID");
+ for (vid = vid_begin; vid < vid_end; vid++)
+ if (!smi->ops->is_vlan_valid(smi, vid))
+ return;
if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
dev_err(smi->dev, "port is DSA or CPU port\n");
- for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+ for (vid = vid_begin; vid <= vid_end; ++vid) {
int pvid_val = 0;
- dev_info(smi->dev, "add VLAN %04x\n", vid);
+ dev_info(smi->dev, "add VLAN %04x to port %d, %s, %s\n",
+ vid,
+ port,
+ untagged ? "untagged" : "tagged",
+ pvid ? " PVID" : "no PVID");
+
member |= BIT(port);
if (untagged)
@@ -437,15 +466,25 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
struct realtek_smi *smi = ds->priv;
+ u16 vid_begin = vlan->vid_begin;
+ u16 vid_end = vlan->vid_end;
u16 vid;
int ret;
- dev_info(smi->dev, "del VLAN on port %d\n", port);
+ if (vid_begin == 0) {
+ dev_info(smi->dev, "remove port %d from VLAN 0 (no-op)\n",
+ port);
+ if (vid_end == 0)
+ return 0;
+ /* Skip VLAN 0 and start with VLAN 1 */
+ vid_begin = 1;
+ }
- for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+ for (vid = vid_begin; vid <= vid_end; ++vid) {
int i;
- dev_info(smi->dev, "del VLAN %04x\n", vid);
+ dev_info(smi->dev, "remove VLAN %04x from port %d\n",
+ vid, port);
for (i = 0; i < smi->num_vlan_mc; i++) {
struct rtl8366_vlan_mc vlanmc;
--
2.26.2
^ permalink raw reply related
* [net-next PATCH 5/5 v2] net: dsa: rtl8366: Use top VLANs for default
From: Linus Walleij @ 2020-06-17 8:31 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
In-Reply-To: <20200617083132.1847234-1-linus.walleij@linaro.org>
The RTL8366 DSA switches will not work unless we set
up a default VLAN for each port. We are currently using
e.g. VLAN 1..6 for a 5-port switch as default VLANs.
This is not very helpful for users, move it to allocate
the top VLANs for default instead, for example on
RTL8366RB there are 16 VLANs so instead of using
VLAN 1..6 as default use VLAN 10..15 so VLAN 1
thru VLAN 9 is available for users.
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebase on v5.8-rc1.
---
drivers/net/dsa/rtl8366.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 7f0691a6da13..4e7562b41598 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -260,8 +260,8 @@ static int rtl8366_set_default_vlan_and_pvid(struct realtek_smi *smi,
u16 vid;
int ret;
- /* This is the reserved default VLAN for this port */
- vid = port + 1;
+ /* Use the top VLANs for per-port default VLAN */
+ vid = smi->num_vlan_mc - smi->num_ports + port;
if (port == smi->cpu_port)
/* For the CPU port, make all ports members of this
--
2.26.2
^ permalink raw reply related
* [net-next PATCH 3/5 v2] net: dsa: rtl8366: Split out default VLAN config
From: Linus Walleij @ 2020-06-17 8:31 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
In-Reply-To: <20200617083132.1847234-1-linus.walleij@linaro.org>
We loop over the ports to initialize the default VLAN
and PVID for each port. As we need to reuse the
code to reinitialize a single port, break out the
function rtl8366_set_default_vlan_and_pvid().
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.8-rc1 and other changes.
---
drivers/net/dsa/rtl8366.c | 70 ++++++++++++++++++++++++---------------
1 file changed, 43 insertions(+), 27 deletions(-)
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index ac88caca5ad4..66bd1241204c 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -253,6 +253,48 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
}
EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
+static int rtl8366_set_default_vlan_and_pvid(struct realtek_smi *smi,
+ int port)
+{
+ u32 mask;
+ u16 vid;
+ int ret;
+
+ /* This is the reserved default VLAN for this port */
+ vid = port + 1;
+
+ if (port == smi->cpu_port)
+ /* For the CPU port, make all ports members of this
+ * VLAN.
+ */
+ mask = GENMASK(smi->num_ports - 1, 0);
+ else
+ /* For all other ports, enable itself plus the
+ * CPU port.
+ */
+ mask = BIT(port) | BIT(smi->cpu_port);
+
+ /* For each port, set the port as member of VLAN (port+1)
+ * and untagged, except for the CPU port: the CPU port (5) is
+ * member of VLAN 6 and so are ALL the other ports as well.
+ * Use filter 0 (no filter).
+ */
+ dev_info(smi->dev, "Set VLAN %04x portmask to %08x (port %d %s)\n",
+ vid, mask, port, (port == smi->cpu_port) ?
+ "CPU PORT and all other ports" : "and CPU port");
+ ret = rtl8366_set_vlan(smi, vid, mask, mask, 0);
+ if (ret)
+ return ret;
+
+ dev_info(smi->dev, "Set PVID %04x on port %d\n",
+ vid, port);
+ ret = rtl8366_set_pvid(smi, port, vid);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
int rtl8366_init_vlan(struct realtek_smi *smi)
{
int port;
@@ -266,33 +308,7 @@ int rtl8366_init_vlan(struct realtek_smi *smi)
* it with the VLAN (port+1)
*/
for (port = 0; port < smi->num_ports; port++) {
- u32 mask;
-
- if (port == smi->cpu_port)
- /* For the CPU port, make all ports members of this
- * VLAN.
- */
- mask = GENMASK(smi->num_ports - 1, 0);
- else
- /* For all other ports, enable itself plus the
- * CPU port.
- */
- mask = BIT(port) | BIT(smi->cpu_port);
-
- /* For each port, set the port as member of VLAN (port+1)
- * and untagged, except for the CPU port: the CPU port (5) is
- * member of VLAN 6 and so are ALL the other ports as well.
- * Use filter 0 (no filter).
- */
- dev_info(smi->dev, "VLAN%d port mask for port %d, %08x\n",
- (port + 1), port, mask);
- ret = rtl8366_set_vlan(smi, (port + 1), mask, mask, 0);
- if (ret)
- return ret;
-
- dev_info(smi->dev, "VLAN%d port %d, PVID set to %d\n",
- (port + 1), port, (port + 1));
- ret = rtl8366_set_pvid(smi, port, (port + 1));
+ ret = rtl8366_set_default_vlan_and_pvid(smi, port);
if (ret)
return ret;
}
--
2.26.2
^ permalink raw reply related
* [net-next PATCH 2/5 v2] net: dsa: rtl8366rb: Support the CPU DSA tag
From: Linus Walleij @ 2020-06-17 8:31 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
In-Reply-To: <20200617083132.1847234-1-linus.walleij@linaro.org>
This activates the support to use the CPU tag to properly
direct ingress traffic to the right port.
Bit 15 in register RTL8368RB_CPU_CTRL_REG can be set to
1 to disable the insertion of the CPU tag which is what
the code currently does. The bit 15 define calls this
setting RTL8368RB_CPU_INSTAG which is confusing since the
iverse meaning is implied: programmers may think that
setting this bit to 1 will *enable* inserting the tag
rather than disablinbg it, so rename this setting in
bit 15 to RTL8368RB_CPU_NO_TAG which is more to the
point.
After this e.g. ping works out-of-the-box with the
RTL8366RB.
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Update the commit message to explain why we are renaming
bit 15 in RTL8368RB_CPU_CTRL_REG.
---
drivers/net/dsa/Kconfig | 1 +
drivers/net/dsa/rtl8366rb.c | 31 ++++++++-----------------------
2 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index d0024cb30a7b..468b3c4273c5 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -70,6 +70,7 @@ config NET_DSA_QCA8K
config NET_DSA_REALTEK_SMI
tristate "Realtek SMI Ethernet switch family support"
depends on NET_DSA
+ select NET_DSA_TAG_RTL4_A
select FIXED_PHY
select IRQ_DOMAIN
select REALTEK_PHY
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index fd1977590cb4..48f1ff746799 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -109,8 +109,8 @@
/* CPU port control reg */
#define RTL8368RB_CPU_CTRL_REG 0x0061
#define RTL8368RB_CPU_PORTS_MSK 0x00FF
-/* Enables inserting custom tag length/type 0x8899 */
-#define RTL8368RB_CPU_INSTAG BIT(15)
+/* Disables inserting custom tag length/type 0x8899 */
+#define RTL8368RB_CPU_NO_TAG BIT(15)
#define RTL8366RB_SMAR0 0x0070 /* bits 0..15 */
#define RTL8366RB_SMAR1 0x0071 /* bits 16..31 */
@@ -844,16 +844,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
if (ret)
return ret;
- /* Enable CPU port and enable inserting CPU tag
+ /* Enable CPU port with custom DSA tag 8899.
*
- * Disabling RTL8368RB_CPU_INSTAG here will change the behaviour
- * of the switch totally and it will start talking Realtek RRCP
- * internally. It is probably possible to experiment with this,
- * but then the kernel needs to understand and handle RRCP first.
+ * If you set RTL8368RB_CPU_NO_TAG (bit 15) in this registers
+ * the custom tag is turned off.
*/
ret = regmap_update_bits(smi->map, RTL8368RB_CPU_CTRL_REG,
0xFFFF,
- RTL8368RB_CPU_INSTAG | BIT(smi->cpu_port));
+ BIT(smi->cpu_port));
if (ret)
return ret;
@@ -967,21 +965,8 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
int port,
enum dsa_tag_protocol mp)
{
- /* For now, the RTL switches are handled without any custom tags.
- *
- * It is possible to turn on "custom tags" by removing the
- * RTL8368RB_CPU_INSTAG flag when enabling the port but what it
- * does is unfamiliar to DSA: ethernet frames of type 8899, the Realtek
- * Remote Control Protocol (RRCP) start to appear on the CPU port of
- * the device. So this is not the ordinary few extra bytes in the
- * frame. Instead it appears that the switch starts to talk Realtek
- * RRCP internally which means a pretty complex RRCP implementation
- * decoding and responding the RRCP protocol is needed to exploit this.
- *
- * The OpenRRCP project (dormant since 2009) have reverse-egineered
- * parts of the protocol.
- */
- return DSA_TAG_PROTO_NONE;
+ /* This switch uses the 4 byte protocol A Realtek DSA tag */
+ return DSA_TAG_PROTO_RTL4_A;
}
static void rtl8366rb_adjust_link(struct dsa_switch *ds, int port,
--
2.26.2
^ permalink raw reply related
* [net-next PATCH 1/5 v2] net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag
From: Linus Walleij @ 2020-06-17 8:31 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli
Cc: netdev, Linus Walleij, DENG Qingfang, Mauri Sandberg
This implements the known parts of the Realtek 4 byte
tag protocol version 0xA, as found in the RTL8366RB
DSA switch.
It is designated as protocol version 0xA as a
different Realtek 4 byte tag format with protocol
version 0x9 is known to exist in the Realtek RTL8306
chips.
The tag and switch chip lacks public documentation, so
the tag format has been reverse-engineered from
packet dumps. As only ingress traffic has been available
for analysis an egress tag has not been possible to
develop (even using educated guesses about bit fields)
so this is as far as it gets. It is not known if the
switch even supports egress tagging.
Excessive attempts to figure out the egress tag format
was made. When nothing else worked, I just tried all bit
combinations with 0xannp where a is protocol and p is
port. I looped through all values several times trying
to get a response from ping, without any positive
result.
Using just these ingress tags however, the switch
functionality is vastly improved and the packets find
their way into the destination port without any
tricky VLAN configuration. On the D-Link DIR-685 the
LAN ports now come up and respond to ping without
any command line configuration so this is a real
improvement for users.
Egress packets need to be restricted to the proper
target ports using VLAN, which the RTL8366RB DSA
switch driver already sets up.
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Drop some netdev_dbg() calls that was just littering.
- Rebase on v5.8-rc1
---
include/net/dsa.h | 2 +
net/dsa/Kconfig | 7 +++
net/dsa/Makefile | 1 +
net/dsa/tag_rtl4_a.c | 131 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 141 insertions(+)
create mode 100644 net/dsa/tag_rtl4_a.c
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 50389772c597..2b37943f09a4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -44,6 +44,7 @@ struct phylink_link_state;
#define DSA_TAG_PROTO_KSZ8795_VALUE 14
#define DSA_TAG_PROTO_OCELOT_VALUE 15
#define DSA_TAG_PROTO_AR9331_VALUE 16
+#define DSA_TAG_PROTO_RTL4_A_VALUE 17
enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
@@ -63,6 +64,7 @@ enum dsa_tag_protocol {
DSA_TAG_PROTO_KSZ8795 = DSA_TAG_PROTO_KSZ8795_VALUE,
DSA_TAG_PROTO_OCELOT = DSA_TAG_PROTO_OCELOT_VALUE,
DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE,
+ DSA_TAG_PROTO_RTL4_A = DSA_TAG_PROTO_RTL4_A_VALUE,
};
struct packet_type;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index d5bc6ac599ef..1f9b9b11008c 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -86,6 +86,13 @@ config NET_DSA_TAG_KSZ
Say Y if you want to enable support for tagging frames for the
Microchip 8795/9477/9893 families of switches.
+config NET_DSA_TAG_RTL4_A
+ tristate "Tag driver for Realtek 4 byte protocol A tags"
+ help
+ Say Y or M if you want to enable support for tagging frames for the
+ Realtek switches with 4 byte protocol A tags, sich as found in
+ the Realtek RTL8366RB.
+
config NET_DSA_TAG_OCELOT
tristate "Tag driver for Ocelot family of switches"
select PACKING
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 108486cfdeef..4f47b2025ff5 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
obj-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
+obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
new file mode 100644
index 000000000000..df82249aa1a7
--- /dev/null
+++ b/net/dsa/tag_rtl4_a.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handler for Realtek 4 byte DSA switch tags
+ * Currently only supports protocol "A" found in RTL8366RB
+ * Copyright (c) 2020 Linus Walleij <linus.walleij@linaro.org>
+ *
+ * This "proprietary tag" header looks like so:
+ *
+ * -------------------------------------------------
+ * | MAC DA | MAC SA | 0x8899 | 2 bytes tag | Type |
+ * -------------------------------------------------
+ *
+ * The 2 bytes tag form a 16 bit big endian word. The exact
+ * meaning has been guess from packet dumps from ingress
+ * frames, as no working egress traffic has been available
+ * we do not know the format of the egress tags or if they
+ * are even supported.
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/bits.h>
+
+#include "dsa_priv.h"
+
+#define RTL4_A_HDR_LEN 4
+#define RTL4_A_ETHERTYPE 0x8899
+#define RTL4_A_PROTOCOL_SHIFT 12
+/*
+ * 0x1 = Realtek Remote Control protocol (RRCP)
+ * 0x2/0x3 seems to be used for loopback testing
+ * 0x9 = RTL8306 DSA protocol
+ * 0xa = RTL8366RB DSA protocol
+ */
+#define RTL4_A_PROTOCOL_RTL8366RB 0xa
+
+static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ /*
+ * Just let it pass thru, we don't know if it is possible
+ * to tag a frame with the 0x8899 ethertype and direct it
+ * to a specific port, all attempts at reverse-engineering have
+ * ended up with the frames getting dropped.
+ *
+ * The VLAN set-up needs to restrict the frames to the right port.
+ *
+ * If you have documentation on the tagging format for RTL8366RB
+ * (tag type A) then please contribute.
+ */
+ return skb;
+}
+
+static struct sk_buff *rtl4a_tag_rcv(struct sk_buff *skb,
+ struct net_device *dev,
+ struct packet_type *pt)
+{
+ u16 protport;
+ __be16 *p;
+ u16 etype;
+ u8 flags;
+ u8 *tag;
+ u8 prot;
+ u8 port;
+
+ if (unlikely(!pskb_may_pull(skb, RTL4_A_HDR_LEN)))
+ return NULL;
+
+ /* The RTL4 header has its own custom Ethertype 0x8899 and that
+ * starts right at the beginning of the packet, after the src
+ * ethernet addr. Apparantly skb->data always points 2 bytes in,
+ * behind the Ethertype.
+ */
+ tag = skb->data - 2;
+ p = (__be16 *)tag;
+ etype = ntohs(*p);
+ if (etype != RTL4_A_ETHERTYPE) {
+ /* Not custom, just pass through */
+ netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
+ return skb;
+ }
+ p = (__be16 *)(tag + 2);
+ protport = ntohs(*p);
+ /* The 4 upper bits are the protocol */
+ prot = (protport >> RTL4_A_PROTOCOL_SHIFT) & 0x0f;
+ if (prot != RTL4_A_PROTOCOL_RTL8366RB) {
+ netdev_err(dev, "unknown realtek protocol 0x%01x\n", prot);
+ return NULL;
+ }
+ port = protport & 0xff;
+
+ /* Remove RTL4 tag and recalculate checksum */
+ skb_pull_rcsum(skb, RTL4_A_HDR_LEN);
+
+ /* Move ethernet DA and SA in front of the data */
+ memmove(skb->data - ETH_HLEN,
+ skb->data - ETH_HLEN - RTL4_A_HDR_LEN,
+ 2 * ETH_ALEN);
+
+ skb->dev = dsa_master_find_slave(dev, 0, port);
+ if (!skb->dev) {
+ netdev_dbg(dev, "could not find slave for port %d\n", port);
+ return NULL;
+ }
+
+ skb->offload_fwd_mark = 1;
+
+ return skb;
+}
+
+static int rtl4a_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+ int *offset)
+{
+ *offset = RTL4_A_HDR_LEN;
+ /* Skip past the tag and fetch the encapsulated Ethertype */
+ *proto = ((__be16 *)skb->data)[1];
+
+ return 0;
+}
+
+static const struct dsa_device_ops rtl4a_netdev_ops = {
+ .name = "rtl4a",
+ .proto = DSA_TAG_PROTO_RTL4_A,
+ .xmit = rtl4a_tag_xmit,
+ .rcv = rtl4a_tag_rcv,
+ .flow_dissect = rtl4a_tag_flow_dissect,
+ .overhead = RTL4_A_HDR_LEN,
+};
+module_dsa_tag_driver(rtl4a_netdev_ops);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL4_A);
--
2.26.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