* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-18 13:14 UTC (permalink / raw)
To: 'Thomas Gleixner'
Cc: 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Paolo Bonzini', 'Dave Hansen',
'Ashok Raj', 'Peter Zijlstra',
'Ravi V Shankar', 'Xiaoyao Li ',
'Christopherson Sean J', 'Kalle Valo',
'Michael Chan', 'linux-kernel', 'x86',
'kvm@vger.kernel.org', 'netdev@vger.kernel.org',
'linux-wireless@vger.kernel.org'
In-Reply-To: <alpine.DEB.2.21.1904181347210.3174@nanos.tec.linutronix.de>
From: Thomas Gleixner
> Sent: 18 April 2019 12:49
> On Thu, 18 Apr 2019, David Laight wrote:
> > From: David Laight
> > > Sent: 18 April 2019 10:21
> > > From: Fenghua Yu
> > > > Sent: 17 April 2019 22:34
> > > >
> > > > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > > > operate on bitmap defined in x86_capability.
> > > >
> > > > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > > > the location is at:
> > > > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> > > >
> > > > Since base address of x86_capability may not be aligned to unsigned long,
> > > > the single unsigned long location may cross two cache lines and
> > > > accessing the location by locked BTS/BTR introductions will cause
> > > > split lock.
> > >
> > > Isn't the problem that the type (and definition) of x86_capability[] are wrong.
> > > If the 'bitmap' functions are used for it, it should be defined as a bitmap.
> > > This would make it 'unsigned long' not __u32.
> > >
> > > This type munging of bitmaps only works on LE systems.
> > >
> > > OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
> > > ISTR some of the associated functions use byte accesses.
> > >
> > > Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
> > > 32bit accesses.
> >
> > A quick look shows that this isn't the only __32[] that is being
> > cast to (unsigned long) and then to set/test/clear_bit() in those
> > files.
> >
> > I wonder how much other code is applying such casts?
>
> The reason for the cpuid stuff using u32 is that this is actually the width
> of the information retrieved from CPUID.
Right, but you shouldn't (as has been found out) cast pointers
to integer types.
Running
grep -r --include '*.[ch]' '_bit([^(]*, *([^)]* ' .
over the entire kernel source tree shows quite a few 'dubious' casts.
They'll be doubly dubious on BE systems.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8188ee: Remove extraneous file
From: Kalle Valo @ 2019-04-18 13:08 UTC (permalink / raw)
To: Larry Finger; +Cc: linux-wireless, pkshih, Larry Finger, Andrew Morton
In-Reply-To: <20190417003547.26271-1-Larry.Finger@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Somehow file drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej was
> incorporated into the sources. Obviously, it can be removed.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
Patch applied to wireless-drivers-next.git, thanks.
b5250c9c14c1 rtlwifi: rtl8188ee: Remove extraneous file
--
https://patchwork.kernel.org/patch/10904279/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* pull-request: iwlwifi-next 2019-04-18
From: Luca Coelho @ 2019-04-18 12:58 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, linuxwifi, david.e.box, joe.konno
[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]
Hi Kalle,
This is the third batch of patches intended for v5.2. This includes
the last patchset I sent. Usual development work. More details about
the contents in the tag description.
I have sent this out before and kbuildbot reported success.
Please let me know if there are any issues.
Cheers,
Luca.
The following changes since commit bdfc4027de15ef0a396b7fc49a9f9c094378e9f4:
rtlwifi: rtl8723ae: Make rtl8723e_dm_refresh_rate_adaptive_mask static (2019-04-13 14:17:52 +0300)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git iwlwifi-next-for-kalle-2019-04-18
for you to fetch changes up to 61404c6e6a9fd2f02e18b2ff9defe1b3b98e25a3:
iwlwifi: dbg_ini: set dump bit only when trigger collection is certain (2019-04-18 15:25:20 +0300)
----------------------------------------------------------------
Third batch of patches intended for v5.2
* Bump the 20000-series FW API version supported;
* Work on the new debugging infra continues;
* One clean-up to prevent a bogus warning with clang;
* A small cleanup in the PCI ID list;
* Work on new hardware continues;
* RTT confidence indication support for FTM;
* An improvement in HE rate-scaling;
----------------------------------------------------------------
Avraham Stern (1):
iwlwifi: mvm: support rtt confidence indication
Gregory Greenman (1):
iwlwifi: rs: consider LDPC capability in case of HE
Luca Coelho (2):
iwlwifi: bump FW API to 47 for 22000 series
iwlwifi: remove unused 0x40C0 PCI device IDs
Nathan Chancellor (1):
iwlwifi: mvm: Change an 'else if' into an 'else' in iwl_mvm_send_add_bcast_sta
Shahar S Matityahu (6):
iwlwifi: dbg_ini: support notification and dhc regions type parsing
iwlwifi: add FW_INFO debug level
iwlwifi: dbg_ini: add debug prints to the ini flows
iwlwifi: dbg: add periphery memory dumping support to ax210 device family
iwlwifi: dbg: add lmac and umac PC registers to periphery dump
iwlwifi: dbg_ini: set dump bit only when trigger collection is certain
Shaul Triebitz (2):
iwlwifi: mvm: set 512 TX queue slots for AX210 devices
iwlwifi: unite macros with same meaning
drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 19 +--------------
drivers/net/wireless/intel/iwlwifi/fw/api/location.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 3 +++
drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
drivers/net/wireless/intel/iwlwifi/fw/file.h | 1 +
drivers/net/wireless/intel/iwlwifi/iwl-config.h | 1 -
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +-
drivers/net/wireless/intel/iwlwifi/iwl-debug.h | 2 ++
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 8 +++---
drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 13 +++++++++-
drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c | 16 ++++++++----
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 11 ++++++---
drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info-gen3.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info.c | 6 ++---
drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 5 ----
drivers/net/wireless/intel/iwlwifi/pcie/internal.h | 4 ---
drivers/net/wireless/intel/iwlwifi/pcie/trans-gen2.c | 7 +++---
drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 2 ++
drivers/net/wireless/intel/iwlwifi/pcie/tx.c | 8 +++---
19 files changed, 230 insertions(+), 81 deletions(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 3/4] iwlwifi: mvm: properly check debugfs dentry before using it
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo
Cc: linux-wireless, Greg Kroah-Hartman, Laura Abbott, Gabriel Ramirez,
Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, stable
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
debugfs can now report an error code if something went wrong instead of
just NULL. So if the return value is to be used as a "real" dentry, it
needs to be checked if it is an error before dereferencing it.
This is now happening because of ff9fb72bc077 ("debugfs: return error
values, not NULL"). If multiple iwlwifi devices are in the system, this
can cause problems when the driver attempts to create the main debugfs
directory again. Later on in the code we fail horribly by trying to
dereference a pointer that is an error value.
Reported-by: Laura Abbott <labbott@redhat.com>
Reported-by: Gabriel Ramirez <gabriello.ramirez@gmail.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: stable <stable@vger.kernel.org> # 5.0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
index 2453ceabf00d..738eddb2e7ac 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
@@ -774,6 +774,11 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
return;
mvmvif->dbgfs_dir = debugfs_create_dir("iwlmvm", dbgfs_dir);
+ if (IS_ERR_OR_NULL(mvmvif->dbgfs_dir)) {
+ IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
+ dbgfs_dir);
+ return;
+ }
if (!mvmvif->dbgfs_dir) {
IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
--
2.20.1
^ permalink raw reply related
* [PATCH 4/4] iwlwifi: cfg: use family 22560 based_params for AX210 family
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shaul Triebitz, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Shaul Triebitz <shaul.triebitz@intel.com>
Specifically, max_tfd_queue_size should be 0x10000 like in
22560 family and not 0x100 like in 22000 family.
Signed-off-by: Shaul Triebitz <shaul.triebitz@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
index eb6defb6d0cd..0a87d87fbb4f 100644
--- a/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
+++ b/drivers/net/wireless/intel/iwlwifi/cfg/22000.c
@@ -201,7 +201,7 @@ static const struct iwl_ht_params iwl_22000_ht_params = {
#define IWL_DEVICE_AX210 \
IWL_DEVICE_AX200_COMMON, \
.device_family = IWL_DEVICE_FAMILY_AX210, \
- .base_params = &iwl_22000_base_params, \
+ .base_params = &iwl_22560_base_params, \
.csr = &iwl_csr_v1, \
.min_txq_size = 128
--
2.20.1
^ permalink raw reply related
* [PATCH 2/4] iwlwifi: dbg_ini: check debug TLV type explicitly
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
In ini debug TLVs bit 24 is set. The driver relies on it in the memory
allocation for the debug configuration. This implementation is
problematic in case of a new debug TLV that is not supported yet is added
and uses bit 24. In such a scenario the driver allocate space without
using it which causes errors in the apply point enabling flow.
Solve it by explicitly checking if a given TLV is part of the list of
the supported ini debug TLVs.
Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
Fixes: f14cda6f3b31 ("iwlwifi: trans: parse and store debug ini TLVs")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/fw/file.h | 15 +++++++++------
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 ++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h b/drivers/net/wireless/intel/iwlwifi/fw/file.h
index 641c95d03b15..e06407dc088b 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
@@ -93,7 +93,7 @@ struct iwl_ucode_header {
} u;
};
-#define IWL_UCODE_INI_TLV_GROUP BIT(24)
+#define IWL_UCODE_INI_TLV_GROUP 0x1000000
/*
* new TLV uCode file layout
@@ -148,11 +148,14 @@ enum iwl_ucode_tlv_type {
IWL_UCODE_TLV_UMAC_DEBUG_ADDRS = 54,
IWL_UCODE_TLV_LMAC_DEBUG_ADDRS = 55,
IWL_UCODE_TLV_FW_RECOVERY_INFO = 57,
- IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION = IWL_UCODE_INI_TLV_GROUP | 0x1,
- IWL_UCODE_TLV_TYPE_HCMD = IWL_UCODE_INI_TLV_GROUP | 0x2,
- IWL_UCODE_TLV_TYPE_REGIONS = IWL_UCODE_INI_TLV_GROUP | 0x3,
- IWL_UCODE_TLV_TYPE_TRIGGERS = IWL_UCODE_INI_TLV_GROUP | 0x4,
- IWL_UCODE_TLV_TYPE_DEBUG_FLOW = IWL_UCODE_INI_TLV_GROUP | 0x5,
+
+ IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION = IWL_UCODE_INI_TLV_GROUP + 0x1,
+ IWL_UCODE_TLV_DEBUG_BASE = IWL_UCODE_TLV_TYPE_BUFFER_ALLOCATION,
+ IWL_UCODE_TLV_TYPE_HCMD = IWL_UCODE_INI_TLV_GROUP + 0x2,
+ IWL_UCODE_TLV_TYPE_REGIONS = IWL_UCODE_INI_TLV_GROUP + 0x3,
+ IWL_UCODE_TLV_TYPE_TRIGGERS = IWL_UCODE_INI_TLV_GROUP + 0x4,
+ IWL_UCODE_TLV_TYPE_DEBUG_FLOW = IWL_UCODE_INI_TLV_GROUP + 0x5,
+ IWL_UCODE_TLV_DEBUG_MAX = IWL_UCODE_TLV_TYPE_DEBUG_FLOW,
/* TLVs 0x1000-0x2000 are for internal driver usage */
IWL_UCODE_TLV_FW_DBG_DUMP_LST = 0x1000,
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index 5798f434f68f..c7070760a10a 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -126,7 +126,8 @@ void iwl_alloc_dbg_tlv(struct iwl_trans *trans, size_t len, const u8 *data,
len -= ALIGN(tlv_len, 4);
data += sizeof(*tlv) + ALIGN(tlv_len, 4);
- if (!(tlv_type & IWL_UCODE_INI_TLV_GROUP))
+ if (tlv_type < IWL_UCODE_TLV_DEBUG_BASE ||
+ tlv_type > IWL_UCODE_TLV_DEBUG_MAX)
continue;
hdr = (void *)&tlv->data[0];
--
2.20.1
^ permalink raw reply related
* [PATCH 0/4] iwlwifi: fixes intended for 5.1 2019-04-18
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Luca Coelho
From: Luca Coelho <luciano.coelho@intel.com>
Hi,
This is the third patchset with fixes for 5.1.
The changes are:
* Fix an oops when creating debugfs entries;
* Fix bug when trying to capture debugging info while in rfkill;
* Prevent potential uninitialized memory dumps into debugging logs;
* Fix some initialization parameters for AX210 devices;
Not in this series, but sent before separately:
* Fix an oops with non-MSIX devices;
As usual, I'm pushing this to a pending branch, for kbuild bot, and
will send a pull-request later.
Please review.
Cheers,
Luca.
Greg Kroah-Hartman (1):
iwlwifi: mvm: properly check debugfs dentry before using it
Johannes Berg (1):
iwlwifi: mvm: don't attempt debug collection in rfkill
Shahar S Matityahu (1):
iwlwifi: dbg_ini: check debug TLV type explicitly
Shaul Triebitz (1):
iwlwifi: cfg: use family 22560 based_params for AX210 family
drivers/net/wireless/intel/iwlwifi/cfg/22000.c | 2 +-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 15 +++++++++------
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 ++-
.../net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 5 +++++
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 +++-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 2 +-
6 files changed, 21 insertions(+), 10 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/4] iwlwifi: mvm: don't attempt debug collection in rfkill
From: Luca Coelho @ 2019-04-18 12:15 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Johannes Berg, Luca Coelho
In-Reply-To: <20190418121544.32040-1-luca@coelho.fi>
From: Johannes Berg <johannes.berg@intel.com>
If we fail to initialize because rfkill is enabled, then trying
to do debug collection currently just fails. Prevent that in the
high-level code, although we should probably also fix the lower
level code to do things more carefully.
It's not 100% clear that it fixes this commit, as the original
dump code at the time might've been more careful. In any case,
we don't really need to dump anything in this expected scenario.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Fixes: 7125648074e8 ("iwlwifi: add fw dump upon RT ucode start failure")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 +++-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index 00a47f6f1d81..ab68b5d53ec9 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -1121,7 +1121,9 @@ int iwl_mvm_up(struct iwl_mvm *mvm)
ret = iwl_mvm_load_rt_fw(mvm);
if (ret) {
IWL_ERR(mvm, "Failed to start RT ucode: %d\n", ret);
- iwl_fw_dbg_error_collect(&mvm->fwrt, FW_DBG_TRIGGER_DRIVER);
+ if (ret != -ERFKILL)
+ iwl_fw_dbg_error_collect(&mvm->fwrt,
+ FW_DBG_TRIGGER_DRIVER);
goto error;
}
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
index ba27dce4c2bb..13681b03c10e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
@@ -834,7 +834,7 @@ iwl_op_mode_mvm_start(struct iwl_trans *trans, const struct iwl_cfg *cfg,
mutex_lock(&mvm->mutex);
iwl_mvm_ref(mvm, IWL_MVM_REF_INIT_UCODE);
err = iwl_run_init_mvm_ucode(mvm, true);
- if (err)
+ if (err && err != -ERFKILL)
iwl_fw_dbg_error_collect(&mvm->fwrt, FW_DBG_TRIGGER_DRIVER);
if (!iwlmvm_mod_params.init_dbg || !err)
iwl_mvm_stop_device(mvm);
--
2.20.1
^ permalink raw reply related
* Re: brcmfmac: NULL pointer dereference during brcmf_detach() after firmware crash
From: Rafał Miłecki @ 2019-04-18 11:55 UTC (permalink / raw)
To: Arend Van Spriel
Cc: linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER <brcm80211-dev-list.pdl@broadcom.com>,,
Aaron Blair
In-Reply-To: <CACna6rxR4t1MpniasnUfyiuBhc9tTkJRx=O7GrMnmVPOXCZ-dQ@mail.gmail.com>
On Fri, 15 Feb 2019 at 07:15, Rafał Miłecki <zajec5@gmail.com> wrote:
> On Thu, 14 Feb 2019 at 23:37, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
> > On 2/14/2019 11:30 PM, Rafał Miłecki wrote:
> > > I've just found a well reproducible brcmfmac crash (NULL pointer
> > > dereference).
> > >
> > > Steps:
> > > 1. Wait for or trigger a FullMAC firmware crash
> > > 2. Wait for some skb to get queued on a flowring
> > > 3. Call rmmod brcmfmac
> > >
> > > Problem:
> > > There is a NULL pointer dereference in one of the brcmf_detach() calls.
> > >
> > > Explanation:
> > > brcmf_detach() first frees all "ifp"s and then deletes flowrings. If any
> > > flowring has a skb it results in calling brcmf_txfinalize() which tries
> > > to access "ifp" (struct brcmf_if) which is a NULL.
> >
> > Hi Rafał,
> >
> > Thanks for diving in. That was my suspicion. Does it mean you are
> > working on a patch or shall I take care of it.
>
> It would be nice to have someone more experienced with detaching &
> rings look at it. Is adding a simple
> if (ifp)
> enough? Or should that code get redesigned? Should we e.g. reorder detach order?
Hi Arend, would you find a moment to look at that crash, please?
--
Rafał
^ permalink raw reply
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Thomas Gleixner @ 2019-04-18 11:49 UTC (permalink / raw)
To: David Laight
Cc: 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Paolo Bonzini', 'Dave Hansen',
'Ashok Raj', 'Peter Zijlstra',
'Ravi V Shankar', 'Xiaoyao Li ',
'Christopherson Sean J', 'Kalle Valo',
'Michael Chan', 'linux-kernel', 'x86',
'kvm@vger.kernel.org', 'netdev@vger.kernel.org',
'linux-wireless@vger.kernel.org'
In-Reply-To: <698b9419c81b443fb9c180d2b27fd34a@AcuMS.aculab.com>
On Thu, 18 Apr 2019, David Laight wrote:
> From: David Laight
> > Sent: 18 April 2019 10:21
> > From: Fenghua Yu
> > > Sent: 17 April 2019 22:34
> > >
> > > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > > operate on bitmap defined in x86_capability.
> > >
> > > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > > the location is at:
> > > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> > >
> > > Since base address of x86_capability may not be aligned to unsigned long,
> > > the single unsigned long location may cross two cache lines and
> > > accessing the location by locked BTS/BTR introductions will cause
> > > split lock.
> >
> > Isn't the problem that the type (and definition) of x86_capability[] are wrong.
> > If the 'bitmap' functions are used for it, it should be defined as a bitmap.
> > This would make it 'unsigned long' not __u32.
> >
> > This type munging of bitmaps only works on LE systems.
> >
> > OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
> > ISTR some of the associated functions use byte accesses.
> >
> > Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
> > 32bit accesses.
>
> A quick look shows that this isn't the only __32[] that is being
> cast to (unsigned long) and then to set/test/clear_bit() in those
> files.
>
> I wonder how much other code is applying such casts?
The reason for the cpuid stuff using u32 is that this is actually the width
of the information retrieved from CPUID.
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-18 11:08 UTC (permalink / raw)
To: David Laight, 'Fenghua Yu', 'Thomas Gleixner',
'Ingo Molnar', 'Borislav Petkov',
'H Peter Anvin', 'Paolo Bonzini',
'Dave Hansen', 'Ashok Raj',
'Peter Zijlstra', 'Ravi V Shankar',
'Xiaoyao Li ', 'Christopherson Sean J',
'Kalle Valo', 'Michael Chan'
Cc: 'linux-kernel', 'x86',
'kvm@vger.kernel.org', 'netdev@vger.kernel.org',
'linux-wireless@vger.kernel.org'
In-Reply-To: <c8ca959a4bc549b59baca563660d42d6@AcuMS.aculab.com>
From: David Laight
> Sent: 18 April 2019 10:21
> From: Fenghua Yu
> > Sent: 17 April 2019 22:34
> >
> > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > operate on bitmap defined in x86_capability.
> >
> > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > the location is at:
> > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> >
> > Since base address of x86_capability may not be aligned to unsigned long,
> > the single unsigned long location may cross two cache lines and
> > accessing the location by locked BTS/BTR introductions will cause
> > split lock.
>
> Isn't the problem that the type (and definition) of x86_capability[] are wrong.
> If the 'bitmap' functions are used for it, it should be defined as a bitmap.
> This would make it 'unsigned long' not __u32.
>
> This type munging of bitmaps only works on LE systems.
>
> OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
> ISTR some of the associated functions use byte accesses.
>
> Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
> 32bit accesses.
A quick look shows that this isn't the only __32[] that is being
cast to (unsigned long) and then to set/test/clear_bit() in those
files.
I wonder how much other code is applying such casts?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] NFC: st95hf: fix spelling mistake: "receieve" -> "receive"
From: Mukesh Ojha @ 2019-04-18 10:42 UTC (permalink / raw)
To: Colin King, Samuel Ortiz, Daniel Mack, linux-wireless
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190418095756.9264-1-colin.king@canonical.com>
On 4/18/2019 3:27 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in a dev_error message. Fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/nfc/st95hf/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
> index a50a95cfcfd8..d72d48243141 100644
> --- a/drivers/nfc/st95hf/core.c
> +++ b/drivers/nfc/st95hf/core.c
> @@ -327,7 +327,7 @@ static int st95hf_echo_command(struct st95hf_context *st95context)
> &echo_response);
> if (result) {
> dev_err(&st95context->spicontext.spidev->dev,
> - "err: echo response receieve error = 0x%x\n", result);
> + "err: echo response receive error = 0x%x\n", result);
> return result;
> }
>
^ permalink raw reply
* [PATCH] NFC: st95hf: fix spelling mistake: "receieve" -> "receive"
From: Colin King @ 2019-04-18 9:57 UTC (permalink / raw)
To: Samuel Ortiz, Daniel Mack, linux-wireless; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There is a spelling mistake in a dev_error message. Fix it.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/nfc/st95hf/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index a50a95cfcfd8..d72d48243141 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -327,7 +327,7 @@ static int st95hf_echo_command(struct st95hf_context *st95context)
&echo_response);
if (result) {
dev_err(&st95context->spicontext.spidev->dev,
- "err: echo response receieve error = 0x%x\n", result);
+ "err: echo response receive error = 0x%x\n", result);
return result;
}
--
2.20.1
^ permalink raw reply related
* RE: [PATCH v7 04/21] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-18 9:20 UTC (permalink / raw)
To: 'Fenghua Yu', Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H Peter Anvin, Paolo Bonzini, Dave Hansen,
Ashok Raj, Peter Zijlstra, Ravi V Shankar, Xiaoyao Li ,
Christopherson Sean J, Kalle Valo, Michael Chan
Cc: linux-kernel, x86, kvm@vger.kernel.org, netdev@vger.kernel.org,
linux-wireless@vger.kernel.org
In-Reply-To: <1555536851-17462-5-git-send-email-fenghua.yu@intel.com>
From: Fenghua Yu
> Sent: 17 April 2019 22:34
>
> set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> operate on bitmap defined in x86_capability.
>
> Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> the location is at:
> base address of x86_capability + (bit offset in x86_capability / 64) * 8
>
> Since base address of x86_capability may not be aligned to unsigned long,
> the single unsigned long location may cross two cache lines and
> accessing the location by locked BTS/BTR introductions will cause
> split lock.
Isn't the problem that the type (and definition) of x86_capability[] are wrong.
If the 'bitmap' functions are used for it, it should be defined as a bitmap.
This would make it 'unsigned long' not __u32.
This type munging of bitmaps only works on LE systems.
OTOH the locked BTS/BTR instructions could be changed to use 32 bit accesses.
ISTR some of the associated functions use byte accesses.
Perhaps there ought to be asm wrappers for BTS/BTR that do 8bit and
32bit accesses.
>
> To fix the split lock issue, align x86_capability to size of unsigned long
> so that the location will be always within one cache line.
>
> Changing x86_capability's type to unsigned long may also fix the issue
> because x86_capability will be naturally aligned to size of unsigned long.
> But this needs additional code changes. So choose the simpler solution
> by setting the array's alignment to size of unsigned long.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/include/asm/processor.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 2bb3a648fc12..7c62b9ad6e5a 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -93,7 +93,9 @@ struct cpuinfo_x86 {
> __u32 extended_cpuid_level;
> /* Maximum supported CPUID level, -1=no CPUID: */
> int cpuid_level;
> - __u32 x86_capability[NCAPINTS + NBUGINTS];
> + /* Aligned to size of unsigned long to avoid split lock in atomic ops */
> + __u32 x86_capability[NCAPINTS + NBUGINTS]
> + __aligned(sizeof(unsigned long));
> char x86_vendor_id[16];
> char x86_model_id[64];
> /* in KB - valid for CPUS which support this call: */
> --
> 2.19.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2] ath10k: fix different tx duration output
From: Toke Høiland-Jørgensen @ 2019-04-18 8:07 UTC (permalink / raw)
To: leiwa; +Cc: ath10k, linux-wireless
In-Reply-To: <298a645b17aac0f5c466f011225533b0@codeaurora.org>
leiwa@codeaurora.org writes:
> On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote:
>> Lei Wang <leiwa@codeaurora.org> writes:
>>
>>> TX duration output of tx_stats in debugfs and station dump had big
>>> difference because they got tx duration value from different statistic
>>> data. We should use the same statistic data.
>>
>> So are you sure you picked the most accurate one of the two? :)
>>
>> -Toke
>
> Hi Toke,
>
> Yes.
> Now for ath10k, there are two ways to get tx duration output.
> One is got from tx_stats in debugfs reported by firmware. It is a total
> value including all the frames which created by host and firmware sent
> to the peer.
> And the second is calculated from
> ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here
> the tx duration just includes the data frames sent from host to the
> peer.
So the difference is that the former includes control frames as well? Is
that the only difference? And what exactly is a "big difference" (from
the commit message)?
> So the first value is preferable for station dump.
Hmm, I'm not sure if I agree with this. I specifically added the
tx_duration to the station dump to be able to get the values used by the
airtime scheduler. This breaks with this patch.
-Toke
^ permalink raw reply
* Re: [PATCH v2] ath10k: fix different tx duration output
From: leiwa @ 2019-04-18 7:26 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: ath10k, linux-wireless
In-Reply-To: <87mukprnie.fsf@toke.dk>
On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote:
> Lei Wang <leiwa@codeaurora.org> writes:
>
>> TX duration output of tx_stats in debugfs and station dump had big
>> difference because they got tx duration value from different statistic
>> data. We should use the same statistic data.
>
> So are you sure you picked the most accurate one of the two? :)
>
> -Toke
Hi Toke,
Yes.
Now for ath10k, there are two ways to get tx duration output.
One is got from tx_stats in debugfs reported by firmware. It is a total
value including all the frames which created by host and firmware sent
to the peer.
And the second is calculated from
ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here
the tx duration just includes the data frames sent from host to the
peer.
So the first value is preferable for station dump.
Thanks.
Lei
^ permalink raw reply
* Re: [PATCH v7 15/21] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Thomas Gleixner @ 2019-04-18 6:41 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <20190418005711.GB18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 12:47:24AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> >
> > > The interface /sys/device/system/cpu/split_lock_detect is added
> > > to allow user to control split lock detection and show current split
> > > lock detection setting.
> > >
> > > Writing [yY1] or [oO][nN] to the file enables split lock detection and
> > > writing [nN0] or [oO][fF] disables split lock detection. Split lock
> > > detection is enabled or disabled on all CPUs.
> > >
> > > Reading the file returns current global split lock detection setting:
> > > 0: disabled
> > > 1: enabled
> >
> > Again, You explain WHAT this patch does and still there is zero
> > justification why this sysfs knob is needed at all. I still do not see any
> > reason why this knob should exist.
>
> An important application has split lock issues which are already discovered
> and need to be fixed. But before the issues are fixed, sysadmin still wants to
> run the application without rebooting the system, the sysfs knob can be useful
> to turn off split lock detection. After the application is done, split lock
> detection will be enabled again through the sysfs knob.
Are you sure that you are talking about the real world? I might buy the
'off' part somehow, but the 'on' part is beyond theoretical.
Even the 'off' part is dubious on a multi user machine. I personally would
neither think about using the sysfs knob nor about rebooting the machine
simply because I'd consider a lock operation accross a cacheline an malicious
DoS attempt. Why would I allow that?
So in reality the sysadmin will either move the workload to a machine w/o
the #AC magic or just tell the user to fix his crap.
> Without the sysfs knob, sysadmin has to reboot the system with kernel option
> "no_split_lock_detect" to run the application before the split lock issues
> are fixed.
>
> Is this a valid justification why the sysfs knob is needed? If it is, I can
> add the justification in the next version.
Why has this information not been in the changelog right away? I'm really
tired of asking the same questions and pointing you to
Documentation/process over and over.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST_CTL
From: Thomas Gleixner @ 2019-04-18 6:31 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <20190418012848.GC18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> > > +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> > > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> >
> > Contrary to things like cpufeatures or MSR bits, it's pretty useless to
> > have a separate patch for this. Please fold this into the place which
> > actualy uses it.
>
> Can I fold this patch into the KVM patch 0013 which first uses (reads) the
> variable? But the variable will be set in later patches when enabling split
> lock feature (patch 0014) and when enabling/disabling split lock feature
> (patch 0015).
>
> Is this a right sequence to fit the variable in the patch set?
As I said in the other reply, you are assuming that the content of that MSR
is 0. Which might be true now, but is that true in a year from now?
So you really want to at least initialize the variable by reading the MSR
_before_ you make use of it in KVM.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Thomas Gleixner @ 2019-04-18 6:16 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <20190417234728.GA18776@romley-ivt3.sc.intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 18, 2019 at 01:19:41AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2019, Fenghua Yu wrote:
> >
> > > The kernel option clearcpuid currently only takes feature bit which
> > > can be changed from kernel to kernel.
> > >
> > > Extend clearcpuid to use cap flag string, which is defined in
> > > x86_cap_flags[] and won't be changed from kernel to kernel.
> > > And user can easily get the cap flag string from /proc/cpuinfo.
> >
> > If your machine dies because init triggers #AC then please explain how that
> > easily can be read from /proc/cpuinfo and how the sysadmin can figure out
> > what the heck he needs to write on the kernel command line.
> >
> > The whole 'clearcpuid' thing should have never been merged. It's a pure
> > testing/debugging thing. And no, we are not going to proliferate it and
> > extend it for dubious value. Quite the contrary, we should simply rip it
> > out.
>
> So I can remove the four 'clearcpuid' related patches 0018-0021 in the next
> version, right?
Yes please. They are unrelated to this problem and 'noac' is way more admin
friendly than that.
Thanks,
tglx
^ permalink raw reply
* Re: Kali Linux 4.19.13 VM, Ralink RT3072 - Not Finding Wireless Networks
From: Tom Psyborg @ 2019-04-18 2:08 UTC (permalink / raw)
To: Kevan Salva; +Cc: linux-wireless
In-Reply-To: <CAAM6Ncf0=1wBbnomxH=o8r9-edhSk2zr_xzhJeA1uWdzRZ84Eg@mail.gmail.com>
On 18/04/2019, Kevan Salva <kenico15@gmail.com> wrote:
> Hello All,
>
> I am running Kali Linux (4.19.13) in a VM. I have a USB network
> adapter - a Ralink RT3072, passed through to the VM. However, it is
> not finding any wireless networks. This issue occurs on both VMware
> and on Virtualbox, but not on a live install of the same OS. This is
> the output I get from dmesg: pastebin.com/HaXUDNug . What may be the
> issue here?
>
> With much appreciation,
>
> Kevan
>
do you get such issues if running older kali/linux version on the same system?
^ permalink raw reply
* Kali Linux 4.19.13 VM, Ralink RT3072 - Not Finding Wireless Networks
From: Kevan Salva @ 2019-04-18 1:57 UTC (permalink / raw)
To: linux-wireless
Hello All,
I am running Kali Linux (4.19.13) in a VM. I have a USB network
adapter - a Ralink RT3072, passed through to the VM. However, it is
not finding any wireless networks. This issue occurs on both VMware
and on Virtualbox, but not on a live install of the same OS. This is
the output I get from dmesg: pastebin.com/HaXUDNug . What may be the
issue here?
With much appreciation,
Kevan
^ permalink raw reply
* Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST_CTL
From: Fenghua Yu @ 2019-04-18 1:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <alpine.DEB.2.21.1904180009200.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > MSR TEST_CTL (0x33) value is cached in per CPU variable msr_test_ctl_cache.
>
> It _is_ cached? How so?
>
> > The cached value will be used in virutalization to avoid costly MSR read.
> >
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com>
>
> That SOB chain is bogus.
>
> > ---
> > arch/x86/include/asm/cpu.h | 1 +
> > arch/x86/kernel/cpu/intel.c | 3 +++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> > index 4e03f53fc079..cd7493f20234 100644
> > --- a/arch/x86/include/asm/cpu.h
> > +++ b/arch/x86/include/asm/cpu.h
> > @@ -40,6 +40,7 @@ int mwait_usable(const struct cpuinfo_x86 *);
> > unsigned int x86_family(unsigned int sig);
> > unsigned int x86_model(unsigned int sig);
> > unsigned int x86_stepping(unsigned int sig);
> > +DECLARE_PER_CPU(u64, msr_test_ctl_cache);
> > #ifdef CONFIG_CPU_SUP_INTEL
> > void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
> > #else
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index 62f61a961eb6..997d683d3c27 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -31,6 +31,9 @@
> > #include <asm/apic.h>
> > #endif
> >
> > +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
>
> Contrary to things like cpufeatures or MSR bits, it's pretty useless to
> have a separate patch for this. Please fold this into the place which
> actualy uses it.
Can I fold this patch into the KVM patch 0013 which first uses (reads) the
variable? But the variable will be set in later patches when enabling split
lock feature (patch 0014) and when enabling/disabling split lock feature
(patch 0015).
Is this a right sequence to fit the variable in the patch set?
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 15/21] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Fenghua Yu @ 2019-04-18 0:57 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <alpine.DEB.2.21.1904180042090.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 12:47:24AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > The interface /sys/device/system/cpu/split_lock_detect is added
> > to allow user to control split lock detection and show current split
> > lock detection setting.
> >
> > Writing [yY1] or [oO][nN] to the file enables split lock detection and
> > writing [nN0] or [oO][fF] disables split lock detection. Split lock
> > detection is enabled or disabled on all CPUs.
> >
> > Reading the file returns current global split lock detection setting:
> > 0: disabled
> > 1: enabled
>
> Again, You explain WHAT this patch does and still there is zero
> justification why this sysfs knob is needed at all. I still do not see any
> reason why this knob should exist.
An important application has split lock issues which are already discovered
and need to be fixed. But before the issues are fixed, sysadmin still wants to
run the application without rebooting the system, the sysfs knob can be useful
to turn off split lock detection. After the application is done, split lock
detection will be enabled again through the sysfs knob.
Without the sysfs knob, sysadmin has to reboot the system with kernel option
"no_split_lock_detect" to run the application before the split lock issues
are fixed.
Is this a valid justification why the sysfs knob is needed? If it is, I can
add the justification in the next version.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Fenghua Yu @ 2019-04-17 23:47 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <alpine.DEB.2.21.1904180106400.3174@nanos.tec.linutronix.de>
On Thu, Apr 18, 2019 at 01:19:41AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Fenghua Yu wrote:
>
> > The kernel option clearcpuid currently only takes feature bit which
> > can be changed from kernel to kernel.
> >
> > Extend clearcpuid to use cap flag string, which is defined in
> > x86_cap_flags[] and won't be changed from kernel to kernel.
> > And user can easily get the cap flag string from /proc/cpuinfo.
>
> If your machine dies because init triggers #AC then please explain how that
> easily can be read from /proc/cpuinfo and how the sysadmin can figure out
> what the heck he needs to write on the kernel command line.
>
> The whole 'clearcpuid' thing should have never been merged. It's a pure
> testing/debugging thing. And no, we are not going to proliferate it and
> extend it for dubious value. Quite the contrary, we should simply rip it
> out.
So I can remove the four 'clearcpuid' related patches 0018-0021 in the next
version, right?
>
> Add a simple 'noac' or whatever command line option, which is documented
> proper and can easily be mapped to a #AC crash during boot.
OK. I will do this.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v7 18/21] x86/clearcpuid: Support feature flag string in kernel option clearcpuid
From: Thomas Gleixner @ 2019-04-17 23:19 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1555536851-17462-19-git-send-email-fenghua.yu@intel.com>
On Wed, 17 Apr 2019, Fenghua Yu wrote:
> The kernel option clearcpuid currently only takes feature bit which
> can be changed from kernel to kernel.
>
> Extend clearcpuid to use cap flag string, which is defined in
> x86_cap_flags[] and won't be changed from kernel to kernel.
> And user can easily get the cap flag string from /proc/cpuinfo.
If your machine dies because init triggers #AC then please explain how that
easily can be read from /proc/cpuinfo and how the sysadmin can figure out
what the heck he needs to write on the kernel command line.
The whole 'clearcpuid' thing should have never been merged. It's a pure
testing/debugging thing. And no, we are not going to proliferate it and
extend it for dubious value. Quite the contrary, we should simply rip it
out.
Add a simple 'noac' or whatever command line option, which is documented
proper and can easily be mapped to a #AC crash during boot.
Thanks,
tglx
^ 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