* Re: KASAN: use-after-free Read in rds_cong_queue_updates (2)
From: Santosh Shilimkar @ 2018-07-23 17:38 UTC (permalink / raw)
To: syzbot, davem, linux-kernel, linux-rdma, netdev, rds-devel,
syzkaller-bugs
In-Reply-To: <000000000000cdb5450571adfe40@google.com>
On 7/23/2018 10:30 AM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: fcf4793e278e tls: check RCV_SHUTDOWN in tls_wait_data
> git tree: net
> console output: https://syzkaller.appspot.com/x/log.txt?x=1738cb2c400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=c0bdc4175608181c
> dashboard link:
> https://syzkaller.appspot.com/bug?extid=470ae97a39f16146af45
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+470ae97a39f16146af45@syzkaller.appspotmail.com
>
dup: syzbot+4c20b3866171ce8441d2@syzkaller.appspotmail.com
^ permalink raw reply
* Re: [PATCH] MIPS: Change definition of cpu_relax() for Loongson-3
From: Paul Burton @ 2018-07-23 17:37 UTC (permalink / raw)
To: 陈华才
Cc: Ralf Baechle, James Hogan, linux-mips, Fuxin Zhang, wuzhangjin,
stable, Alan Stern, Andrea Parri, Will Deacon, Peter Zijlstra,
Boqun Feng, Nicholas Piggin, David Howells, Jade Alglave,
Luc Maranget, Paul E. McKenney, Akira Yokosawa, LKML
In-Reply-To: <tencent_49EB501232FD02AC001F9E93@qq.com>
Hi Huacai,
On Sat, Jul 21, 2018 at 09:35:59AM +0800, 陈华才 wrote:
> SFB can improve the memory bandwidth as much as 30%, and we are
> planning to enable SFB by default. So, we want to control cpu_relax()
> under CONFIG_CPU_LOONGSON3, not under CONFIG_LOONGSON3_ENHANCEMENT.
OK, applied to mips-next for 4.19 with changes to the commit message &
comment.
Thanks,
Paul
^ permalink raw reply
* Re: rte_mbuf library likely()/unlikely()
From: Stephen Hemminger @ 2018-07-23 17:37 UTC (permalink / raw)
To: Morten Brørup; +Cc: Olivier Matz, dev
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35B421EE@smartserver.smartshare.dk>
On Mon, 23 Jul 2018 15:53:42 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> Hi Olivier,
>
>
>
> I noticed that __rte_pktmbuf_read() could do with an unlikely(), so I went through the entire library. Here are my suggested modifications.
>
>
>
>
>
> diff -bu rte_mbuf.c.orig rte_mbuf.c
>
> --- rte_mbuf.c.orig 2018-07-23 15:13:22.000000000 +0200
>
> +++ rte_mbuf.c 2018-07-23 15:32:53.000000000 +0200
>
> @@ -173,19 +173,19 @@
>
> {
>
> unsigned int nb_segs, pkt_len;
>
>
>
> - if (m == NULL)
>
> + if (unlikely(m == NULL))
>
> rte_panic("mbuf is NULL\n");
>
>
Adding is unlikely is not necessary since rte_panic is marked with cold attribute
which has the same effect.
^ permalink raw reply
* [Intel-wired-lan] [PATCH 13/13] ice: Change struct members from bool to u8
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
Recent versions of checkpatch have a new warning based on a documented
preference of Linus to not use bool in structures due to wasted space and
the size of bool is implementation dependent. For more information, see
the email thread at https://lkml.org/lkml/2017/11/21/384.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice.h | 8 ++++----
drivers/net/ethernet/intel/ice/ice_switch.h | 6 +++---
drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
drivers/net/ethernet/intel/ice/ice_type.h | 16 ++++++++--------
4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index ed071ea75f20..868f4a1d0f72 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -196,9 +196,9 @@ struct ice_vsi {
struct list_head tmp_sync_list; /* MAC filters to be synced */
struct list_head tmp_unsync_list; /* MAC filters to be unsynced */
- bool irqs_ready;
- bool current_isup; /* Sync 'link up' logging */
- bool stat_offsets_loaded;
+ u8 irqs_ready;
+ u8 current_isup; /* Sync 'link up' logging */
+ u8 stat_offsets_loaded;
/* queue information */
u8 tx_mapping_mode; /* ICE_MAP_MODE_[CONTIG|SCATTER] */
@@ -269,7 +269,7 @@ struct ice_pf {
struct ice_hw_port_stats stats;
struct ice_hw_port_stats stats_prev;
struct ice_hw hw;
- bool stat_prev_loaded; /* has previous stats been loaded */
+ u8 stat_prev_loaded; /* has previous stats been loaded */
char int_name[ICE_INT_NAME_STR_LEN];
};
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
index 6f4a0d159dbf..9b8ec128ee31 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.h
+++ b/drivers/net/ethernet/intel/ice/ice_switch.h
@@ -17,7 +17,7 @@ struct ice_vsi_ctx {
u16 vsis_unallocated;
u16 flags;
struct ice_aqc_vsi_props info;
- bool alloc_from_pool;
+ u8 alloc_from_pool;
};
enum ice_sw_fwd_act_type {
@@ -94,8 +94,8 @@ struct ice_fltr_info {
u8 qgrp_size;
/* Rule creations populate these indicators basing on the switch type */
- bool lb_en; /* Indicate if packet can be looped back */
- bool lan_en; /* Indicate if packet can be forwarded to the uplink */
+ u8 lb_en; /* Indicate if packet can be looped back */
+ u8 lan_en; /* Indicate if packet can be forwarded to the uplink */
};
/* Bookkeeping structure to hold bitmap of VSIs corresponding to VSI list id */
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index 567067b650c4..31bc998fe200 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -143,7 +143,7 @@ struct ice_ring {
u16 next_to_use;
u16 next_to_clean;
- bool ring_active; /* is ring online or not */
+ u8 ring_active; /* is ring online or not */
/* stats structs */
struct ice_q_stats stats;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 99c8a9a71b5e..97c366e0ca59 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -83,7 +83,7 @@ struct ice_link_status {
u64 phy_type_low;
u16 max_frame_size;
u16 link_speed;
- bool lse_ena; /* Link Status Event notification */
+ u8 lse_ena; /* Link Status Event notification */
u8 link_info;
u8 an_info;
u8 ext_info;
@@ -101,7 +101,7 @@ struct ice_phy_info {
struct ice_link_status link_info_old;
u64 phy_type_low;
enum ice_media_type media_type;
- bool get_link_info;
+ u8 get_link_info;
};
/* Common HW capabilities for SW use */
@@ -167,7 +167,7 @@ struct ice_nvm_info {
u32 oem_ver; /* OEM version info */
u16 sr_words; /* Shadow RAM size in words */
u16 ver; /* NVM package version */
- bool blank_nvm_mode; /* is NVM empty (no FW present) */
+ u8 blank_nvm_mode; /* is NVM empty (no FW present) */
};
/* Max number of port to queue branches w.r.t topology */
@@ -181,7 +181,7 @@ struct ice_sched_node {
struct ice_aqc_txsched_elem_data info;
u32 agg_id; /* aggregator group id */
u16 vsi_id;
- bool in_use; /* suspended or in use */
+ u8 in_use; /* suspended or in use */
u8 tx_sched_layer; /* Logical Layer (1-9) */
u8 num_children;
u8 tc_num;
@@ -218,7 +218,7 @@ struct ice_sched_vsi_info {
struct ice_sched_tx_policy {
u16 max_num_vsis;
u8 max_num_lan_qs_per_tc[ICE_MAX_TRAFFIC_CLASS];
- bool rdma_ena;
+ u8 rdma_ena;
};
struct ice_port_info {
@@ -243,7 +243,7 @@ struct ice_port_info {
struct list_head agg_list; /* lists all aggregator */
u8 lport;
#define ICE_LPORT_MASK 0xff
- bool is_vf;
+ u8 is_vf;
};
struct ice_switch_info {
@@ -287,7 +287,7 @@ struct ice_hw {
u8 max_cgds;
u8 sw_entry_point_layer;
- bool evb_veb; /* true for VEB, false for VEPA */
+ u8 evb_veb; /* true for VEB, false for VEPA */
struct ice_bus_info bus;
struct ice_nvm_info nvm;
struct ice_hw_dev_caps dev_caps; /* device capabilities */
@@ -318,7 +318,7 @@ struct ice_hw {
u8 itr_gran_100;
u8 itr_gran_50;
u8 itr_gran_25;
- bool ucast_shared; /* true if VSIs can share unicast addr */
+ u8 ucast_shared; /* true if VSIs can share unicast addr */
};
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 12/13] ice: Fix potential return of uninitialized value
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
In ice_vsi_setup_[tx|rx]_rings, err is uninitialized which can result in
a garbage value return to the caller. Fix that.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index e5b8eb0c4581..3782569b450a 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4887,7 +4887,7 @@ int ice_down(struct ice_vsi *vsi)
*/
static int ice_vsi_setup_tx_rings(struct ice_vsi *vsi)
{
- int i, err;
+ int i, err = 0;
if (!vsi->num_txq) {
dev_err(&vsi->back->pdev->dev, "VSI %d has 0 Tx queues\n",
@@ -4912,7 +4912,7 @@ static int ice_vsi_setup_tx_rings(struct ice_vsi *vsi)
*/
static int ice_vsi_setup_rx_rings(struct ice_vsi *vsi)
{
- int i, err;
+ int i, err = 0;
if (!vsi->num_rxq) {
dev_err(&vsi->back->pdev->dev, "VSI %d has 0 Rx queues\n",
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 11/13] ice: Fix couple of null pointer dereference issues
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
When ice_ena_msix_range() fails to reserve vectors, a devm_kfree() warning
was seen in the error flow path. So check pf->irq_tracker before use in
ice_clear_interrupt_scheme().
In ice_vsi_cfg(), check vsi->netdev before use.
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index bcd39654ab4b..e5b8eb0c4581 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3259,8 +3259,10 @@ static void ice_clear_interrupt_scheme(struct ice_pf *pf)
if (test_bit(ICE_FLAG_MSIX_ENA, pf->flags))
ice_dis_msix(pf);
- devm_kfree(&pf->pdev->dev, pf->irq_tracker);
- pf->irq_tracker = NULL;
+ if (pf->irq_tracker) {
+ devm_kfree(&pf->pdev->dev, pf->irq_tracker);
+ pf->irq_tracker = NULL;
+ }
}
/**
@@ -4114,11 +4116,12 @@ static int ice_vsi_cfg(struct ice_vsi *vsi)
{
int err;
- ice_set_rx_mode(vsi->netdev);
-
- err = ice_restore_vlan(vsi);
- if (err)
- return err;
+ if (vsi->netdev) {
+ ice_set_rx_mode(vsi->netdev);
+ err = ice_restore_vlan(vsi);
+ if (err)
+ return err;
+ }
err = ice_vsi_cfg_txqs(vsi);
if (!err)
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 10/13] ice: Update to interrupts enabled in OICR
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
Remove the following interrupt causes that are not applicable or not
handled:
- PFINT_OICR_HLP_RDY_M
- PFINT_OICR_CPM_RDY_M
- PFINT_OICR_GPIO_M
- PFINT_OICR_STORM_DETECT_M
Add the following interrupt cause that's actually handled in ice_misc_intr:
- PFINT_OICR_PE_CRITERR_M
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> Removed defines from ice_hw_autogen.h]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_hw_autogen.h | 8 --------
drivers/net/ethernet/intel/ice/ice_main.c | 9 +++------
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
index 499904874b3f..6076fc87df9d 100644
--- a/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
+++ b/drivers/net/ethernet/intel/ice/ice_hw_autogen.h
@@ -121,10 +121,6 @@
#define PFINT_FW_CTL_CAUSE_ENA_S 30
#define PFINT_FW_CTL_CAUSE_ENA_M BIT(PFINT_FW_CTL_CAUSE_ENA_S)
#define PFINT_OICR 0x0016CA00
-#define PFINT_OICR_HLP_RDY_S 14
-#define PFINT_OICR_HLP_RDY_M BIT(PFINT_OICR_HLP_RDY_S)
-#define PFINT_OICR_CPM_RDY_S 15
-#define PFINT_OICR_CPM_RDY_M BIT(PFINT_OICR_CPM_RDY_S)
#define PFINT_OICR_ECC_ERR_S 16
#define PFINT_OICR_ECC_ERR_M BIT(PFINT_OICR_ECC_ERR_S)
#define PFINT_OICR_MAL_DETECT_S 19
@@ -133,10 +129,6 @@
#define PFINT_OICR_GRST_M BIT(PFINT_OICR_GRST_S)
#define PFINT_OICR_PCI_EXCEPTION_S 21
#define PFINT_OICR_PCI_EXCEPTION_M BIT(PFINT_OICR_PCI_EXCEPTION_S)
-#define PFINT_OICR_GPIO_S 22
-#define PFINT_OICR_GPIO_M BIT(PFINT_OICR_GPIO_S)
-#define PFINT_OICR_STORM_DETECT_S 24
-#define PFINT_OICR_STORM_DETECT_M BIT(PFINT_OICR_STORM_DETECT_S)
#define PFINT_OICR_HMC_ERR_S 26
#define PFINT_OICR_HMC_ERR_M BIT(PFINT_OICR_HMC_ERR_S)
#define PFINT_OICR_PE_CRITERR_S 28
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 71cc5b6e6237..bcd39654ab4b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1702,15 +1702,12 @@ static void ice_ena_misc_vector(struct ice_pf *pf)
wr32(hw, PFINT_OICR_ENA, 0); /* disable all */
rd32(hw, PFINT_OICR); /* read to clear */
- val = (PFINT_OICR_HLP_RDY_M |
- PFINT_OICR_CPM_RDY_M |
- PFINT_OICR_ECC_ERR_M |
+ val = (PFINT_OICR_ECC_ERR_M |
PFINT_OICR_MAL_DETECT_M |
PFINT_OICR_GRST_M |
PFINT_OICR_PCI_EXCEPTION_M |
- PFINT_OICR_GPIO_M |
- PFINT_OICR_STORM_DETECT_M |
- PFINT_OICR_HMC_ERR_M);
+ PFINT_OICR_HMC_ERR_M |
+ PFINT_OICR_PE_CRITERR_M);
wr32(hw, PFINT_OICR_ENA, val);
--
2.14.3
^ permalink raw reply related
* Re: [net-next v5 3/3] net/tls: Remove redundant array allocation.
From: Dave Watson @ 2018-07-23 16:35 UTC (permalink / raw)
To: David Miller; +Cc: vakul.garg, netdev, borisp, aviadye, Doron Roberts-Kedes
In-Reply-To: <20180721.192532.520509909556885779.davem@davemloft.net>
On 07/21/18 07:25 PM, David Miller wrote:
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Thu, 19 Jul 2018 21:56:13 +0530
>
> > In function decrypt_skb(), array allocation in case when sgout is NULL
> > is unnecessary. Instead, local variable sgin_arr[] can be used.
> >
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
>
> Hmmm...
>
> Dave, can you take a look at this? Do you think there might have
> been a reason you felt that you needed to dynamically allocate
> the scatterlists when you COW and skb and do in-place decryption?
>
> I guess this change is ok, nsg can only get smaller when the SKB
> is COW'd.
>
> > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> > if (!sgout) {
> > nsg = skb_cow_data(skb, 0, &unused) + 1;
> > - sgin = kmalloc_array(nsg, sizeof(*sgin), sk->sk_allocation);
> > sgout = sgin;
> > }
I don't think this patch is safe as-is. sgin_arr is a stack array of
size MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that
it walks the whole chain of skbs from skb->next, and can return any
number of segments. Therefore we need to heap allocate. I think I
copied the IPSEC code here.
For perf though, we could use the stack array if skb_cow_data returns
<= MAX_SKB_FRAGS.
This code is slightly confusing though, since we don't heap allocate
in the zerocopy case - what happens is that skb_to_sgvec returns
-EMSGSIZE, and we fall back to the non-zerocopy case, and return again
to this function, where we then hit the skb_cow_data path and heap
allocate.
^ permalink raw reply
* [Intel-wired-lan] [PATCH 09/13] ice: Set VLAN flags correctly
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Brett Creeley <brett.creeley@intel.com>
Set the ICE_AQ_VSI_PVLAN_MODE_ALL bit to allow the driver to add a VLAN
tag to all packets it sends.
Signed-off-by: Brett Creeley <brett.creeley@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 586c6e615a98..71cc5b6e6237 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3779,6 +3779,9 @@ static int ice_vsi_manage_vlan_stripping(struct ice_vsi *vsi, bool ena)
ctxt.info.port_vlan_flags = ICE_AQ_VSI_PVLAN_EMOD_NOTHING;
}
+ /* Allow all packets untagged/tagged */
+ ctxt.info.port_vlan_flags |= ICE_AQ_VSI_PVLAN_MODE_ALL;
+
ctxt.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
ctxt.vsi_num = vsi->vsi_num;
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 08/13] ice: Don't explicitly set port_vlan_bits to 0
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Brett Creeley <brett.creeley@intel.com>
This patch fixes the following smatch warning originally reported by
Dan Carpenter:
ice_set_dflt_vsi_ctx() warn: odd binop '0x0 & 0x18'
In ice_set_dflt_vsi_ctx() we are currently doing logic that is intended
to explicitly clear bits 3 and 4 for port_vlan_flags. Clearing these
bits results in legacy behavior (showing VLAN, DEI, and UP) in the
descriptors. The issue was this was reporting the Smatch error shown
below. To fix this remove this logic because the port_vlan_flags field
is set to 0 initially by memset so by default we have the correct legacy
behavior for bits 3 and 4. A comment was added above where we set
port_vlan_flags in ice_set_dflt_vsi_ctx() to note that this is the
desired behavior.
Signed-off-by: Brett Creeley <brett.creeley@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index feeca75912ec..586c6e615a98 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1367,14 +1367,13 @@ static void ice_set_dflt_vsi_ctx(struct ice_vsi_ctx *ctxt)
ctxt->info.sw_flags = ICE_AQ_VSI_SW_FLAG_SRC_PRUNE;
/* Traffic from VSI can be sent to LAN */
ctxt->info.sw_flags2 = ICE_AQ_VSI_SW_FLAG_LAN_ENA;
- /* Allow all packets untagged/tagged */
+ /* By default bits 3 and 4 in port_vlan_flags are 0's which results in
+ * legacy behavior (show VLAN, DEI, and UP) in descriptor. Also, allow
+ * all packets untagged/tagged.
+ */
ctxt->info.port_vlan_flags = ((ICE_AQ_VSI_PVLAN_MODE_ALL &
ICE_AQ_VSI_PVLAN_MODE_M) >>
ICE_AQ_VSI_PVLAN_MODE_S);
- /* Show VLAN/UP from packets in Rx descriptors */
- ctxt->info.port_vlan_flags |= ((ICE_AQ_VSI_PVLAN_EMOD_STR_BOTH &
- ICE_AQ_VSI_PVLAN_EMOD_M) >>
- ICE_AQ_VSI_PVLAN_EMOD_S);
/* Have 1:1 UP mapping for both ingress/egress tables */
table |= ICE_UP_TABLE_TRANSLATE(0, 0);
table |= ICE_UP_TABLE_TRANSLATE(1, 1);
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 07/13] ice: Use order_base_2 to calculate higher power of 2
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
Currently, we use a combination of ilog2 and is_power_of_2() to
calculate the next power of 2 for the qcount. This appears to be causing
a warning on some combinations of GCC and the Linux kernel:
MODPOST 1 modules
WARNING: "____ilog2_NaN" [ice.ko] undefined!
This appears to because because GCC realizes that qcount could be zero
in some circumstances and thus attempts to link against the
intentionally undefined ___ilog2_NaN function.
The order_base_2 function is intentionally defined to return 0 when
passed 0 as an argument, and thus will be safe to use here.
This not only fixes the warning but makes the resulting code slightly
cleaner, and is really what we should have used originally.
Also update the comment to make it more clear that we are rounding up,
not just incrementing the ilog2 of qcount unconditionally.
Without this patch, we get warnings when building against the upstream
kernel.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> minor cleanup for upstream submission]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 81b04e1d7ea2..feeca75912ec 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1313,11 +1313,8 @@ static void ice_vsi_setup_q_map(struct ice_vsi *vsi, struct ice_vsi_ctx *ctxt)
qcount = numq_tc;
}
- /* find higher power-of-2 of qcount */
- pow = ilog2(qcount);
-
- if (!is_power_of_2(qcount))
- pow++;
+ /* find the (rounded up) power-of-2 of qcount */
+ pow = order_base_2(qcount);
for (i = 0; i < ICE_MAX_TRAFFIC_CLASS; i++) {
if (!(vsi->tc_cfg.ena_tc & BIT(i))) {
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 06/13] ice: Fix bugs in control queue processing
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
This patch is a consolidation of multiple bug fixes for control queue
processing.
1) In ice_clean_adminq_subtask() remove unnecessary reads/writes to
registers. The bits PFINT_FW_CTL, PFINT_MBX_CTL and PFINT_SB_CTL
are not set when an interrupt arrives, which means that clearing them
again can be omitted.
2) Get an accurate value in "pending" by re-reading the control queue
head register from the hardware.
3) Fix a corner case involving lost control queue messages by checking
for new control messages (using ice_ctrlq_pending) before exiting the
cleanup routine.
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_controlq.c | 5 ++++-
drivers/net/ethernet/intel/ice/ice_main.c | 26 ++++++++++++++++++++++----
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index c064416080e7..62be72fdc8f3 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -1065,8 +1065,11 @@ ice_clean_rq_elem(struct ice_hw *hw, struct ice_ctl_q_info *cq,
clean_rq_elem_out:
/* Set pending if needed, unlock and return */
- if (pending)
+ if (pending) {
+ /* re-read HW head to calculate actual pending messages */
+ ntu = (u16)(rd32(hw, cq->rq.head) & cq->rq.head_mask);
*pending = (u16)((ntc > ntu ? cq->rq.count : 0) + (ntu - ntc));
+ }
clean_rq_elem_err:
mutex_unlock(&cq->rq_lock);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 450911345144..81b04e1d7ea2 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -916,6 +916,21 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
return pending && (i == ICE_DFLT_IRQ_WORK);
}
+/**
+ * ice_ctrlq_pending - check if there is a difference between ntc and ntu
+ * @hw: pointer to hardware info
+ * @cq: control queue information
+ *
+ * returns true if there are pending messages in a queue, false if there aren't
+ */
+static bool ice_ctrlq_pending(struct ice_hw *hw, struct ice_ctl_q_info *cq)
+{
+ u16 ntu;
+
+ ntu = (u16)(rd32(hw, cq->rq.head) & cq->rq.head_mask);
+ return cq->rq.next_to_clean != ntu;
+}
+
/**
* ice_clean_adminq_subtask - clean the AdminQ rings
* @pf: board private structure
@@ -923,7 +938,6 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
static void ice_clean_adminq_subtask(struct ice_pf *pf)
{
struct ice_hw *hw = &pf->hw;
- u32 val;
if (!test_bit(__ICE_ADMINQ_EVENT_PENDING, pf->state))
return;
@@ -933,9 +947,13 @@ static void ice_clean_adminq_subtask(struct ice_pf *pf)
clear_bit(__ICE_ADMINQ_EVENT_PENDING, pf->state);
- /* re-enable Admin queue interrupt causes */
- val = rd32(hw, PFINT_FW_CTL);
- wr32(hw, PFINT_FW_CTL, (val | PFINT_FW_CTL_CAUSE_ENA_M));
+ /* There might be a situation where new messages arrive to a control
+ * queue between processing the last message and clearing the
+ * EVENT_PENDING bit. So before exiting, check queue head again (using
+ * ice_ctrlq_pending) and process new messages if any.
+ */
+ if (ice_ctrlq_pending(hw, &hw->adminq))
+ __ice_clean_ctrlq(pf, ICE_CTL_Q_ADMIN);
ice_flush(hw);
}
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 05/13] ice: Clean control queues only when they are initialized
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Preethi Banala <preethi.banala@intel.com>
Clean control queues only when they are initialized. One of the ways to
validate if the basic initialization is done is by checking value of
cq->sq.head and cq->rq.head variables that specify the register address.
This patch adds a check to avoid NULL pointer dereference crash when tried
to shutdown uninitialized control queue.
Signed-off-by: Preethi Banala <preethi.banala@intel.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_controlq.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index 7c511f144ed6..c064416080e7 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -597,10 +597,14 @@ static enum ice_status ice_init_check_adminq(struct ice_hw *hw)
return 0;
init_ctrlq_free_rq:
- ice_shutdown_rq(hw, cq);
- ice_shutdown_sq(hw, cq);
- mutex_destroy(&cq->sq_lock);
- mutex_destroy(&cq->rq_lock);
+ if (cq->rq.head) {
+ ice_shutdown_rq(hw, cq);
+ mutex_destroy(&cq->rq_lock);
+ }
+ if (cq->sq.head) {
+ ice_shutdown_sq(hw, cq);
+ mutex_destroy(&cq->sq_lock);
+ }
return status;
}
@@ -706,10 +710,14 @@ static void ice_shutdown_ctrlq(struct ice_hw *hw, enum ice_ctl_q q_type)
return;
}
- ice_shutdown_sq(hw, cq);
- ice_shutdown_rq(hw, cq);
- mutex_destroy(&cq->sq_lock);
- mutex_destroy(&cq->rq_lock);
+ if (cq->sq.head) {
+ ice_shutdown_sq(hw, cq);
+ mutex_destroy(&cq->sq_lock);
+ }
+ if (cq->rq.head) {
+ ice_shutdown_rq(hw, cq);
+ mutex_destroy(&cq->rq_lock);
+ }
}
/**
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 04/13] ice: Report stats for allocated queues via ethtool stats
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
It is not safe to have the string table for statistics change order or
size over the lifetime of a given netdevice. This is because of the
nature of the 3-step process for obtaining stats. First, userspace
performs a request for the size of the strings table. Second it performs
a separate request for the strings themselves, after allocating space
for the table. Third, it requests the stats themselves, also allocating
space for the table.
If the size decreased, there is potential to see garbage data or stats
values. In the worst case, we could potentially see stats values become
mis-aligned with their strings, so that it looks like a statistic is
being reported differently than it actually is.
Even worse, if the size increased, there is potential that the strings
table or stats table was not allocated large enough and the stats code
could access and write to memory it should not, potentially resulting in
undefined behavior and system crashes.
It isn't even safe if the size always changes under the RTNL lock. This
is because the calls take place over multiple userspace commands, so it
is not possible to hold the RTNL lock for the entire duration of
obtaining strings and stats. Further, not all consumers of the ethtool
API are the userspace ethtool program, and it is possible that one
assumes the strings will not change (valid under the current contract),
and thus only requests the stats values when requesting stats in a loop.
Finally, it's not possible in the general case to detect when the size
changes, because it is quite possible that one value which could impact
the stat size increased, while another decreased. This would result in
the same total number of stats, but reordering them so that stats no
longer line up with the strings they belong to. Since only size changes
aren't enough, we would need some sort of hash or token to determine
when the strings no longer match. This would require extending the
ethtool stats commands, but there is no more space in the relevant
structures.
The real solution to resolve this would be to add a completely new API
for stats, probably over netlink.
In the ice driver, the only thing impacting the stats that is not
constant is the number of queues. Instead of reporting stats for each
used queue, report stats for each allocated queue. We do not change the
number of queues allocated for a given netdevice, as we pass this into
the alloc_etherdev_mq() function to set the num_tx_queues and
num_rx_queues.
This resolves the potential bugs at the slight cost of displaying many
queue statistics which will not be activated.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice.h | 7 ++++
drivers/net/ethernet/intel/ice/ice_ethtool.c | 52 +++++++++++++++++++++-------
2 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index d8b5fff581e7..ed071ea75f20 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -89,6 +89,13 @@ extern const char ice_drv_ver[];
#define ice_for_each_rxq(vsi, i) \
for ((i) = 0; (i) < (vsi)->num_rxq; (i)++)
+/* Macros for each allocated tx/rx ring whether used or not in a VSI */
+#define ice_for_each_alloc_txq(vsi, i) \
+ for ((i) = 0; (i) < (vsi)->alloc_txq; (i)++)
+
+#define ice_for_each_alloc_rxq(vsi, i) \
+ for ((i) = 0; (i) < (vsi)->alloc_rxq; (i)++)
+
struct ice_tc_info {
u16 qoffset;
u16 qcount;
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 1db304c01d10..807b683a5707 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -26,7 +26,7 @@ static int ice_q_stats_len(struct net_device *netdev)
{
struct ice_netdev_priv *np = netdev_priv(netdev);
- return ((np->vsi->num_txq + np->vsi->num_rxq) *
+ return ((np->vsi->alloc_txq + np->vsi->alloc_rxq) *
(sizeof(struct ice_q_stats) / sizeof(u64)));
}
@@ -218,7 +218,7 @@ static void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
p += ETH_GSTRING_LEN;
}
- ice_for_each_txq(vsi, i) {
+ ice_for_each_alloc_txq(vsi, i) {
snprintf(p, ETH_GSTRING_LEN,
"tx-queue-%u.tx_packets", i);
p += ETH_GSTRING_LEN;
@@ -226,7 +226,7 @@ static void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
p += ETH_GSTRING_LEN;
}
- ice_for_each_rxq(vsi, i) {
+ ice_for_each_alloc_rxq(vsi, i) {
snprintf(p, ETH_GSTRING_LEN,
"rx-queue-%u.rx_packets", i);
p += ETH_GSTRING_LEN;
@@ -253,6 +253,24 @@ static int ice_get_sset_count(struct net_device *netdev, int sset)
{
switch (sset) {
case ETH_SS_STATS:
+ /* The number (and order) of strings reported *must* remain
+ * constant for a given netdevice. This function must not
+ * report a different number based on run time parameters
+ * (such as the number of queues in use, or the setting of
+ * a private ethtool flag). This is due to the nature of the
+ * ethtool stats API.
+ *
+ * Userspace programs such as ethtool must make 3 separate
+ * ioctl requests, one for size, one for the strings, and
+ * finally one for the stats. Since these cross into
+ * userspace, changes to the number or size could result in
+ * undefined memory access or incorrect string<->value
+ * correlations for statistics.
+ *
+ * Even if it appears to be safe, changes to the size or
+ * order of strings will suffer from race conditions and are
+ * not safe.
+ */
return ICE_ALL_STATS_LEN(netdev);
default:
return -EOPNOTSUPP;
@@ -280,18 +298,26 @@ ice_get_ethtool_stats(struct net_device *netdev,
/* populate per queue stats */
rcu_read_lock();
- ice_for_each_txq(vsi, j) {
+ ice_for_each_alloc_txq(vsi, j) {
ring = READ_ONCE(vsi->tx_rings[j]);
- if (!ring)
- continue;
- data[i++] = ring->stats.pkts;
- data[i++] = ring->stats.bytes;
+ if (ring) {
+ data[i++] = ring->stats.pkts;
+ data[i++] = ring->stats.bytes;
+ } else {
+ data[i++] = 0;
+ data[i++] = 0;
+ }
}
- ice_for_each_rxq(vsi, j) {
+ ice_for_each_alloc_rxq(vsi, j) {
ring = READ_ONCE(vsi->rx_rings[j]);
- data[i++] = ring->stats.pkts;
- data[i++] = ring->stats.bytes;
+ if (ring) {
+ data[i++] = ring->stats.pkts;
+ data[i++] = ring->stats.bytes;
+ } else {
+ data[i++] = 0;
+ data[i++] = 0;
+ }
}
rcu_read_unlock();
@@ -519,7 +545,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)
goto done;
}
- for (i = 0; i < vsi->num_txq; i++) {
+ for (i = 0; i < vsi->alloc_txq; i++) {
/* clone ring and setup updated count */
tx_rings[i] = *vsi->tx_rings[i];
tx_rings[i].count = new_tx_cnt;
@@ -551,7 +577,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)
goto done;
}
- for (i = 0; i < vsi->num_rxq; i++) {
+ for (i = 0; i < vsi->alloc_rxq; i++) {
/* clone ring and setup updated count */
rx_rings[i] = *vsi->rx_rings[i];
rx_rings[i].count = new_rx_cnt;
--
2.14.3
^ permalink raw reply related
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Skip repeated calls to i915_gem_set_wedged() (rev2)
From: Patchwork @ 2018-07-23 17:37 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
In-Reply-To: <20180723145335.24579-1-chris@chris-wilson.co.uk>
== Series Details ==
Series: drm/i915: Skip repeated calls to i915_gem_set_wedged() (rev2)
URL : https://patchwork.freedesktop.org/series/47067/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
9b348bfd231b drm/i915: Skip repeated calls to i915_gem_set_wedged()
-:58: WARNING:MEMORY_BARRIER: memory barrier without comment
#58: FILE: drivers/gpu/drm/i915/i915_gem.c:3386:
+ smp_mb__before_atomic();
total: 0 errors, 1 warnings, 0 checks, 68 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* [Intel-wired-lan] [PATCH 03/13] ice: Fix missing shift
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
The ITR index of the interrupt cause for OICR and FW interrupts is not
being shifted correctly so when masked with PFINT_*_CTL_ITR_INDX_M it
will always be zero. Luckily, ICE_RX_ITR is zero anyway so the end result
is the same, but the shift should be fixed in case the value of ICE_RX_ITR
ever changes or it is substituted with another value that needs to be
shifted properly.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com> cleaned up commit message]
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 78756853ade1..450911345144 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2058,15 +2058,17 @@ static int ice_req_irq_msix_misc(struct ice_pf *pf)
skip_req_irq:
ice_ena_misc_vector(pf);
- val = (pf->oicr_idx & PFINT_OICR_CTL_MSIX_INDX_M) |
- (ICE_RX_ITR & PFINT_OICR_CTL_ITR_INDX_M) |
- PFINT_OICR_CTL_CAUSE_ENA_M;
+ val = ((pf->oicr_idx & PFINT_OICR_CTL_MSIX_INDX_M) |
+ ((ICE_RX_ITR << PFINT_OICR_CTL_ITR_INDX_S) &
+ PFINT_OICR_CTL_ITR_INDX_M) |
+ PFINT_OICR_CTL_CAUSE_ENA_M);
wr32(hw, PFINT_OICR_CTL, val);
/* This enables Admin queue Interrupt causes */
- val = (pf->oicr_idx & PFINT_FW_CTL_MSIX_INDX_M) |
- (ICE_RX_ITR & PFINT_FW_CTL_ITR_INDX_M) |
- PFINT_FW_CTL_CAUSE_ENA_M;
+ val = ((pf->oicr_idx & PFINT_FW_CTL_MSIX_INDX_M) |
+ ((ICE_RX_ITR << PFINT_FW_CTL_ITR_INDX_S) &
+ PFINT_FW_CTL_ITR_INDX_M) |
+ PFINT_FW_CTL_CAUSE_ENA_M);
wr32(hw, PFINT_FW_CTL, val);
itr_gran = hw->itr_gran_200;
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 02/13] ice: Cleanup magic number
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
Use define for the unit size shift of the Rx LAN context descriptor base
address instead of the magic number 7.
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h | 1 +
drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
index d23a91665b46..068dbc740b76 100644
--- a/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
+++ b/drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h
@@ -265,6 +265,7 @@ enum ice_rx_flex_desc_status_error_0_bits {
struct ice_rlan_ctx {
u16 head;
u16 cpuid; /* bigger than needed, see above for reason */
+#define ICE_RLAN_BASE_S 7
u64 base;
u16 qlen;
#define ICE_RLAN_CTX_DBUF_S 7
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 5299caf55a7f..78756853ade1 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3986,7 +3986,7 @@ static int ice_setup_rx_ctx(struct ice_ring *ring)
/* clear the context structure first */
memset(&rlan_ctx, 0, sizeof(rlan_ctx));
- rlan_ctx.base = ring->dma >> 7;
+ rlan_ctx.base = ring->dma >> ICE_RLAN_BASE_S;
rlan_ctx.qlen = ring->count;
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 01/13] ice: Fix static analyser warning
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <20180723173752.19676-1-anirudh.venkataramanan@intel.com>
This patch fixes one of the smatch (a static analysis tool) warnings
reported by Dan Carpenter. ICE_LG_ACT_GENERIC_OFFSET_M is the right mask
to use but it appears that ICE_LG_ACT_GENERIC_VALUE_M was used instead,
which resulted in the following smatch warning:
ice_add_marker_act() warn: odd binop '0x380000 & 0x7fff8'
Additionally, use a new define ICE_LG_ACT_GENERIC_OFF_RX_DESC_PROF_IDX
instead of magic number '7'.
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
---
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 1 +
drivers/net/ethernet/intel/ice/ice_switch.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 7541ec2270b3..6d3e11659ba5 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -594,6 +594,7 @@ struct ice_sw_rule_lg_act {
#define ICE_LG_ACT_GENERIC_OFFSET_M (0x7 << ICE_LG_ACT_GENERIC_OFFSET_S)
#define ICE_LG_ACT_GENERIC_PRIORITY_S 22
#define ICE_LG_ACT_GENERIC_PRIORITY_M (0x7 << ICE_LG_ACT_GENERIC_PRIORITY_S)
+#define ICE_LG_ACT_GENERIC_OFF_RX_DESC_PROF_IDX 7
/* Action = 7 - Set Stat count */
#define ICE_LG_ACT_STAT_COUNT 0x7
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 723d15f1e90b..750d2051a976 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -645,7 +645,8 @@ ice_add_marker_act(struct ice_hw *hw, struct ice_fltr_mgmt_list_entry *m_ent,
act |= (1 << ICE_LG_ACT_GENERIC_VALUE_S) & ICE_LG_ACT_GENERIC_VALUE_M;
lg_act->pdata.lg_act.act[1] = cpu_to_le32(act);
- act = (7 << ICE_LG_ACT_GENERIC_OFFSET_S) & ICE_LG_ACT_GENERIC_VALUE_M;
+ act = (ICE_LG_ACT_GENERIC_OFF_RX_DESC_PROF_IDX <<
+ ICE_LG_ACT_GENERIC_OFFSET_S) & ICE_LG_ACT_GENERIC_OFFSET_M;
/* Third action Marker value */
act |= ICE_LG_ACT_GENERIC;
--
2.14.3
^ permalink raw reply related
* [Intel-wired-lan] [PATCH 00/13] Bug fixes for ice
From: Anirudh Venkataramanan @ 2018-07-23 17:37 UTC (permalink / raw)
To: intel-wired-lan
This patch set fixes multiple bugs for the ice driver.
Anirudh Venkataramanan (4):
ice: Fix static analyser warning
ice: Cleanup magic number
ice: Fix bugs in control queue processing
ice: Fix couple of null pointer dereference issues
Brett Creeley (2):
ice: Don't explicitly set port_vlan_bits to 0
ice: Set VLAN flags correctly
Bruce Allan (3):
ice: Fix missing shift
ice: Update to interrupts enabled in OICR
ice: Change struct members from bool to u8
Jacob Keller (2):
ice: Report stats for allocated queues via ethtool stats
ice: Use order_base_2 to calculate higher power of 2
Jesse Brandeburg (1):
ice: Fix potential return of uninitialized value
Preethi Banala (1):
ice: Clean control queues only when they are initialized
drivers/net/ethernet/intel/ice/ice.h | 15 ++--
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 1 +
drivers/net/ethernet/intel/ice/ice_controlq.c | 29 +++++---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 52 ++++++++++----
drivers/net/ethernet/intel/ice/ice_hw_autogen.h | 8 ---
drivers/net/ethernet/intel/ice/ice_lan_tx_rx.h | 1 +
drivers/net/ethernet/intel/ice/ice_main.c | 91 +++++++++++++++----------
drivers/net/ethernet/intel/ice/ice_switch.c | 3 +-
drivers/net/ethernet/intel/ice/ice_switch.h | 6 +-
drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
drivers/net/ethernet/intel/ice/ice_type.h | 16 ++---
11 files changed, 141 insertions(+), 83 deletions(-)
--
2.14.3
^ permalink raw reply
* Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality
From: Doug Anderson @ 2018-07-23 17:37 UTC (permalink / raw)
To: Rob Herring
Cc: Kishon Vijay Abraham I, Mark Rutland, devicetree,
linux-kernel@vger.kernel.org, Vivek Gautam, Manu Gautam,
Varadarajan Narayanan
In-Reply-To: <CAL_Jsq+X=RZK-GU+9LqK-0O1A=cYeudhHvvnrXm6xv=j1Ck4_w@mail.gmail.com>
Hi,
On Mon, Jul 23, 2018 at 7:04 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Fri, Jul 20, 2018 at 10:26 AM, Rob Herring <robh@kernel.org> wrote:
>> > On Fri, Jul 20, 2018 at 9:13 AM Doug Anderson <dianders@chromium.org> wrote:
>> >>
>> >> Rob,
>> >>
>> >> On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@kernel.org> wrote:
>> >> > On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
>> >> >> A few patches have landed for the qcom-qmp PHY that affect how you
>> >> >> would write a device tree node. ...yet the bindings weren't updated.
>> >> >> Let's remedy the situation and make the bindings refelect reality.
>> >> >
>> >> > "dt-bindings: phy: ..." for the subject.
>> >>
>> >> Sorry. Every subsystem has different conventions for this so I
>> >> usually just do a "git log" on the file and make my best guess. I'll
>> >> try to remember this for next time though.
>> >
>> > NP. I'd like to add this info to MAINTAINERS or maybe a git commit
>> > hook could figure this out automagically.
>> >
>> >> In this case, though, it looks like this already landed. I see this
>> >> patch in Kishon's next branch.
>> >>
>> >>
>> >> >> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
>> >> >> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
>> >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> >> >> ---
>> >> >>
>> >> >> .../devicetree/bindings/phy/qcom-qmp-phy.txt | 14 ++++++++++++--
>> >> >> 1 file changed, 12 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> index 266a1bb8bb6e..0c7629e88bf3 100644
>> >> >> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> @@ -12,7 +12,14 @@ Required properties:
>> >> >> "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>> >> >> "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
>> >> >>
>> >> >> - - reg: offset and length of register set for PHY's common serdes block.
>> >> >> + - reg:
>> >> >> + - For "qcom,sdm845-qmp-usb3-phy":
>> >> >> + - index 0: address and length of register set for PHY's common serdes
>> >> >> + block.
>> >> >> + - named register "dp_com" (using reg-names): address and length of the
>> >> >> + DP_COM control block.
>> >> >
>> >> > You need to list reg-names and what are the names for the other 2
>> >> > regions?
>> >>
>> >> Here's the code works. You can tell me how you want this expressed in
>> >> the bindings:
>> >>
>> >> * In all cases the driver maps its main memory range (for the common
>> >> serdes block) without specifying any name. This is equivalent to
>> >> asking for index 0.
>> >>
>> >> * For "qcom,sdm845-qmp-usb3-phy" the driver requests a second memory
>> >> range by name using the name "dp_com".
>> >>
>> >> ...basically the driver is inconsistent between using names and
>> >> indices and I was trying to document that fact.
>> >
>> > That's fine as long as the indices are fixed.
>> >
>> >>
>> >> I guess options:
>> >>
>> >> 1. I could reword this so it's clearer (open to suggestions)
>> >>
>> >> 2. I could add something to the bindings saying that the first reg
>> >> name should be "reg-base" or something. Then the question is whether
>> >> we should go to the code and start enforcing that. If we do choose to
>> >> enforce it then it's technically breaking compatibility (though I
>> >> doubt there is any real dts in the wild). If we don't choose to
>> >> enforce it then why did we bother saying what it should be named?
>> >
>> > I think you need to state index 1 is dp_com (and only for
>> > "qcom,sdm845-qmp-usb3-phy") and a name for index 0. 'reg-base' doesn't
>> > seem great, but I don't have another suggestion.
>>
>> ...but why do we bother giving "dp_com" a name if we're saying it has
>> to be index 1? It feels like the author of the driver was trying to
>> transition from specifying to specifying registers by index to
>> specifying them by name, but left the first register specified by
>> index for compatibility (or code simplicity?). It seems like the
>> whole point of referring to things by name is to _not_ force the index
>> number.
>
> No. Specifying the order and indexes is how bindings are done.
> "-names" is extra information, not a license to change the rules.
OK.
Just for context: I'm not trying to be argumentative or anything--I
just seem to be lacking a fundamental understanding of why reg-names
exists and when it should be used. I'm trying to ask questions so I
can have a better intuition here and submit better patches / do better
reviews. I'm sorry if I'm coming off as a jerk. :( I'm not trying
to be.
Do you happen to know if there's anything written up that explains all
the conventions around reg-names and I can just read that?
For context, it seems to me that clocks and registers have different
conventions, but I certainly could be wrong. From what I've seen
clocks are often specified in bindings by name and referred to in code
by name. Conversely reg-names seems to be highly discouraged and
people are told to just refer to them by index. The difference
between the two always seemed strange to me, though I always assumed
it was because it was more common to just have one or two register
ranges but a whole chunk of clocks (and more get added over time as
the driver matures).
>> I'm imagining a future compatible string that adds yet another address
>> range. Presumably we'd refer to this by name too. In that case both
>> of these would be fine:
>>
>> reg = <0xa x>, <0xb x>, <0xc x>;
>> reg-names = "reg-base", "dp_com", "new_range"
>>
>> reg = <0xa x>, <0xc x>, <0xb x>;
>> reg-names = "reg-base", "new_range", "dp_com"
>
> But why. There is absolutely no need to support both of these.
I guess I'm trying to understand the motivation for reg-names. If you
say that something must be index 1 and also have reg-name of "dp_com"
then the reg-name isn't buying us anything and maybe the more proper
fix is to change the driver to work by index and drop the whole name
thing here? Should I do that?
> What you could need to support is this:
>
> reg-names = "reg-base", "new_range";
>
> The order is still fixed, but we have optional entries in the middle.
> And that is the case when using -names is helpful. Otherwise, "-names"
> is just unnecessary bloat to DT.
From what you're saying the _only_ reason you'd ever want to use
reg-names is if there is an optional register range. Is that right?
I think for me intuitively I expect that drivers will change and
expand over time and I'd expect more / different (and maybe optional)
ranges to be added. In general I feel like named registers scales
better with less complexity.
>> > For the driver, it's not the driver's job to enforce any of this.
>>
>> Sure, but anything that the driver doesn't enforce people will get
>> wrong. In other words if we say that the name of the first register
>> ought to be "reg-base" but don't enforce it then someone will later
>> come in and say it's stupid that one of the names uses a dash and the
>> other an underscore. They'll change "reg-base" and it will work.
>> Someone else will decide that "reg-base" is a dumb name and will
>> change it to "serdes". It will still work. Now we have a bindings
>> that claims "reg-base" and lots of different device trees in practice.
>
> You are arguing against making the binding stricter and then worrying
> about the driver getting things wrong. Stricter bindings leave less to
> interpretation by the driver and less chance to get things wrong.
> Unless "anything goes" and then the binding can never be "wrong".
I guess I'm saying that my intuition says that over-specifying things
seems bad to me. The driver will either ask for a register by name or
by index and not both. In my mind either is a fine way to do it so as
long as the driver and the binding is consistent then we have a
functioning system. Specifying that people have to do get both the
index and name right basically gives people twice as many places to do
things wrong.
> Again, it is not the kernel's job to validate bindings even though we
> have little else right now. That will change soon hopefully.
>
>> If the whole "keep things compatible" is important then what matters
>> more is what's happening in practice, not what's should happen in
>> theory.
>>
>> IMO if the driver isn't enforcing that the first field be called
>> "reg-base" then it shouldn't be in the bindings doc.
>>
>>
>> BTW: I use the name "reg-base" in my example because that's what the
>> register was named in downstream device trees that I found. Even if
>> the name isn't terribly appealing, if it's not terribly objectionable
>> I'd rather pick something that's closer to what people used in
>> practice.
>
> I don't really care. People just make shit up anyway.
>
>>
>> ---
>>
>> How about this?
>
> No.
>
>>
>> - reg:
>> - For "qcom,sdm845-qmp-usb3-phy" (names mapped by reg-names):
>> - "reg-base": address and length of register set for PHY's common serdes
>> block. MUST BE THE FIRST RANGE LISTED IN THE LIST (AKA index 0). Note
>> that the name of "reg-base" is suggested but not enforced.
>> - "dp-com": address and length of the DP_COM control block.
>> - For all others (only one range--don't use reg-names)
>> - offset and length of register set for PHY's common serdes block.
OK, I guess I will try again then and you can tell me when I get it
right (unless you tell me I should just change the driver to not use
named registers at all). How about:
- reg:
- For "qcom,sdm845-qmp-usb3-phy":
- index 0: address and length of register set for PHY's common serdes
block.
- index 1: address and length of the DP_COM control block.
- For all others:
- index 0: address and length of register set for PHY's common serdes
block.
- reg-names:
- For "qcom,sdm845-qmp-usb3-phy":
- Should be: "reg-base", "dp_com"
- For all others:
- The reg-names property shouldn't be defined.
-Doug
^ permalink raw reply
* Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality
From: Doug Anderson @ 2018-07-23 17:37 UTC (permalink / raw)
To: Rob Herring
Cc: Kishon Vijay Abraham I, Mark Rutland, devicetree,
linux-kernel@vger.kernel.org, Vivek Gautam, Manu Gautam,
Varadarajan Narayanan
In-Reply-To: <CAL_Jsq+X=RZK-GU+9LqK-0O1A=cYeudhHvvnrXm6xv=j1Ck4_w@mail.gmail.com>
Hi,
On Mon, Jul 23, 2018 at 7:04 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Fri, Jul 20, 2018 at 10:26 AM, Rob Herring <robh@kernel.org> wrote:
>> > On Fri, Jul 20, 2018 at 9:13 AM Doug Anderson <dianders@chromium.org> wrote:
>> >>
>> >> Rob,
>> >>
>> >> On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@kernel.org> wrote:
>> >> > On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
>> >> >> A few patches have landed for the qcom-qmp PHY that affect how you
>> >> >> would write a device tree node. ...yet the bindings weren't updated.
>> >> >> Let's remedy the situation and make the bindings refelect reality.
>> >> >
>> >> > "dt-bindings: phy: ..." for the subject.
>> >>
>> >> Sorry. Every subsystem has different conventions for this so I
>> >> usually just do a "git log" on the file and make my best guess. I'll
>> >> try to remember this for next time though.
>> >
>> > NP. I'd like to add this info to MAINTAINERS or maybe a git commit
>> > hook could figure this out automagically.
>> >
>> >> In this case, though, it looks like this already landed. I see this
>> >> patch in Kishon's next branch.
>> >>
>> >>
>> >> >> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
>> >> >> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
>> >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> >> >> ---
>> >> >>
>> >> >> .../devicetree/bindings/phy/qcom-qmp-phy.txt | 14 ++++++++++++--
>> >> >> 1 file changed, 12 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> index 266a1bb8bb6e..0c7629e88bf3 100644
>> >> >> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> >> @@ -12,7 +12,14 @@ Required properties:
>> >> >> "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>> >> >> "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
>> >> >>
>> >> >> - - reg: offset and length of register set for PHY's common serdes block.
>> >> >> + - reg:
>> >> >> + - For "qcom,sdm845-qmp-usb3-phy":
>> >> >> + - index 0: address and length of register set for PHY's common serdes
>> >> >> + block.
>> >> >> + - named register "dp_com" (using reg-names): address and length of the
>> >> >> + DP_COM control block.
>> >> >
>> >> > You need to list reg-names and what are the names for the other 2
>> >> > regions?
>> >>
>> >> Here's the code works. You can tell me how you want this expressed in
>> >> the bindings:
>> >>
>> >> * In all cases the driver maps its main memory range (for the common
>> >> serdes block) without specifying any name. This is equivalent to
>> >> asking for index 0.
>> >>
>> >> * For "qcom,sdm845-qmp-usb3-phy" the driver requests a second memory
>> >> range by name using the name "dp_com".
>> >>
>> >> ...basically the driver is inconsistent between using names and
>> >> indices and I was trying to document that fact.
>> >
>> > That's fine as long as the indices are fixed.
>> >
>> >>
>> >> I guess options:
>> >>
>> >> 1. I could reword this so it's clearer (open to suggestions)
>> >>
>> >> 2. I could add something to the bindings saying that the first reg
>> >> name should be "reg-base" or something. Then the question is whether
>> >> we should go to the code and start enforcing that. If we do choose to
>> >> enforce it then it's technically breaking compatibility (though I
>> >> doubt there is any real dts in the wild). If we don't choose to
>> >> enforce it then why did we bother saying what it should be named?
>> >
>> > I think you need to state index 1 is dp_com (and only for
>> > "qcom,sdm845-qmp-usb3-phy") and a name for index 0. 'reg-base' doesn't
>> > seem great, but I don't have another suggestion.
>>
>> ...but why do we bother giving "dp_com" a name if we're saying it has
>> to be index 1? It feels like the author of the driver was trying to
>> transition from specifying to specifying registers by index to
>> specifying them by name, but left the first register specified by
>> index for compatibility (or code simplicity?). It seems like the
>> whole point of referring to things by name is to _not_ force the index
>> number.
>
> No. Specifying the order and indexes is how bindings are done.
> "-names" is extra information, not a license to change the rules.
OK.
Just for context: I'm not trying to be argumentative or anything--I
just seem to be lacking a fundamental understanding of why reg-names
exists and when it should be used. I'm trying to ask questions so I
can have a better intuition here and submit better patches / do better
reviews. I'm sorry if I'm coming off as a jerk. :( I'm not trying
to be.
Do you happen to know if there's anything written up that explains all
the conventions around reg-names and I can just read that?
For context, it seems to me that clocks and registers have different
conventions, but I certainly could be wrong. From what I've seen
clocks are often specified in bindings by name and referred to in code
by name. Conversely reg-names seems to be highly discouraged and
people are told to just refer to them by index. The difference
between the two always seemed strange to me, though I always assumed
it was because it was more common to just have one or two register
ranges but a whole chunk of clocks (and more get added over time as
the driver matures).
>> I'm imagining a future compatible string that adds yet another address
>> range. Presumably we'd refer to this by name too. In that case both
>> of these would be fine:
>>
>> reg = <0xa x>, <0xb x>, <0xc x>;
>> reg-names = "reg-base", "dp_com", "new_range"
>>
>> reg = <0xa x>, <0xc x>, <0xb x>;
>> reg-names = "reg-base", "new_range", "dp_com"
>
> But why. There is absolutely no need to support both of these.
I guess I'm trying to understand the motivation for reg-names. If you
say that something must be index 1 and also have reg-name of "dp_com"
then the reg-name isn't buying us anything and maybe the more proper
fix is to change the driver to work by index and drop the whole name
thing here? Should I do that?
> What you could need to support is this:
>
> reg-names = "reg-base", "new_range";
>
> The order is still fixed, but we have optional entries in the middle.
> And that is the case when using -names is helpful. Otherwise, "-names"
> is just unnecessary bloat to DT.
>From what you're saying the _only_ reason you'd ever want to use
reg-names is if there is an optional register range. Is that right?
I think for me intuitively I expect that drivers will change and
expand over time and I'd expect more / different (and maybe optional)
ranges to be added. In general I feel like named registers scales
better with less complexity.
>> > For the driver, it's not the driver's job to enforce any of this.
>>
>> Sure, but anything that the driver doesn't enforce people will get
>> wrong. In other words if we say that the name of the first register
>> ought to be "reg-base" but don't enforce it then someone will later
>> come in and say it's stupid that one of the names uses a dash and the
>> other an underscore. They'll change "reg-base" and it will work.
>> Someone else will decide that "reg-base" is a dumb name and will
>> change it to "serdes". It will still work. Now we have a bindings
>> that claims "reg-base" and lots of different device trees in practice.
>
> You are arguing against making the binding stricter and then worrying
> about the driver getting things wrong. Stricter bindings leave less to
> interpretation by the driver and less chance to get things wrong.
> Unless "anything goes" and then the binding can never be "wrong".
I guess I'm saying that my intuition says that over-specifying things
seems bad to me. The driver will either ask for a register by name or
by index and not both. In my mind either is a fine way to do it so as
long as the driver and the binding is consistent then we have a
functioning system. Specifying that people have to do get both the
index and name right basically gives people twice as many places to do
things wrong.
> Again, it is not the kernel's job to validate bindings even though we
> have little else right now. That will change soon hopefully.
>
>> If the whole "keep things compatible" is important then what matters
>> more is what's happening in practice, not what's should happen in
>> theory.
>>
>> IMO if the driver isn't enforcing that the first field be called
>> "reg-base" then it shouldn't be in the bindings doc.
>>
>>
>> BTW: I use the name "reg-base" in my example because that's what the
>> register was named in downstream device trees that I found. Even if
>> the name isn't terribly appealing, if it's not terribly objectionable
>> I'd rather pick something that's closer to what people used in
>> practice.
>
> I don't really care. People just make shit up anyway.
>
>>
>> ---
>>
>> How about this?
>
> No.
>
>>
>> - reg:
>> - For "qcom,sdm845-qmp-usb3-phy" (names mapped by reg-names):
>> - "reg-base": address and length of register set for PHY's common serdes
>> block. MUST BE THE FIRST RANGE LISTED IN THE LIST (AKA index 0). Note
>> that the name of "reg-base" is suggested but not enforced.
>> - "dp-com": address and length of the DP_COM control block.
>> - For all others (only one range--don't use reg-names)
>> - offset and length of register set for PHY's common serdes block.
OK, I guess I will try again then and you can tell me when I get it
right (unless you tell me I should just change the driver to not use
named registers at all). How about:
- reg:
- For "qcom,sdm845-qmp-usb3-phy":
- index 0: address and length of register set for PHY's common serdes
block.
- index 1: address and length of the DP_COM control block.
- For all others:
- index 0: address and length of register set for PHY's common serdes
block.
- reg-names:
- For "qcom,sdm845-qmp-usb3-phy":
- Should be: "reg-base", "dp_com"
- For all others:
- The reg-names property shouldn't be defined.
-Doug
^ permalink raw reply
* Re: [PATCH v3 4/5] compress/zlib: support burst enqueue/dequeue
From: Stephen Hemminger @ 2018-07-23 17:35 UTC (permalink / raw)
To: Verma, Shally
Cc: pablo.de.lara.guarch@intel.com, dev@dpdk.org,
Athreya, Narayana Prasad, Challa, Mahipal, Gupta, Ashish,
Sahu, Sunila
In-Reply-To: <CY4PR0701MB3634D6C72203CEC71EC6EF4EF0560@CY4PR0701MB3634.namprd07.prod.outlook.com>
On Mon, 23 Jul 2018 17:14:26 +0000
"Verma, Shally" <Shally.Verma@cavium.com> wrote:
> >-----Original Message-----
> >From: Stephen Hemminger <stephen@networkplumber.org>
> >Sent: 23 July 2018 22:24
> >To: Verma, Shally <Shally.Verma@cavium.com>
> >Cc: pablo.de.lara.guarch@intel.com; dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Challa,
> >Mahipal <Mahipal.Challa@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>;
> >Sahu, Sunila <Sunila.Sahu@cavium.com>
> >Subject: Re: [dpdk-dev] [PATCH v3 4/5] compress/zlib: support burst enqueue/dequeue
> >
> >External Email
> >
> >On Sat, 21 Jul 2018 23:47:48 +0530
> >Shally Verma <shally.verma@caviumnetworks.com> wrote:
> >
> >> -/** Parse comp xform and set private xform/stream parameters */
> >> +/** Compute next mbuf in the list, assign data buffer and length,
> >> + * returns 0 if mbuf is NULL
> >> + */
> >> +#define COMPUTE_BUF(mbuf, data, len) \
> >> + ((mbuf = mbuf->next) ? \
> >> + (data = rte_pktmbuf_mtod(mbuf, uint8_t *)), \
> >> + (len = rte_pktmbuf_data_len(mbuf)) : 0)
> >> +
> >
> >Could this be an inline not a macro?
> [Shally] Again what goes in favour of inline here? Just curious to know if DPDK has any preferred guidelines regarding this?
>
> Thanks
> Shally
Macros have no type checking and are harder to debug.
They should only be used when doing generic code (ie template like).
^ permalink raw reply
* data corruption with 'splt' workload to XFS on DM cache with its 3 underlying devices being on same NVMe device
From: Mike Snitzer @ 2018-07-23 16:33 UTC (permalink / raw)
To: linux-nvme, linux-block, dm-devel
Hi,
I've opened the following public BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1607527
Feel free to add comments to that BZ if you have a redhat bugzilla
account.
But otherwise, happy to get as much feedback and discussion going purely
on the relevant lists. I've taken ~1.5 weeks to categorize and isolate
this issue. But I've reached a point where I'm getting diminishing
returns and could _really_ use the collective eyeballs and expertise of
the community. This is by far one of the most nasty cases of corruption
I've seen in a while. Not sure where the ultimate cause of corruption
lies (that the money question) but it _feels_ rooted in NVMe and is
unique to this particular workload I've stumbled onto via customer
escalation and then trying to replicate an rbd device using a more
approachable one (request-based DM multipath in this case).
>From the BZ's comment#0:
The following occurs with latest v4.18-rc3 and v4.18-rc6 and also occurs
with v4.15. When corruption occurs from this test it also destroys the
DOS partition table (created during step 0 below).. yeah, corruption is
_that_ bad. Almost like the corruption is temporal (recently accessed
regions of the NVMe device)?
Anyway: I stumbled onto rampant corruption when using request-based DM
multipath ontop of an NVMe device (not exclusive to a particular drive
either, happens to NVMe devices from multiple vendors). But the
corruption only occurs if the request-based multipath IO is issued to an
NVMe device in parallel to other IO issued to the _same_ underlying NVMe
by the DM cache target. See topology detailed below (at the very end of
this comment).. basically all 3 devices that are used to create a DM
cache device need to be backed by the same NVMe device (via partitions
or linear volumes).
Again, using request-based DM multipath for dm-cache's "slow" device is
_required_ to reproduce. Not 100% clear why really... other than
request-based DM multipath builds large IOs (due to merging).
--- Additional comment from Mike Snitzer on 2018-07-20 10:14:09 EDT ---
To reproduce this issue using device-mapper-test-suite:
0) Partition an NVMe device. First primary partition with at least a
5GB, seconf primary partition with at least 48GB.
NOTE: larger partitions (e.g. 1: 50GB 2: >= 220GB) can be used to
reproduce XFS corruption much quicker.
1) create a request-based multipath device ontop of an NVMe device,
e.g.:
#!/bin/sh
modprobe dm-service-time
DEVICE=/dev/nvme1n1p2
SIZE=`blockdev --getsz $DEVICE`
echo "0 $SIZE multipath 2 queue_mode mq 0 1 1 service-time 0 1 2 $DEVICE
1000 1" | dmsetup create nvme_mpath
# Just a note for how to fail/reinstate path:
# dmsetup message nvme_mpath 0 "fail_path $DEVICE"
# dmsetup message nvme_mpath 0 "reinstate_path $DEVICE"
2) checkout device-mapper-test-suite from my github repo:
git clone git://github.com/snitm/device-mapper-test-suite.git
cd device-mapper-test-suite
git checkout -b devel origin/devel
3) follow device-mapper-test-suite's README.md to get it all setup
4) Configure /root/.dmtest/config with something like:
profile :nvme_shared do
metadata_dev '/dev/nvme1n1p1'
#data_dev '/dev/nvme1n1p2'
data_dev '/dev/mapper/nvme_mpath'
end
default_profile :nvme_shared
------
NOTE: configured 'metadata_dev' gets carved up by
device-mapper-test-suite to provide both the dm-cache's metadata device
and the "fast" data device. The configured 'data_dev' is used for
dm-cache's "slow" data device.
5) run the test:
# tail -f /var/log/messages &
# time dmtest run --suite cache -n /split_large_file/
6) If multipath device failed the lone NVMe path you'll need to
reinstate the path before the next iteration of your test, e.g. (from #1
above):
dmsetup message nvme_mpath 0 "reinstate_path $DEVICE"
--- Additional comment from Mike Snitzer on 2018-07-20 12:02:45 EDT ---
(In reply to Mike Snitzer from comment #6)
> SO seems pretty clear something is still wrong with request-based DM
> multipath ontop of NVMe... sadly we don't have any negative check in
> blk-core, NVMe or elsewhere to offer any clue :(
Building on this comment:
"Anyway, fact that I'm getting this corruption on multiple different
NVMe drives: I am definitely concerned that this BZ is due to a bug
somewhere in NVMe core (or block core code that is specific to NVMe)."
I'm left thinking that request-based DM multipath is somehow causing
NVMe's SG lists or other infrastructure to be "wrong" and it is
resulting in corruption. I get corruption to the dm-cache's metadata
device (which while theoretically unrelated as its a separate device
from the "slow" dm-cache data device) if the dm-cache slow data device
is backed by request-based dm-multipath ontop of NVMe (which is a
partition from the _same_ NVMe device that is used by the dm-cache
metadata device).
Basically I'm back to thinking NVMe is corrupting the data due to the IO
pattern or nature of the cloned requests dm-multipath is issuing. And
it is causing corruption to other NVMe partitions on the same parent
NVMe device. Certainly that is a concerning hypothesis but I'm not
seeing much else that would explain this weird corruption.
If I don't use the same NVMe device (with multiple partitions) for _all_
3 sub-devices that dm-cache needs I don't see the corruption. It is
almost like the mix of IO issued by DM cache's metadata (on nvme1n1p1
using dm-linear) and "fast" device (also on nvme1n1p1 via dm-linear
volume) in conjunction with IO issued by request-based DM multipath to
NVMe for "slow" device (on nvme1n1p2) is triggering NVMe to respond
negatively. But this same observation can be made on completely
different hardware using 2 totally different NVMe devices:
testbed1: Intel Corporation Optane SSD 900P Series (2700)
testbed2: Samsung Electronics Co Ltd NVMe SSD Controller 171X (rev 03)
Which is why it feels like some bug in Linux (be it dm-rq.c, blk-core.c,
blk-merge.c or the common NVMe driver)
topology before starting the device-mapper-test-suite test:
# lsblk /dev/nvme1n1
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
nvme1n1 259:1 0 745.2G 0 disk
├─nvme1n1p2 259:5 0 695.2G 0 part
│ └─nvme_mpath 253:2 0 695.2G 0 dm
└─nvme1n1p1 259:4 0 50G 0 part
topology during the device-mapper-test-suite test:
# lsblk /dev/nvme1n1
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
nvme1n1 259:1 0 745.2G 0 disk
├─nvme1n1p2 259:5 0 695.2G 0 part
│ └─nvme_mpath 253:2 0 695.2G 0 dm
│ └─test-dev-458572 253:5 0 48G 0 dm
│ └─test-dev-613083 253:6 0 48G 0 dm
/root/snitm/git/device-mapper-test-suite/kernel_builds
└─nvme1n1p1 259:4 0 50G 0 part
├─test-dev-126378 253:4 0 4G 0 dm
│ └─test-dev-613083 253:6 0 48G 0 dm
/root/snitm/git/device-mapper-test-suite/kernel_builds
└─test-dev-652491 253:3 0 40M 0 dm
└─test-dev-613083 253:6 0 48G 0 dm
/root/snitm/git/device-mapper-test-suite/kernel_builds
pruning that tree a bit (removing the dm-cache device 253:6) for
clarity:
# lsblk /dev/nvme1n1
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
nvme1n1 259:1 0 745.2G 0 disk
├─nvme1n1p2 259:5 0 695.2G 0 part
│ └─nvme_mpath 253:2 0 695.2G 0 dm
│ └─test-dev-458572 253:5 0 48G 0 dm
└─nvme1n1p1 259:4 0 50G 0 part
├─test-dev-126378 253:4 0 4G 0 dm
└─test-dev-652491 253:3 0 40M 0 dm
40M device is dm-cache "metadata" device
4G device is dm-cache "fast" data device
48G device is dm-cache "slow" data device
^ permalink raw reply
* master - lvconvert: reject conversions of LVs under snapshot
From: Heinz Mauelshagen @ 2018-07-23 17:35 UTC (permalink / raw)
To: lvm-devel
Gitweb: https://sourceware.org/git/?p=lvm2.git;a=commitdiff;h=2214dc12c34890c78b05456f58d0aa5d6dd08f4c
Commit: 2214dc12c34890c78b05456f58d0aa5d6dd08f4c
Parent: 778ce8d80830a60d258b419720be3e45a7ee9f0b
Author: Heinz Mauelshagen <heinzm@redhat.com>
AuthorDate: Mon Jul 23 19:20:18 2018 +0200
Committer: Heinz Mauelshagen <heinzm@redhat.com>
CommitterDate: Mon Jul 23 19:35:34 2018 +0200
lvconvert: reject conversions of LVs under snapshot
Conversions of LVs under snapshot to thinpool or cachepool
correctly fail but leave them inactive and provide cryptic
error messages like 'Internal error: #LVs (10) != #visible
LVs (2) + #snapshots (1) + #internal LVs (5) in VG VG'.
Reject and provide better error message.
Resolves: rhbz1514146
---
tools/lvconvert.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index 0bb1dc9..4871f7f 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -4235,6 +4235,12 @@ static int _lvconvert_to_pool_or_swap_metadata_single(struct cmd_context *cmd,
return 0;
};
+ if (lv_is_origin(lv)) {
+ log_error("Cannot convert logical volume %s under snapshot.",
+ display_lvname(lv));
+ return 0;
+ };
+
if (cmd->position_argc > 1) {
/* First pos arg is required LV, remaining are optional PVs. */
if (!(use_pvh = create_pv_list(cmd->mem, lv->vg, cmd->position_argc - 1, cmd->position_argv + 1, 0)))
^ permalink raw reply related
* [Intel-wired-lan] [RFC PATCH] ixgbe: ixgbe_get_completion_timeout() can be static
From: Jeff Kirsher @ 2018-07-23 17:35 UTC (permalink / raw)
To: intel-wired-lan
In-Reply-To: <CAKgT0UdHYJdWeh5_bhYViOqVYX7-N9MVHrEj=e1MK7RO7TxjJA@mail.gmail.com>
On Mon, 2018-07-23 at 09:24 -0700, Alexander Duyck wrote:
> On Fri, Jul 20, 2018 at 10:53 PM, kbuild test robot
> <fengguang.wu@intel.com> wrote:
> >
> > Fixes: 23f3effc3f69 ("ixgbe: Refactor queue disable logic to take
> > completion time into account")
> > Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
>
> Jeff do you think you could fold this into the current completion
> timeout patch?
Yes, I can do that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180723/b1d30cb7/attachment-0001.asc>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.