[alsa-devel] [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
global_lock is defined as an unsigned long and accessing only its lower 32 bits from sysfs is incorrect, as we need to consider other 32 bits for big endian 64 bit systems. There are no such platforms yet, but the code needs to be robust for such a case.
Fix that by passing a local variable to debugfs_create_bool() and assigning its value to global_lock later.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- V3->V4: - Create a local variable instead of changing type of global_lock (Rafael) - Drop the stable tag - BCC'd a lot of people (rather than cc'ing them) to make sure - the series reaches them - mailing lists do not block the patchset due to long cc list - and we don't spam the BCC'd people for every reply --- drivers/acpi/ec_sys.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c index b4c216bab22b..b44b91331a56 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) struct dentry *dev_dir; char name[64]; umode_t mode = 0400; + u32 val;
if (ec_device_count == 0) { acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL); @@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) goto error; - if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, - (u32 *)&first_ec->global_lock)) + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) goto error;
+ first_ec->global_lock = val; + if (write_support) mode = 0600; if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
Its a bit odd that debugfs_create_bool() takes 'u32 *' as an argument, when all it needs is a boolean pointer.
It would be better to update this API to make it accept 'bool *' instead, as that will make it more consistent and often more convenient. Over that bool takes just a byte (mostly).
That required updates to all user sites as well, in the same commit updating the API. regmap core was also using debugfs_{read|write}_file_bool(), directly and variable types were updated for that to be bool as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Mark Brown broonie@kernel.org Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- V3->V4: - Updated commit log to make it clear - Drop 'global_lock = !!tmp' change, as that's not required at all. - s/u32/bool for ec_sys.c - Added Ack from Charles. - BCC'd a lot of people (rather than cc'ing them) to make sure - the series reaches them - mailing lists do not block the patchset due to long cc list - and we don't spam the BCC'd people for every reply --- Documentation/filesystems/debugfs.txt | 2 +- arch/arm64/kernel/debug-monitors.c | 4 ++-- drivers/acpi/ec_sys.c | 2 +- drivers/acpi/internal.h | 2 +- drivers/base/regmap/internal.h | 6 +++--- drivers/base/regmap/regcache-lzo.c | 4 ++-- drivers/base/regmap/regcache.c | 24 ++++++++++++------------ drivers/bluetooth/hci_qca.c | 4 ++-- drivers/iommu/amd_iommu_init.c | 2 +- drivers/iommu/amd_iommu_types.h | 2 +- drivers/misc/mei/mei_dev.h | 2 +- drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 4 ++-- drivers/net/wireless/ath/ath10k/core.h | 2 +- drivers/net/wireless/ath/ath5k/ath5k.h | 2 +- drivers/net/wireless/ath/ath9k/hw.c | 2 +- drivers/net/wireless/ath/ath9k/hw.h | 4 ++-- drivers/net/wireless/b43/debugfs.c | 18 +++++++++--------- drivers/net/wireless/b43/debugfs.h | 2 +- drivers/net/wireless/b43legacy/debugfs.c | 10 +++++----- drivers/net/wireless/b43legacy/debugfs.h | 2 +- drivers/net/wireless/iwlegacy/common.h | 6 +++--- drivers/net/wireless/iwlwifi/mvm/mvm.h | 6 +++--- drivers/scsi/snic/snic_trc.c | 4 ++-- drivers/scsi/snic/snic_trc.h | 2 +- drivers/uwb/uwb-debug.c | 2 +- fs/debugfs/file.c | 6 +++--- include/linux/debugfs.h | 4 ++-- include/linux/edac.h | 2 +- include/linux/fault-inject.h | 2 +- kernel/futex.c | 4 ++-- lib/dma-debug.c | 2 +- mm/failslab.c | 8 ++++---- mm/page_alloc.c | 8 ++++---- sound/soc/codecs/wm_adsp.h | 2 +- 34 files changed, 79 insertions(+), 79 deletions(-)
diff --git a/Documentation/filesystems/debugfs.txt b/Documentation/filesystems/debugfs.txt index 463f595733e8..4f45f71149cb 100644 --- a/Documentation/filesystems/debugfs.txt +++ b/Documentation/filesystems/debugfs.txt @@ -105,7 +105,7 @@ a variable of type size_t. Boolean values can be placed in debugfs with:
struct dentry *debugfs_create_bool(const char *name, umode_t mode, - struct dentry *parent, u32 *value); + struct dentry *parent, bool *value);
A read on the resulting file will yield either Y (for non-zero values) or N, followed by a newline. If written to, it will accept either upper- or diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index cebf78661a55..1c4cd4a0d7cc 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -58,7 +58,7 @@ static u32 mdscr_read(void) * Allow root to disable self-hosted debug from userspace. * This is useful if you want to connect an external JTAG debugger. */ -static u32 debug_enabled = 1; +static bool debug_enabled = true;
static int create_debug_debugfs_entry(void) { @@ -69,7 +69,7 @@ fs_initcall(create_debug_debugfs_entry);
static int __init early_debug_disable(char *buf) { - debug_enabled = 0; + debug_enabled = false; return 0; }
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c index b44b91331a56..f025b044a1cf 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -110,7 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) struct dentry *dev_dir; char name[64]; umode_t mode = 0400; - u32 val; + bool val;
if (ec_device_count == 0) { acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL); diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 9e426210c2a8..5a72e2b140fc 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -138,7 +138,7 @@ struct acpi_ec { unsigned long gpe; unsigned long command_addr; unsigned long data_addr; - unsigned long global_lock; + bool global_lock; unsigned long flags; unsigned long reference_count; struct mutex mutex; diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index cc557886ab23..5b907f2c62b9 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -122,9 +122,9 @@ struct regmap { unsigned int num_reg_defaults_raw;
/* if set, only the cache is modified not the HW */ - u32 cache_only; + bool cache_only; /* if set, only the HW is modified not the cache */ - u32 cache_bypass; + bool cache_bypass; /* if set, remember to free reg_defaults_raw */ bool cache_free;
@@ -132,7 +132,7 @@ struct regmap { const void *reg_defaults_raw; void *cache; /* if set, the cache contains newer data than the HW */ - u32 cache_dirty; + bool cache_dirty; /* if set, the HW registers are known to match map->reg_defaults */ bool no_sync_defaults;
diff --git a/drivers/base/regmap/regcache-lzo.c b/drivers/base/regmap/regcache-lzo.c index 2d53f6f138e1..736e0d378567 100644 --- a/drivers/base/regmap/regcache-lzo.c +++ b/drivers/base/regmap/regcache-lzo.c @@ -355,9 +355,9 @@ static int regcache_lzo_sync(struct regmap *map, unsigned int min, if (ret > 0 && val == map->reg_defaults[ret].def) continue;
- map->cache_bypass = 1; + map->cache_bypass = true; ret = _regmap_write(map, i, val); - map->cache_bypass = 0; + map->cache_bypass = false; if (ret) return ret; dev_dbg(map->dev, "Synced register %#x, value %#x\n", diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index 6f8a13ec32a4..4c07802986b2 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -54,11 +54,11 @@ static int regcache_hw_init(struct regmap *map) return -ENOMEM;
if (!map->reg_defaults_raw) { - u32 cache_bypass = map->cache_bypass; + bool cache_bypass = map->cache_bypass; dev_warn(map->dev, "No cache defaults, reading back from HW\n");
/* Bypass the cache access till data read from HW*/ - map->cache_bypass = 1; + map->cache_bypass = true; tmp_buf = kmalloc(map->cache_size_raw, GFP_KERNEL); if (!tmp_buf) { ret = -ENOMEM; @@ -285,9 +285,9 @@ static int regcache_default_sync(struct regmap *map, unsigned int min, if (!regcache_reg_needs_sync(map, reg, val)) continue;
- map->cache_bypass = 1; + map->cache_bypass = true; ret = _regmap_write(map, reg, val); - map->cache_bypass = 0; + map->cache_bypass = false; if (ret) { dev_err(map->dev, "Unable to sync register %#x. %d\n", reg, ret); @@ -315,7 +315,7 @@ int regcache_sync(struct regmap *map) int ret = 0; unsigned int i; const char *name; - unsigned int bypass; + bool bypass;
BUG_ON(!map->cache_ops);
@@ -333,7 +333,7 @@ int regcache_sync(struct regmap *map) map->async = true;
/* Apply any patch first */ - map->cache_bypass = 1; + map->cache_bypass = true; for (i = 0; i < map->patch_regs; i++) { ret = _regmap_write(map, map->patch[i].reg, map->patch[i].def); if (ret != 0) { @@ -342,7 +342,7 @@ int regcache_sync(struct regmap *map) goto out; } } - map->cache_bypass = 0; + map->cache_bypass = false;
if (map->cache_ops->sync) ret = map->cache_ops->sync(map, 0, map->max_register); @@ -384,7 +384,7 @@ int regcache_sync_region(struct regmap *map, unsigned int min, { int ret = 0; const char *name; - unsigned int bypass; + bool bypass;
BUG_ON(!map->cache_ops);
@@ -637,11 +637,11 @@ static int regcache_sync_block_single(struct regmap *map, void *block, if (!regcache_reg_needs_sync(map, regtmp, val)) continue;
- map->cache_bypass = 1; + map->cache_bypass = true;
ret = _regmap_write(map, regtmp, val);
- map->cache_bypass = 0; + map->cache_bypass = false; if (ret != 0) { dev_err(map->dev, "Unable to sync register %#x. %d\n", regtmp, ret); @@ -668,14 +668,14 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data, dev_dbg(map->dev, "Writing %zu bytes for %d registers from 0x%x-0x%x\n", count * val_bytes, count, base, cur - map->reg_stride);
- map->cache_bypass = 1; + map->cache_bypass = true;
ret = _regmap_raw_write(map, base, *data, count * val_bytes); if (ret) dev_err(map->dev, "Unable to sync registers %#x-%#x. %d\n", base, cur - map->reg_stride, ret);
- map->cache_bypass = 0; + map->cache_bypass = false;
*data = NULL;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 6b9b91267959..509477681661 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -80,8 +80,8 @@ struct qca_data { spinlock_t hci_ibs_lock; /* HCI_IBS state lock */ u8 tx_ibs_state; /* HCI_IBS transmit side power state*/ u8 rx_ibs_state; /* HCI_IBS receive side power state */ - u32 tx_vote; /* Clock must be on for TX */ - u32 rx_vote; /* Clock must be on for RX */ + bool tx_vote; /* Clock must be on for TX */ + bool rx_vote; /* Clock must be on for RX */ struct timer_list tx_idle_timer; u32 tx_idle_delay; struct timer_list wake_retrans_timer; diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c index 5ef347a13cb5..c59314523f4c 100644 --- a/drivers/iommu/amd_iommu_init.c +++ b/drivers/iommu/amd_iommu_init.c @@ -138,7 +138,7 @@ u16 amd_iommu_last_bdf; /* largest PCI device id we have to handle */ LIST_HEAD(amd_iommu_unity_map); /* a list of required unity mappings we find in ACPI */ -u32 amd_iommu_unmap_flush; /* if true, flush on every unmap */ +bool amd_iommu_unmap_flush; /* if true, flush on every unmap */
LIST_HEAD(amd_iommu_list); /* list of all AMD IOMMUs in the system */ diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h index f65908841be0..861550a1ad1f 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -674,7 +674,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap; * If true, the addresses will be flushed on unmap time, not when * they are reused */ -extern u32 amd_iommu_unmap_flush; +extern bool amd_iommu_unmap_flush;
/* Smallest max PASID supported by any IOMMU in the system */ extern u32 amd_iommu_max_pasid; diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h index e25ee16c658e..d74b6aa8ae27 100644 --- a/drivers/misc/mei/mei_dev.h +++ b/drivers/misc/mei/mei_dev.h @@ -528,7 +528,7 @@ struct mei_device { DECLARE_BITMAP(host_clients_map, MEI_CLIENTS_MAX); unsigned long me_client_index;
- u32 allow_fixed_address; + bool allow_fixed_address;
struct mei_cl wd_cl; enum mei_wd_states wd_state; diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h index fa0c7b54ec7a..5384f999c24b 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h @@ -767,8 +767,8 @@ struct adapter { bool tid_release_task_busy;
struct dentry *debugfs_root; - u32 use_bd; /* Use SGE Back Door intfc for reading SGE Contexts */ - u32 trace_rss; /* 1 implies that different RSS flit per filter is + bool use_bd; /* Use SGE Back Door intfc for reading SGE Contexts */ + bool trace_rss; /* 1 implies that different RSS flit per filter is * used per filter else if 0 default RSS flit is * used for all 4 filters. */ diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 12542144fe12..b6a08177b6ee 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -680,7 +680,7 @@ struct ath10k { bool monitor_started; unsigned int filter_flags; unsigned long dev_flags; - u32 dfs_block_radar_events; + bool dfs_block_radar_events;
/* protected by conf_mutex */ bool radar_enabled; diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h index fa6e89e5c421..ba12f7f4061d 100644 --- a/drivers/net/wireless/ath/ath5k/ath5k.h +++ b/drivers/net/wireless/ath/ath5k/ath5k.h @@ -1367,7 +1367,7 @@ struct ath5k_hw { u8 ah_retry_long; u8 ah_retry_short;
- u32 ah_use_32khz_clock; + bool ah_use_32khz_clock;
u8 ah_coverage_class; bool ah_ack_bitrate_high; diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c index 1dd0339de372..8823fadada89 100644 --- a/drivers/net/wireless/ath/ath9k/hw.c +++ b/drivers/net/wireless/ath/ath9k/hw.c @@ -385,7 +385,7 @@ static void ath9k_hw_init_config(struct ath_hw *ah)
ah->config.dma_beacon_response_time = 1; ah->config.sw_beacon_response_time = 6; - ah->config.cwm_ignore_extcca = 0; + ah->config.cwm_ignore_extcca = false; ah->config.analog_shiftreg = 1;
ah->config.rx_intr_mitigation = true; diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h index e8454db17634..52971b48ab6a 100644 --- a/drivers/net/wireless/ath/ath9k/hw.h +++ b/drivers/net/wireless/ath/ath9k/hw.h @@ -332,14 +332,14 @@ enum ath9k_hw_hang_checks { struct ath9k_ops_config { int dma_beacon_response_time; int sw_beacon_response_time; - u32 cwm_ignore_extcca; + bool cwm_ignore_extcca; u32 pcie_waen; u8 analog_shiftreg; u32 ofdm_trig_low; u32 ofdm_trig_high; u32 cck_trig_high; u32 cck_trig_low; - u32 enable_paprd; + bool enable_paprd; int serialize_regmode; bool rx_intr_mitigation; bool tx_intr_mitigation; diff --git a/drivers/net/wireless/b43/debugfs.c b/drivers/net/wireless/b43/debugfs.c index e807bd930647..b4bcd94aff6c 100644 --- a/drivers/net/wireless/b43/debugfs.c +++ b/drivers/net/wireless/b43/debugfs.c @@ -676,15 +676,15 @@ static void b43_add_dynamic_debug(struct b43_wldev *dev) e->dyn_debug_dentries[id] = d; \ } while (0)
- add_dyn_dbg("debug_xmitpower", B43_DBG_XMITPOWER, 0); - add_dyn_dbg("debug_dmaoverflow", B43_DBG_DMAOVERFLOW, 0); - add_dyn_dbg("debug_dmaverbose", B43_DBG_DMAVERBOSE, 0); - add_dyn_dbg("debug_pwork_fast", B43_DBG_PWORK_FAST, 0); - add_dyn_dbg("debug_pwork_stop", B43_DBG_PWORK_STOP, 0); - add_dyn_dbg("debug_lo", B43_DBG_LO, 0); - add_dyn_dbg("debug_firmware", B43_DBG_FIRMWARE, 0); - add_dyn_dbg("debug_keys", B43_DBG_KEYS, 0); - add_dyn_dbg("debug_verbose_stats", B43_DBG_VERBOSESTATS, 0); + add_dyn_dbg("debug_xmitpower", B43_DBG_XMITPOWER, false); + add_dyn_dbg("debug_dmaoverflow", B43_DBG_DMAOVERFLOW, false); + add_dyn_dbg("debug_dmaverbose", B43_DBG_DMAVERBOSE, false); + add_dyn_dbg("debug_pwork_fast", B43_DBG_PWORK_FAST, false); + add_dyn_dbg("debug_pwork_stop", B43_DBG_PWORK_STOP, false); + add_dyn_dbg("debug_lo", B43_DBG_LO, false); + add_dyn_dbg("debug_firmware", B43_DBG_FIRMWARE, false); + add_dyn_dbg("debug_keys", B43_DBG_KEYS, false); + add_dyn_dbg("debug_verbose_stats", B43_DBG_VERBOSESTATS, false);
#undef add_dyn_dbg } diff --git a/drivers/net/wireless/b43/debugfs.h b/drivers/net/wireless/b43/debugfs.h index 50517b801cb4..d05377745011 100644 --- a/drivers/net/wireless/b43/debugfs.h +++ b/drivers/net/wireless/b43/debugfs.h @@ -68,7 +68,7 @@ struct b43_dfsentry { u32 shm32read_addr_next;
/* Enabled/Disabled list for the dynamic debugging features. */ - u32 dyn_debug[__B43_NR_DYNDBG]; + bool dyn_debug[__B43_NR_DYNDBG]; /* Dentries for the dynamic debugging entries. */ struct dentry *dyn_debug_dentries[__B43_NR_DYNDBG]; }; diff --git a/drivers/net/wireless/b43legacy/debugfs.c b/drivers/net/wireless/b43legacy/debugfs.c index 1965edb765a2..090910ea259e 100644 --- a/drivers/net/wireless/b43legacy/debugfs.c +++ b/drivers/net/wireless/b43legacy/debugfs.c @@ -369,11 +369,11 @@ static void b43legacy_add_dynamic_debug(struct b43legacy_wldev *dev) e->dyn_debug_dentries[id] = d; \ } while (0)
- add_dyn_dbg("debug_xmitpower", B43legacy_DBG_XMITPOWER, 0); - add_dyn_dbg("debug_dmaoverflow", B43legacy_DBG_DMAOVERFLOW, 0); - add_dyn_dbg("debug_dmaverbose", B43legacy_DBG_DMAVERBOSE, 0); - add_dyn_dbg("debug_pwork_fast", B43legacy_DBG_PWORK_FAST, 0); - add_dyn_dbg("debug_pwork_stop", B43legacy_DBG_PWORK_STOP, 0); + add_dyn_dbg("debug_xmitpower", B43legacy_DBG_XMITPOWER, false); + add_dyn_dbg("debug_dmaoverflow", B43legacy_DBG_DMAOVERFLOW, false); + add_dyn_dbg("debug_dmaverbose", B43legacy_DBG_DMAVERBOSE, false); + add_dyn_dbg("debug_pwork_fast", B43legacy_DBG_PWORK_FAST, false); + add_dyn_dbg("debug_pwork_stop", B43legacy_DBG_PWORK_STOP, false);
#undef add_dyn_dbg } diff --git a/drivers/net/wireless/b43legacy/debugfs.h b/drivers/net/wireless/b43legacy/debugfs.h index ae3b0d0fa849..9ee32158b947 100644 --- a/drivers/net/wireless/b43legacy/debugfs.h +++ b/drivers/net/wireless/b43legacy/debugfs.h @@ -47,7 +47,7 @@ struct b43legacy_dfsentry { struct b43legacy_txstatus_log txstatlog;
/* Enabled/Disabled list for the dynamic debugging features. */ - u32 dyn_debug[__B43legacy_NR_DYNDBG]; + bool dyn_debug[__B43legacy_NR_DYNDBG]; /* Dentries for the dynamic debugging entries. */ struct dentry *dyn_debug_dentries[__B43legacy_NR_DYNDBG]; }; diff --git a/drivers/net/wireless/iwlegacy/common.h b/drivers/net/wireless/iwlegacy/common.h index 5b972798bdff..ce52cf114fde 100644 --- a/drivers/net/wireless/iwlegacy/common.h +++ b/drivers/net/wireless/iwlegacy/common.h @@ -1425,9 +1425,9 @@ struct il_priv { #endif /* CONFIG_IWLEGACY_DEBUGFS */
struct work_struct txpower_work; - u32 disable_sens_cal; - u32 disable_chain_noise_cal; - u32 disable_tx_power_cal; + bool disable_sens_cal; + bool disable_chain_noise_cal; + bool disable_tx_power_cal; struct work_struct run_time_calib_work; struct timer_list stats_periodic; struct timer_list watchdog; diff --git a/drivers/net/wireless/iwlwifi/mvm/mvm.h b/drivers/net/wireless/iwlwifi/mvm/mvm.h index b95a07ec9e36..72e8a03a5293 100644 --- a/drivers/net/wireless/iwlwifi/mvm/mvm.h +++ b/drivers/net/wireless/iwlwifi/mvm/mvm.h @@ -649,7 +649,7 @@ struct iwl_mvm { const struct iwl_fw_bcast_filter *bcast_filters; #ifdef CONFIG_IWLWIFI_DEBUGFS struct { - u32 override; /* u32 for debugfs_create_bool */ + bool override; struct iwl_bcast_filter_cmd cmd; } dbgfs_bcast_filtering; #endif @@ -673,7 +673,7 @@ struct iwl_mvm { bool disable_power_off; bool disable_power_off_d3;
- u32 scan_iter_notif_enabled; /* must be u32 for debugfs_create_bool */ + bool scan_iter_notif_enabled;
struct debugfs_blob_wrapper nvm_hw_blob; struct debugfs_blob_wrapper nvm_sw_blob; @@ -729,7 +729,7 @@ struct iwl_mvm { int n_nd_channels; bool net_detect; #ifdef CONFIG_IWLWIFI_DEBUGFS - u32 d3_wake_sysassert; /* must be u32 for debugfs_create_bool */ + bool d3_wake_sysassert; bool d3_test_active; bool store_d3_resume_sram; void *d3_resume_sram; diff --git a/drivers/scsi/snic/snic_trc.c b/drivers/scsi/snic/snic_trc.c index 28a40a7ade38..f00ebf4717e0 100644 --- a/drivers/scsi/snic/snic_trc.c +++ b/drivers/scsi/snic/snic_trc.c @@ -148,7 +148,7 @@ snic_trc_init(void)
trc->max_idx = (tbuf_sz / SNIC_TRC_ENTRY_SZ); trc->rd_idx = trc->wr_idx = 0; - trc->enable = 1; + trc->enable = true; SNIC_INFO("Trace Facility Enabled.\n Trace Buffer SZ %lu Pages.\n", tbuf_sz / PAGE_SIZE); ret = 0; @@ -169,7 +169,7 @@ snic_trc_free(void) { struct snic_trc *trc = &snic_glob->trc;
- trc->enable = 0; + trc->enable = false; snic_trc_debugfs_term();
if (trc->buf) { diff --git a/drivers/scsi/snic/snic_trc.h b/drivers/scsi/snic/snic_trc.h index 427faee5f97e..b37f8867bfde 100644 --- a/drivers/scsi/snic/snic_trc.h +++ b/drivers/scsi/snic/snic_trc.h @@ -45,7 +45,7 @@ struct snic_trc { u32 max_idx; /* Max Index into trace buffer */ u32 rd_idx; u32 wr_idx; - u32 enable; /* Control Variable for Tracing */ + bool enable; /* Control Variable for Tracing */
struct dentry *trc_enable; /* debugfs file object */ struct dentry *trc_file; diff --git a/drivers/uwb/uwb-debug.c b/drivers/uwb/uwb-debug.c index 0b1e5a9449b5..991374b13571 100644 --- a/drivers/uwb/uwb-debug.c +++ b/drivers/uwb/uwb-debug.c @@ -55,7 +55,7 @@ struct uwb_dbg { struct uwb_pal pal;
- u32 accept; + bool accept; struct list_head rsvs;
struct dentry *root_d; diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 6c55ade071c3..b70c20fae502 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -439,7 +439,7 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf, size_t count, loff_t *ppos) { char buf[3]; - u32 *val = file->private_data; + bool *val = file->private_data;
if (*val) buf[0] = 'Y'; @@ -457,7 +457,7 @@ ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf, char buf[32]; size_t buf_size; bool bv; - u32 *val = file->private_data; + bool *val = file->private_data;
buf_size = min(count, (sizeof(buf)-1)); if (copy_from_user(buf, user_buf, buf_size)) @@ -503,7 +503,7 @@ static const struct file_operations fops_bool = { * code. */ struct dentry *debugfs_create_bool(const char *name, umode_t mode, - struct dentry *parent, u32 *value) + struct dentry *parent, bool *value) { return debugfs_create_file(name, mode, parent, value, &fops_bool); } diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h index 9beb636b97eb..8321fe3058d6 100644 --- a/include/linux/debugfs.h +++ b/include/linux/debugfs.h @@ -92,7 +92,7 @@ struct dentry *debugfs_create_size_t(const char *name, umode_t mode, struct dentry *debugfs_create_atomic_t(const char *name, umode_t mode, struct dentry *parent, atomic_t *value); struct dentry *debugfs_create_bool(const char *name, umode_t mode, - struct dentry *parent, u32 *value); + struct dentry *parent, bool *value);
struct dentry *debugfs_create_blob(const char *name, umode_t mode, struct dentry *parent, @@ -243,7 +243,7 @@ static inline struct dentry *debugfs_create_atomic_t(const char *name, umode_t m
static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent, - u32 *value) + bool *value) { return ERR_PTR(-ENODEV); } diff --git a/include/linux/edac.h b/include/linux/edac.h index da3b72e95db3..7c6b7ba55589 100644 --- a/include/linux/edac.h +++ b/include/linux/edac.h @@ -772,7 +772,7 @@ struct mem_ctl_info { #ifdef CONFIG_EDAC_DEBUG struct dentry *debugfs; u8 fake_inject_layer[EDAC_MAX_LAYERS]; - u32 fake_inject_ue; + bool fake_inject_ue; u16 fake_inject_count; #endif }; diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h index 798fad9e420d..3159a7dba034 100644 --- a/include/linux/fault-inject.h +++ b/include/linux/fault-inject.h @@ -18,7 +18,7 @@ struct fault_attr { atomic_t times; atomic_t space; unsigned long verbose; - u32 task_filter; + bool task_filter; unsigned long stacktrace_depth; unsigned long require_start; unsigned long require_end; diff --git a/kernel/futex.c b/kernel/futex.c index 6e443efc65f4..395b967841b4 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -267,10 +267,10 @@ static struct futex_hash_bucket *futex_queues; static struct { struct fault_attr attr;
- u32 ignore_private; + bool ignore_private; } fail_futex = { .attr = FAULT_ATTR_INITIALIZER, - .ignore_private = 0, + .ignore_private = false, };
static int __init setup_fail_futex(char *str) diff --git a/lib/dma-debug.c b/lib/dma-debug.c index dace71fe41f7..fcb65d2a0b94 100644 --- a/lib/dma-debug.c +++ b/lib/dma-debug.c @@ -100,7 +100,7 @@ static LIST_HEAD(free_entries); static DEFINE_SPINLOCK(free_entries_lock);
/* Global disable flag - will be set in case of an error */ -static u32 global_disable __read_mostly; +static bool global_disable __read_mostly;
/* Early initialization disable flag, set at the end of dma_debug_init */ static bool dma_debug_initialized __read_mostly; diff --git a/mm/failslab.c b/mm/failslab.c index fefaabaab76d..98fb490311eb 100644 --- a/mm/failslab.c +++ b/mm/failslab.c @@ -3,12 +3,12 @@
static struct { struct fault_attr attr; - u32 ignore_gfp_wait; - int cache_filter; + bool ignore_gfp_wait; + bool cache_filter; } failslab = { .attr = FAULT_ATTR_INITIALIZER, - .ignore_gfp_wait = 1, - .cache_filter = 0, + .ignore_gfp_wait = true, + .cache_filter = false, };
bool should_failslab(size_t size, gfp_t gfpflags, unsigned long cache_flags) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 48aaf7b9f253..805bbad2e24e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2159,13 +2159,13 @@ struct page *buffered_rmqueue(struct zone *preferred_zone, static struct { struct fault_attr attr;
- u32 ignore_gfp_highmem; - u32 ignore_gfp_wait; + bool ignore_gfp_highmem; + bool ignore_gfp_wait; u32 min_order; } fail_page_alloc = { .attr = FAULT_ATTR_INITIALIZER, - .ignore_gfp_wait = 1, - .ignore_gfp_highmem = 1, + .ignore_gfp_wait = true, + .ignore_gfp_highmem = true, .min_order = 1, };
diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h index 579a6350fb01..2d117cf0e953 100644 --- a/sound/soc/codecs/wm_adsp.h +++ b/sound/soc/codecs/wm_adsp.h @@ -53,7 +53,7 @@ struct wm_adsp {
int fw; int fw_ver; - u32 running; + bool running;
struct list_head ctl_list;
On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V3->V4:
- Create a local variable instead of changing type of global_lock (Rafael)
Err, surely that wasn't what Rafael meant, since it's clearly impossible to use a pointer to the stack, assign to it once, and the expect anything to wkr at all ...
johannes
On 25-09-15, 19:42, Johannes Berg wrote:
On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V3->V4:
- Create a local variable instead of changing type of global_lock (Rafael)
Err, surely that wasn't what Rafael meant, since it's clearly impossible to use a pointer to the stack, assign to it once, and the expect anything to wkr at all ...
Sorry, I am not sure on what wouldn't work with this patch but this is what Rafael said, and I don't think I wrote it differently :)
Rafael wrote:
Actually, what about adding a local u32 variable, say val, here and doing
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) goto error; if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
(u32 *)&first_ec->global_lock))
&first_ec->global_lock))if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
goto error;first_ec->global_lock = val;
And then you can turn val into bool just fine without changing the structure definition.
Rafael wrote:
Actually, what about adding a local u32 variable, say val, here and doing
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) goto error; if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
(u32 *)&first_ec->global_lock))
&first_ec->global_lock))if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
goto error;first_ec->global_lock = val;
And then you can turn val into bool just fine without changing the structure definition.
Ok, then, but that means Rafael is completely wrong ... debugfs_create_bool() takes a *pointer* and it needs to be long-lived, it can't be on the stack. You also don't get a call when it changes.
If you cannot change the struct definition then you must implement a debugfs file with its own read/write handlers.
johannes
On 25-09-15, 20:49, Johannes Berg wrote:
Ok, then, but that means Rafael is completely wrong ... debugfs_create_bool() takes a *pointer* and it needs to be long-lived, it can't be on the stack. You also don't get a call when it changes.
Ahh, ofcourse. My bad as well...
I think we can change structure definition but will wait for Rafael's comment before that.
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote:
On 25-09-15, 20:49, Johannes Berg wrote:
Ok, then, but that means Rafael is completely wrong ... debugfs_create_bool() takes a *pointer* and it needs to be long-lived, it can't be on the stack. You also don't get a call when it changes.
Ahh, ofcourse. My bad as well...
Well, sorry about the wrong suggestion.
I think we can change structure definition but will wait for Rafael's comment before that.
OK, change the structure then.
Thanks, Rafael
On Friday, September 25, 2015 10:26:22 PM Rafael J. Wysocki wrote:
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote:
On 25-09-15, 20:49, Johannes Berg wrote:
Ok, then, but that means Rafael is completely wrong ... debugfs_create_bool() takes a *pointer* and it needs to be long-lived, it can't be on the stack. You also don't get a call when it changes.
Ahh, ofcourse. My bad as well...
Well, sorry about the wrong suggestion.
I think we can change structure definition but will wait for Rafael's comment before that.
OK, change the structure then.
But here's a question.
You're going to change that into bool in the next patch, right?
So what if bool is a byte and the field is not word-aligned and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case?
Thanks, Rafael
On 25 September 2015 at 13:33, Rafael J. Wysocki rjw@rjwysocki.net wrote:
You're going to change that into bool in the next patch, right?
Yeah.
So what if bool is a byte and the field is not word-aligned
Its between two 'unsigned long' variables today, and the struct isn't packed. So, it will be aligned, isn't it?
and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case?
I didn't understood why a read-modify-write is special here? That's what will happen to most of the non-word-sized fields anyway?
Probably I didn't understood what you meant..
-- viresh
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote:
On 25 September 2015 at 13:33, Rafael J. Wysocki rjw@rjwysocki.net wrote:
You're going to change that into bool in the next patch, right?
Yeah.
So what if bool is a byte and the field is not word-aligned
Its between two 'unsigned long' variables today, and the struct isn't packed. So, it will be aligned, isn't it?
and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case?
I didn't understood why a read-modify-write is special here? That's what will happen to most of the non-word-sized fields anyway?
Probably I didn't understood what you meant..
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
Thanks, Rafael
On 25-09-15, 22:58, Rafael J. Wysocki wrote:
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
But then two CPUs can update the same variable as well, and we must have proper locking in place to fix that.
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
On Fri, Sep 25, 2015 at 11:44 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 25-09-15, 22:58, Rafael J. Wysocki wrote:
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
But then two CPUs can update the same variable as well, and we must have proper locking in place to fix that.
Right.
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Is that not a problem in all of the places modified by the [2/2]?
How can the future users of the API know what to do to avoid potential problems with it?
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
Thanks, Rafael
On 25 September 2015 at 15:19, Rafael J. Wysocki rafael@kernel.org wrote:
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device?
Is that not a problem in all of the places modified by the [2/2]?
Right, but its not new AFAICT.
We already have u32 fields in those structs and on 64 bit machines we have the same read-update-write problems for two consecutive u32's. Right?
How can the future users of the API know what to do to avoid potential problems with it?
No idea really :)
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want.
-- viresh
On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote:
On 25 September 2015 at 15:19, Rafael J. Wysocki rafael@kernel.org wrote:
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device?
No, if you need any locking to access variable, you cannot use the simple debugfs helpers but have to provide your own functions.
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want.
It still makes sense to keep it separate I think, the patch is clearly different from the other parts.
Arnd
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote:
On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote:
On 25 September 2015 at 15:19, Rafael J. Wysocki rafael@kernel.org wrote:
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device?
No, if you need any locking to access variable, you cannot use the simple debugfs helpers but have to provide your own functions.
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want.
It still makes sense to keep it separate I think, the patch is clearly different from the other parts.
I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go.
Thanks, Rafael
On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote:
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote:
On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote:
On 25 September 2015 at 15:19, Rafael J. Wysocki rafael@kernel.org wrote:
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device?
No, if you need any locking to access variable, you cannot use the simple debugfs helpers but have to provide your own functions.
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want.
It still makes sense to keep it separate I think, the patch is clearly different from the other parts.
I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go.
It's only important to keep the 34-file multi-subsystem trivial cleanup that doesn't change any functionality separate from the bugfix. If you like to avoid patching one of the files twice, the alternative would be to first change the API for all other instances from u32 to bool and leave ACPI alone, and then do the second patch that changes ACPI from long to bool.
Arnd
On Monday, September 28, 2015 10:24:58 AM Arnd Bergmann wrote:
On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote:
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote:
On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote:
On 25 September 2015 at 15:19, Rafael J. Wysocki rafael@kernel.org wrote:
So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking?
Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device?
No, if you need any locking to access variable, you cannot use the simple debugfs helpers but have to provide your own functions.
Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here.
I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want.
It still makes sense to keep it separate I think, the patch is clearly different from the other parts.
I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go.
It's only important to keep the 34-file multi-subsystem trivial cleanup that doesn't change any functionality separate from the bugfix.
Which isn't a bugfix really, because the EC code is not run on any big-endian systems to my knowledge. And it won't matter after the [2/2] anyway.
And the changelog of it doesn't really makes sense, because it talks about future systems, but after the [2/2] no future systems will run that code in the first place.
If you like to avoid patching one of the files twice, the alternative would be to first change the API for all other instances from u32 to bool and leave ACPI alone, and then do the second patch that changes ACPI from long to bool.
My point is that this patch doesn't matter. It doesn't really fix anything and the result of it goes away after the second patch.
The only marginal value of having it as a separate commit is in case if (a) we need to revert the [2/2] for some reason and (b) ACPI-based ARM systems (the big-endian ones) become full-hardware at one point. You know what the chances of that are, though. :-)
That said I've ACKed the patch, because I don't care that much. I'm not exactly sure why you care either.
Thanks, Rafael
On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote:
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote:
On 25 September 2015 at 13:33, Rafael J. Wysocki rjw@rjwysocki.net wrote:
You're going to change that into bool in the next patch, right?
Yeah.
So what if bool is a byte and the field is not word-aligned
Its between two 'unsigned long' variables today, and the struct isn't packed. So, it will be aligned, isn't it?
and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case?
I didn't understood why a read-modify-write is special here? That's what will happen to most of the non-word-sized fields anyway?
Probably I didn't understood what you meant..
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
James
On Saturday, September 26, 2015 12:52:08 PM James Bottomley wrote:
On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote:
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote:
On 25 September 2015 at 13:33, Rafael J. Wysocki rjw@rjwysocki.net wrote:
You're going to change that into bool in the next patch, right?
Yeah.
So what if bool is a byte and the field is not word-aligned
Its between two 'unsigned long' variables today, and the struct isn't packed. So, it will be aligned, isn't it?
and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case?
I didn't understood why a read-modify-write is special here? That's what will happen to most of the non-word-sized fields anyway?
Probably I didn't understood what you meant..
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
OK, thanks!
Rafael
From: Rafael J. Wysocki
Sent: 27 September 2015 15:09
...
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
Does linux still support those old Alphas?
The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations.
OK, thanks!
You still have to ensure the compiler doesn't do wider rmw cycles. I believe the recent versions of gcc won't do wider accesses for volatile data.
David
On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote:
From: Rafael J. Wysocki
Sent: 27 September 2015 15:09
...
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
Does linux still support those old Alphas?
The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations.
That's different: it's an atomic RMW operation. The problem with the alpha was that the operation wasn't atomic (meaning that it can't be interrupted and no intermediate output states are visible).
OK, thanks!
You still have to ensure the compiler doesn't do wider rmw cycles. I believe the recent versions of gcc won't do wider accesses for volatile data.
I don't understand this comment. You seem to be implying gcc would do a 64 bit RMW for a 32 bit store ... that would be daft when a single instruction exists to perform the operation on all architectures.
James
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
Sent: 28 September 2015 15:27 On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote:
From: Rafael J. Wysocki
Sent: 27 September 2015 15:09
...
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
Does linux still support those old Alphas?
The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations.
That's different: it's an atomic RMW operation. The problem with the alpha was that the operation wasn't atomic (meaning that it can't be interrupted and no intermediate output states are visible).
It is only atomic if prefixed by the 'lock' prefix. Normally the read and write are separate bus cycles.
You still have to ensure the compiler doesn't do wider rmw cycles. I believe the recent versions of gcc won't do wider accesses for volatile data.
I don't understand this comment. You seem to be implying gcc would do a 64 bit RMW for a 32 bit store ... that would be daft when a single instruction exists to perform the operation on all architectures.
Read the object code and weep... It is most likely to happen for operations that are rmw (eg bit set). For instance the arm cpu has limited offsets for 16bit accesses, for normal structures the compiler is likely to use a 32bit rmw sequence for a 16bit field that has a large offset. The C language allows the compiler to do it for any access (IIRC including volatiles).
David
On Mon, 2015-09-28 at 14:50 +0000, David Laight wrote:
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
Sent: 28 September 2015 15:27 On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote:
From: Rafael J. Wysocki
Sent: 27 September 2015 15:09
...
Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0.
CPU A writes 1 to x and CPU B writes 2 to y at the same time.
What's the result?
I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions.
Does linux still support those old Alphas?
The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations.
That's different: it's an atomic RMW operation. The problem with the alpha was that the operation wasn't atomic (meaning that it can't be interrupted and no intermediate output states are visible).
It is only atomic if prefixed by the 'lock' prefix. Normally the read and write are separate bus cycles.
The essential point is that x86 has atomic bit ops and byte writes. Early alpha did not.
You still have to ensure the compiler doesn't do wider rmw cycles. I believe the recent versions of gcc won't do wider accesses for volatile data.
I don't understand this comment. You seem to be implying gcc would do a 64 bit RMW for a 32 bit store ... that would be daft when a single instruction exists to perform the operation on all architectures.
Read the object code and weep... It is most likely to happen for operations that are rmw (eg bit set). For instance the arm cpu has limited offsets for 16bit accesses, for normal structures the compiler is likely to use a 32bit rmw sequence for a 16bit field that has a large offset. The C language allows the compiler to do it for any access (IIRC including volatiles).
I think you might be confusing different things. Most RISC CPUs can't do 32 bit store immediates because there aren't enough bits in their arsenal, so they tend to split 32 bit loads into a left and right part (first the top then the offset). This (and other things) are mostly what you see in code. However, 32 bit register stores are still atomic, which is all we require. It's not really the compiler's fault, it's mostly an architectural limitation.
James
From: James Bottomley
Sent: 28 September 2015 16:12
The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations.
That's different: it's an atomic RMW operation. The problem with the alpha was that the operation wasn't atomic (meaning that it can't be interrupted and no intermediate output states are visible).
It is only atomic if prefixed by the 'lock' prefix. Normally the read and write are separate bus cycles.
The essential point is that x86 has atomic bit ops and byte writes. Early alpha did not.
Early alpha didn't have any byte accesses.
On x86 if you have the following: struct { char a; volatile char b; } *foo; foo->a |= 4;
The compiler is likely to generate a 'bis #4, 0(rbx)' (or similar) and the cpu will do two 32bit memory cycles that read and write the 'volatile' field 'b'. (gcc definitely used to do this...)
A lot of fields were made 32bit (and probably not bitfields) in the linux kernel tree a year or two ago to avoid this very problem.
You still have to ensure the compiler doesn't do wider rmw cycles. I believe the recent versions of gcc won't do wider accesses for volatile data.
I don't understand this comment. You seem to be implying gcc would do a 64 bit RMW for a 32 bit store ... that would be daft when a single instruction exists to perform the operation on all architectures.
Read the object code and weep... It is most likely to happen for operations that are rmw (eg bit set). For instance the arm cpu has limited offsets for 16bit accesses, for normal structures the compiler is likely to use a 32bit rmw sequence for a 16bit field that has a large offset. The C language allows the compiler to do it for any access (IIRC including volatiles).
I think you might be confusing different things. Most RISC CPUs can't do 32 bit store immediates because there aren't enough bits in their arsenal, so they tend to split 32 bit loads into a left and right part (first the top then the offset). This (and other things) are mostly what you see in code. However, 32 bit register stores are still atomic, which is all we require. It's not really the compiler's fault, it's mostly an architectural limitation.
No, I'm not talking about how 32bit constants are generated. I'm talking about structure offsets.
David
On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
global_lock is defined as an unsigned long and accessing only its lower 32 bits from sysfs is incorrect, as we need to consider other 32 bits for big endian 64 bit systems. There are no such platforms yet, but the code needs to be robust for such a case.
Fix that by passing a local variable to debugfs_create_bool() and assigning its value to global_lock later.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com
Greg, please take this one if the [2/2] looks good to you.
V3->V4:
- Create a local variable instead of changing type of global_lock (Rafael)
- Drop the stable tag
- BCC'd a lot of people (rather than cc'ing them) to make sure
- the series reaches them
- mailing lists do not block the patchset due to long cc list
- and we don't spam the BCC'd people for every reply
drivers/acpi/ec_sys.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c index b4c216bab22b..b44b91331a56 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) struct dentry *dev_dir; char name[64]; umode_t mode = 0400;
u32 val;
if (ec_device_count == 0) { acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
@@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) goto error;
- if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
(u32 *)&first_ec->global_lock))
if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) goto error;
first_ec->global_lock = val;
if (write_support) mode = 0600; if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
Thanks, Rafael
On Friday, September 25, 2015 10:18:13 PM Rafael J. Wysocki wrote:
On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
global_lock is defined as an unsigned long and accessing only its lower 32 bits from sysfs is incorrect, as we need to consider other 32 bits for big endian 64 bit systems. There are no such platforms yet, but the code needs to be robust for such a case.
Fix that by passing a local variable to debugfs_create_bool() and assigning its value to global_lock later.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com
Greg, please take this one if the [2/2] looks good to you.
Ouch, turns out it was a bad idea. Please scratch that.
Thanks, Rafael
Dne 25. 9. 2015 18:42 napsal uživatel "Viresh Kumar" < viresh.kumar@linaro.org>:
global_lock is defined as an unsigned long and accessing only its lower 32 bits from sysfs is incorrect, as we need to consider other 32 bits for big endian 64 bit systems. There are no such platforms yet, but the code needs to be robust for such a case.
Fix that by passing a local variable to debugfs_create_bool() and assigning its value to global_lock later.
But this has to crash whenever the file is read as val's storage is gone at that moment already, right?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V3->V4:
- Create a local variable instead of changing type of global_lock (Rafael)
- Drop the stable tag
- BCC'd a lot of people (rather than cc'ing them) to make sure
- the series reaches them
- mailing lists do not block the patchset due to long cc list
- and we don't spam the BCC'd people for every reply
drivers/acpi/ec_sys.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c index b4c216bab22b..b44b91331a56 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
unsigned int ec_device_count)
struct dentry *dev_dir; char name[64]; umode_t mode = 0400;
u32 val; if (ec_device_count == 0) { acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);@@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
unsigned int ec_device_count)
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32
*)&first_ec->gpe))
goto error;
if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,(u32 *)&first_ec->global_lock))
if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) goto error;first_ec->global_lock = val;if (write_support) mode = 0600; if (!debugfs_create_file("io", mode, dev_dir, ec,
&acpi_ec_io_ops))
-- 2.4.0
On 26 September 2015 at 22:31, Jiri Slaby jirislaby@gmail.com wrote:
But this has to crash whenever the file is read as val's storage is gone at that moment already, right?
Yeah, its fixed now in the new version. This was a *really* bad idea :(
participants (8)
-
Arnd Bergmann -
David Laight -
James Bottomley -
Jiri Slaby -
Johannes Berg -
Rafael J. Wysocki -
Rafael J. Wysocki -
Viresh Kumar