* [PATCH net-next 09/11] qed: Hold a single array for SBs
From: Yuval Mintz @ 2017-06-01 12:29 UTC (permalink / raw)
To: davem, netdev; +Cc: Yuval Mintz
In-Reply-To: <20170601122911.22493-1-Yuval.Mintz@cavium.com>
A PF today holds 2 different arrays - one holding information
about the HW configuration and one holding information about
the SBs that are used by the protocol drivers.
These arrays aren't really connected - e.g., protocol driver
initializing a given SB would not mark the same SB as occupied
in the HW shadow array.
Move into a single array [at least for PFs] - hold the mapping
of the driver-protocol SBs on the HW entry which they configure.
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed.h | 4 ---
drivers/net/ethernet/qlogic/qed/qed_fcoe.c | 5 +++-
drivers/net/ethernet/qlogic/qed/qed_int.c | 46 ++++++++++++++++++++++-------
drivers/net/ethernet/qlogic/qed/qed_int.h | 23 +++++++++++----
drivers/net/ethernet/qlogic/qed/qed_iscsi.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_roce.c | 5 ++--
drivers/net/ethernet/qlogic/qed/qed_vf.c | 27 +++++++++++++++--
drivers/net/ethernet/qlogic/qed/qed_vf.h | 18 +++++++++++
8 files changed, 103 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index e0becec..ffc0807 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -495,10 +495,6 @@ struct qed_hwfn {
bool b_rdma_enabled_in_prs;
u32 rdma_prs_search_reg;
- /* Array of sb_info of all status blocks */
- struct qed_sb_info *sbs_info[MAX_SB_PER_PF_MIMD];
- u16 num_sbs;
-
struct qed_cxt_mngr *p_cxt_mngr;
/* Flag indicating whether interrupts are enabled or not*/
diff --git a/drivers/net/ethernet/qlogic/qed/qed_fcoe.c b/drivers/net/ethernet/qlogic/qed/qed_fcoe.c
index cb342f1..3fc4ff2 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_fcoe.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_fcoe.c
@@ -183,7 +183,10 @@ qed_sp_fcoe_func_start(struct qed_hwfn *p_hwfn,
p_data->q_params.queue_relative_offset = (u8)tmp;
for (i = 0; i < fcoe_pf_params->num_cqs; i++) {
- tmp = cpu_to_le16(p_hwfn->sbs_info[i]->igu_sb_id);
+ u16 igu_sb_id;
+
+ igu_sb_id = qed_get_igu_sb_id(p_hwfn, i);
+ tmp = cpu_to_le16(igu_sb_id);
p_data->q_params.cq_cmdq_sb_num_arr[i] = tmp;
}
diff --git a/drivers/net/ethernet/qlogic/qed/qed_int.c b/drivers/net/ethernet/qlogic/qed/qed_int.c
index 96eee1e..4f068a3 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_int.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_int.c
@@ -1452,7 +1452,7 @@ static u16 qed_get_pf_igu_sb_id(struct qed_hwfn *p_hwfn, u16 vector_id)
return QED_SB_INVALID_IDX;
}
-static u16 qed_get_igu_sb_id(struct qed_hwfn *p_hwfn, u16 sb_id)
+u16 qed_get_igu_sb_id(struct qed_hwfn *p_hwfn, u16 sb_id)
{
u16 igu_sb_id;
@@ -1485,8 +1485,19 @@ int qed_int_sb_init(struct qed_hwfn *p_hwfn,
sb_info->igu_sb_id = qed_get_igu_sb_id(p_hwfn, sb_id);
if (sb_id != QED_SP_SB_ID) {
- p_hwfn->sbs_info[sb_id] = sb_info;
- p_hwfn->num_sbs++;
+ if (IS_PF(p_hwfn->cdev)) {
+ struct qed_igu_info *p_info;
+ struct qed_igu_block *p_block;
+
+ p_info = p_hwfn->hw_info.p_igu_info;
+ p_block = &p_info->entry[sb_info->igu_sb_id];
+
+ p_block->sb_info = sb_info;
+ p_block->status &= ~QED_IGU_STATUS_FREE;
+ p_info->usage.free_cnt--;
+ } else {
+ qed_vf_set_sb_info(p_hwfn, sb_id, sb_info);
+ }
}
sb_info->cdev = p_hwfn->cdev;
@@ -1515,20 +1526,35 @@ int qed_int_sb_init(struct qed_hwfn *p_hwfn,
int qed_int_sb_release(struct qed_hwfn *p_hwfn,
struct qed_sb_info *sb_info, u16 sb_id)
{
- if (sb_id == QED_SP_SB_ID) {
- DP_ERR(p_hwfn, "Do Not free sp sb using this function");
- return -EINVAL;
- }
+ struct qed_igu_block *p_block;
+ struct qed_igu_info *p_info;
+
+ if (!sb_info)
+ return 0;
/* zero status block and ack counter */
sb_info->sb_ack = 0;
memset(sb_info->sb_virt, 0, sizeof(*sb_info->sb_virt));
- if (p_hwfn->sbs_info[sb_id] != NULL) {
- p_hwfn->sbs_info[sb_id] = NULL;
- p_hwfn->num_sbs--;
+ if (IS_VF(p_hwfn->cdev)) {
+ qed_vf_set_sb_info(p_hwfn, sb_id, NULL);
+ return 0;
}
+ p_info = p_hwfn->hw_info.p_igu_info;
+ p_block = &p_info->entry[sb_info->igu_sb_id];
+
+ /* Vector 0 is reserved to Default SB */
+ if (!p_block->vector_number) {
+ DP_ERR(p_hwfn, "Do Not free sp sb using this function");
+ return -EINVAL;
+ }
+
+ /* Lose reference to client's SB info, and fix counters */
+ p_block->sb_info = NULL;
+ p_block->status |= QED_IGU_STATUS_FREE;
+ p_info->usage.free_cnt++;
+
return 0;
}
diff --git a/drivers/net/ethernet/qlogic/qed/qed_int.h b/drivers/net/ethernet/qlogic/qed/qed_int.h
index 273e73a..bc61c50 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_int.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_int.h
@@ -202,18 +202,20 @@ void qed_int_disable_post_isr_release(struct qed_dev *cdev);
#define QED_SB_INVALID_IDX 0xffff
struct qed_igu_block {
- u8 status;
+ u8 status;
#define QED_IGU_STATUS_FREE 0x01
#define QED_IGU_STATUS_VALID 0x02
#define QED_IGU_STATUS_PF 0x04
#define QED_IGU_STATUS_DSB 0x08
- u8 vector_number;
- u8 function_id;
- u8 is_pf;
+ u8 vector_number;
+ u8 function_id;
+ u8 is_pf;
/* Index inside IGU [meant for back reference] */
- u16 igu_sb_id;
+ u16 igu_sb_id;
+
+ struct qed_sb_info *sb_info;
};
struct qed_igu_info {
@@ -224,7 +226,16 @@ struct qed_igu_info {
};
-/* TODO Names of function may change... */
+/**
+ * @brief Translate the weakly-defined client sb-id into an IGU sb-id
+ *
+ * @param p_hwfn
+ * @param sb_id - user provided sb_id
+ *
+ * @return an index inside IGU CAM where the SB resides
+ */
+u16 qed_get_igu_sb_id(struct qed_hwfn *p_hwfn, u16 sb_id);
+
/**
* @brief return a pointer to an unused valid SB
*
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
index 43a20a6..bc8ce09 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iscsi.c
@@ -220,7 +220,7 @@ qed_sp_iscsi_func_start(struct qed_hwfn *p_hwfn,
p_queue->cmdq_sb_pi = p_params->gl_cmd_pi;
for (i = 0; i < p_params->num_queues; i++) {
- val = p_hwfn->sbs_info[i]->igu_sb_id;
+ val = qed_get_igu_sb_id(p_hwfn, i);
p_queue->cq_cmdq_sb_num_arr[i] = cpu_to_le16(val);
}
diff --git a/drivers/net/ethernet/qlogic/qed/qed_roce.c b/drivers/net/ethernet/qlogic/qed/qed_roce.c
index eb1a5cf..b9434b7 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_roce.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_roce.c
@@ -581,6 +581,7 @@ static int qed_rdma_start_fw(struct qed_hwfn *p_hwfn,
struct qed_sp_init_data init_data;
struct qed_spq_entry *p_ent;
u32 cnq_id, sb_id;
+ u16 igu_sb_id;
int rc;
DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "Starting FW\n");
@@ -612,10 +613,10 @@ static int qed_rdma_start_fw(struct qed_hwfn *p_hwfn,
for (cnq_id = 0; cnq_id < params->desired_cnq; cnq_id++) {
sb_id = qed_rdma_get_sb_id(p_hwfn, cnq_id);
+ igu_sb_id = qed_get_igu_sb_id(p_hwfn, sb_id);
+ p_ramrod->cnq_params[cnq_id].sb_num = cpu_to_le16(igu_sb_id);
p_cnq_params = &p_ramrod->cnq_params[cnq_id];
p_cnq_pbl_list = ¶ms->cnq_pbl_list[cnq_id];
- p_cnq_params->sb_num =
- cpu_to_le16(p_hwfn->sbs_info[sb_id]->igu_sb_id);
p_cnq_params->sb_index = p_hwfn->pf_params.rdma_pf_params.gl_pi;
p_cnq_params->num_pbl_pages = p_cnq_pbl_list->num_pbl_pages;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_vf.c b/drivers/net/ethernet/qlogic/qed/qed_vf.c
index 11d71e5..3703b22 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_vf.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_vf.c
@@ -792,9 +792,12 @@ int qed_vf_pf_vport_start(struct qed_hwfn *p_hwfn,
req->only_untagged = only_untagged;
/* status blocks */
- for (i = 0; i < p_hwfn->vf_iov_info->acquire_resp.resc.num_sbs; i++)
- if (p_hwfn->sbs_info[i])
- req->sb_addr[i] = p_hwfn->sbs_info[i]->sb_phys;
+ for (i = 0; i < p_hwfn->vf_iov_info->acquire_resp.resc.num_sbs; i++) {
+ struct qed_sb_info *p_sb = p_hwfn->vf_iov_info->sbs_info[i];
+
+ if (p_sb)
+ req->sb_addr[i] = p_sb->sb_phys;
+ }
/* add list termination tlv */
qed_add_tlv(p_hwfn, &p_iov->offset,
@@ -1240,6 +1243,24 @@ u16 qed_vf_get_igu_sb_id(struct qed_hwfn *p_hwfn, u16 sb_id)
return p_iov->acquire_resp.resc.hw_sbs[sb_id].hw_sb_id;
}
+void qed_vf_set_sb_info(struct qed_hwfn *p_hwfn,
+ u16 sb_id, struct qed_sb_info *p_sb)
+{
+ struct qed_vf_iov *p_iov = p_hwfn->vf_iov_info;
+
+ if (!p_iov) {
+ DP_NOTICE(p_hwfn, "vf_sriov_info isn't initialized\n");
+ return;
+ }
+
+ if (sb_id >= PFVF_MAX_SBS_PER_VF) {
+ DP_NOTICE(p_hwfn, "Can't configure SB %04x\n", sb_id);
+ return;
+ }
+
+ p_iov->sbs_info[sb_id] = p_sb;
+}
+
int qed_vf_read_bulletin(struct qed_hwfn *p_hwfn, u8 *p_change)
{
struct qed_vf_iov *p_iov = p_hwfn->vf_iov_info;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_vf.h b/drivers/net/ethernet/qlogic/qed/qed_vf.h
index 34ac70b..d7e1a12 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_vf.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_vf.h
@@ -627,6 +627,14 @@ struct qed_vf_iov {
* this has to be propagated as it affects the fastpath.
*/
bool b_pre_fp_hsi;
+
+ /* Current day VFs are passing the SBs physical address on vport
+ * start, and as they lack an IGU mapping they need to store the
+ * addresses of previously registered SBs.
+ * Even if we were to change configuration flow, due to backward
+ * compatibility [with older PFs] we'd still need to store these.
+ */
+ struct qed_sb_info *sbs_info[PFVF_MAX_SBS_PER_VF];
};
#ifdef CONFIG_QED_SRIOV
@@ -837,6 +845,16 @@ int qed_vf_pf_release(struct qed_hwfn *p_hwfn);
u16 qed_vf_get_igu_sb_id(struct qed_hwfn *p_hwfn, u16 sb_id);
/**
+ * @brief Stores [or removes] a configured sb_info.
+ *
+ * @param p_hwfn
+ * @param sb_id - zero-based SB index [for fastpath]
+ * @param sb_info - may be NULL [during removal].
+ */
+void qed_vf_set_sb_info(struct qed_hwfn *p_hwfn,
+ u16 sb_id, struct qed_sb_info *p_sb);
+
+/**
* @brief qed_vf_pf_vport_start - perform vport start for VF.
*
* @param p_hwfn
--
2.9.4
^ permalink raw reply related
* [PATCH net-next 11/11] qed: No need to reset SBs on IOV init
From: Yuval Mintz @ 2017-06-01 12:29 UTC (permalink / raw)
To: davem, netdev; +Cc: Yuval Mintz
In-Reply-To: <20170601122911.22493-1-Yuval.Mintz@cavium.com>
Since we're resetting the IGU CAM each time we initialize the PF
device, there's no need to reset the VF SBs again when initializing
IOV.
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_sriov.c | 30 +----------------------------
drivers/net/ethernet/qlogic/qed/qed_sriov.h | 5 ++---
3 files changed, 4 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 939e85c..7649f35 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -1030,7 +1030,7 @@ void qed_resc_setup(struct qed_dev *cdev)
qed_int_setup(p_hwfn, p_hwfn->p_main_ptt);
- qed_iov_setup(p_hwfn, p_hwfn->p_main_ptt);
+ qed_iov_setup(p_hwfn);
#ifdef CONFIG_QED_LL2
if (p_hwfn->using_ll2)
qed_ll2_setup(p_hwfn);
diff --git a/drivers/net/ethernet/qlogic/qed/qed_sriov.c b/drivers/net/ethernet/qlogic/qed/qed_sriov.c
index b7dda60..6ca2892 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sriov.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.c
@@ -378,33 +378,6 @@ static int qed_iov_pci_cfg_info(struct qed_dev *cdev)
return 0;
}
-static void qed_iov_clear_vf_igu_blocks(struct qed_hwfn *p_hwfn,
- struct qed_ptt *p_ptt)
-{
- struct qed_igu_block *p_sb;
- u16 sb_id;
- u32 val;
-
- if (!p_hwfn->hw_info.p_igu_info) {
- DP_ERR(p_hwfn,
- "qed_iov_clear_vf_igu_blocks IGU Info not initialized\n");
- return;
- }
-
- for (sb_id = 0; sb_id < QED_MAPPING_MEMORY_SIZE(p_hwfn->cdev);
- sb_id++) {
- p_sb = &p_hwfn->hw_info.p_igu_info->entry[sb_id];
- if ((p_sb->status & QED_IGU_STATUS_FREE) &&
- !(p_sb->status & QED_IGU_STATUS_PF)) {
- val = qed_rd(p_hwfn, p_ptt,
- IGU_REG_MAPPING_MEMORY + sb_id * 4);
- SET_FIELD(val, IGU_MAPPING_LINE_VALID, 0);
- qed_wr(p_hwfn, p_ptt,
- IGU_REG_MAPPING_MEMORY + 4 * sb_id, val);
- }
- }
-}
-
static void qed_iov_setup_vfdb(struct qed_hwfn *p_hwfn)
{
struct qed_hw_sriov_info *p_iov = p_hwfn->cdev->p_iov_info;
@@ -555,13 +528,12 @@ int qed_iov_alloc(struct qed_hwfn *p_hwfn)
return qed_iov_allocate_vfdb(p_hwfn);
}
-void qed_iov_setup(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
+void qed_iov_setup(struct qed_hwfn *p_hwfn)
{
if (!IS_PF_SRIOV(p_hwfn) || !IS_PF_SRIOV_ALLOC(p_hwfn))
return;
qed_iov_setup_vfdb(p_hwfn);
- qed_iov_clear_vf_igu_blocks(p_hwfn, p_ptt);
}
void qed_iov_free(struct qed_hwfn *p_hwfn)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_sriov.h b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
index 81a497c..801cc00 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h
@@ -316,9 +316,8 @@ int qed_iov_alloc(struct qed_hwfn *p_hwfn);
* @brief qed_iov_setup - setup sriov related resources
*
* @param p_hwfn
- * @param p_ptt
*/
-void qed_iov_setup(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt);
+void qed_iov_setup(struct qed_hwfn *p_hwfn);
/**
* @brief qed_iov_free - free sriov related resources
@@ -397,7 +396,7 @@ static inline int qed_iov_alloc(struct qed_hwfn *p_hwfn)
return 0;
}
-static inline void qed_iov_setup(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt)
+static inline void qed_iov_setup(struct qed_hwfn *p_hwfn)
{
}
--
2.9.4
^ permalink raw reply related
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Sebastian Ott @ 2017-06-01 12:34 UTC (permalink / raw)
To: Xin Long
Cc: David S. Miller, Haidong Li, Nikolay Aleksandrov, Ivan Vecera,
Stephen Hemminger, network dev, LKML, Heiko Carstens,
Martin Schwidefsky
In-Reply-To: <CADvbK_c_bXnPL5NdADj=aCv8f-A0wPKdUmQJqm=XDRf0rizXcA@mail.gmail.com>
On Thu, 1 Jun 2017, Xin Long wrote:
> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
> <sebott@linux.vnet.ibm.com> wrote:
> > [...]
> I couldn't see any bridge-related thing here, and it couldn't be reproduced
> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
> is something else in you machine.
>
> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
> machine normally, then:
> # brctl addbr br0
> # ip link set br0 up
> # brctl stp br0 on
>
> to check if it will still hang.
Nope. That doesn't hang.
> If it can't be reproduced in this way, pls add this on your kernel:
>
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
> br->stp_enabled = BR_KERNEL_STP;
> br_debug(br, "using kernel STP\n");
>
> + WARN_ON(1);
> /* To start timers on any ports left in blocking */
> mod_timer(&br->hello_timer, jiffies + br->hello_time);
> br_port_state_selection(br);
> + pr_warn("hello timer start done\n");
> }
>
> spin_unlock_bh(&br->lock);
> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
> index 60b6fe2..c98b3e5 100644
> --- a/net/bridge/br_stp_timer.c
> +++ b/net/bridge/br_stp_timer.c
> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
> if (br->dev->flags & IFF_UP) {
> br_config_bpdu_generation(br);
>
> - if (br->stp_enabled == BR_KERNEL_STP)
> + if (br->stp_enabled != BR_USER_STP)
> mod_timer(&br->hello_timer,
> round_jiffies(jiffies + br->hello_time));
>
>
> let's see if it hangs when starting the timer. Thanks.
No hang either:
[ 134.018104] ------------[ cut here ]------------
[ 134.018144] WARNING: CPU: 1 PID: 1339 at net/bridge/br_stp_if.c:181 br_stp_set_enabled+0x154/0x2b0 [bridge]
[ 134.018149] Modules linked in: bridge stp llc rdma_ucm ib_ucm ib_uverbs [...]
[ 134.018257] CPU: 1 PID: 1339 Comm: brctl Not tainted 4.12.0-rc3-00011-gf511c0b-dirty #587
[ 134.018262] Hardware name: IBM 2827 H66 705 (LPAR)
[ 134.018266] task: 00000000d141c100 task.stack: 00000000d1430000
[ 134.018271] Krnl PSW : 0704f00180000000 000003ff802bc4c4 (br_stp_set_enabled+0x154/0x2b0 [bridge])
[ 134.018286] R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:3 PM:0 RI:0 EA:3
[ 134.018294] Krnl GPRS: 00000000c5eae501 00000000000005dc 0000000000000bb8 0000000000000001
[ 134.018298] 000003ff802bc42c 00000000d1433c78 0000000000000001 00000000d3ad2d60
[ 134.018303] 0000000000000002 000003ff802c21a8 00000000d3ad2d60 00000000fffffffe
[ 134.018308] 00000000d1671738 00000000000026a0 000003ff802bc42c 00000000d1433c38
[ 134.018320] Krnl Code: 000003ff802bc4b4: e54ca9180001 mvhi 2328(%r10),1
000003ff802bc4ba: c00400000000 brcl 0,3ff802bc4ba
#000003ff802bc4c0: a7f40001 brc 15,3ff802bc4c2
>000003ff802bc4c4: c418ffffb5aa lgrl %r1,3ff802b3018
000003ff802bc4ca: 4120ac10 la %r2,3088(%r10)
000003ff802bc4ce: e33010000004 lg %r3,0(%r1)
000003ff802bc4d4: e330a8d80008 ag %r3,2264(%r10)
000003ff802bc4da: c0e5ffffbc8b brasl %r14,3ff802b3df0
[ 134.018374] Call Trace:
[ 134.018384] ([<000003ff802bc42c>] br_stp_set_enabled+0xbc/0x2b0 [bridge])
[ 134.018393] [<000003ff802c21d2>] set_stp_state+0x2a/0x40 [bridge]
[ 134.018402] [<000003ff802c0f30>] store_bridge_parm+0xa8/0xf8 [bridge]
[ 134.018410] [<00000000004012f2>] kernfs_fop_write+0x132/0x208
[ 134.018417] [<000000000036088e>] __vfs_write+0x36/0x140
[ 134.018422] [<0000000000361b54>] vfs_write+0xbc/0x1a0
[ 134.018427] [<000000000036323e>] SyS_write+0x66/0xc0
[ 134.018434] [<00000000008ccc80>] system_call+0xc4/0x28c
[ 134.018438] 5 locks held by brctl/1339:
[ 134.018443] #0: (sb_writers#5){.+.+.+}, at: [<0000000000361b3e>] vfs_write+0xa6/0x1a0
[ 134.018462] #1: (&of->mutex){+.+.+.}, at: [<0000000000401372>] kernfs_fop_write+0x1b2/0x208
[ 134.018478] #2: (s_active#116){.+.+.+}, at: [<000000000040137e>] kernfs_fop_write+0x1be/0x208
[ 134.018496] #3: (rtnl_mutex){+.+.+.}, at: [<000003ff802c0f08>] store_bridge_parm+0x80/0xf8 [bridge]
[ 134.018517] #4: (&(&br->lock)->rlock){+.....}, at: [<000003ff802bc42c>] br_stp_set_enabled+0xbc/0x2b0 [bridge]
[ 134.018537] Last Breaking-Event-Address:
[ 134.018546] [<000003ff802bc4c0>] br_stp_set_enabled+0x150/0x2b0 [bridge]
[ 134.018551] ---[ end trace 0fc342e82de9b3d7 ]---
[ 134.018638] hello timer start done
In the system dump I observed that 3 cpus are within mod_timer (different
timers) and spin for some lock (one of them is the console driver which
explains the missing messages).
Using a different config with object debugging enabled I got this
interesting output:
[ 18.759850] virbr0: port 1(virbr0-nic) entered disabled state
[ 18.825885] ODEBUG: free active (active state 0) object type: timer_list hint: br_hello_timer_expired+0x0/0xb8 [bridge]
[ 18.826081] ------------[ cut here ]------------
[ 18.826085] WARNING: CPU: 1 PID: 519 at lib/debugobjects.c:289 debug_print_object+0xb0/0xd0
[ 18.826087] Modules linked in: bridge stp llc rng_core ghash_s390 prng [...]
[ 18.826118] CPU: 1 PID: 519 Comm: libvirtd Not tainted 4.12.0-rc3-00002-g475ef2f #359
[ 18.826120] Hardware name: IBM 2827 H66 705 (LPAR)
[ 18.826123] task: 000000006dca4100 task.stack: 000000006e7b0000
[ 18.826125] Krnl PSW : 0404d00180000000 00000000006420d0 (debug_print_object+0xb0/0xd0)
[ 18.826131] R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3
[ 18.826135] Krnl GPRS: ffffffffffffffe9 0000000080000001 000000000000006b 0000000000a973f4
[ 18.826138] 0000000000292296 0000000000000000 0000000000a8cca2 0000000001e3bf18
[ 18.826140] 0000000001e3bf10 0000000069c4e2a8 0000000069c4c2a8 0000000000ab2f48
[ 18.826143] 0000000000cc6f18 000000006fce91a8 00000000006420cc 000000006e7b3ad0
[ 18.826152] Krnl Code: 00000000006420c0: c02000242a65 larl %r2,ac758a
00000000006420c6: c0e5ffe280c1 brasl %r14,292248
#00000000006420cc: a7f40001 brc 15,6420ce
>00000000006420d0: c41d0032960a lrl %r1,c94ce4
00000000006420d6: e340f0e80004 lg %r4,232(%r15)
00000000006420dc: a71a0001 ahi %r1,1
00000000006420e0: eb6ff0a80004 lmg %r6,%r15,168(%r15)
00000000006420e6: c41f003295ff strl %r1,c94ce4
[ 18.826177] Call Trace:
[ 18.826180] ([<00000000006420cc>] debug_print_object+0xac/0xd0)
[ 18.826183] [<00000000006431ba>] __debug_check_no_obj_freed+0xca/0x258
[ 18.826185] [<0000000000319a44>] kfree+0x264/0x410
[ 18.826188] [<00000000006a8e46>] device_release+0x76/0xb0
[ 18.826191] [<000000000060e67e>] kobject_put+0xde/0x1d8
[ 18.826194] [<00000000007bd14e>] netdev_run_todo+0x2be/0x2d0
[ 18.826201] [<000003ff8096b762>] br_del_bridge+0x82/0x98 [bridge]
[ 18.826208] [<000003ff8096d750>] br_ioctl_deviceless_stub+0x100/0x140 [bridge]
[ 18.826211] [<000000000078e562>] sock_ioctl+0x1a2/0x2f0
[ 18.826214] [<000000000035dc3c>] do_vfs_ioctl+0x714/0x7a8
[ 18.826217] [<000000000035dd4c>] SyS_ioctl+0x7c/0xb0
[ 18.826220] [<00000000008f2300>] system_call+0xc4/0x274
[ 18.826222] INFO: lockdep is turned off.
[ 18.826224] Last Breaking-Event-Address:
[ 18.826227] [<00000000006420cc>] debug_print_object+0xac/0xd0
[ 18.826230] ---[ end trace 765b1870ef16b23f ]---
Regards,
Sebastian
^ permalink raw reply
* [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 12:38 UTC (permalink / raw)
To: dvyukov, kcc, edumazet, davem, stephen; +Cc: linux-kernel, netdev
KMSAN reported a use of uninitialized memory in dev_set_alias(),
which was caused by calling strlcpy() (which in turn called strlen())
on the user-supplied non-terminated string.
Signed-off-by: Alexander Potapenko <glider@google.com>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov
For the record, here is the KMSAN report:
==================================================================
BUG: KMSAN: use of unitialized memory in strlcpy+0x8b/0x1b0
CPU: 0 PID: 1062 Comm: probe Not tainted 4.11.0-rc5+ #2763
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x143/0x1b0 lib/dump_stack.c:52
kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:1016
__kmsan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:491
strlen lib/string.c:484
strlcpy+0x8b/0x1b0 lib/string.c:144
dev_set_alias+0x295/0x360 net/core/dev.c:1255
do_setlink+0xfe5/0x5750 net/core/rtnetlink.c:2007
rtnl_setlink+0x469/0x4f0 net/core/rtnetlink.c:2249
rtnetlink_rcv_msg+0x5da/0xb40 net/core/rtnetlink.c:4107
netlink_rcv_skb+0x339/0x5a0 net/netlink/af_netlink.c:2339
rtnetlink_rcv+0x83/0xa0 net/core/rtnetlink.c:4113
netlink_unicast_kernel net/netlink/af_netlink.c:1272
netlink_unicast+0x13b7/0x1480 net/netlink/af_netlink.c:1298
netlink_sendmsg+0x10b8/0x10f0 net/netlink/af_netlink.c:1844
sock_sendmsg_nosec net/socket.c:633
sock_sendmsg net/socket.c:643
sock_write_iter+0x395/0x440 net/socket.c:857
call_write_iter ./include/linux/fs.h:1733
new_sync_write fs/read_write.c:497
__vfs_write+0x619/0x700 fs/read_write.c:510
vfs_write+0x47e/0x8a0 fs/read_write.c:558
SYSC_write+0x17e/0x2f0 fs/read_write.c:605
SyS_write+0x87/0xb0 fs/read_write.c:597
entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
RIP: 0033:0x4052f1
RSP: 002b:00007fde1ed05d80 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004052f1
RDX: 0000000000000026 RSI: 00000000006cf0c0 RDI: 0000000000000032
RBP: 00007fde1ed05da0 R08: 00007fde1ed06700 R09: 00007fde1ed06700
R10: 00007fde1ed069d0 R11: 0000000000000293 R12: 0000000000000000
R13: 0000000000000000 R14: 00007fde1ed069c0 R15: 00007fde1ed06700
origin: 00000000aec00057
save_stack_trace+0x59/0x60 arch/x86/kernel/stacktrace.c:59
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:352
kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:247
kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:260
slab_alloc_node mm/slub.c:2743
__kmalloc_node_track_caller+0x1f4/0x390 mm/slub.c:4349
__kmalloc_reserve net/core/skbuff.c:138
__alloc_skb+0x2cd/0x740 net/core/skbuff.c:231
alloc_skb ./include/linux/skbuff.h:933
netlink_alloc_large_skb net/netlink/af_netlink.c:1144
netlink_sendmsg+0x934/0x10f0 net/netlink/af_netlink.c:1819
sock_sendmsg_nosec net/socket.c:633
sock_sendmsg net/socket.c:643
sock_write_iter+0x395/0x440 net/socket.c:857
call_write_iter ./include/linux/fs.h:1733
new_sync_write fs/read_write.c:497
__vfs_write+0x619/0x700 fs/read_write.c:510
vfs_write+0x47e/0x8a0 fs/read_write.c:558
SYSC_write+0x17e/0x2f0 fs/read_write.c:605
SyS_write+0x87/0xb0 fs/read_write.c:597
entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
==================================================================
and the reproducer:
==================================================================
#include <pthread.h>
int sock = -1;
char buf[] = "\x26\x00\x00\x00\x13\x00\x47\xf1\x07\x01\xc1\xb0"
"\x0e\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00"
"\x09\xef\x18\xff\xff\x00\xf1\x32\x05\x00\x14\x00"
"\x6e\x35";
void do_work(int arg) {
switch (arg) {
case 0:
sock = socket(0x10, 0x3, 0x0);
break;
case 1:
write(sock, buf, 0x26);
break;
}
}
void *thread_fn(void *arg) {
int actual_arg = (int)arg;
int iter = 10000, i;
for (i = 0; i < iter; i++) {
do_work(actual_arg);
usleep(100);
}
return NULL;
}
int main() {
pthread_t thr[4];
int i;
for (i = 0; i < 4; i++) {
pthread_create(&thr, NULL, thread_fn, (void*)(i % 2));
}
sleep(10);
return 0;
}
==================================================================
---
net/core/dev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index fca407b4a6ea..3e3b29133cc9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
return -ENOMEM;
dev->ifalias = new_ifalias;
- strlcpy(dev->ifalias, alias, len+1);
+ /* alias comes from the userspace and may not be zero-terminated. */
+ memcpy(dev->ifalias, alias, len);
+ dev->ifalias[len] = 0;
return len;
}
--
2.13.0.219.gdb65acc882-goog
^ permalink raw reply related
* Cherry pick commit *net: better skb->sender_cpu and skb->napi_id cohabitation* to 4.4 series?
From: Paul Menzel @ 2017-06-01 12:43 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet; +Cc: netdev, Alexei Starovoitov, it+linux-netdev
Dear David, dear Eric,
We noticed, that we manually apply commit 52bd2d62ce67 (net: better
skb->sender_cpu and skb->napi_id cohabitation), present since v4.5-rc1,
to our Linux 4.4 images.
Would it be useful to have it included in the 4.4 series?
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Russell King - ARM Linux @ 2017-06-01 12:51 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601122350.GA9282@lunn.ch>
On Thu, Jun 01, 2017 at 02:23:50PM +0200, Andrew Lunn wrote:
> On Thu, Jun 01, 2017 at 11:26:36AM +0100, Russell King wrote:
> > genphy_restart_aneg() can only restart autonegotiation on clause 22
> > PHYs. Add a phy_restart_aneg() function which selects between the
> > clause 22 and clause 45 restart functionality depending on the PHY
> > type.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/phy.c | 23 +++++++++++++++++++++--
> > include/linux/phy.h | 1 +
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 82ab8fb82587..25b24789a409 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -149,6 +149,25 @@ static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
> > return 0;
> > }
> >
> > +/**
> > + * phy_restart_aneg - restart auto-negotiation
> > + * @phydev: target phy_device struct
> > + *
> > + * Restart the autonegotiation on @phydev. Returns >= 0 on success or
> > + * negative errno on error.
> > + */
> > +int phy_restart_aneg(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + if (phydev->is_c45)
> > + ret = genphy_c45_restart_aneg(phydev);
> > + else
> > + ret = genphy_restart_aneg(phydev);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_restart_aneg);
>
> Hi Russell
>
> Isn't the same sort of thing needed in phy_aneg_done()?
No, because phy_aneg_done() already has hooks in it which PHY drivers
can use to override the default C22 implementation.
I did toy with providing a similar conditional in phy_aneg_done(), but
decided it was better to use the existing hooks where present, rather
than needlessly adding additional code.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 5/5] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Andrew Lunn @ 2017-06-01 12:51 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1dGNJc-000442-8H@rmk-PC.armlinux.org.uk>
> +static int mv3310_read_status(struct phy_device *phydev)
> +{
> + u32 mmd_mask = phydev->c45_ids.devices_in_package;
> + int val;
> +
> + /* The vendor devads do not report link status. Avoid the PHYXS
> + * instance as there are three, and its status depends on the MAC
> + * being appropriately configured for the negotiated speed.
> + */
> + mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2) |
> + BIT(MDIO_MMD_PHYXS));
> +
> + phydev->speed = SPEED_UNKNOWN;
> + phydev->duplex = DUPLEX_UNKNOWN;
> + phydev->lp_advertising = 0;
> + phydev->link = 0;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
> + if (val < 0)
> + return val;
> +
> + if (val & MDIO_STAT1_LSTATUS)
> + return mv3310_read_10gbr_status(phydev);
> +
> + val = genphy_c45_read_link(phydev, mmd_mask);
> + if (val < 0)
> + return val;
> +
> + phydev->link = val > 0 ? 1 : 0;
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> + if (val < 0)
> + return val;
> +
> + if (val & MDIO_AN_STAT1_COMPLETE) {
> + val = genphy_c45_read_lpa(phydev);
> + if (val < 0)
> + return val;
> +
> + /* Read the link partner's 1G advertisment */
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_STAT1000);
> + if (val < 0)
> + return val;
> +
> + phydev->lp_advertising |= mii_stat1000_to_ethtool_lpa_t(val);
> +
> + if (phydev->autoneg == AUTONEG_ENABLE) {
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_RESULT);
> + if (val < 0)
> + return val;
> +
> + if (val & MV_AN_RESULT_SPD_10000)
> + phydev->speed = SPEED_10000;
> + else if (val & MV_AN_RESULT_SPD_1000)
> + phydev->speed = SPEED_1000;
> + else if (val & MV_AN_RESULT_SPD_100)
> + phydev->speed = SPEED_100;
> + else if (val & MV_AN_RESULT_SPD_10)
> + phydev->speed = SPEED_10;
> +
> + phydev->duplex = DUPLEX_FULL;
> + }
> + }
> +
> + if (phydev->autoneg != AUTONEG_ENABLE) {
> + val = genphy_c45_read_pma(phydev);
> + if (val < 0)
> + return val;
> + }
> +
> + if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
> + phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
> + /* The PHY automatically switches its serdes interface (and
> + * active PHYXS instance) between Cisco SGMII and 10GBase-KR
> + * modes according to the speed. Florian suggests setting
> + * phydev->interface to communicate this to the MAC. Only do
> + * this if we are already in either SGMII or 10GBase-KR mode.
> + */
Hi Russell
Just for my understanding. The MAC should check phydev->interface in
its adjust_link function? Can we document this here please. I wounder
if there is somewhere in the generic code we should also document
this?
> +static struct phy_driver mv3310_drivers[] = {
> + {
> + .phy_id = 0x002b09aa,
> + .phy_id_mask = 0xffffffff,
How about adding this ID to include/linux/marvell_phy.h? Or do you
think this is not actually a Marvell ID, but some 3rd party which
Marvell has licensed it from?
Andrew
^ permalink raw reply
* [PATCH net] bnx2x: Fix Multi-Cos
From: Yuval Mintz @ 2017-06-01 12:57 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, Yuval Mintz
Apparently multi-cos isn't working for bnx2x quite some time -
driver implements ndo_select_queue() to allow queue-selection
for FCoE, but the regular L2 flow would cause it to modulo the
fallback's result by the number of queues.
The fallback would return a queue matching the needed tc
[via __skb_tx_hash()], but since the modulo is by the number of TSS
queues where number of TCs is not accounted, transmission would always
be done by a queue configured into using TC0.
Fixes: ada7c19e6d27 ("bnx2x: use XPS if possible for bnx2x_select_queue instead of pure hash")
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
Hi Dave,
Please consider applying this to `net'.
Thanks,
Yuval
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index eccb3d1..5f49334 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -1926,7 +1926,7 @@ u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb,
}
/* select a non-FCoE queue */
- return fallback(dev, skb) % BNX2X_NUM_ETH_QUEUES(bp);
+ return fallback(dev, skb) % (BNX2X_NUM_ETH_QUEUES(bp) * bp->max_cos);
}
void bnx2x_set_num_queues(struct bnx2x *bp)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Andrew Lunn @ 2017-06-01 13:05 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601125132.GY22219@n2100.armlinux.org.uk>
> > > +/**
> > > + * phy_restart_aneg - restart auto-negotiation
> > > + * @phydev: target phy_device struct
> > > + *
> > > + * Restart the autonegotiation on @phydev. Returns >= 0 on success or
> > > + * negative errno on error.
> > > + */
> > > +int phy_restart_aneg(struct phy_device *phydev)
> > > +{
> > > + int ret;
> > > +
> > > + if (phydev->is_c45)
> > > + ret = genphy_c45_restart_aneg(phydev);
> > > + else
> > > + ret = genphy_restart_aneg(phydev);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(phy_restart_aneg);
> >
> > Hi Russell
> >
> > Isn't the same sort of thing needed in phy_aneg_done()?
>
> No, because phy_aneg_done() already has hooks in it which PHY drivers
> can use to override the default C22 implementation.
>
> I did toy with providing a similar conditional in phy_aneg_done(), but
> decided it was better to use the existing hooks where present, rather
> than needlessly adding additional code.
Hi Russell
So you are saying a 10G PHY driver always needs to have a aneg_done
callback, even if it just needs to call phygen_c45_aneg_done?
This seems a bit error prone. I can see somebody writing a 10G driver,
leaving out aneg_done() and having the c22 version called. Is the read
of MII_BMSR likely to return 0xffff, since the register does not
exist? If so, genphy_aneg_done() is likely to always return
BMSR_ANEGCOMPLETE.
Seems like a trap waiting for somebody to fall into it. The additional
code might be worth it to avoid placing this trap.
Andrew
^ permalink raw reply
* Re: [PATCH 5/5] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Russell King - ARM Linux @ 2017-06-01 13:06 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601125150.GE9282@lunn.ch>
On Thu, Jun 01, 2017 at 02:51:50PM +0200, Andrew Lunn wrote:
> > +static int mv3310_read_status(struct phy_device *phydev)
> > +{
> > + u32 mmd_mask = phydev->c45_ids.devices_in_package;
> > + int val;
> > +
> > + /* The vendor devads do not report link status. Avoid the PHYXS
> > + * instance as there are three, and its status depends on the MAC
> > + * being appropriately configured for the negotiated speed.
> > + */
> > + mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2) |
> > + BIT(MDIO_MMD_PHYXS));
> > +
> > + phydev->speed = SPEED_UNKNOWN;
> > + phydev->duplex = DUPLEX_UNKNOWN;
> > + phydev->lp_advertising = 0;
> > + phydev->link = 0;
> > + phydev->pause = 0;
> > + phydev->asym_pause = 0;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MDIO_STAT1_LSTATUS)
> > + return mv3310_read_10gbr_status(phydev);
> > +
> > + val = genphy_c45_read_link(phydev, mmd_mask);
> > + if (val < 0)
> > + return val;
> > +
> > + phydev->link = val > 0 ? 1 : 0;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MDIO_AN_STAT1_COMPLETE) {
> > + val = genphy_c45_read_lpa(phydev);
> > + if (val < 0)
> > + return val;
> > +
> > + /* Read the link partner's 1G advertisment */
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_STAT1000);
> > + if (val < 0)
> > + return val;
> > +
> > + phydev->lp_advertising |= mii_stat1000_to_ethtool_lpa_t(val);
> > +
> > + if (phydev->autoneg == AUTONEG_ENABLE) {
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_RESULT);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MV_AN_RESULT_SPD_10000)
> > + phydev->speed = SPEED_10000;
> > + else if (val & MV_AN_RESULT_SPD_1000)
> > + phydev->speed = SPEED_1000;
> > + else if (val & MV_AN_RESULT_SPD_100)
> > + phydev->speed = SPEED_100;
> > + else if (val & MV_AN_RESULT_SPD_10)
> > + phydev->speed = SPEED_10;
> > +
> > + phydev->duplex = DUPLEX_FULL;
> > + }
> > + }
> > +
> > + if (phydev->autoneg != AUTONEG_ENABLE) {
> > + val = genphy_c45_read_pma(phydev);
> > + if (val < 0)
> > + return val;
> > + }
> > +
> > + if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
> > + phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
> > + /* The PHY automatically switches its serdes interface (and
> > + * active PHYXS instance) between Cisco SGMII and 10GBase-KR
> > + * modes according to the speed. Florian suggests setting
> > + * phydev->interface to communicate this to the MAC. Only do
> > + * this if we are already in either SGMII or 10GBase-KR mode.
> > + */
>
> Hi Russell
>
> Just for my understanding. The MAC should check phydev->interface in
> its adjust_link function? Can we document this here please. I wounder
> if there is somewhere in the generic code we should also document
> this?
Yes, it would need to, since it would be unreliable to determine from
the link mode bits.
> > +static struct phy_driver mv3310_drivers[] = {
> > + {
> > + .phy_id = 0x002b09aa,
> > + .phy_id_mask = 0xffffffff,
>
> How about adding this ID to include/linux/marvell_phy.h? Or do you
> think this is not actually a Marvell ID, but some 3rd party which
> Marvell has licensed it from?
The vendor part is not the conventional Marvell ID. The 3310 appears
to be two or three different vendors IP all glued together, some of
which is in a way which violates the clause 45 register definitions.
Eg, there's multiple instances of PHYXS at 4K offsets, but C45
indicates that these are "reserved" and the place for vendor specific
stuff is in the two vendor MMDs. Same goes for some of the other
MMDs as well. I'll call these "MMD sub-blocks".
Some MMD sub-blocks have a Marvell ID in them, but others do not.
The OUI for the ID listed above is apparently "FiberHome Digital
Technology Co.,Ltd." but you'll also find the ID "0141 0daa" in some
of the MMD sub-blocks which corresponds with Marvell.
We can't use the sub-blocks though, because as I say, that's rather
non-standard and against the Clause 45 spec.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Russell King - ARM Linux @ 2017-06-01 13:09 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601130527.GG9282@lunn.ch>
On Thu, Jun 01, 2017 at 03:05:27PM +0200, Andrew Lunn wrote:
> So you are saying a 10G PHY driver always needs to have a aneg_done
> callback, even if it just needs to call phygen_c45_aneg_done?
>
> This seems a bit error prone. I can see somebody writing a 10G driver,
> leaving out aneg_done() and having the c22 version called. Is the read
> of MII_BMSR likely to return 0xffff, since the register does not
> exist? If so, genphy_aneg_done() is likely to always return
> BMSR_ANEGCOMPLETE.
Don't forget that the read will fail, so phy_read() will return a
negative number. This practically (and I've tested it) means that
genphy_aneg_done() never indicates negotiation completion, and the
link never comes up.
> Seems like a trap waiting for somebody to fall into it. The additional
> code might be worth it to avoid placing this trap.
The "trap" means a non-functional driver, since the link doesn't
come up.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Andrew Lunn @ 2017-06-01 13:19 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601130900.GA22219@n2100.armlinux.org.uk>
On Thu, Jun 01, 2017 at 02:09:00PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 01, 2017 at 03:05:27PM +0200, Andrew Lunn wrote:
> > So you are saying a 10G PHY driver always needs to have a aneg_done
> > callback, even if it just needs to call phygen_c45_aneg_done?
> >
> > This seems a bit error prone. I can see somebody writing a 10G driver,
> > leaving out aneg_done() and having the c22 version called. Is the read
> > of MII_BMSR likely to return 0xffff, since the register does not
> > exist? If so, genphy_aneg_done() is likely to always return
> > BMSR_ANEGCOMPLETE.
>
> Don't forget that the read will fail, so phy_read() will return a
> negative number.
By fail, you mean return something like -EIO or -ETIMEOUT? Is this
guaranteed in the code somewhere? This particular Marvell PHY only
does c45. But i could imagine some other PHYs answering a c22 request
with 0xffff.
Andrew
^ permalink raw reply
* [PATCH net-next] mlxsw: spectrum: Implement the ethtool flash_device callback
From: Yotam Gigi @ 2017-06-01 13:26 UTC (permalink / raw)
To: netdev, idosch, eladr, ogerlitz, ilant, jiri, arkadis; +Cc: Yotam Gigi
Add callback to the ethtool flash_device op. This callback uses the mlxfw
module to flash the new firmware file to the device.
As the firmware flash process takes about 20 seconds and ethtool takes the
rtnl lock during the flash_device callback, release the rtnl lock at the
beginning of the flash process and take it again before leaving the
callback. This way, the rtnl is not held during the process. To make sure
the device does not get deleted during the flash process, take a reference
to it before releasing the rtnl lock.
Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 51 +++++++++++++++++++++-----
1 file changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index e25c1fd..84b6f36 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -321,6 +321,21 @@ static const struct mlxfw_dev_ops mlxsw_sp_mlxfw_dev_ops = {
.fsm_release = mlxsw_sp_fsm_release
};
+static int mlxsw_sp_firmware_flash(struct mlxsw_sp *mlxsw_sp,
+ const struct firmware *firmware)
+{
+ struct mlxsw_sp_mlxfw_dev mlxsw_sp_mlxfw_dev = {
+ .mlxfw_dev = {
+ .ops = &mlxsw_sp_mlxfw_dev_ops,
+ .psid = mlxsw_sp->bus_info->psid,
+ .psid_size = strlen(mlxsw_sp->bus_info->psid),
+ },
+ .mlxsw_sp = mlxsw_sp
+ };
+
+ return mlxfw_firmware_flash(&mlxsw_sp_mlxfw_dev.mlxfw_dev, firmware);
+}
+
static bool mlxsw_sp_fw_rev_ge(const struct mlxsw_fw_rev *a,
const struct mlxsw_fw_rev *b)
{
@@ -334,14 +349,6 @@ static bool mlxsw_sp_fw_rev_ge(const struct mlxsw_fw_rev *a,
static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
{
const struct mlxsw_fw_rev *rev = &mlxsw_sp->bus_info->fw_rev;
- struct mlxsw_sp_mlxfw_dev mlxsw_sp_mlxfw_dev = {
- .mlxfw_dev = {
- .ops = &mlxsw_sp_mlxfw_dev_ops,
- .psid = mlxsw_sp->bus_info->psid,
- .psid_size = strlen(mlxsw_sp->bus_info->psid),
- },
- .mlxsw_sp = mlxsw_sp
- };
const struct firmware *firmware;
int err;
@@ -361,7 +368,7 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
return err;
}
- err = mlxfw_firmware_flash(&mlxsw_sp_mlxfw_dev.mlxfw_dev, firmware);
+ err = mlxsw_sp_firmware_flash(mlxsw_sp, firmware);
release_firmware(firmware);
return err;
}
@@ -2495,6 +2502,31 @@ mlxsw_sp_port_set_link_ksettings(struct net_device *dev,
return 0;
}
+static int mlxsw_sp_flash_device(struct net_device *dev,
+ struct ethtool_flash *flash)
+{
+ struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+ struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+ const struct firmware *firmware;
+ int err;
+
+ if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
+ return -EOPNOTSUPP;
+
+ dev_hold(dev);
+ rtnl_unlock();
+
+ err = request_firmware_direct(&firmware, flash->data, &dev->dev);
+ if (err)
+ goto out;
+ err = mlxsw_sp_firmware_flash(mlxsw_sp, firmware);
+ release_firmware(firmware);
+out:
+ rtnl_lock();
+ dev_put(dev);
+ return err;
+}
+
static const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
.get_drvinfo = mlxsw_sp_port_get_drvinfo,
.get_link = ethtool_op_get_link,
@@ -2506,6 +2538,7 @@ static const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
.get_sset_count = mlxsw_sp_port_get_sset_count,
.get_link_ksettings = mlxsw_sp_port_get_link_ksettings,
.set_link_ksettings = mlxsw_sp_port_set_link_ksettings,
+ .flash_device = mlxsw_sp_flash_device,
};
static int
--
2.8.4
^ permalink raw reply related
* Re: [PATCH v2 net-next 1/3] perf, bpf: Add BPF support to all perf_event types
From: Peter Zijlstra @ 2017-06-01 13:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S . Miller, Brendan Gregg, Daniel Borkmann, Teng Qin,
netdev, linux-kernel
In-Reply-To: <20170530190339.svpp53bggfznc476@hirez.programming.kicks-ass.net>
On Tue, May 30, 2017 at 09:03:39PM +0200, Peter Zijlstra wrote:
> On Tue, May 30, 2017 at 10:37:46AM -0700, Alexei Starovoitov wrote:
> > On 5/30/17 9:51 AM, Peter Zijlstra wrote:
>
> > > I'm not entirely sure I see how that is required. Should per task not
> > > already work? The WARN that's there will only trigger if you call them
> > > on the wrong task, which is something you shouldn't do anyway.
> >
> > The kernel WARN is considered to be a bug of bpf infra. That's the
> > reason we do all these checks at map update time and at run-time.
> > The bpf program authors should be able to do all possible experiments
> > until their scripts work. Dealing with kernel warns and reboots is not
> > something user space folks like to do.
> > Today bpf_perf_event_read() for per-task events isn't really
> > working due to event->oncpu != cpu runtime check in there.
> > If we convert warns to returns the existing scripts will continue
> > to work as-is and per-task will be possible.
>
> Ah yes.. I always forget that side of things (not ever having seen a
> bpf thing working doesn't help either of course).
>
> Let me consider that for a bit..
OK, do that. Something like the below should do I suppose.
It will return -EOPNOTSUPP for permanent failure (it cannot ever work)
and -EINVAL for temporary failure (could work if you call it on another
task/cpu).
---
kernel/events/core.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8d6acaeeea17..22b6407da374 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3637,10 +3637,10 @@ static inline u64 perf_event_count(struct perf_event *event)
* will not be local and we cannot read them atomically
* - must not have a pmu::count method
*/
-u64 perf_event_read_local(struct perf_event *event)
+int perf_event_read_local(struct perf_event *event, u64 *value)
{
unsigned long flags;
- u64 val;
+ int ret = 0;
/*
* Disabling interrupts avoids all counter scheduling (context
@@ -3648,25 +3648,37 @@ u64 perf_event_read_local(struct perf_event *event)
*/
local_irq_save(flags);
- /* If this is a per-task event, it must be for current */
- WARN_ON_ONCE((event->attach_state & PERF_ATTACH_TASK) &&
- event->hw.target != current);
-
- /* If this is a per-CPU event, it must be for this CPU */
- WARN_ON_ONCE(!(event->attach_state & PERF_ATTACH_TASK) &&
- event->cpu != smp_processor_id());
-
/*
* It must not be an event with inherit set, we cannot read
* all child counters from atomic context.
*/
- WARN_ON_ONCE(event->attr.inherit);
+ if (event->attr.inherit) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
/*
* It must not have a pmu::count method, those are not
* NMI safe.
*/
- WARN_ON_ONCE(event->pmu->count);
+ if (event->pmu->count) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ /* If this is a per-task event, it must be for current */
+ if ((event->attach_state & PERF_ATTACH_TASK) &&
+ event->hw.target != current) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* If this is a per-CPU event, it must be for this CPU */
+ if (!(event->attach_state & PERF_ATTACH_TASK) &&
+ event->cpu != smp_processor_id()) {
+ ret = -EINVAL;
+ goto out;
+ }
/*
* If the event is currently on this CPU, its either a per-task event,
@@ -3676,10 +3688,11 @@ u64 perf_event_read_local(struct perf_event *event)
if (event->oncpu == smp_processor_id())
event->pmu->read(event);
- val = local64_read(&event->count);
+ *value = local64_read(&event->count);
+out:
local_irq_restore(flags);
- return val;
+ return ret;
}
static int perf_event_read(struct perf_event *event, bool group)
^ permalink raw reply related
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Yury Norov @ 2017-06-01 13:47 UTC (permalink / raw)
To: Alexander Potapenko
Cc: dvyukov, kcc, edumazet, davem, stephen, linux-kernel, netdev
In-Reply-To: <20170601123829.51794-1-glider@google.com>
On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
> KMSAN reported a use of uninitialized memory in dev_set_alias(),
> which was caused by calling strlcpy() (which in turn called strlen())
> on the user-supplied non-terminated string.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> v3: removed the multi-line comment
> v2: fixed an off-by-one error spotted by Dmitry Vyukov
[...]
> ---
> net/core/dev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fca407b4a6ea..3e3b29133cc9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
> return -ENOMEM;
> dev->ifalias = new_ifalias;
>
> - strlcpy(dev->ifalias, alias, len+1);
> + /* alias comes from the userspace and may not be zero-terminated. */
So if the comment is correct, you'd use copy_from_user() instead.
> + memcpy(dev->ifalias, alias, len);
> + dev->ifalias[len] = 0;
> return len;
> }
>
> --
> 2.13.0.219.gdb65acc882-goog
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 13:50 UTC (permalink / raw)
To: Yury Norov
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <20170601134737.7dp2pbnek26b6kqf@yury-thinkpad>
On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
>> KMSAN reported a use of uninitialized memory in dev_set_alias(),
>> which was caused by calling strlcpy() (which in turn called strlen())
>> on the user-supplied non-terminated string.
>>
>> Signed-off-by: Alexander Potapenko <glider@google.com>
>> ---
>> v3: removed the multi-line comment
>> v2: fixed an off-by-one error spotted by Dmitry Vyukov
>
> [...]
>
>> ---
>> net/core/dev.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fca407b4a6ea..3e3b29133cc9 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
>> return -ENOMEM;
>> dev->ifalias = new_ifalias;
>>
>> - strlcpy(dev->ifalias, alias, len+1);
>> + /* alias comes from the userspace and may not be zero-terminated. */
>
> So if the comment is correct, you'd use copy_from_user() instead.
Well, the contents of |alias| have been previously copied from the
userspace, but this is a pointer to a kernel buffer, as the function
prototype tells.
Do you think a confusion is possible here?
>> + memcpy(dev->ifalias, alias, len);
>> + dev->ifalias[len] = 0;
>> return len;
>> }
>>
>> --
>> 2.13.0.219.gdb65acc882-goog
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* RE: [PATCH][net-next] net: dsa: make function ksz_rcv static
From: Woojung.Huh @ 2017-06-01 13:53 UTC (permalink / raw)
To: colin.king, UNGLinuxDriver, andrew, vivien.didelot, f.fainelli,
davem, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20170601122242.14341-1-colin.king@canonical.com>
> function ksz_rcv can be made static as it does not need to be
> in global scope. Reformat arguments to make it checkpatch warning
> free too.
>
> Cleans up sparse warning: "symbol 'ksz_rcv' was not declared. Should
> it be static?"
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
^ permalink raw reply
* Re: [PATCH v2 net-next 0/8] Introduce bpf ID
From: David Ahern @ 2017-06-01 13:55 UTC (permalink / raw)
To: Jakub Kicinski, Martin KaFai Lau, Hannes Frederic Sowa
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20170531192256.0e0d0ce1@cakuba.lan>
On 5/31/17 8:22 PM, Jakub Kicinski wrote:
> On Wed, 31 May 2017 11:58:54 -0700, Martin KaFai Lau wrote:
>> This patch series:
>> 1) Introduce ID for both bpf_prog and bpf_map.
>> 2) Add bpf commands to iterate the prog IDs and map
>> IDs of the system.
>> 3) Add bpf commands to get a prog/map fd from an ID
>> 4) Add bpf command to get prog/map info from a fd.
>> The prog/map info is a jump start in this patchset
>> and it is not meant to be a complete list. They can
>> be extended in the future patches.
>
> Hi! IIUC there were plans at some point to store the BPF byte code in
> the kernel, the way it was loaded from the user space and before it
> went through the verifier. Are you still planning on implementing that?
> It would be handy for offloads to have the original byte code, things
> like call inlining make translations of maps calls a bit challenging,
> since we try to run through the verifier again..
>
Save the instructions:
https://github.com/dsahern/linux/commit/5315578db7b0886a3e8e3bef6a56e828ae2bd2c4
Let userspace read it back:
https://github.com/dsahern/linux/commit/619982064b9b582fc5d4a504feffca1730610c4e
One more in between in the branch:
https://github.com/dsahern/linux/tree/bpf/retrieve-bpf-wip
Now that the id code is in, it needs to be returned on link dumps, route
dumps, etc and accessible via fdinfo. The id can be turned into an fd
and the fd read using the above patches.
^ permalink raw reply
* commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Cyril Hrubis @ 2017-06-01 14:00 UTC (permalink / raw)
To: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, Soheil Hassas Yeganeh, WANG Cong,
Yaogong Wang, Steffen Klassert, Al Viro, netdev, linux-kernel
Hi!
I've started to wonder why is ping eating 100% CPU shortly after I've
upgraded my machine to 4.10 and here is what I found:
The ping main_loop() sleeps in poll() on its socket, the poll() usually times
out, at least that's what strace suggets which causes ping to sleep for ~1s in
the kernel.
See ping source at:
https://github.com/iputils/iputils/blob/master/ping_common.c#L587
The poll() seems to start returning POLLERR immediatelly after poll() is called
on the socket in a case that connection has dropped for a short while. It seems to be easily reproducible with:
* Starting ping with some ip address i.e. ping 4.2.2.2
* Letting it ping for a minute or so
* Disconnection a WAN cable from your AP
* After a minute or so ping ends up bussy looping on
poll() that returns with POLLERR immediatelly
* After plugging the cable back the problem gets only
worse since we now spend 99% of the time bussy looping
on the poll() syscall
* And my CPU fan starts to scream loudly
I've bisected the problem to this commit:
commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
Author: Soheil Hassas Yeganeh <soheil@google.com>
Date: Thu Nov 3 18:24:27 2016 -0400
sock: do not set sk_err in sock_dequeue_err_skb
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Nikolay Aleksandrov @ 2017-06-01 14:00 UTC (permalink / raw)
To: Sebastian Ott, Xin Long
Cc: David S. Miller, Haidong Li, Ivan Vecera, Stephen Hemminger,
network dev, LKML, Heiko Carstens, Martin Schwidefsky
In-Reply-To: <alpine.LFD.2.20.1706011415500.1688@schleppi>
On 01/06/17 15:34, Sebastian Ott wrote:
> On Thu, 1 Jun 2017, Xin Long wrote:
>> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
>> <sebott@linux.vnet.ibm.com> wrote:
>>> [...]
>> I couldn't see any bridge-related thing here, and it couldn't be reproduced
>> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
>> is something else in you machine.
>>
>> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
>> machine normally, then:
>> # brctl addbr br0
>> # ip link set br0 up
>> # brctl stp br0 on
>>
>> to check if it will still hang.
>
> Nope. That doesn't hang.
>
>
>> If it can't be reproduced in this way, pls add this on your kernel:
>>
>> --- a/net/bridge/br_stp_if.c
>> +++ b/net/bridge/br_stp_if.c
>> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
>> br->stp_enabled = BR_KERNEL_STP;
>> br_debug(br, "using kernel STP\n");
>>
>> + WARN_ON(1);
>> /* To start timers on any ports left in blocking */
>> mod_timer(&br->hello_timer, jiffies + br->hello_time);
>> br_port_state_selection(br);
>> + pr_warn("hello timer start done\n");
>> }
>>
>> spin_unlock_bh(&br->lock);
>> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
>> index 60b6fe2..c98b3e5 100644
>> --- a/net/bridge/br_stp_timer.c
>> +++ b/net/bridge/br_stp_timer.c
>> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>> if (br->dev->flags & IFF_UP) {
>> br_config_bpdu_generation(br);
>>
>> - if (br->stp_enabled == BR_KERNEL_STP)
>> + if (br->stp_enabled != BR_USER_STP)
>> mod_timer(&br->hello_timer,
>> round_jiffies(jiffies + br->hello_time));
>>
>>
>> let's see if it hangs when starting the timer. Thanks.
>
> No hang either:
>
[snip]
Could you please try the patch below ?
---
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 4efd5d54498a..89110319ef0f 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -173,7 +173,8 @@ static void br_stp_start(struct net_bridge *br)
br_debug(br, "using kernel STP\n");
/* To start timers on any ports left in blocking */
- mod_timer(&br->hello_timer, jiffies + br->hello_time);
+ if (br->dev->flags & IFF_UP)
+ mod_timer(&br->hello_timer, jiffies + br->hello_time);
br_port_state_selection(br);
}
^ permalink raw reply related
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Yury Norov @ 2017-06-01 14:04 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <CAG_fn=U0n4Sn4_44e63FjsYPTTDX4ivAbTcuL2s3erFc+g6n2g@mail.gmail.com>
On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote:
> On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
> >> KMSAN reported a use of uninitialized memory in dev_set_alias(),
> >> which was caused by calling strlcpy() (which in turn called strlen())
> >> on the user-supplied non-terminated string.
> >>
> >> Signed-off-by: Alexander Potapenko <glider@google.com>
> >> ---
> >> v3: removed the multi-line comment
> >> v2: fixed an off-by-one error spotted by Dmitry Vyukov
> >
> > [...]
> >
> >> ---
> >> net/core/dev.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index fca407b4a6ea..3e3b29133cc9 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
> >> return -ENOMEM;
> >> dev->ifalias = new_ifalias;
> >>
> >> - strlcpy(dev->ifalias, alias, len+1);
> >> + /* alias comes from the userspace and may not be zero-terminated. */
> >
> > So if the comment is correct, you'd use copy_from_user() instead.
> Well, the contents of |alias| have been previously copied from the
> userspace, but this is a pointer to a kernel buffer, as the function
> prototype tells.
> Do you think a confusion is possible here?
Yes, I think so... If pointer comes from userspace, it normally points to
userspace data. If you have the data already copied, just say:
+ /* alias may not be zero-terminated. */
Or say nothing, because at the first glance, using the strlcpy()
instead of simple memcpy() looks weird in this case - you know the
length of the string from the beginning, and should not recalculate it
again.
Yury
> >> + memcpy(dev->ifalias, alias, len);
> >> + dev->ifalias[len] = 0;
> >> return len;
> >> }
> >>
> >> --
> >> 2.13.0.219.gdb65acc882-goog
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 14:06 UTC (permalink / raw)
To: Yury Norov
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <20170601140453.fp7iusjomq65tu6i@yury-thinkpad>
On Thu, Jun 1, 2017 at 4:04 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote:
>> On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
>> > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
>> >> KMSAN reported a use of uninitialized memory in dev_set_alias(),
>> >> which was caused by calling strlcpy() (which in turn called strlen())
>> >> on the user-supplied non-terminated string.
>> >>
>> >> Signed-off-by: Alexander Potapenko <glider@google.com>
>> >> ---
>> >> v3: removed the multi-line comment
>> >> v2: fixed an off-by-one error spotted by Dmitry Vyukov
>> >
>> > [...]
>> >
>> >> ---
>> >> net/core/dev.c | 4 +++-
>> >> 1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/core/dev.c b/net/core/dev.c
>> >> index fca407b4a6ea..3e3b29133cc9 100644
>> >> --- a/net/core/dev.c
>> >> +++ b/net/core/dev.c
>> >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
>> >> return -ENOMEM;
>> >> dev->ifalias = new_ifalias;
>> >>
>> >> - strlcpy(dev->ifalias, alias, len+1);
>> >> + /* alias comes from the userspace and may not be zero-terminated. */
>> >
>> > So if the comment is correct, you'd use copy_from_user() instead.
>> Well, the contents of |alias| have been previously copied from the
>> userspace, but this is a pointer to a kernel buffer, as the function
>> prototype tells.
>> Do you think a confusion is possible here?
>
> Yes, I think so... If pointer comes from userspace, it normally points to
> userspace data. If you have the data already copied, just say:
> + /* alias may not be zero-terminated. */
>
> Or say nothing, because at the first glance, using the strlcpy()
> instead of simple memcpy() looks weird in this case - you know the
> length of the string from the beginning, and should not recalculate it
> again.
You're right, I'll just drop this line.
> Yury
>
>> >> + memcpy(dev->ifalias, alias, len);
>> >> + dev->ifalias[len] = 0;
>> >> return len;
>> >> }
>> >>
>> >> --
>> >> 2.13.0.219.gdb65acc882-goog
>>
>>
>>
>> --
>> Alexander Potapenko
>> Software Engineer
>>
>> Google Germany GmbH
>> Erika-Mann-Straße, 33
>> 80636 München
>>
>> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
>> Registergericht und -nummer: Hamburg, HRB 86891
>> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Soheil Hassas Yeganeh @ 2017-06-01 14:10 UTC (permalink / raw)
To: Cyril Hrubis
Cc: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, WANG Cong, Yaogong Wang,
Steffen Klassert, Al Viro, netdev, linux-kernel
In-Reply-To: <20170601140048.GA24401@rei.lan>
On Thu, Jun 1, 2017 at 10:00 AM, Cyril Hrubis <chrubis@suse.cz> wrote:
> I've bisected the problem to this commit:
>
> commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
> Author: Soheil Hassas Yeganeh <soheil@google.com>
> Date: Thu Nov 3 18:24:27 2016 -0400
>
> sock: do not set sk_err in sock_dequeue_err_skb
Hi Cyril,
I'm sorry for the problem, and thank you for the report.
Two questions:
1. Could you double check whether you have the following commit in your tree?
commit 83a1a1a70e87f676fbb6086b26b6ac7f7fdd107d
Author: Soheil Hassas Yeganeh <soheil@google.com>
Date: Wed Nov 30 14:01:08 2016 -0500
sock: reset sk_err for ICMP packets read from error queue
2. I've also have sent a fix to iputils on
https://github.com/iputils/iputils/pull/75. Would you be kind to try
that pull request as well?
Thanks,
Soheil
^ permalink raw reply
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Nikolay Aleksandrov @ 2017-06-01 14:16 UTC (permalink / raw)
To: Sebastian Ott, Xin Long
Cc: David S. Miller, Haidong Li, Ivan Vecera, Stephen Hemminger,
network dev, LKML, Heiko Carstens, Martin Schwidefsky
In-Reply-To: <a6495939-350d-31a4-88db-852344a19a02@cumulusnetworks.com>
On 01/06/17 17:00, Nikolay Aleksandrov wrote:
> On 01/06/17 15:34, Sebastian Ott wrote:
>> On Thu, 1 Jun 2017, Xin Long wrote:
>>> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
>>> <sebott@linux.vnet.ibm.com> wrote:
>>>> [...]
>>> I couldn't see any bridge-related thing here, and it couldn't be reproduced
>>> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
>>> is something else in you machine.
>>>
>>> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
>>> machine normally, then:
>>> # brctl addbr br0
>>> # ip link set br0 up
>>> # brctl stp br0 on
>>>
>>> to check if it will still hang.
>>
>> Nope. That doesn't hang.
>>
>>
>>> If it can't be reproduced in this way, pls add this on your kernel:
>>>
>>> --- a/net/bridge/br_stp_if.c
>>> +++ b/net/bridge/br_stp_if.c
>>> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
>>> br->stp_enabled = BR_KERNEL_STP;
>>> br_debug(br, "using kernel STP\n");
>>>
>>> + WARN_ON(1);
>>> /* To start timers on any ports left in blocking */
>>> mod_timer(&br->hello_timer, jiffies + br->hello_time);
>>> br_port_state_selection(br);
>>> + pr_warn("hello timer start done\n");
>>> }
>>>
>>> spin_unlock_bh(&br->lock);
>>> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
>>> index 60b6fe2..c98b3e5 100644
>>> --- a/net/bridge/br_stp_timer.c
>>> +++ b/net/bridge/br_stp_timer.c
>>> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>>> if (br->dev->flags & IFF_UP) {
>>> br_config_bpdu_generation(br);
>>>
>>> - if (br->stp_enabled == BR_KERNEL_STP)
>>> + if (br->stp_enabled != BR_USER_STP)
>>> mod_timer(&br->hello_timer,
>>> round_jiffies(jiffies + br->hello_time));
>>>
>>>
>>> let's see if it hangs when starting the timer. Thanks.
>>
>> No hang either:
>>
> [snip]
> Could you please try the patch below ?
>
> ---
>
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 4efd5d54498a..89110319ef0f 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -173,7 +173,8 @@ static void br_stp_start(struct net_bridge *br)
> br_debug(br, "using kernel STP\n");
>
> /* To start timers on any ports left in blocking */
> - mod_timer(&br->hello_timer, jiffies + br->hello_time);
> + if (br->dev->flags & IFF_UP)
> + mod_timer(&br->hello_timer, jiffies + br->hello_time);
> br_port_state_selection(br);
> }
>
>
Ah nevermind, this patch reverts it back to the previous state.
^ permalink raw reply
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Cyril Hrubis @ 2017-06-01 14:31 UTC (permalink / raw)
To: Soheil Hassas Yeganeh
Cc: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, WANG Cong, Yaogong Wang,
Steffen Klassert, Al Viro, netdev, linux-kernel
In-Reply-To: <CACSApvZiQTYSVkS62YdadjabUTqwO9O2=Sq4ELuO3eoJ_LM_ZQ@mail.gmail.com>
Hi!
> > I've bisected the problem to this commit:
> >
> > commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
> > Author: Soheil Hassas Yeganeh <soheil@google.com>
> > Date: Thu Nov 3 18:24:27 2016 -0400
> >
> > sock: do not set sk_err in sock_dequeue_err_skb
>
> Hi Cyril,
>
> I'm sorry for the problem, and thank you for the report.
>
> Two questions:
> 1. Could you double check whether you have the following commit in your tree?
>
> commit 83a1a1a70e87f676fbb6086b26b6ac7f7fdd107d
> Author: Soheil Hassas Yeganeh <soheil@google.com>
> Date: Wed Nov 30 14:01:08 2016 -0500
> sock: reset sk_err for ICMP packets read from error queue
I've started bisecting on v4.11 and see the problem on v4.10 on another
machine, the patch should be there in both cases and the bug is easily
reproducible.
> 2. I've also have sent a fix to iputils on
> https://github.com/iputils/iputils/pull/75. Would you be kind to try
> that pull request as well?
That fixed the problem, you can add:
Tested-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ 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