* [PATCH bpf-next 1/9] bpf: Add bpf helper bpf_tcp_enter_cwr
From: brakmo @ 2019-02-19 5:38 UTC (permalink / raw)
To: netdev; +Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann --cc=Kernel Team
This patch adds a new bpf helper BPF_FUNC_tcp_enter_cwr
"int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)".
It is added to BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog
which currently can be attached to the ingress and egress
path.
This helper makes a tcp_sock enter CWR state. It can be used
by a bpf_prog to manage egress network bandwidth limit per
cgroupv2. A later patch will have a sample program to
show how it can be used to limit bandwidth usage per cgroupv2.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 9 ++++++++-
kernel/bpf/verifier.c | 4 ++++
net/core/filter.c | 14 ++++++++++++++
4 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..525628c913c9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -195,6 +195,7 @@ enum bpf_arg_type {
ARG_PTR_TO_SOCKET, /* pointer to bpf_sock */
ARG_PTR_TO_SPIN_LOCK, /* pointer to bpf_spin_lock */
ARG_PTR_TO_SOCK_COMMON, /* pointer to sock_common */
+ ARG_PTR_TO_TCP_SOCK, /* pointer to tcp_sock */
};
/* type of values returned from helper functions */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..9e9f4f1a0370 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2359,6 +2359,12 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)
+ * Description
+ * Make a tcp_sock enter CWR state.
+ * Return
+ * 0
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2463,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(tcp_enter_cwr),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..95fb385c6f3c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2424,6 +2424,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
return -EFAULT;
}
meta->ptr_id = reg->id;
+ } else if (arg_type == ARG_PTR_TO_TCP_SOCK) {
+ expected_type = PTR_TO_TCP_SOCK;
+ if (type != expected_type)
+ goto err_type;
} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
if (meta->func_id == BPF_FUNC_spin_lock) {
if (process_spin_lock(env, regno, true))
diff --git a/net/core/filter.c b/net/core/filter.c
index b584cb42a803..f51c4a781844 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5426,6 +5426,18 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
.arg1_type = ARG_PTR_TO_SOCK_COMMON,
};
+BPF_CALL_1(bpf_tcp_enter_cwr, struct tcp_sock *, tp)
+{
+ tcp_enter_cwr((struct sock *)tp);
+ return 0;
+}
+
+static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = {
+ .func = bpf_tcp_enter_cwr,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_TCP_SOCK,
+};
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
@@ -5585,6 +5597,8 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
#ifdef CONFIG_INET
case BPF_FUNC_tcp_sock:
return &bpf_tcp_sock_proto;
+ case BPF_FUNC_tcp_enter_cwr:
+ return &bpf_tcp_enter_cwr_proto;
#endif
default:
return sk_filter_func_proto(func_id, prog);
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 2/9] bpf: Test bpf_tcp_enter_cwr in test_verifier
From: brakmo @ 2019-02-19 5:38 UTC (permalink / raw)
To: netdev; +Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann --cc=Kernel Team
This test ensures the verifier has checked the arg1 of
BPF_FUNC_tcp_enter_cwr is of ARG_PTR_TO_TCP_SOCK type.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
tools/testing/selftests/bpf/verifier/sock.c | 33 +++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/sock.c b/tools/testing/selftests/bpf/verifier/sock.c
index 0ddfdf76aba5..b07a083eeb59 100644
--- a/tools/testing/selftests/bpf/verifier/sock.c
+++ b/tools/testing/selftests/bpf/verifier/sock.c
@@ -382,3 +382,36 @@
.result = REJECT,
.errstr = "type=tcp_sock expected=sock",
},
+{
+ "bpf_tcp_enter_cwr(skb->sk)",
+ .insns = {
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct __sk_buff, sk)),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_enter_cwr),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .result = REJECT,
+ .errstr = "type=sock_common expected=tcp_sock",
+},
+{
+ "bpf_tcp_enter_cwr(bpf_tcp_sock(skb->sk))",
+ .insns = {
+ BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct __sk_buff, sk)),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_enter_cwr),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .result = ACCEPT,
+},
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] net: phy: at803x: dont inline helpers
From: Vinod Koul @ 2019-02-19 5:39 UTC (permalink / raw)
To: Sekhar Nori
Cc: David Miller, linux-arm-msm, bjorn.andersson, netdev,
niklas.cassel, andrew, f.fainelli, peter.ujfalusi,
marc.w.gonzalez
In-Reply-To: <26c3046d-8b1c-e601-7cad-c9a7bc267033@ti.com>
On 19-02-19, 11:05, Sekhar Nori wrote:
> Hi Vinod,
>
> On 19/02/19 10:32 AM, Vinod Koul wrote:
> > Hello Dave,
> >
> > On 18-02-19, 16:28, David Miller wrote:
> >> From: Vinod Koul <vkoul@kernel.org>
> >> Date: Mon, 18 Feb 2019 15:48:52 +0530
> >>
> >>> Some helpers were inlined, but makes more sense to allow compiler
> >>> to do the right optiomazations instead, so remove inline for
> >>> at803x_disable_rx_delay() and at803x_disable_tx_delay()
> >>>
> >>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >>
> >> Two problem with this patch series.
> >>
> >> 1) It fixes a bug in 'net' but the patches only apply cleanly to
> >> 'net-next'. Please respin this against 'net'.
> >
> > I assume the 'net' is fixes going into current -rcX. This fixes issues
> > reported on linux-next due to patches in net-next, so these fixes should
> > go to net-next as well.
>
> net-next should be part of subject prefix in this case. See
> Documentation/networking/netdev-FAQ.rst
Thanks for the pointer Sekhar, will do :)
--
~Vinod
^ permalink raw reply
* Re: [PATCH v3 perf,bpf 08/11] perf, bpf: save btf information as headers to perf.data
From: Song Liu @ 2019-02-19 5:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Netdev, linux-kernel, ast@kernel.org, daniel@iogearbox.net,
Kernel Team, peterz@infradead.org, acme@redhat.com,
jolsa@kernel.org, namhyung@kernel.org
In-Reply-To: <20190217230637.GK7443@krava>
> On Feb 17, 2019, at 3:06 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Feb 15, 2019 at 01:53:51PM -0800, Song Liu wrote:
>
> SNIP
>
>> +static int process_btf(struct feat_fd *ff, void *data __maybe_unused)
>> +{
>> + struct perf_env *env = &ff->ph->env;
>> + u32 count, i;
>> +
>> + if (do_read_u32(ff, &count))
>> + return -1;
>> +
>> + down_write(&env->bpf_progs.bpf_info_lock);
>> +
>> + for (i = 0; i < count; ++i) {
>> + struct btf_node btf_node;
>> + struct btf_node *node;
>> +
>> + if (__do_read(ff, &btf_node, sizeof(struct btf_node)))
>> + return -1;
>> +
>> + node = malloc(sizeof(struct btf_node) + btf_node.data_size);
>> + if (!node)
>> + return -1;
>> +
>> + node->id = btf_node.id;
>> + node->data_size = btf_node.data_size;
>> +
>> + if (__do_read(ff, node->data, btf_node.data_size)) {
>> + free(node);
>> + return -1;
>> + }
>
> hows endianity swap handled got struct btf_node and data in here?
>
> thanks,
> jirka
I thought about endianity at some point, but forgot about it. Let
me see how to fix it.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH v3 perf,bpf 05/11] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Song Liu @ 2019-02-19 5:52 UTC (permalink / raw)
To: Jiri Olsa
Cc: Netdev, linux-kernel, ast@kernel.org, daniel@iogearbox.net,
Kernel Team, peterz@infradead.org, acme@redhat.com,
jolsa@kernel.org, namhyung@kernel.org
In-Reply-To: <20190217230516.GA7443@krava>
> On Feb 17, 2019, at 3:05 PM, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Feb 15, 2019 at 01:53:48PM -0800, Song Liu wrote:
>
> SNIP
>
>> info_linear = bpf_program__get_prog_info_linear(fd, arrays);
>> if (IS_ERR_OR_NULL(info_linear)) {
>> @@ -151,8 +165,8 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_tool *tool,
>> machine, process);
>> }
>>
>> - /* Synthesize PERF_RECORD_BPF_EVENT */
>> if (opts->bpf_event) {
>> + /* Synthesize PERF_RECORD_BPF_EVENT */
>> *bpf_event = (struct bpf_event){
>> .header = {
>> .type = PERF_RECORD_BPF_EVENT,
>> @@ -165,6 +179,19 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_tool *tool,
>> memcpy(bpf_event->tag, info->tag, BPF_TAG_SIZE);
>> memset((void *)event + event->header.size, 0, machine->id_hdr_size);
>> event->header.size += machine->id_hdr_size;
>> +
>> + /* save bpf_prog_info to env */
>> + info_node = malloc(sizeof(struct bpf_prog_info_node));
>> + if (info_node) {
>> + info_node->info_linear = info_linear;
>> + perf_env__insert_bpf_prog_info(env, info_node);
>> + info_linear = NULL;
>> + }
>
> what if the allocation fails? we don't care?
>
> jirka
My original plan is to just ignore it and accept that this program
doesn't have annotation. Any suggestion on what would be a better
approach?
Thanks,
Song
^ permalink raw reply
* Re: [PATCH ipsec] xfrm: Fix inbound traffic via XFRM interfaces across network namespaces
From: Steffen Klassert @ 2019-02-19 6:04 UTC (permalink / raw)
To: Tobias Brunner; +Cc: netdev
In-Reply-To: <862cfc1b-34ad-1755-63c5-af87cc12ff63@strongswan.org>
On Mon, Feb 18, 2019 at 10:49:39AM +0100, Tobias Brunner wrote:
> After moving an XFRM interface to another namespace it stays associated
> with the original namespace (net in `struct xfrm_if` and the list keyed
> with `xfrmi_net_id`), allowing processes in the new namespace to use
> SAs/policies that were created in the original namespace. For instance,
> this allows a keying daemon in one namespace to establish IPsec SAs for
> other namespaces without processes there having access to the keys or IKE
> credentials.
>
> This worked fine for outbound traffic, however, for inbound traffic the
> lookup for the interfaces and the policies used the incorrect namespace
> (the one the XFRM interface was moved to).
>
> Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Applied, thanks a lot Tobias!
^ permalink raw reply
* [PATCH net-next v3 0/2] net: phy: at803x: Update delays for RGMII modes
From: Vinod Koul @ 2019-02-19 6:17 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable
phy delay for RGMII mode") caused regression on am335x-evmsk board.
This board expects the Phy delay to be enabled but specified RGMII mode
which refers to delays being disabled. So fix this by disabling delay only
for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes.
While at it, as pointed by Dave, don't inline the helpers.
[1]: https://www.spinics.net/lists/netdev/msg550749.html
Vinod Koul (2):
net: phy: at803x: don't inline helpers
net: phy: at803x: disable delay only for RGMII mode
drivers/net/phy/at803x.c | 51 ++++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 13 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net-next v3 1/2] net: phy: at803x: don't inline helpers
From: Vinod Koul @ 2019-02-19 6:17 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
In-Reply-To: <20190219061800.31025-1-vkoul@kernel.org>
Some helpers were inlined, but makes more sense to allow compiler
to do the right optimizations instead, so remove inline for
at803x_disable_rx_delay() and at803x_disable_tx_delay()
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/net/phy/at803x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 8ff12938ab47..c6e7d800fd7a 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -110,13 +110,13 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
return phy_write(phydev, AT803X_DEBUG_DATA, val);
}
-static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+static int at803x_disable_rx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
AT803X_DEBUG_RX_CLK_DLY_EN, 0);
}
-static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+static int at803x_disable_tx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
AT803X_DEBUG_TX_CLK_DLY_EN, 0);
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v3 2/2] net: phy: at803x: disable delay only for RGMII mode
From: Vinod Koul @ 2019-02-19 6:18 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez, Peter Ujfalusi
In-Reply-To: <20190219061800.31025-1-vkoul@kernel.org>
Per "Documentation/devicetree/bindings/net/ethernet.txt" RGMII mode
should not have delay in PHY whereas RGMII_ID and RGMII_RXID/RGMII_TXID
can have delay in phy.
So disable the delay only for RGMII mode and enable for other modes.
Also treat the default case as disabled delays.
Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++----------
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c6e7d800fd7a..dc1b13f7fc12 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -110,6 +110,18 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
return phy_write(phydev, AT803X_DEBUG_DATA, val);
}
+static int at803x_enable_rx_delay(struct phy_device *phydev)
+{
+ return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
+ AT803X_DEBUG_RX_CLK_DLY_EN);
+}
+
+static int at803x_enable_tx_delay(struct phy_device *phydev)
+{
+ return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
+ AT803X_DEBUG_TX_CLK_DLY_EN);
+}
+
static int at803x_disable_rx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
@@ -255,23 +267,36 @@ static int at803x_config_init(struct phy_device *phydev)
if (ret < 0)
return ret;
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII) {
- ret = at803x_disable_rx_delay(phydev);
+ /* The hardware register default is RX and TX delay enabled, so lets
+ * first disable the RX and TX delays in phy and enable them based
+ * on the mode selected
+ */
+ ret = at803x_disable_rx_delay(phydev);
+ if (ret < 0)
+ return ret;
+ ret = at803x_disable_tx_delay(phydev);
+ if (ret < 0)
+ return ret;
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+ /* If RGMII_ID or RGMII_RXID are specified enable RX delay,
+ * otherwise keep it disabled
+ */
+ ret = at803x_enable_rx_delay(phydev);
if (ret < 0)
return ret;
}
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII) {
- ret = at803x_disable_tx_delay(phydev);
- if (ret < 0)
- return ret;
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+ /* If RGMII_ID or RGMII_TXID are specified enable TX delay,
+ * otherwise keep it disabled
+ */
+ ret = at803x_enable_tx_delay(phydev);
}
- return 0;
+ return ret;
}
static int at803x_ack_interrupt(struct phy_device *phydev)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters
From: Cong Wang @ 2019-02-19 6:42 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
In-Reply-To: <cover.1550271080.git.dcaratti@redhat.com>
On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> currently, the kernel checks for bad values of the control action in
> tcf_action_init_1(), after a successful call to the action's init()
> function. This causes three bad behaviors:
Yeah, I have been complaining about this for a long time,
although slightly differently. The problem here is the lack of
"copy" in RCU mechanism, which makes it nearly impossible
to rollback to the previous state of an action on failure path
of an update, which also makes RCU readers reading a partially
updated action too.
Before I fix the "copy" part, your fixes make sense to me. There
might be some other way to expose the action-specific tcfa_action
opcode, but it might not be better than yours.
BTW, please fold these bad behaviors into each appropriate
patch, and keep the cover letter as an overview of the whole
patchset rather than showing any details.
[...]
> all these three problems can be fixed if we validate the control action
> in the init() function, in the same way as we are already doing for all
> the other parameters.
>
> - patch 1 is a temporary fix for problem 2), but it's reverted at the
> end of the series
Please drop patch 1, it is very unlikely only patch 1 will be backported,
I think the whole patchset should be, therefore we have no reason
to carry a temporary fix here.
Thanks.
^ permalink raw reply
* Re: [PATCH net] vhost: correctly check the return value of translate_desc() in log_used()
From: Jason Wang @ 2019-02-19 6:50 UTC (permalink / raw)
To: David Miller; +Cc: mst, kvm, virtualization, netdev, linux-kernel, stephen
In-Reply-To: <20190215.100356.251698752340585300.davem@davemloft.net>
On 2019/2/16 上午2:03, David Miller wrote:
> From: Jason Wang<jasowang@redhat.com>
> Date: Fri, 15 Feb 2019 15:53:24 +0800
>
>> When fail, translate_desc() returns negative value, otherwise the
>> number of iovs. So we should fail when the return value is negative
>> instead of a blindly check against zero.
>>
>> Reported-by: Stephen Hemminger<stephen@networkplumber.org>
>> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Jason, please put the Fixes tag first.
>
> Thank you.
Ok. Will post V2.
Thanks
^ permalink raw reply
* Re: [PATCH net] vhost: correctly check the return value of translate_desc() in log_used()
From: Jason Wang @ 2019-02-19 6:51 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20190215084502.23feed0e@shemminger-XPS-13-9360>
On 2019/2/16 上午12:45, Stephen Hemminger wrote:
> On Fri, 15 Feb 2019 15:53:24 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> When fail, translate_desc() returns negative value, otherwise the
>> number of iovs. So we should fail when the return value is negative
>> instead of a blindly check against zero.
>>
>> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
>> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Looks good. It is best to put the Addresses-Coverity-Id tag on these kind
> of bug fixes so that the automated tools see it.
Ok. Will do this in V2.
Thanks
^ permalink raw reply
* [PATCH net V2] vhost: correctly check the return value of translate_desc() in log_used()
From: Jason Wang @ 2019-02-19 6:53 UTC (permalink / raw)
To: mst, jasowang; +Cc: kvm, virtualization, netdev, linux-kernel
When fail, translate_desc() returns negative value, otherwise the
number of iovs. So we should fail when the return value is negative
instead of a blindly check against zero.
Detected by CoverityScan, CID# 1442593: Control flow issues (DEADCODE)
Fixes: cc5e71075947 ("vhost: log dirty page correctly")
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 24a129fcdd61..a2e5dc7716e2 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1788,7 +1788,7 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
len, iov, 64, VHOST_ACCESS_WO);
- if (ret)
+ if (ret < 0)
return ret;
for (i = 0; i < ret; i++) {
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2] net: dsa: qca8k: Enable delay for RGMII_ID mode
From: Vinod Koul @ 2019-02-19 6:59 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Michal Vokáč
RGMII_ID specifies that we should have internal delay, so resurrect the
delay addition routine but under the RGMII_ID mode.
Fixes: 40269aa9f40a ("net: dsa: qca8k: disable delay for RGMII mode")
Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/net/dsa/qca8k.c | 12 ++++++++++++
drivers/net/dsa/qca8k.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..195a8a87b984 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -443,6 +443,18 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
val = QCA8K_PORT_PAD_RGMII_EN;
qca8k_write(priv, reg, val);
break;
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ /* RGMII_ID needs internal delay. This is enabled through
+ * PORT5_PAD_CTRL for all ports, rather than individual port
+ * registers
+ */
+ qca8k_write(priv, reg,
+ QCA8K_PORT_PAD_RGMII_EN |
+ QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+ QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+ qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+ QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+ break;
case PHY_INTERFACE_MODE_SGMII:
qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
break;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 613fe5c50236..d146e54c8a6c 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -40,6 +40,7 @@
((0x8 + (x & 0x3)) << 22)
#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) \
((0x10 + (x & 0x3)) << 20)
+#define QCA8K_MAX_DELAY 3
#define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24)
#define QCA8K_PORT_PAD_SGMII_EN BIT(7)
#define QCA8K_REG_MODULE_EN 0x030
--
2.20.1
^ permalink raw reply related
* Re: [RFC PATCH net-next v3 07/21] ethtool: implement EVENT notifications
From: Michal Kubecek @ 2019-02-19 7:02 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, David Miller, Jakub Kicinski, Jiri Pirko, linux-kernel
In-Reply-To: <20190218234608.GI14879@lunn.ch>
On Tue, Feb 19, 2019 at 12:46:08AM +0100, Andrew Lunn wrote:
> On Mon, Feb 18, 2019 at 07:21:59PM +0100, Michal Kubecek wrote:
> > Three types of netlink notifications are introduced:
> >
> > - ETHA_EVENT_NEWDEV to notify about newly registered network devices
> > - ETHA_EVENT_DELDEV to notify about unregistered network devices
> > - ETHA_EVENT_RENAMEDEV to notify about renamed network device
> >
> > The notifications are triggered by NETDEV_REGISTER, NETDEV_UNREGISTER and
> > NETDEV_CHANGENAME notifiers.
> >
> > These notifications are intended for applications and daemons monitoring
> > ethtool events to allow updating the list of existing devices without
> > having to open another socket for rtnetlink.
>
> Hi Michal
>
> Does ETHA_EVENT_RENAMEDEV actually contain enough information to avoid
> needing a rtnetlink lookup? If i understand the code correctly, all
> you have is the new name. You don't know the old name?
>
> Having said that, i don't see an easy way to get access to the old
> name when handling the NETDEV_CHANGENAME.
We don't have the old name and without modifying the NETDEV_CHANGENAME
notifier to pass it, there is probably no way to get it. But the ethtool
notification also contains ifindex so that userspace applications which
want to track the configuration can remember both and identify the
renamed device by ifindex.
Michal
^ permalink raw reply
* [tipc-discussion][net 0/1] tipc: fix race condition between sleep/wakeup
From: Tung Nguyen @ 2019-02-19 7:03 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion
Fix issue of hung sendto() in user space.
Tung Nguyen (1):
tipc: fix race condition causing hung sendto
net/tipc/socket.c | 4 ++++
1 file changed, 4 insertions(+)
mode change 100644 => 100755 net/tipc/socket.c
--
2.17.1
^ permalink raw reply
* [tipc-discussion][net 1/1] tipc: fix race condition causing hung sendto
From: Tung Nguyen @ 2019-02-19 7:03 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion
In-Reply-To: <20190219070310.23888-1-tung.q.nguyen@dektech.com.au>
When sending multicast messages via blocking socket,
if sending link is congested (tsk->cong_link_cnt is set to 1),
the sending thread will be put into sleeping state. However,
tipc_sk_filter_rcv() is called under socket spin lock but
tipc_wait_for_cond() is not. So, there is no guarantee that
the setting of tsk->cong_link_cnt to 0 in tipc_sk_proto_rcv() in
CPU-1 will be perceived by CPU-0. If that is the case, the sending
thread in CPU-0 after being waken up, will continue to see
tsk->cong_link_cnt as 1 and put the sending thread into sleeping
state again. The sending thread will sleep forever.
CPU-0 | CPU-1
tipc_wait_for_cond() |
{ |
// condition_ = !tsk->cong_link_cnt |
while ((rc_ = !(condition_))) { |
... |
release_sock(sk_); |
wait_woken(); |
| if (!sock_owned_by_user(sk))
| tipc_sk_filter_rcv()
| {
| ...
| tipc_sk_proto_rcv()
| {
| ...
| tsk->cong_link_cnt--;
| ...
| sk->sk_write_space(sk);
| ...
| }
| ...
| }
sched_annotate_sleep(); |
lock_sock(sk_); |
remove_wait_queue(); |
} |
} |
This commit fixes it by adding memory barrier to tipc_sk_proto_rcv()
and tipc_wait_for_cond().
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
---
net/tipc/socket.c | 4 ++++
1 file changed, 4 insertions(+)
mode change 100644 => 100755 net/tipc/socket.c
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
old mode 100644
new mode 100755
index 1217c90a363b..d8f054d45941
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -383,6 +383,8 @@ static int tipc_sk_sock_err(struct socket *sock, long *timeout)
int rc_; \
\
while ((rc_ = !(condition_))) { \
+ /* coupled with smp_wmb() in tipc_sk_proto_rcv() */ \
+ smp_rmb(); \
DEFINE_WAIT_FUNC(wait_, woken_wake_function); \
sk_ = (sock_)->sk; \
rc_ = tipc_sk_sock_err((sock_), timeo_); \
@@ -1982,6 +1984,8 @@ static void tipc_sk_proto_rcv(struct sock *sk,
return;
case SOCK_WAKEUP:
tipc_dest_del(&tsk->cong_links, msg_orignode(hdr), 0);
+ /* coupled with smp_rmb() in tipc_wait_for_cond() */
+ smp_wmb();
tsk->cong_link_cnt--;
wakeup = true;
break;
--
2.17.1
^ permalink raw reply related
* [PATCH] phy/marvell: prevent unneeded page switching
From: Frank de Brabander @ 2019-02-19 7:21 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: Frank de Brabander, netdev
marvell_read_status() no longer first switches to the fiber page, instead
looks at the already active register page first. In case the link is down,
switches to the other (fiber/copper) register page.
Unneeded register page switching of the Marvell 88E1510 PHY can cause
the ethernet driver to register a link down & up event.
Signed-off-by: Frank de Brabander <debrabander@gmail.com>
---
drivers/net/phy/marvell.c | 59 ++++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index abb7876..31de2db 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -194,6 +194,11 @@ static int marvell_read_page(struct phy_device *phydev)
return __phy_read(phydev, MII_MARVELL_PHY_PAGE);
}
+static int marvell_get_page(struct phy_device *phydev)
+{
+ return phy_read(phydev, MII_MARVELL_PHY_PAGE);
+}
+
static int marvell_write_page(struct phy_device *phydev, int page)
{
return __phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
@@ -1245,47 +1250,59 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
* Some Marvell's phys have two modes: fiber and copper.
* Both need status checked.
* Description:
- * First, check the fiber link and status.
- * If the fiber link is down, check the copper link and status which
- * will be the default value if both link are down.
+ * First, check the link of the currently selected page. Unneeded page
+ * switches should be prevented, they can trigger the ethernet driver to
+ * register a link down event.
+ * If the first checked link type is down, continue with checking the other
+ * link type. This is needed for platforms that have a fiber and copper link
+ * connected to the same phy.
*/
static int marvell_read_status(struct phy_device *phydev)
{
int err;
+ int page;
- /* Check the fiber mode first */
if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
phydev->supported) &&
phydev->interface != PHY_INTERFACE_MODE_SGMII) {
- err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
- if (err < 0)
- goto error;
+ page = marvell_get_page(phydev);
+ if (page < 0)
+ return page;
+
+ if (page != MII_MARVELL_FIBER_PAGE &&
+ page != MII_MARVELL_COPPER_PAGE) {
+ page = MII_MARVELL_FIBER_PAGE;
- err = marvell_read_status_page(phydev, MII_MARVELL_FIBER_PAGE);
+ err = marvell_set_page(phydev, page);
+ if (err < 0)
+ return err;
+ }
+
+ err = marvell_read_status_page(phydev, page);
if (err < 0)
- goto error;
+ return err;
- /* If the fiber link is up, it is the selected and
- * used link. In this case, we need to stay in the
- * fiber page. Please to be careful about that, avoid
- * to restore Copper page in other functions which
- * could break the behaviour for some fiber phy like
+ /* If the link is up, it is the selected and
+ * used link. In this case, we need to stay in that
+ * page. Please to be careful about that, avoid
+ * to restore the page in other functions which
+ * could break the behaviour for some phy's like
* 88E1512.
*/
if (phydev->link)
return 0;
- /* If fiber link is down, check and save copper mode state */
- err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
+ page = (page == MII_MARVELL_FIBER_PAGE) ?
+ MII_MARVELL_COPPER_PAGE : MII_MARVELL_FIBER_PAGE;
+
+ err = marvell_set_page(phydev, page);
if (err < 0)
- goto error;
+ return err;
+
+ return marvell_read_status_page(phydev, page);
}
return marvell_read_status_page(phydev, MII_MARVELL_COPPER_PAGE);
-
-error:
- marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
- return err;
}
/* marvell_suspend
--
2.7.4
^ permalink raw reply related
* [PATCH] net: ns83820: drop pointless static qualifier in ns83820_probe_phy()
From: Mao Wenan @ 2019-02-19 8:06 UTC (permalink / raw)
To: kernel-janitors, netdev, john.fastabend, hawk, jakub.kicinski,
daniel, ast
There is no need to have the 'int first' static
since new value always be assigned before use it.
The old codes of first dereferencing this variable have
been commented out. So it is useless with 'static int first'.
/*
if (!first) {
...
}
*/
first = 1;
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/net/ethernet/natsemi/ns83820.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 958fced4dacf..fb064df3a1a6 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -1869,7 +1869,7 @@ static unsigned ns83820_mii_write_reg(struct ns83820 *dev, unsigned phy, unsigne
static void ns83820_probe_phy(struct net_device *ndev)
{
struct ns83820 *dev = PRIV(ndev);
- static int first;
+ int first;
int i;
#define MII_PHYIDR1 0x02
#define MII_PHYIDR2 0x03
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] net: ns83820: drop pointless static qualifier in ns83820_probe_phy()
From: Julia Lawall @ 2019-02-19 8:01 UTC (permalink / raw)
To: Mao Wenan
Cc: kernel-janitors, netdev, john.fastabend, hawk, jakub.kicinski,
daniel, ast
In-Reply-To: <20190219080659.89732-1-maowenan@huawei.com>
On Tue, 19 Feb 2019, Mao Wenan wrote:
> There is no need to have the 'int first' static
> since new value always be assigned before use it.
> The old codes of first dereferencing this variable have
> been commented out. So it is useless with 'static int first'.
> /*
> if (!first) {
> ...
> }
> */
> first = 1;
Do you need the variable at all?
In the code, the commenting out is actually done with #if 0. It could be
good for the commit log to reflect the code more accurately.
Can the #if 0 and code be removed? Apparently it predates git.
A commented out printk below also predates git.
julia
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/net/ethernet/natsemi/ns83820.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index 958fced4dacf..fb064df3a1a6 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -1869,7 +1869,7 @@ static unsigned ns83820_mii_write_reg(struct ns83820 *dev, unsigned phy, unsigne
> static void ns83820_probe_phy(struct net_device *ndev)
> {
> struct ns83820 *dev = PRIV(ndev);
> - static int first;
> + int first;
> int i;
> #define MII_PHYIDR1 0x02
> #define MII_PHYIDR2 0x03
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH 0/2] ARM: dts: am335x-evm/evmsk: Fix PHY mode for ethernet
From: Peter Ujfalusi @ 2019-02-19 8:02 UTC (permalink / raw)
To: Tony Lindgren
Cc: bcousson, linux-omap, devicetree, linux-arm-kernel, nsekhar,
grygorii.strashko, vkoul, netdev, f.fainelli, marc.w.gonzalez,
niklas.cassel
In-Reply-To: <20190218162613.GH15711@atomide.com>
Hi Tony,
On 18/02/2019 18.26, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [190218 16:22]:
>>
>>
>> On 18/02/2019 16.44, Tony Lindgren wrote:
>>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [190218 14:36]:
>>>> Hi,
>>>>
>>>> cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") broke the
>>>> ethernet networking on evmsk (and most likely on the evm as well):
>>>> https://patchwork.ozlabs.org/patch/1028527/
>>>>
>>>> v1 patch to fix the situation:
>>>> https://patchwork.ozlabs.org/patch/1040617/
>>>>
>>>> It turned out that the at803x driver is actually broken and need to be fixed
>>>> along with the DT data.
>>>>
>>>> The following series is proposed to fix the driver:
>>>> https://patchwork.ozlabs.org/project/netdev/list/?series=92611
>>>>
>>>> but the PHT mode needs to be switched to rgmii-id from rgmii-txid:
>>>> The rx delay is enabled by default and the driver never disabled it so when
>>>> asking rgmii-txid it actually got rgmii-id.
>>>>
>>>> The patch can be backported to stable, I have tested that it is not causing
>>>> regression with the old, broken driver.
>>>
>>> Can the dts changes be merged before the driver changes or
>>> does it cause the phy to stop working?
>>
>> The phy is not working atm, but this change will not cause regression
>> even if it is merged first.
>
> OK so sounds like these are OK to wait for v5.1 merge window
> then as the dts changes alone won't fix anything?
I think it would be better to send these to 5.0 to avoid the regression
siting in linux-next.
The patches are actually doing to the correct thing (we need rgmii-id,
not rmgii-txid for these boards).
It was just a matter of luck that it worked with rgmii-txid as the
driver was broken and did not disabled the rxid when it should have.
Tested the patch on top of mainline
(b5372fe5dc84235dbe04998efdede3c4daa866a9) was the topmost commit after
the rc7 tag.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* [PATCH net-next] net: rose: add missing dev_put() on error in rose_bind
From: YueHaibing @ 2019-02-19 8:06 UTC (permalink / raw)
To: ralf, davem; +Cc: linux-kernel, netdev, linux-hams, YueHaibing
when capable check failed, dev_put should
be call before return -EACCES.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/rose/af_rose.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index d00a0ef..c96f63f 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -689,8 +689,10 @@ static int rose_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
rose->source_call = user->call;
ax25_uid_put(user);
} else {
- if (ax25_uid_policy && !capable(CAP_NET_BIND_SERVICE))
+ if (ax25_uid_policy && !capable(CAP_NET_BIND_SERVICE)) {
+ dev_put(dev);
return -EACCES;
+ }
rose->source_call = *source;
}
--
2.7.0
^ permalink raw reply related
* Re: [PATCH net-next v3 0/2] net: phy: at803x: Update delays for RGMII modes
From: Peter Ujfalusi @ 2019-02-19 8:09 UTC (permalink / raw)
To: Vinod Koul, David S Miller
Cc: linux-arm-msm, Bjorn Andersson, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Marc Gonzalez
In-Reply-To: <20190219061800.31025-1-vkoul@kernel.org>
Hi Vinod,
On 19/02/2019 8.17, Vinod Koul wrote:
> Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable
> phy delay for RGMII mode") caused regression on am335x-evmsk board.
> This board expects the Phy delay to be enabled but specified RGMII mode
> which refers to delays being disabled. So fix this by disabling delay only
> for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes.
>
> While at it, as pointed by Dave, don't inline the helpers.
>
> [1]: https://www.spinics.net/lists/netdev/msg550749.html
Not sure what was changed since v2, but with
https://patchwork.ozlabs.org/project/netdev/list/?series=92672
ethernet is working on am335x-evmsk. thank you:
Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com>
> Vinod Koul (2):
> net: phy: at803x: don't inline helpers
> net: phy: at803x: disable delay only for RGMII mode
>
> drivers/net/phy/at803x.c | 51 ++++++++++++++++++++++++++++++----------
> 1 file changed, 38 insertions(+), 13 deletions(-)
>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH] net: ns83820: drop pointless static qualifier in ns83820_probe_phy()
From: maowenan @ 2019-02-19 8:09 UTC (permalink / raw)
To: Julia Lawall
Cc: kernel-janitors, netdev, john.fastabend, hawk, jakub.kicinski,
daniel, ast
In-Reply-To: <alpine.DEB.2.21.1902190857300.2570@hadrien>
On 2019/2/19 16:01, Julia Lawall wrote:
>
>
> On Tue, 19 Feb 2019, Mao Wenan wrote:
>
>> There is no need to have the 'int first' static
>> since new value always be assigned before use it.
>> The old codes of first dereferencing this variable have
>> been commented out. So it is useless with 'static int first'.
>> /*
>> if (!first) {
>> ...
>> }
>> */
>> first = 1;
>
> Do you need the variable at all?
>
> In the code, the commenting out is actually done with #if 0. It could be
> good for the commit log to reflect the code more accurately.
>
OK, the original codes are using #if 0 to comment out, but when I generate patch
the "#if 0" has been removed, so I use "/*...*/" to reflect that.
> Can the #if 0 and code be removed? Apparently it predates git.
>
> A commented out printk below also predates git.
>
I think all the "#if 0" comment out codes and printk codes can be removed, shall I send
v2 to do this?
> julia
>
>>
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>> drivers/net/ethernet/natsemi/ns83820.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
>> index 958fced4dacf..fb064df3a1a6 100644
>> --- a/drivers/net/ethernet/natsemi/ns83820.c
>> +++ b/drivers/net/ethernet/natsemi/ns83820.c
>> @@ -1869,7 +1869,7 @@ static unsigned ns83820_mii_write_reg(struct ns83820 *dev, unsigned phy, unsigne
>> static void ns83820_probe_phy(struct net_device *ndev)
>> {
>> struct ns83820 *dev = PRIV(ndev);
>> - static int first;
>> + int first;
>> int i;
>> #define MII_PHYIDR1 0x02
>> #define MII_PHYIDR2 0x03
>> --
>> 2.20.1
>>
>>
>
> .
>
^ permalink raw reply
* Re: [PATCH] net: ns83820: drop pointless static qualifier in ns83820_probe_phy()
From: Julia Lawall @ 2019-02-19 8:15 UTC (permalink / raw)
To: maowenan
Cc: kernel-janitors, netdev, john.fastabend, hawk, jakub.kicinski,
daniel, ast
In-Reply-To: <578c4208-956d-b905-6a50-5ad04620a349@huawei.com>
On Tue, 19 Feb 2019, maowenan wrote:
>
>
> On 2019/2/19 16:01, Julia Lawall wrote:
> >
> >
> > On Tue, 19 Feb 2019, Mao Wenan wrote:
> >
> >> There is no need to have the 'int first' static
> >> since new value always be assigned before use it.
> >> The old codes of first dereferencing this variable have
> >> been commented out. So it is useless with 'static int first'.
> >> /*
> >> if (!first) {
> >> ...
> >> }
> >> */
> >> first = 1;
> >
> > Do you need the variable at all?
> >
> > In the code, the commenting out is actually done with #if 0. It could be
> > good for the commit log to reflect the code more accurately.
> >
> OK, the original codes are using #if 0 to comment out, but when I generate patch
> the "#if 0" has been removed, so I use "/*...*/" to reflect that.
I don't understand the above comment. The proposed patch doesn't have any
impact on the #if 0 lines.
>
> > Can the #if 0 and code be removed? Apparently it predates git.
> >
> > A commented out printk below also predates git.
> >
> I think all the "#if 0" comment out codes and printk codes can be removed, shall I send
> v2 to do this?
It seems reasonable to me.
julia
> > julia
> >
> >>
> >> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> >> ---
> >> drivers/net/ethernet/natsemi/ns83820.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> >> index 958fced4dacf..fb064df3a1a6 100644
> >> --- a/drivers/net/ethernet/natsemi/ns83820.c
> >> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> >> @@ -1869,7 +1869,7 @@ static unsigned ns83820_mii_write_reg(struct ns83820 *dev, unsigned phy, unsigne
> >> static void ns83820_probe_phy(struct net_device *ndev)
> >> {
> >> struct ns83820 *dev = PRIV(ndev);
> >> - static int first;
> >> + int first;
> >> int i;
> >> #define MII_PHYIDR1 0x02
> >> #define MII_PHYIDR2 0x03
> >> --
> >> 2.20.1
> >>
> >>
> >
> > .
> >
>
>
^ permalink raw reply
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