* [RESEND PATCH v3 6/6] Documentation: charger: max14577: Document exported sysfs entry
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse, linux-kernel, linux-api
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski@samsung.com>
Document the 'fast charge timer' setting exported by max14577 driver
through sysfs entry.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 78c7baca3587..83ee67ebf0e9 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -18,3 +18,17 @@ Description:
This file is writeable and can be used to set the assumed
battery 'full level'. As batteries age, this value has to be
amended over time.
+
+What: /sys/class/power_supply/max14577-charger/device/fast_charge_timer
+Date: July 2014
+KernelVersion: 3.17.0
+Contact: Krzysztof Kozlowski <k.kozlowski@samsung.com>
+Description:
+ This entry shows and sets the maximum time the max14577
+ charger operates in fast-charge mode. When the timer expires
+ the device will terminate fast-charge mode (charging current
+ will drop to 0 A) and will trigger interrupt.
+
+ Valid values:
+ - 5, 6 or 7 (hours),
+ - 0: disabled.
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 5/6] devicetree: mfd: max14577: Add device tree bindings document
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Tomasz Figa, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Add document describing device tree bindings for MAX14577 MFD
drivers: MFD core, extcon, regulator and charger.
Both MAX14577 and MAX77836 chipsets are documented.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Reviewed-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
Documentation/devicetree/bindings/mfd/max14577.txt | 146 +++++++++++++++++++++
1 file changed, 146 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/max14577.txt
diff --git a/Documentation/devicetree/bindings/mfd/max14577.txt b/Documentation/devicetree/bindings/mfd/max14577.txt
new file mode 100644
index 000000000000..236264c10b92
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max14577.txt
@@ -0,0 +1,146 @@
+Maxim MAX14577/77836 Multi-Function Device
+
+MAX14577 is a Multi-Function Device with Micro-USB Interface Circuit, Li+
+Battery Charger and SFOUT LDO output for powering USB devices. It is
+interfaced to host controller using I2C.
+
+MAX77836 additionally contains PMIC (with two LDO regulators) and Fuel Gauge.
+
+
+Required properties:
+- compatible : Must be "maxim,max14577" or "maxim,max77836".
+- reg : I2C slave address for the max14577 chip (0x25 for max14577/max77836)
+- interrupts : IRQ line for the chip.
+- interrupt-parent : The parent interrupt controller.
+
+
+Required nodes:
+ - charger :
+ Node for configuring the charger driver.
+ Required properties:
+ - compatible : "maxim,max14577-charger"
+ or "maxim,max77836-charger"
+ - maxim,fast-charge-uamp : Current in uA for Fast Charge;
+ Valid values:
+ - for max14577: 90000 - 950000;
+ - for max77836: 45000 - 475000;
+ - maxim,eoc-uamp : Current in uA for End-Of-Charge mode;
+ Valid values:
+ - for max14577: 50000 - 200000;
+ - for max77836: 5000 - 100000;
+ - maxim,ovp-uvolt : OverVoltage Protection Threshold in uV;
+ In an overvoltage condition, INT asserts and charging
+ stops. Valid values:
+ - 6000000, 6500000, 7000000, 7500000;
+ - maxim,constant-uvolt : Battery Constant Voltage in uV;
+ Valid values:
+ - 4000000 - 4280000 (step by 20000);
+ - 4350000;
+
+
+Optional nodes:
+- max14577-muic/max77836-muic :
+ Node used only by extcon consumers.
+ Required properties:
+ - compatible : "maxim,max14577-muic" or "maxim,max77836-muic"
+
+- regulators :
+ Required properties:
+ - compatible : "maxim,max14577-regulator"
+ or "maxim,max77836-regulator"
+
+ May contain a sub-node per regulator from the list below. Each
+ sub-node should contain the constraints and initialization information
+ for that regulator. See regulator.txt for a description of standard
+ properties for these sub-nodes.
+
+ List of valid regulator names:
+ - for max14577: CHARGER, SAFEOUT.
+ - for max77836: CHARGER, SAFEOUT, LDO1, LDO2.
+
+ The SAFEOUT is a fixed voltage regulator so there is no need to specify
+ voltages for it.
+
+
+Example:
+
+#include <dt-bindings/interrupt-controller/irq.h>
+
+max14577@25 {
+ compatible = "maxim,max14577";
+ reg = <0x25>;
+ interrupt-parent = <&gpx1>;
+ interrupts = <5 IRQ_TYPE_NONE>;
+
+ muic: max14577-muic {
+ compatible = "maxim,max14577-muic";
+ };
+
+ regulators {
+ compatible = "maxim,max14577-regulator";
+
+ SAFEOUT {
+ regulator-name = "SAFEOUT";
+ };
+ CHARGER {
+ regulator-name = "CHARGER";
+ regulator-min-microamp = <90000>;
+ regulator-max-microamp = <950000>;
+ regulator-boot-on;
+ };
+ };
+
+ charger {
+ compatible = "maxim,max14577-charger";
+
+ maxim,constant-uvolt = <4350000>;
+ maxim,fast-charge-uamp = <450000>;
+ maxim,eoc-uamp = <50000>;
+ maxim,ovp-uvolt = <6500000>;
+ };
+};
+
+
+max77836@25 {
+ compatible = "maxim,max77836";
+ reg = <0x25>;
+ interrupt-parent = <&gpx1>;
+ interrupts = <5 IRQ_TYPE_NONE>;
+
+ muic: max77836-muic {
+ compatible = "maxim,max77836-muic";
+ };
+
+ regulators {
+ compatible = "maxim,max77836-regulator";
+
+ SAFEOUT {
+ regulator-name = "SAFEOUT";
+ };
+ CHARGER {
+ regulator-name = "CHARGER";
+ regulator-min-microamp = <90000>;
+ regulator-max-microamp = <950000>;
+ regulator-boot-on;
+ };
+ LDO1 {
+ regulator-name = "LDO1";
+ regulator-min-microvolt = <2700000>;
+ regulator-max-microvolt = <2700000>;
+ };
+ LDO2 {
+ regulator-name = "LDO2";
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <3950000>;
+ };
+ };
+
+ charger {
+ compatible = "maxim,max77836-charger";
+
+ maxim,constant-uvolt = <4350000>;
+ maxim,fast-charge-uamp = <225000>;
+ maxim,eoc-uamp = <7500>;
+ maxim,ovp-uvolt = <6500000>;
+ };
+};
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 4/6] power: max17040: Add ID for MAX77836 Fuel Gauge block
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Anton Vorontsov
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
MAX77836 has the same Fuel Gauge as MAX17040/17048. The max17040 driver
can be safely re-used. The patch adds MAX77836 ID to array of
i2c_device_id.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
Cc: Dmitry Eremin-Solenikov <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
---
drivers/power/max17040_battery.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/power/max17040_battery.c b/drivers/power/max17040_battery.c
index 0fbac861080d..165ffe381803 100644
--- a/drivers/power/max17040_battery.c
+++ b/drivers/power/max17040_battery.c
@@ -278,6 +278,7 @@ static SIMPLE_DEV_PM_OPS(max17040_pm_ops, max17040_suspend, max17040_resume);
static const struct i2c_device_id max17040_id[] = {
{ "max17040", 0 },
+ { "max77836-battery", 0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, max17040_id);
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 3/6] charger: max14577: Configure battery-dependent settings from DTS and sysfs
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse, linux-kernel, linux-api
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Jenny Tc
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski@samsung.com>
Remove hard-coded values for:
- Fast Charge current,
- End Of Charge current,
- Fast Charge timer,
- Overvoltage Protection Threshold,
- Battery Constant Voltage,
and use DTS or sysfs to configure them. This allows using the max14577 charger
driver with different batteries.
Now the charger driver requires valid configuration data from DTS. In
case of wrong configuration data it fails during probe. Patch adds
of_compatible to the charger mfd cell in MFD driver core.
The fast charge timer is configured through sysfs entry.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Jenny Tc <jenny.tc@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
drivers/mfd/max14577.c | 5 +-
drivers/power/Kconfig | 1 +
drivers/power/max14577_charger.c | 311 +++++++++++++++++++++++++++++++----
include/linux/mfd/max14577-private.h | 19 +++
include/linux/mfd/max14577.h | 7 +
5 files changed, 314 insertions(+), 29 deletions(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c
index e6f25aa0ded8..b8af263be594 100644
--- a/drivers/mfd/max14577.c
+++ b/drivers/mfd/max14577.c
@@ -116,7 +116,10 @@ static const struct mfd_cell max14577_devs[] = {
.name = "max14577-regulator",
.of_compatible = "maxim,max14577-regulator",
},
- { .name = "max14577-charger", },
+ {
+ .name = "max14577-charger",
+ .of_compatible = "maxim,max14577-charger",
+ },
};
static const struct mfd_cell max77836_devs[] = {
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 94086a5238c6..6c152f494c4a 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -320,6 +320,7 @@ config CHARGER_MANAGER
config CHARGER_MAX14577
tristate "Maxim MAX14577/77836 battery charger driver"
depends on MFD_MAX14577
+ select SYSFS
help
Say Y to enable support for the battery charger control sysfs and
platform data of MAX14577/77836 MUICs.
diff --git a/drivers/power/max14577_charger.c b/drivers/power/max14577_charger.c
index 19c8f42abf24..c125756eab69 100644
--- a/drivers/power/max14577_charger.c
+++ b/drivers/power/max14577_charger.c
@@ -19,6 +19,7 @@
#include <linux/platform_device.h>
#include <linux/power_supply.h>
#include <linux/mfd/max14577-private.h>
+#include <linux/mfd/max14577.h>
struct max14577_charger {
struct device *dev;
@@ -27,6 +28,8 @@ struct max14577_charger {
unsigned int charging_state;
unsigned int battery_state;
+
+ struct max14577_charger_platform_data *pdata;
};
/*
@@ -178,15 +181,131 @@ static int max14577_get_present(struct max14577_charger *chg)
return 1;
}
+static inline int max14577_set_fast_charge_timer(struct max14577_charger *chg,
+ unsigned long hours)
+{
+ u8 reg_data;
+
+ switch (hours) {
+ case 5 ... 7:
+ reg_data = hours - 3;
+ break;
+ case 0:
+ /* Disable */
+ reg_data = 0x7;
+ break;
+ default:
+ dev_err(chg->dev, "Wrong value for Fast-Charge Timer: %lu\n",
+ hours);
+ return -EINVAL;
+ }
+ reg_data <<= CHGCTRL1_TCHW_SHIFT;
+
+ return max14577_update_reg(chg->max14577->regmap,
+ MAX14577_REG_CHGCTRL1, CHGCTRL1_TCHW_MASK, reg_data);
+}
+
+static inline int max14577_init_constant_voltage(struct max14577_charger *chg,
+ unsigned int uvolt)
+{
+ u8 reg_data;
+
+ if (uvolt < MAXIM_CHARGER_CONSTANT_VOLTAGE_MIN ||
+ uvolt > MAXIM_CHARGER_CONSTANT_VOLTAGE_MAX)
+ return -EINVAL;
+
+ if (uvolt == 4200000)
+ reg_data = 0x0;
+ else if (uvolt == MAXIM_CHARGER_CONSTANT_VOLTAGE_MAX)
+ reg_data = 0x1f;
+ else if (uvolt <= 4280000) {
+ unsigned int val = uvolt;
+
+ val -= MAXIM_CHARGER_CONSTANT_VOLTAGE_MIN;
+ val /= MAXIM_CHARGER_CONSTANT_VOLTAGE_STEP;
+ if (uvolt <= 4180000)
+ reg_data = 0x1 + val;
+ else
+ reg_data = val; /* Fix for gap between 4.18V and 4.22V */
+ } else
+ return -EINVAL;
+
+ reg_data <<= CHGCTRL3_MBCCVWRC_SHIFT;
+
+ return max14577_write_reg(chg->max14577->regmap,
+ MAX14577_CHG_REG_CHG_CTRL3, reg_data);
+}
+
+static inline int max14577_init_eoc(struct max14577_charger *chg,
+ unsigned int uamp)
+{
+ unsigned int current_bits = 0xf;
+ u8 reg_data;
+
+ switch (chg->max14577->dev_type) {
+ case MAXIM_DEVICE_TYPE_MAX77836:
+ if (uamp < 5000)
+ return -EINVAL; /* Requested current is too low */
+
+ if (uamp >= 7500 && uamp < 10000)
+ current_bits = 0x0;
+ else if (uamp <= 50000) {
+ /* <5000, 7499> and <10000, 50000> */
+ current_bits = uamp / 5000;
+ } else {
+ uamp = min(uamp, 100000U) - 50000U;
+ current_bits = 0xa + uamp / 10000;
+ }
+ break;
+
+ case MAXIM_DEVICE_TYPE_MAX14577:
+ default:
+ if (uamp < MAX14577_CHARGER_EOC_CURRENT_LIMIT_MIN)
+ return -EINVAL; /* Requested current is too low */
+
+ uamp = min(uamp, MAX14577_CHARGER_EOC_CURRENT_LIMIT_MAX);
+ uamp -= MAX14577_CHARGER_EOC_CURRENT_LIMIT_MIN;
+ current_bits = uamp / MAX14577_CHARGER_EOC_CURRENT_LIMIT_STEP;
+ break;
+ }
+
+ reg_data = current_bits << CHGCTRL5_EOCS_SHIFT;
+
+ return max14577_update_reg(chg->max14577->regmap,
+ MAX14577_CHG_REG_CHG_CTRL5, CHGCTRL5_EOCS_MASK,
+ reg_data);
+}
+
+static inline int max14577_init_fast_charge(struct max14577_charger *chg,
+ unsigned int uamp)
+{
+ u8 reg_data;
+ int ret;
+ const struct maxim_charger_current *limits =
+ &maxim_charger_currents[chg->max14577->dev_type];
+
+ ret = maxim_charger_calc_reg_current(limits, uamp, uamp, ®_data);
+ if (ret) {
+ dev_err(chg->dev, "Wrong value for fast charge: %u\n", uamp);
+ return ret;
+ }
+
+ return max14577_update_reg(chg->max14577->regmap,
+ MAX14577_CHG_REG_CHG_CTRL4,
+ CHGCTRL4_MBCICHWRCL_MASK | CHGCTRL4_MBCICHWRCH_MASK,
+ reg_data);
+}
+
/*
* Sets charger registers to proper and safe default values.
* Some of these values are equal to defaults in MAX14577E
* data sheet but there are minor differences.
*/
-static void max14577_charger_reg_init(struct max14577_charger *chg)
+static int max14577_charger_reg_init(struct max14577_charger *chg)
{
struct regmap *rmap = chg->max14577->regmap;
u8 reg_data;
+ int ret;
/*
* Charger-Type Manual Detection, default off (set CHGTYPMAN to 0)
@@ -198,10 +317,6 @@ static void max14577_charger_reg_init(struct max14577_charger *chg)
CDETCTRL1_CHGDETEN_MASK | CDETCTRL1_CHGTYPMAN_MASK,
reg_data);
- /* Battery Fast-Charge Timer, set to: 6hrs */
- reg_data = 0x3 << CHGCTRL1_TCHW_SHIFT;
- max14577_write_reg(rmap, MAX14577_REG_CHGCTRL1, reg_data);
-
/*
* Wall-Adapter Rapid Charge, default on
* Battery-Charger, default on
@@ -210,32 +325,46 @@ static void max14577_charger_reg_init(struct max14577_charger *chg)
reg_data |= 0x1 << CHGCTRL2_MBCHOSTEN_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL2, reg_data);
- /* Battery-Charger Constant Voltage (CV) Mode, set to: 4.35V */
- reg_data = 0xf << CHGCTRL3_MBCCVWRC_SHIFT;
- max14577_write_reg(rmap, MAX14577_REG_CHGCTRL3, reg_data);
-
- /*
- * Fast Battery-Charge Current Low,
- * default 200-950mA (max14577) / 100-475mA (max77836)
- *
- * Fast Battery-Charge Current High,
- * set to 450mA (max14577) / 225mA (max77836)
- */
- reg_data = 0x1 << CHGCTRL4_MBCICHWRCL_SHIFT;
- reg_data |= 0x5 << CHGCTRL4_MBCICHWRCH_SHIFT;
- max14577_write_reg(rmap, MAX14577_REG_CHGCTRL4, reg_data);
-
- /* End-of-Charge Current, set to 50mA (max14577) / 7.5mA (max77836) */
- reg_data = 0x0 << CHGCTRL5_EOCS_SHIFT;
- max14577_write_reg(rmap, MAX14577_REG_CHGCTRL5, reg_data);
-
/* Auto Charging Stop, default off */
reg_data = 0x0 << CHGCTRL6_AUTOSTOP_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL6, reg_data);
- /* Overvoltage-Protection Threshold, set to 6.5V */
- reg_data = 0x2 << CHGCTRL7_OTPCGHCVS_SHIFT;
+ ret = max14577_init_constant_voltage(chg, chg->pdata->constant_uvolt);
+ if (ret)
+ return ret;
+
+ ret = max14577_init_eoc(chg, chg->pdata->eoc_uamp);
+ if (ret)
+ return ret;
+
+ ret = max14577_init_fast_charge(chg, chg->pdata->fast_charge_uamp);
+ if (ret)
+ return ret;
+
+ ret = max14577_set_fast_charge_timer(chg,
+ MAXIM_CHARGER_FAST_CHARGE_TIMER_DEFAULT);
+ if (ret)
+ return ret;
+
+ /* Initialize Overvoltage-Protection Threshold */
+ switch (chg->pdata->ovp_uvolt) {
+ case 7500000:
+ reg_data = 0x0;
+ break;
+ case 6000000:
+ case 6500000:
+ case 7000000:
+ reg_data = 0x1 + (chg->pdata->ovp_uvolt - 6000000) / 500000;
+ break;
+ default:
+ dev_err(chg->dev, "Wrong value for OVP: %u\n",
+ chg->pdata->ovp_uvolt);
+ return -EINVAL;
+ }
+ reg_data <<= CHGCTRL7_OTPCGHCVS_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL7, reg_data);
+
+ return 0;
}
/* Support property from charger */
@@ -295,6 +424,110 @@ static int max14577_charger_get_property(struct power_supply *psy,
return ret;
}
+#ifdef CONFIG_OF
+static struct max14577_charger_platform_data *max14577_charger_dt_init(
+ struct platform_device *pdev)
+{
+ struct max14577_charger_platform_data *pdata;
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ if (!np) {
+ dev_err(&pdev->dev, "No charger OF node\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ ret = of_property_read_u32(np, "maxim,constant-uvolt",
+ &pdata->constant_uvolt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot parse maxim,constant-uvolt field from DT\n");
+ return ERR_PTR(ret);
+ }
+
+ ret = of_property_read_u32(np, "maxim,fast-charge-uamp",
+ &pdata->fast_charge_uamp);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot parse maxim,fast-charge-uamp field from DT\n");
+ return ERR_PTR(ret);
+ }
+
+ ret = of_property_read_u32(np, "maxim,eoc-uamp", &pdata->eoc_uamp);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot parse maxim,eoc-uamp field from DT\n");
+ return ERR_PTR(ret);
+ }
+
+ ret = of_property_read_u32(np, "maxim,ovp-uvolt", &pdata->ovp_uvolt);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot parse maxim,ovp-uvolt field from DT\n");
+ return ERR_PTR(ret);
+ }
+
+ return pdata;
+}
+#else /* CONFIG_OF */
+static struct max14577_charger_platform_data *max14577_charger_dt_init(
+ struct platform_device *pdev)
+{
+ return NULL;
+}
+#endif /* CONFIG_OF */
+
+static ssize_t show_fast_charge_timer(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct max14577_charger *chg = dev_get_drvdata(dev);
+ u8 reg_data;
+ int ret;
+ unsigned int val;
+
+ ret = max14577_read_reg(chg->max14577->regmap, MAX14577_REG_CHGCTRL1,
+ ®_data);
+ if (ret)
+ return ret;
+
+ reg_data &= CHGCTRL1_TCHW_MASK;
+ reg_data >>= CHGCTRL1_TCHW_SHIFT;
+ switch (reg_data) {
+ case 0x2 ... 0x4:
+ val = reg_data + 3;
+ break;
+ case 0x7:
+ val = 0;
+ break;
+ default:
+ val = 5;
+ break;
+ }
+
+ return scnprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t store_fast_charge_timer(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ struct max14577_charger *chg = dev_get_drvdata(dev);
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ ret = max14577_set_fast_charge_timer(chg, val);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static DEVICE_ATTR(fast_charge_timer, S_IRUGO | S_IWUSR,
+ show_fast_charge_timer, store_fast_charge_timer);
+
static int max14577_charger_probe(struct platform_device *pdev)
{
struct max14577_charger *chg;
@@ -309,7 +542,13 @@ static int max14577_charger_probe(struct platform_device *pdev)
chg->dev = &pdev->dev;
chg->max14577 = max14577;
- max14577_charger_reg_init(chg);
+ chg->pdata = max14577_charger_dt_init(pdev);
+ if (IS_ERR_OR_NULL(chg->pdata))
+ return PTR_ERR(chg->pdata);
+
+ ret = max14577_charger_reg_init(chg);
+ if (ret)
+ return ret;
chg->charger.name = "max14577-charger",
chg->charger.type = POWER_SUPPLY_TYPE_BATTERY,
@@ -317,19 +556,35 @@ static int max14577_charger_probe(struct platform_device *pdev)
chg->charger.num_properties = ARRAY_SIZE(max14577_charger_props),
chg->charger.get_property = max14577_charger_get_property,
+ ret = device_create_file(&pdev->dev, &dev_attr_fast_charge_timer);
+ if (ret) {
+ dev_err(&pdev->dev, "failed: create sysfs entry\n");
+ return ret;
+ }
+
ret = power_supply_register(&pdev->dev, &chg->charger);
if (ret) {
dev_err(&pdev->dev, "failed: power supply register\n");
- return ret;
+ goto err;
}
+ /* Check for valid values for charger */
+ BUILD_BUG_ON(MAX14577_CHARGER_EOC_CURRENT_LIMIT_MIN +
+ MAX14577_CHARGER_EOC_CURRENT_LIMIT_STEP * 0xf !=
+ MAX14577_CHARGER_EOC_CURRENT_LIMIT_MAX);
return 0;
+
+err:
+ device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
+
+ return ret;
}
static int max14577_charger_remove(struct platform_device *pdev)
{
struct max14577_charger *chg = platform_get_drvdata(pdev);
+ device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
power_supply_unregister(&chg->charger);
return 0;
diff --git a/include/linux/mfd/max14577-private.h b/include/linux/mfd/max14577-private.h
index 7d514839c764..f01c1fae4d84 100644
--- a/include/linux/mfd/max14577-private.h
+++ b/include/linux/mfd/max14577-private.h
@@ -293,6 +293,25 @@ enum max14577_charger_reg {
#define MAX77836_CHARGER_CURRENT_LIMIT_HIGH_STEP 25000U
#define MAX77836_CHARGER_CURRENT_LIMIT_MAX 475000U
+/*
+ * MAX14577 charger End-Of-Charge current limits
+ * (as in CHGCTRL5 register), uA
+ */
+#define MAX14577_CHARGER_EOC_CURRENT_LIMIT_MIN 50000U
+#define MAX14577_CHARGER_EOC_CURRENT_LIMIT_STEP 10000U
+#define MAX14577_CHARGER_EOC_CURRENT_LIMIT_MAX 200000U
+
+/*
+ * MAX14577/MAX77836 Battery Constant Voltage
+ * (as in CHGCTRL3 register), uV
+ */
+#define MAXIM_CHARGER_CONSTANT_VOLTAGE_MIN 4000000U
+#define MAXIM_CHARGER_CONSTANT_VOLTAGE_STEP 20000U
+#define MAXIM_CHARGER_CONSTANT_VOLTAGE_MAX 4350000U
+
+/* Default value for fast charge timer, in hours */
+#define MAXIM_CHARGER_FAST_CHARGE_TIMER_DEFAULT 5
+
/* MAX14577 regulator SFOUT LDO voltage, fixed, uV */
#define MAX14577_REGULATOR_SAFEOUT_VOLTAGE 4900000
diff --git a/include/linux/mfd/max14577.h b/include/linux/mfd/max14577.h
index 3c098d57b1d1..ccfaf952c31b 100644
--- a/include/linux/mfd/max14577.h
+++ b/include/linux/mfd/max14577.h
@@ -54,6 +54,13 @@ struct max14577_regulator_platform_data {
struct device_node *of_node;
};
+struct max14577_charger_platform_data {
+ u32 constant_uvolt;
+ u32 fast_charge_uamp;
+ u32 eoc_uamp;
+ u32 ovp_uvolt;
+};
+
/*
* MAX14577 MFD platform data
*/
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 2/6] regulator/mfd: max14577: Export symbols for calculating charger current
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse, linux-kernel, linux-api
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski@samsung.com>
This patch prepares for changing the max14577 charger driver to allow
configuring battery-dependent settings from DTS.
The patch moves from regulator driver to MFD core driver and exports:
- function for calculating register value for charger's current;
- table of limits for chargers (MAX14577, MAX77836).
Previously they were used only by the max14577 regulator driver. In next
patch the charger driver will use them as well. Exporting them will
reduce unnecessary code duplication.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Mark Brown <broonie@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
drivers/mfd/max14577.c | 95 ++++++++++++++++++++++++++++++++++++
drivers/regulator/max14577.c | 80 ++----------------------------
include/linux/mfd/max14577-private.h | 22 ++++-----
include/linux/mfd/max14577.h | 23 +++++++++
4 files changed, 133 insertions(+), 87 deletions(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c
index 4a5e885383f8..e6f25aa0ded8 100644
--- a/drivers/mfd/max14577.c
+++ b/drivers/mfd/max14577.c
@@ -26,6 +26,87 @@
#include <linux/mfd/max14577.h>
#include <linux/mfd/max14577-private.h>
+/*
+ * Table of valid charger currents for different Maxim chipsets.
+ * It is placed here because it is used by both charger and regulator driver.
+ */
+const struct maxim_charger_current maxim_charger_currents[] = {
+ [MAXIM_DEVICE_TYPE_UNKNOWN] = { 0, 0, 0, 0 },
+ [MAXIM_DEVICE_TYPE_MAX14577] = {
+ .min = MAX14577_CHARGER_CURRENT_LIMIT_MIN,
+ .high_start = MAX14577_CHARGER_CURRENT_LIMIT_HIGH_START,
+ .high_step = MAX14577_CHARGER_CURRENT_LIMIT_HIGH_STEP,
+ .max = MAX14577_CHARGER_CURRENT_LIMIT_MAX,
+ },
+ [MAXIM_DEVICE_TYPE_MAX77836] = {
+ .min = MAX77836_CHARGER_CURRENT_LIMIT_MIN,
+ .high_start = MAX77836_CHARGER_CURRENT_LIMIT_HIGH_START,
+ .high_step = MAX77836_CHARGER_CURRENT_LIMIT_HIGH_STEP,
+ .max = MAX77836_CHARGER_CURRENT_LIMIT_MAX,
+ },
+};
+EXPORT_SYMBOL_GPL(maxim_charger_currents);
+
+/*
+ * maxim_charger_calc_reg_current - Calculate register value for current
+ * @limits: constraints for charger, matching the MBCICHWRC register
+ * @min_ua: minimal requested current, micro Amps
+ * @max_ua: maximum requested current, micro Amps
+ * @dst: destination to store calculated register value
+ *
+ * Calculates the value of MBCICHWRC (Fast Battery Charge Current) register
+ * for given current and stores it under pointed 'dst'. The stored value
+ * combines low bit (MBCICHWRCL) and high bits (MBCICHWRCH). It is also
+ * properly shifted.
+ *
+ * The calculated register value matches the current which:
+ * - is always between <limits.min, limits.max>;
+ * - is always less or equal to max_ua;
+ * - is the highest possible value;
+ * - may be lower than min_ua.
+ *
+ * On success returns 0. On error returns -EINVAL (requested min/max current
+ * is outside of given charger limits) and 'dst' is not set.
+ */
+int maxim_charger_calc_reg_current(const struct maxim_charger_current *limits,
+ unsigned int min_ua, unsigned int max_ua, u8 *dst)
+{
+ unsigned int current_bits = 0xf;
+
+ if (min_ua > max_ua)
+ return -EINVAL;
+
+ if (min_ua > limits->max || max_ua < limits->min)
+ return -EINVAL;
+
+ if (max_ua < limits->high_start) {
+ /*
+ * Less than high_start, so set the minimal current
+ * (turn Low Bit off, 0 as high bits).
+ */
+ *dst = 0x0;
+ return 0;
+ }
+
+ /* max_ua is in range: <high_start, infinite>, cut it to limits.max */
+ max_ua = min(limits->max, max_ua);
+ max_ua -= limits->high_start;
+ /*
+ * There is no risk of overflow 'max_ua' here because:
+ * - max_ua >= limits.high_start
+ * - BUILD_BUG checks that 'limits' are: max >= high_start + high_step
+ */
+ current_bits = max_ua / limits->high_step;
+
+ /* Turn Low Bit on (use range <limits.high_start, limits.max>) ... */
+ *dst = 0x1 << CHGCTRL4_MBCICHWRCL_SHIFT;
+ /* and set proper High Bits */
+ *dst |= current_bits << CHGCTRL4_MBCICHWRCH_SHIFT;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(maxim_charger_calc_reg_current);
+
static const struct mfd_cell max14577_devs[] = {
{
.name = "max14577-muic",
@@ -463,6 +544,20 @@ static int __init max14577_i2c_init(void)
BUILD_BUG_ON(ARRAY_SIZE(max14577_i2c_id) != MAXIM_DEVICE_TYPE_NUM);
BUILD_BUG_ON(ARRAY_SIZE(max14577_dt_match) != MAXIM_DEVICE_TYPE_NUM);
+ /* Valid charger current values must be provided for each chipset */
+ BUILD_BUG_ON(ARRAY_SIZE(maxim_charger_currents) != MAXIM_DEVICE_TYPE_NUM);
+
+ /* Check for valid values for charger */
+ BUILD_BUG_ON(MAX14577_CHARGER_CURRENT_LIMIT_HIGH_START +
+ MAX14577_CHARGER_CURRENT_LIMIT_HIGH_STEP * 0xf !=
+ MAX14577_CHARGER_CURRENT_LIMIT_MAX);
+ BUILD_BUG_ON(MAX14577_CHARGER_CURRENT_LIMIT_HIGH_STEP == 0);
+
+ BUILD_BUG_ON(MAX77836_CHARGER_CURRENT_LIMIT_HIGH_START +
+ MAX77836_CHARGER_CURRENT_LIMIT_HIGH_STEP * 0xf !=
+ MAX77836_CHARGER_CURRENT_LIMIT_MAX);
+ BUILD_BUG_ON(MAX77836_CHARGER_CURRENT_LIMIT_HIGH_STEP == 0);
+
return i2c_add_driver(&max14577_i2c_driver);
}
subsys_initcall(max14577_i2c_init);
diff --git a/drivers/regulator/max14577.c b/drivers/regulator/max14577.c
index 5d9c605cf534..0ff5a20ac958 100644
--- a/drivers/regulator/max14577.c
+++ b/drivers/regulator/max14577.c
@@ -22,42 +22,6 @@
#include <linux/mfd/max14577-private.h>
#include <linux/regulator/of_regulator.h>
-/*
- * Valid limits of current for max14577 and max77836 chargers.
- * They must correspond to MBCICHWRCL and MBCICHWRCH fields in CHGCTRL4
- * register for given chipset.
- */
-struct maxim_charger_current {
- /* Minimal current, set in CHGCTRL4/MBCICHWRCL, uA */
- unsigned int min;
- /*
- * Minimal current when high setting is active,
- * set in CHGCTRL4/MBCICHWRCH, uA
- */
- unsigned int high_start;
- /* Value of one step in high setting, uA */
- unsigned int high_step;
- /* Maximum current of high setting, uA */
- unsigned int max;
-};
-
-/* Table of valid charger currents for different Maxim chipsets */
-static const struct maxim_charger_current maxim_charger_currents[] = {
- [MAXIM_DEVICE_TYPE_UNKNOWN] = { 0, 0, 0, 0 },
- [MAXIM_DEVICE_TYPE_MAX14577] = {
- .min = MAX14577_REGULATOR_CURRENT_LIMIT_MIN,
- .high_start = MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_START,
- .high_step = MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_STEP,
- .max = MAX14577_REGULATOR_CURRENT_LIMIT_MAX,
- },
- [MAXIM_DEVICE_TYPE_MAX77836] = {
- .min = MAX77836_REGULATOR_CURRENT_LIMIT_MIN,
- .high_start = MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_START,
- .high_step = MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_STEP,
- .max = MAX77836_REGULATOR_CURRENT_LIMIT_MAX,
- },
-};
-
static int max14577_reg_is_enabled(struct regulator_dev *rdev)
{
int rid = rdev_get_id(rdev);
@@ -103,8 +67,8 @@ static int max14577_reg_get_current_limit(struct regulator_dev *rdev)
static int max14577_reg_set_current_limit(struct regulator_dev *rdev,
int min_uA, int max_uA)
{
- int i, current_bits = 0xf;
u8 reg_data;
+ int ret;
struct max14577 *max14577 = rdev_get_drvdata(rdev);
const struct maxim_charger_current *limits =
&maxim_charger_currents[max14577->dev_type];
@@ -112,35 +76,9 @@ static int max14577_reg_set_current_limit(struct regulator_dev *rdev,
if (rdev_get_id(rdev) != MAX14577_CHARGER)
return -EINVAL;
- if (min_uA > limits->max || max_uA < limits->min)
- return -EINVAL;
-
- if (max_uA < limits->high_start) {
- /*
- * Less than high_start,
- * so set the minimal current (turn only Low Bit off)
- */
- u8 reg_data = 0x0 << CHGCTRL4_MBCICHWRCL_SHIFT;
- return max14577_update_reg(rdev->regmap,
- MAX14577_CHG_REG_CHG_CTRL4,
- CHGCTRL4_MBCICHWRCL_MASK, reg_data);
- }
-
- /*
- * max_uA is in range: <high_start, inifinite>, so search for
- * valid current starting from maximum current.
- */
- for (i = limits->max; i >= limits->high_start; i -= limits->high_step) {
- if (i <= max_uA)
- break;
- current_bits--;
- }
- BUG_ON(current_bits < 0); /* Cannot happen */
-
- /* Turn Low Bit on (use range high_start-max)... */
- reg_data = 0x1 << CHGCTRL4_MBCICHWRCL_SHIFT;
- /* and set proper High Bits */
- reg_data |= current_bits << CHGCTRL4_MBCICHWRCH_SHIFT;
+ ret = maxim_charger_calc_reg_current(limits, min_uA, max_uA, ®_data);
+ if (ret)
+ return ret;
return max14577_update_reg(rdev->regmap, MAX14577_CHG_REG_CHG_CTRL4,
CHGCTRL4_MBCICHWRCL_MASK | CHGCTRL4_MBCICHWRCH_MASK,
@@ -442,16 +380,6 @@ static struct platform_driver max14577_regulator_driver = {
static int __init max14577_regulator_init(void)
{
- /* Check for valid values for charger */
- BUILD_BUG_ON(MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_START +
- MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_STEP * 0xf !=
- MAX14577_REGULATOR_CURRENT_LIMIT_MAX);
- BUILD_BUG_ON(MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_START +
- MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_STEP * 0xf !=
- MAX77836_REGULATOR_CURRENT_LIMIT_MAX);
- /* Valid charger current values must be provided for each chipset */
- BUILD_BUG_ON(ARRAY_SIZE(maxim_charger_currents) != MAXIM_DEVICE_TYPE_NUM);
-
BUILD_BUG_ON(ARRAY_SIZE(max14577_supported_regulators) != MAX14577_REGULATOR_NUM);
BUILD_BUG_ON(ARRAY_SIZE(max77836_supported_regulators) != MAX77836_REGULATOR_NUM);
diff --git a/include/linux/mfd/max14577-private.h b/include/linux/mfd/max14577-private.h
index d6f321699b89..7d514839c764 100644
--- a/include/linux/mfd/max14577-private.h
+++ b/include/linux/mfd/max14577-private.h
@@ -281,17 +281,17 @@ enum max14577_charger_reg {
#define CHGCTRL7_OTPCGHCVS_SHIFT 0
#define CHGCTRL7_OTPCGHCVS_MASK (0x3 << CHGCTRL7_OTPCGHCVS_SHIFT)
-/* MAX14577 regulator current limits (as in CHGCTRL4 register), uA */
-#define MAX14577_REGULATOR_CURRENT_LIMIT_MIN 90000
-#define MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_START 200000
-#define MAX14577_REGULATOR_CURRENT_LIMIT_HIGH_STEP 50000
-#define MAX14577_REGULATOR_CURRENT_LIMIT_MAX 950000
-
-/* MAX77836 regulator current limits (as in CHGCTRL4 register), uA */
-#define MAX77836_REGULATOR_CURRENT_LIMIT_MIN 45000
-#define MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_START 100000
-#define MAX77836_REGULATOR_CURRENT_LIMIT_HIGH_STEP 25000
-#define MAX77836_REGULATOR_CURRENT_LIMIT_MAX 475000
+/* MAX14577 charger current limits (as in CHGCTRL4 register), uA */
+#define MAX14577_CHARGER_CURRENT_LIMIT_MIN 90000U
+#define MAX14577_CHARGER_CURRENT_LIMIT_HIGH_START 200000U
+#define MAX14577_CHARGER_CURRENT_LIMIT_HIGH_STEP 50000U
+#define MAX14577_CHARGER_CURRENT_LIMIT_MAX 950000U
+
+/* MAX77836 charger current limits (as in CHGCTRL4 register), uA */
+#define MAX77836_CHARGER_CURRENT_LIMIT_MIN 45000U
+#define MAX77836_CHARGER_CURRENT_LIMIT_HIGH_START 100000U
+#define MAX77836_CHARGER_CURRENT_LIMIT_HIGH_STEP 25000U
+#define MAX77836_CHARGER_CURRENT_LIMIT_MAX 475000U
/* MAX14577 regulator SFOUT LDO voltage, fixed, uV */
#define MAX14577_REGULATOR_SAFEOUT_VOLTAGE 4900000
diff --git a/include/linux/mfd/max14577.h b/include/linux/mfd/max14577.h
index c83fbed1c7b6..3c098d57b1d1 100644
--- a/include/linux/mfd/max14577.h
+++ b/include/linux/mfd/max14577.h
@@ -74,4 +74,27 @@ struct max14577_platform_data {
struct max14577_regulator_platform_data *regulators;
};
+/*
+ * Valid limits of current for max14577 and max77836 chargers.
+ * They must correspond to MBCICHWRCL and MBCICHWRCH fields in CHGCTRL4
+ * register for given chipset.
+ */
+struct maxim_charger_current {
+ /* Minimal current, set in CHGCTRL4/MBCICHWRCL, uA */
+ unsigned int min;
+ /*
+ * Minimal current when high setting is active,
+ * set in CHGCTRL4/MBCICHWRCH, uA
+ */
+ unsigned int high_start;
+ /* Value of one step in high setting, uA */
+ unsigned int high_step;
+ /* Maximum current of high setting, uA */
+ unsigned int max;
+};
+
+extern const struct maxim_charger_current maxim_charger_currents[];
+extern int maxim_charger_calc_reg_current(const struct maxim_charger_current *limits,
+ unsigned int min_ua, unsigned int max_ua, u8 *dst);
+
#endif /* __MAX14577_H__ */
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 1/6] charger: max14577: Add support for MAX77836 charger
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse, linux-kernel, linux-api
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Anton Vorontsov
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski@samsung.com>
Add support for MAX77836 charger to the max14577 driver. The MAX77836
charger is almost the same as 14577 model except:
- No dead-battery detection;
- Support for special charger (like in MAX77693);
- Support for DX over-voltage protection (like in MAX77693);
- Lower values of charging current (two times lower current for
slow/fast charge, much lower EOC current);
- Slightly different values in ChgTyp field of STATUS2 register. On
MAX14577 0x6 is reserved and 0x7 dead battery. On the MAX77836 the
0x6 means special charger and 0x7 is reserved. Regardless of these
differences the driver maps them to one enum max14577_muic_charger_type.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
drivers/power/Kconfig | 4 +-
drivers/power/max14577_charger.c | 77 +++++++++++++++++++++++++++++-------
include/linux/mfd/max14577-private.h | 54 ++++++++++++++++++-------
3 files changed, 104 insertions(+), 31 deletions(-)
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index ba6975123071..94086a5238c6 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -318,11 +318,11 @@ config CHARGER_MANAGER
with help of suspend_again support.
config CHARGER_MAX14577
- tristate "Maxim MAX14577 MUIC battery charger driver"
+ tristate "Maxim MAX14577/77836 battery charger driver"
depends on MFD_MAX14577
help
Say Y to enable support for the battery charger control sysfs and
- platform data of MAX14577 MUICs.
+ platform data of MAX14577/77836 MUICs.
config CHARGER_MAX8997
tristate "Maxim MAX8997/MAX8966 PMIC battery charger driver"
diff --git a/drivers/power/max14577_charger.c b/drivers/power/max14577_charger.c
index fad2a75b3604..19c8f42abf24 100644
--- a/drivers/power/max14577_charger.c
+++ b/drivers/power/max14577_charger.c
@@ -1,7 +1,7 @@
/*
- * Battery charger driver for the Maxim 14577
+ * max14577_charger.c - Battery charger driver for the Maxim 14577/77836
*
- * Copyright (C) 2013 Samsung Electronics
+ * Copyright (C) 2013,2014 Samsung Electronics
* Krzysztof Kozlowski <k.kozlowski@samsung.com>
*
* This program is free software; you can redistribute it and/or modify
@@ -25,10 +25,35 @@ struct max14577_charger {
struct max14577 *max14577;
struct power_supply charger;
- unsigned int charging_state;
- unsigned int battery_state;
+ unsigned int charging_state;
+ unsigned int battery_state;
};
+/*
+ * Helper function for mapping values of STATUS2/CHGTYP register on max14577
+ * and max77836 chipsets to enum maxim_muic_charger_type.
+ */
+static enum max14577_muic_charger_type maxim_get_charger_type(
+ enum maxim_device_type dev_type, u8 val) {
+ switch (val) {
+ case MAX14577_CHARGER_TYPE_NONE:
+ case MAX14577_CHARGER_TYPE_USB:
+ case MAX14577_CHARGER_TYPE_DOWNSTREAM_PORT:
+ case MAX14577_CHARGER_TYPE_DEDICATED_CHG:
+ case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
+ case MAX14577_CHARGER_TYPE_SPECIAL_1A:
+ return val;
+ case MAX14577_CHARGER_TYPE_DEAD_BATTERY:
+ case MAX14577_CHARGER_TYPE_RESERVED:
+ if (dev_type == MAXIM_DEVICE_TYPE_MAX77836)
+ val |= 0x8;
+ return val;
+ default:
+ WARN_ONCE(1, "max14577: Unsupported chgtyp register value 0x%02x", val);
+ return val;
+ }
+}
+
static int max14577_get_charger_state(struct max14577_charger *chg)
{
struct regmap *rmap = chg->max14577->regmap;
@@ -89,19 +114,23 @@ static int max14577_get_online(struct max14577_charger *chg)
{
struct regmap *rmap = chg->max14577->regmap;
u8 reg_data;
+ enum max14577_muic_charger_type chg_type;
max14577_read_reg(rmap, MAX14577_MUIC_REG_STATUS2, ®_data);
reg_data = ((reg_data & STATUS2_CHGTYP_MASK) >> STATUS2_CHGTYP_SHIFT);
- switch (reg_data) {
+ chg_type = maxim_get_charger_type(chg->max14577->dev_type, reg_data);
+ switch (chg_type) {
case MAX14577_CHARGER_TYPE_USB:
case MAX14577_CHARGER_TYPE_DEDICATED_CHG:
case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
case MAX14577_CHARGER_TYPE_SPECIAL_1A:
case MAX14577_CHARGER_TYPE_DEAD_BATTERY:
+ case MAX77836_CHARGER_TYPE_SPECIAL_BIAS:
return 1;
case MAX14577_CHARGER_TYPE_NONE:
case MAX14577_CHARGER_TYPE_DOWNSTREAM_PORT:
case MAX14577_CHARGER_TYPE_RESERVED:
+ case MAX77836_CHARGER_TYPE_RESERVED:
default:
return 0;
}
@@ -118,10 +147,12 @@ static int max14577_get_battery_health(struct max14577_charger *chg)
struct regmap *rmap = chg->max14577->regmap;
int state = POWER_SUPPLY_HEALTH_GOOD;
u8 reg_data;
+ enum max14577_muic_charger_type chg_type;
max14577_read_reg(rmap, MAX14577_MUIC_REG_STATUS2, ®_data);
reg_data = ((reg_data & STATUS2_CHGTYP_MASK) >> STATUS2_CHGTYP_SHIFT);
- if (reg_data == MAX14577_CHARGER_TYPE_DEAD_BATTERY) {
+ chg_type = maxim_get_charger_type(chg->max14577->dev_type, reg_data);
+ if (chg_type == MAX14577_CHARGER_TYPE_DEAD_BATTERY) {
state = POWER_SUPPLY_HEALTH_DEAD;
goto state_set;
}
@@ -167,7 +198,7 @@ static void max14577_charger_reg_init(struct max14577_charger *chg)
CDETCTRL1_CHGDETEN_MASK | CDETCTRL1_CHGTYPMAN_MASK,
reg_data);
- /* Battery Fast-Charge Timer, from SM-V700: 6hrs */
+ /* Battery Fast-Charge Timer, set to: 6hrs */
reg_data = 0x3 << CHGCTRL1_TCHW_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL1, reg_data);
@@ -179,19 +210,22 @@ static void max14577_charger_reg_init(struct max14577_charger *chg)
reg_data |= 0x1 << CHGCTRL2_MBCHOSTEN_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL2, reg_data);
- /* Battery-Charger Constant Voltage (CV) Mode, from SM-V700: 4.35V */
+ /* Battery-Charger Constant Voltage (CV) Mode, set to: 4.35V */
reg_data = 0xf << CHGCTRL3_MBCCVWRC_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL3, reg_data);
/*
- * Fast Battery-Charge Current Low, default 200-950mA
- * Fast Battery-Charge Current High, from SM-V700: 450mA
+ * Fast Battery-Charge Current Low,
+ * default 200-950mA (max14577) / 100-475mA (max77836)
+ *
+ * Fast Battery-Charge Current High,
+ * set to 450mA (max14577) / 225mA (max77836)
*/
reg_data = 0x1 << CHGCTRL4_MBCICHWRCL_SHIFT;
reg_data |= 0x5 << CHGCTRL4_MBCICHWRCH_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL4, reg_data);
- /* End-of-Charge Current, from SM-V700: 50mA */
+ /* End-of-Charge Current, set to 50mA (max14577) / 7.5mA (max77836) */
reg_data = 0x0 << CHGCTRL5_EOCS_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL5, reg_data);
@@ -199,7 +233,7 @@ static void max14577_charger_reg_init(struct max14577_charger *chg)
reg_data = 0x0 << CHGCTRL6_AUTOSTOP_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL6, reg_data);
- /* Overvoltage-Protection Threshold, from SM-V700: 6.5V */
+ /* Overvoltage-Protection Threshold, set to 6.5V */
reg_data = 0x2 << CHGCTRL7_OTPCGHCVS_SHIFT;
max14577_write_reg(rmap, MAX14577_REG_CHGCTRL7, reg_data);
}
@@ -215,7 +249,11 @@ static enum power_supply_property max14577_charger_props[] = {
POWER_SUPPLY_PROP_MANUFACTURER,
};
-static const char *model_name = "MAX14577";
+static const char * const model_names[] = {
+ [MAXIM_DEVICE_TYPE_UNKNOWN] = "MAX14577-like",
+ [MAXIM_DEVICE_TYPE_MAX14577] = "MAX14577",
+ [MAXIM_DEVICE_TYPE_MAX77836] = "MAX77836",
+};
static const char *manufacturer = "Maxim Integrated";
static int max14577_charger_get_property(struct power_supply *psy,
@@ -244,7 +282,8 @@ static int max14577_charger_get_property(struct power_supply *psy,
val->intval = max14577_get_online(chg);
break;
case POWER_SUPPLY_PROP_MODEL_NAME:
- val->strval = model_name;
+ BUILD_BUG_ON(ARRAY_SIZE(model_names) != MAXIM_DEVICE_TYPE_NUM);
+ val->strval = model_names[chg->max14577->dev_type];
break;
case POWER_SUPPLY_PROP_MANUFACTURER:
val->strval = manufacturer;
@@ -296,6 +335,13 @@ static int max14577_charger_remove(struct platform_device *pdev)
return 0;
}
+static const struct platform_device_id max14577_charger_id[] = {
+ { "max14577-charger", MAXIM_DEVICE_TYPE_MAX14577, },
+ { "max77836-charger", MAXIM_DEVICE_TYPE_MAX77836, },
+ { }
+};
+MODULE_DEVICE_TABLE(platform, max14577_regulator_id);
+
static struct platform_driver max14577_charger_driver = {
.driver = {
.owner = THIS_MODULE,
@@ -303,9 +349,10 @@ static struct platform_driver max14577_charger_driver = {
},
.probe = max14577_charger_probe,
.remove = max14577_charger_remove,
+ .id_table = max14577_charger_id,
};
module_platform_driver(max14577_charger_driver);
MODULE_AUTHOR("Krzysztof Kozlowski <k.kozlowski@samsung.com>");
-MODULE_DESCRIPTION("MAXIM 14577 charger driver");
+MODULE_DESCRIPTION("Maxim 14577/77836 charger driver");
MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max14577-private.h b/include/linux/mfd/max14577-private.h
index 499253604026..d6f321699b89 100644
--- a/include/linux/mfd/max14577-private.h
+++ b/include/linux/mfd/max14577-private.h
@@ -72,15 +72,33 @@ enum max14577_muic_reg {
MAX14577_MUIC_REG_END,
};
+/*
+ * Combined charger types for max14577 and max77836.
+ *
+ * On max14577 three lower bits map to STATUS2/CHGTYP field.
+ * However the max77836 has different two last values of STATUS2/CHGTYP.
+ * To indicate the difference enum has two additional values for max77836.
+ * These values are just a register value bitwise OR with 0x8.
+ */
enum max14577_muic_charger_type {
- MAX14577_CHARGER_TYPE_NONE = 0,
- MAX14577_CHARGER_TYPE_USB,
- MAX14577_CHARGER_TYPE_DOWNSTREAM_PORT,
- MAX14577_CHARGER_TYPE_DEDICATED_CHG,
- MAX14577_CHARGER_TYPE_SPECIAL_500MA,
- MAX14577_CHARGER_TYPE_SPECIAL_1A,
- MAX14577_CHARGER_TYPE_RESERVED,
- MAX14577_CHARGER_TYPE_DEAD_BATTERY = 7,
+ MAX14577_CHARGER_TYPE_NONE = 0x0,
+ MAX14577_CHARGER_TYPE_USB = 0x1,
+ MAX14577_CHARGER_TYPE_DOWNSTREAM_PORT = 0x2,
+ MAX14577_CHARGER_TYPE_DEDICATED_CHG = 0x3,
+ MAX14577_CHARGER_TYPE_SPECIAL_500MA = 0x4,
+ /* Special 1A or 2A charger */
+ MAX14577_CHARGER_TYPE_SPECIAL_1A = 0x5,
+ /* max14577: reserved, used on max77836 */
+ MAX14577_CHARGER_TYPE_RESERVED = 0x6,
+ /* max14577: dead-battery charing with maximum current 100mA */
+ MAX14577_CHARGER_TYPE_DEAD_BATTERY = 0x7,
+ /*
+ * max77836: special charger (bias on D+/D-),
+ * matches register value of 0x6
+ */
+ MAX77836_CHARGER_TYPE_SPECIAL_BIAS = 0xe,
+ /* max77836: reserved, register value 0x7 */
+ MAX77836_CHARGER_TYPE_RESERVED = 0xf,
};
/* MAX14577 interrupts */
@@ -121,13 +139,15 @@ enum max14577_muic_charger_type {
#define STATUS2_CHGTYP_SHIFT 0
#define STATUS2_CHGDETRUN_SHIFT 3
#define STATUS2_DCDTMR_SHIFT 4
-#define STATUS2_DBCHG_SHIFT 5
+#define MAX14577_STATUS2_DBCHG_SHIFT 5
+#define MAX77836_STATUS2_DXOVP_SHIFT 5
#define STATUS2_VBVOLT_SHIFT 6
#define MAX77836_STATUS2_VIDRM_SHIFT 7
#define STATUS2_CHGTYP_MASK (0x7 << STATUS2_CHGTYP_SHIFT)
#define STATUS2_CHGDETRUN_MASK BIT(STATUS2_CHGDETRUN_SHIFT)
#define STATUS2_DCDTMR_MASK BIT(STATUS2_DCDTMR_SHIFT)
-#define STATUS2_DBCHG_MASK BIT(STATUS2_DBCHG_SHIFT)
+#define MAX14577_STATUS2_DBCHG_MASK BIT(MAX14577_STATUS2_DBCHG_SHIFT)
+#define MAX77836_STATUS2_DXOVP_MASK BIT(MAX77836_STATUS2_DXOVP_SHIFT)
#define STATUS2_VBVOLT_MASK BIT(STATUS2_VBVOLT_SHIFT)
#define MAX77836_STATUS2_VIDRM_MASK BIT(MAX77836_STATUS2_VIDRM_SHIFT)
@@ -177,9 +197,11 @@ enum max14577_muic_charger_type {
#define CTRL3_JIGSET_SHIFT 0
#define CTRL3_BOOTSET_SHIFT 2
#define CTRL3_ADCDBSET_SHIFT 4
+#define CTRL3_WBTH_SHIFT 6
#define CTRL3_JIGSET_MASK (0x3 << CTRL3_JIGSET_SHIFT)
#define CTRL3_BOOTSET_MASK (0x3 << CTRL3_BOOTSET_SHIFT)
#define CTRL3_ADCDBSET_MASK (0x3 << CTRL3_ADCDBSET_SHIFT)
+#define CTRL3_WBTH_MASK (0x3 << CTRL3_WBTH_SHIFT)
/* Slave addr = 0x4A: Charger */
enum max14577_charger_reg {
@@ -210,16 +232,20 @@ enum max14577_charger_reg {
#define CDETCTRL1_CHGTYPMAN_SHIFT 1
#define CDETCTRL1_DCDEN_SHIFT 2
#define CDETCTRL1_DCD2SCT_SHIFT 3
-#define CDETCTRL1_DCHKTM_SHIFT 4
-#define CDETCTRL1_DBEXIT_SHIFT 5
+#define MAX14577_CDETCTRL1_DCHKTM_SHIFT 4
+#define MAX77836_CDETCTRL1_CDLY_SHIFT 4
+#define MAX14577_CDETCTRL1_DBEXIT_SHIFT 5
+#define MAX77836_CDETCTRL1_DCDCPL_SHIFT 5
#define CDETCTRL1_DBIDLE_SHIFT 6
#define CDETCTRL1_CDPDET_SHIFT 7
#define CDETCTRL1_CHGDETEN_MASK BIT(CDETCTRL1_CHGDETEN_SHIFT)
#define CDETCTRL1_CHGTYPMAN_MASK BIT(CDETCTRL1_CHGTYPMAN_SHIFT)
#define CDETCTRL1_DCDEN_MASK BIT(CDETCTRL1_DCDEN_SHIFT)
#define CDETCTRL1_DCD2SCT_MASK BIT(CDETCTRL1_DCD2SCT_SHIFT)
-#define CDETCTRL1_DCHKTM_MASK BIT(CDETCTRL1_DCHKTM_SHIFT)
-#define CDETCTRL1_DBEXIT_MASK BIT(CDETCTRL1_DBEXIT_SHIFT)
+#define MAX14577_CDETCTRL1_DCHKTM_MASK BIT(MAX14577_CDETCTRL1_DCHKTM_SHIFT)
+#define MAX77836_CDETCTRL1_CDDLY_MASK BIT(MAX77836_CDETCTRL1_CDDLY_SHIFT)
+#define MAX14577_CDETCTRL1_DBEXIT_MASK BIT(MAX14577_CDETCTRL1_DBEXIT_SHIFT)
+#define MAX77836_CDETCTRL1_DCDCPL_MASK BIT(MAX77836_CDETCTRL1_DCDCPL_SHIFT)
#define CDETCTRL1_DBIDLE_MASK BIT(CDETCTRL1_DBIDLE_SHIFT)
#define CDETCTRL1_CDPDET_MASK BIT(CDETCTRL1_CDPDET_SHIFT)
--
1.9.1
^ permalink raw reply related
* [RESEND PATCH v3 0/6] charger/mfd: max14577: Add support for MAX77836
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Hi,
This is a resend of third version of patches adding support for
MAX77836 device to the max14577 drivers.
This patchset was already reviewed by some of the maintainers during
previous submissions.
I only need acks from power tree (patches: 1, 3, 4, 6).
The patches 1, 2 and 3 depend on each other so they should be
pulled at once. Patches 4, 5 and 6 can be applied independently.
Lee Jones said he can take the set through his tree. Still I need acks
from power subsystem.
Changes since v2
================
1. charger: Use sysfs instead of DTS for setting the fast charge timer.
The charger driver now selects the CONFIG_SYSFS and exports
a DEVICE_ATTR. (suggested by Mark Rutland)
2. Add patch 6 with documentation of exported sysfs entry for fast
charge timer.
3. charger 3/6: Add missing 'break' in switch parsing valid values
for fast charge timer.
Changes since v1
================
1. charger 3/5: Add an error message for each unsuccessful parse of DT
property (suggested by Mark Rutland).
2. charger 3/5: Use 'u32' type for storing values from DT (suggested
by Mark Rutland).
3. charger 3/5: Remove an error message for memory allocation failure.
The patchset (first and second part of the MAX77836 drivers) has been
on the lists since January. Changelog for the first part of drivers:
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg628696.html
Best regards,
Krzysztof Kozlowski
Krzysztof Kozlowski (6):
charger: max14577: Add support for MAX77836 charger
regulator/mfd: max14577: Export symbols for calculating charger
current
charger: max14577: Configure battery-dependent settings from DTS and
sysfs
power: max17040: Add ID for MAX77836 Fuel Gauge block
devicetree: mfd: max14577: Add device tree bindings document
Documentation: charger: max14577: Document exported sysfs entry
Documentation/ABI/testing/sysfs-class-power | 14 +
Documentation/devicetree/bindings/mfd/max14577.txt | 146 ++++++++
drivers/mfd/max14577.c | 100 +++++-
drivers/power/Kconfig | 5 +-
drivers/power/max14577_charger.c | 370 +++++++++++++++++++--
drivers/power/max17040_battery.c | 1 +
drivers/regulator/max14577.c | 80 +----
include/linux/mfd/max14577-private.h | 95 ++++--
include/linux/mfd/max14577.h | 30 ++
9 files changed, 703 insertions(+), 138 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/max14577.txt
--
1.9.1
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-22 7:35 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Andy Lutomirski, Thomas Gleixner, Darren Hart, Andi Kleen,
Waiman Long, Ingo Molnar, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton
In-Reply-To: <1405989175.25048.5.camel@buesod1.americas.hpqcorp.net>
On Mon, Jul 21, 2014 at 05:32:55PM -0700, Davidlohr Bueso wrote:
> On Mon, 2014-07-21 at 14:31 -0700, Andy Lutomirski wrote:
> > On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > All this is predicated on the fact that syscalls are 'expensive'.
> > > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> > > more expensive due to cacheline misses, which due to the size of the
> > > things is almost guaranteed.
> >
> > 120 - 300 cycles for me, unless tracing happens, and I'm working on
> > reducing the incidence of tracing.
>
> fwiw here's what lmbench's lat_ctx says on my system . For 'accuracy', I
> kept the runs short.
>
> http://www.stgolabs.net/lat_ctx.png
Create a syscall without 'work body', so say something like the below.
Context switches do quite a lot of work (unfortunately).
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index ec255a1646d2..daf27ec10e29 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -323,6 +323,7 @@
314 common sched_setattr sys_sched_setattr
315 common sched_getattr sys_sched_getattr
316 common renameat2 sys_renameat2
+317 common nop sys_nop
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/kernel/sys.c b/kernel/sys.c
index 66a751ebf9d9..1b86614bb551 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2108,6 +2108,11 @@ SYSCALL_DEFINE1(sysinfo, struct sysinfo __user *, info)
return 0;
}
+SYSCALL_DEFINE0(nop)
+{
+ return 0;
+}
+
#ifdef CONFIG_COMPAT
struct compat_sysinfo {
s32 uptime;
^ permalink raw reply related
* [PATCH RFC v3 net-next 3/3] samples: bpf: eBPF dropmon example in C
From: Alexei Starovoitov @ 2014-07-22 3:45 UTC (permalink / raw)
To: David S. Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Arnaldo Carvalho de Melo, Jiri Olsa, Thomas Gleixner,
H. Peter Anvin, Andrew Morton, Kees Cook, linux-api, netdev,
linux-kernel
In-Reply-To: <1406000723-4872-1-git-send-email-ast@plumgrid.com>
semantically this example is the same as dropmon.c which has eBPF in assembler.
In this example both eBPF and user space are in C:
ex2_kern.c - C code that will compile into eBPF and will
be attached to kfree_skb event
ex2_user.c - corresponding user space part that iterates the map
populated by in-kernel eBPF code
Usage:
$ sudo ex2 ex2_kern.o
Should see:
writing bpf-5 -> /sys/kernel/debug/tracing/events/skb/kfree_skb/filter
location 0xffffffff816efc67 count 1
location 0xffffffff815d8030 count 1
location 0xffffffff816efc67 count 3
location 0xffffffff815d8030 count 4
location 0xffffffff816efc67 count 9
location 0xffffffff815d8030 count 7
location 0xffffffff816efc67 count 15
location 0xffffffff815d8030 count 10
location 0xffffffff816efc67 count 23
location 0xffffffff815d8030 count 13
location 0xffffffff816efc67 count 29
Ctrl-C at any time. Kernel will auto cleanup maps and programs
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
samples/bpf/Makefile | 6 ++++--
samples/bpf/ex2_kern.c | 29 +++++++++++++++++++++++++++++
samples/bpf/ex2_user.c | 28 ++++++++++++++++++++++++++++
3 files changed, 61 insertions(+), 2 deletions(-)
create mode 100644 samples/bpf/ex2_kern.c
create mode 100644 samples/bpf/ex2_user.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index c97f408fcd6d..b865a5df5c60 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -2,19 +2,21 @@
obj- := dummy.o
# List of programs to build
-hostprogs-y := sock_example dropmon ex1
+hostprogs-y := sock_example dropmon ex1 ex2
sock_example-objs := sock_example.o libbpf.o
dropmon-objs := dropmon.o libbpf.o
ex1-objs := bpf_load.o libbpf.o ex1_user.o
+ex2-objs := bpf_load.o libbpf.o ex2_user.o
# Tell kbuild to always build the programs
-always := $(hostprogs-y) ex1_kern.o
+always := $(hostprogs-y) ex1_kern.o ex2_kern.o
HOSTCFLAGS += -I$(objtree)/usr/include
HOSTCFLAGS_bpf_load.o += -I$(objtree)/usr/include -Wno-unused-variable
HOSTLOADLIBES_ex1 += -lelf
+HOSTLOADLIBES_ex2 += -lelf
LLC=$(srctree)/tools/bpf/llvm/bld/Debug+Asserts/bin/llc
diff --git a/samples/bpf/ex2_kern.c b/samples/bpf/ex2_kern.c
new file mode 100644
index 000000000000..b4a8e73a3f9d
--- /dev/null
+++ b/samples/bpf/ex2_kern.c
@@ -0,0 +1,29 @@
+#include <linux/skbuff.h>
+#include <linux/netdevice.h>
+#include <uapi/linux/bpf.h>
+#include <trace/bpf_trace.h>
+#include "bpf_helpers.h"
+
+struct bpf_map_def SEC("maps") my_map = {
+ .type = BPF_MAP_TYPE_HASH,
+ .key_size = sizeof(long),
+ .value_size = sizeof(long),
+ .max_entries = 1024,
+};
+
+SEC("events/skb/kfree_skb")
+int bpf_prog2(struct bpf_context *ctx)
+{
+ long loc = ctx->arg2;
+ long init_val = 1;
+ void *value;
+
+ value = bpf_map_lookup_elem(&my_map, &loc);
+ if (value)
+ (*(long *) value) += 1;
+ else
+ bpf_map_update_elem(&my_map, &loc, &init_val);
+ return 0;
+}
+
+char license[] SEC("license") = "GPL";
diff --git a/samples/bpf/ex2_user.c b/samples/bpf/ex2_user.c
new file mode 100644
index 000000000000..8b0fc91f83ca
--- /dev/null
+++ b/samples/bpf/ex2_user.c
@@ -0,0 +1,28 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <linux/bpf.h>
+#include "libbpf.h"
+#include "bpf_load.h"
+
+int main(int ac, char **argv)
+{
+ long key, next_key, value;
+ int i;
+
+ if (load_bpf_file(argv[1]))
+ return 1;
+
+ for (i = 0; i < 10; i++) {
+ key = 0;
+ while (bpf_get_next_key(map_fd[0], &key, &next_key) == 0) {
+ bpf_lookup_elem(map_fd[0], &next_key, &value);
+ printf("location 0x%lx count %ld\n", next_key, value);
+ key = next_key;
+ }
+ if (key)
+ printf("\n");
+ sleep(1);
+ }
+
+ return 0;
+}
--
1.7.9.5
^ permalink raw reply related
* [PATCH RFC v3 net-next 2/3] samples: bpf: eBPF example in C
From: Alexei Starovoitov @ 2014-07-22 3:45 UTC (permalink / raw)
To: David S. Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Arnaldo Carvalho de Melo, Jiri Olsa, Thomas Gleixner,
H. Peter Anvin, Andrew Morton, Kees Cook, linux-api, netdev,
linux-kernel
In-Reply-To: <1406000723-4872-1-git-send-email-ast@plumgrid.com>
ex1_kern.c - C program which will be compiled into eBPF
to filter netif_receive_skb events on skb->dev->name == "lo"
ex1_user.c - corresponding user space component that
forever reads /sys/.../trace_pipe
Usage:
$ sudo ex1 ex1_kern.o
should see:
writing bpf-4 -> /sys/kernel/debug/tracing/events/net/netif_receive_skb/filter
ping-25476 [001] ..s3 5639.718218: __netif_receive_skb_core: skb 4d51700 dev b9e6000
ping-25476 [001] ..s3 5639.718262: __netif_receive_skb_core: skb 4d51400 dev b9e6000
ping-25476 [002] ..s3 5640.716233: __netif_receive_skb_core: skb 5d06500 dev b9e6000
ping-25476 [002] ..s3 5640.716272: __netif_receive_skb_core: skb 5d06300 dev b9e6000
Ctrl-C at any time, kernel will auto cleanup
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
samples/bpf/Makefile | 15 +++++++++++++--
samples/bpf/ex1_kern.c | 27 +++++++++++++++++++++++++++
samples/bpf/ex1_user.c | 11 +++++++++++
3 files changed, 51 insertions(+), 2 deletions(-)
create mode 100644 samples/bpf/ex1_kern.c
create mode 100644 samples/bpf/ex1_user.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index caf1ab93b37c..c97f408fcd6d 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -2,12 +2,23 @@
obj- := dummy.o
# List of programs to build
-hostprogs-y := sock_example dropmon
+hostprogs-y := sock_example dropmon ex1
sock_example-objs := sock_example.o libbpf.o
dropmon-objs := dropmon.o libbpf.o
+ex1-objs := bpf_load.o libbpf.o ex1_user.o
# Tell kbuild to always build the programs
-always := $(hostprogs-y)
+always := $(hostprogs-y) ex1_kern.o
HOSTCFLAGS += -I$(objtree)/usr/include
+
+HOSTCFLAGS_bpf_load.o += -I$(objtree)/usr/include -Wno-unused-variable
+HOSTLOADLIBES_ex1 += -lelf
+
+LLC=$(srctree)/tools/bpf/llvm/bld/Debug+Asserts/bin/llc
+
+%.o: %.c
+ clang $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
+ -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
+ -O2 -emit-llvm -c $< -o -| $(LLC) -o $@
diff --git a/samples/bpf/ex1_kern.c b/samples/bpf/ex1_kern.c
new file mode 100644
index 000000000000..3fa7e4db0be6
--- /dev/null
+++ b/samples/bpf/ex1_kern.c
@@ -0,0 +1,27 @@
+#include <linux/skbuff.h>
+#include <linux/netdevice.h>
+#include <uapi/linux/bpf.h>
+#include <trace/bpf_trace.h>
+#include "bpf_helpers.h"
+
+SEC("events/net/netif_receive_skb")
+int bpf_prog1(struct bpf_context *ctx)
+{
+ /*
+ * attaches to /sys/kernel/debug/tracing/events/net/netif_receive_skb
+ * prints events for loobpack device only
+ */
+ char devname[] = "lo";
+ struct net_device *dev;
+ struct sk_buff *skb = 0;
+
+ skb = (struct sk_buff *) ctx->arg1;
+ dev = bpf_load_pointer(&skb->dev);
+ if (bpf_memcmp(dev->name, devname, 2) == 0) {
+ char fmt[] = "skb %x dev %x\n";
+ bpf_printk(fmt, sizeof(fmt), skb, dev);
+ }
+ return 0;
+}
+
+char license[] SEC("license") = "GPL";
diff --git a/samples/bpf/ex1_user.c b/samples/bpf/ex1_user.c
new file mode 100644
index 000000000000..d90c543ba30c
--- /dev/null
+++ b/samples/bpf/ex1_user.c
@@ -0,0 +1,11 @@
+#include "bpf_load.h"
+
+int main(int ac, char **argv)
+{
+ if (load_bpf_file(argv[1]))
+ return 1;
+
+ read_trace_pipe();
+
+ return 0;
+}
--
1.7.9.5
^ permalink raw reply related
* [PATCH RFC v3 net-next 1/3] samples: bpf: elf file loader
From: Alexei Starovoitov @ 2014-07-22 3:45 UTC (permalink / raw)
To: David S. Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Arnaldo Carvalho de Melo, Jiri Olsa, Thomas Gleixner,
H. Peter Anvin, Andrew Morton, Kees Cook, linux-api, netdev,
linux-kernel
In-Reply-To: <1406000723-4872-1-git-send-email-ast@plumgrid.com>
simple .o parser and loader using BPF syscall.
.o is a standard ELF generated by LLVM backend
/* parses elf file compiled by llvm .c->.o
* creates maps and stores FD into map_fd array
* loads eBPF programs
* returns zero on success
*/
int load_bpf_file(char *path);
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
samples/bpf/bpf_helpers.h | 21 +++++
samples/bpf/bpf_load.c | 228 +++++++++++++++++++++++++++++++++++++++++++++
samples/bpf/bpf_load.h | 18 ++++
3 files changed, 267 insertions(+)
create mode 100644 samples/bpf/bpf_helpers.h
create mode 100644 samples/bpf/bpf_load.c
create mode 100644 samples/bpf/bpf_load.h
diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
new file mode 100644
index 000000000000..356e7d34d174
--- /dev/null
+++ b/samples/bpf/bpf_helpers.h
@@ -0,0 +1,21 @@
+#ifndef __BPF_HELPERS_H
+#define __BPF_HELPERS_H
+
+#define SEC(NAME) __attribute__((section(NAME), used))
+
+static void *(*bpf_load_pointer)(void *unsafe_ptr) = (void *) BPF_FUNC_load_pointer;
+static int (*bpf_printk)(const char *fmt, int fmt_size, ...) = (void *) BPF_FUNC_printk;
+static int (*bpf_memcmp)(void *unsafe_ptr, void *safe_ptr, int size) = (void *) BPF_FUNC_memcmp;
+static void *(*bpf_map_lookup_elem)(void *map, void *key) = (void*) BPF_FUNC_map_lookup_elem;
+static int (*bpf_map_update_elem)(void *map, void *key, void *value) = (void*) BPF_FUNC_map_update_elem;
+static int (*bpf_map_delete_elem)(void *map, void *key) = (void *) BPF_FUNC_map_delete_elem;
+static void (*bpf_dump_stack)(void) = (void *) BPF_FUNC_dump_stack;
+
+struct bpf_map_def {
+ int type;
+ int key_size;
+ int value_size;
+ int max_entries;
+};
+
+#endif
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
new file mode 100644
index 000000000000..7614f19d5282
--- /dev/null
+++ b/samples/bpf/bpf_load.c
@@ -0,0 +1,228 @@
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <libelf.h>
+#include <gelf.h>
+#include <errno.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdbool.h>
+#include <linux/bpf.h>
+#include "libbpf.h"
+#include "bpf_helpers.h"
+#include "bpf_load.h"
+
+#define DEBUGFS "/sys/kernel/debug/tracing/"
+
+static char license[128];
+static bool processed_sec[128];
+int map_fd[MAX_MAPS];
+
+static int load_and_attach(const char *event, char *prog, int size,
+ const struct bpf_map_fixup *fixups, int fixup_len)
+{
+ int fd, event_fd, err;
+ char fmt[32];
+ char path[256] = DEBUGFS;
+
+ fd = bpf_prog_load(BPF_PROG_TYPE_TRACING_FILTER,
+ (struct bpf_insn *)prog, size, license,
+ fixups, fixup_len);
+
+ if (fd < 0) {
+ printf("err %d errno %d\n", fd, errno);
+ return fd;
+ }
+
+
+ snprintf(fmt, sizeof(fmt), "bpf-%d", fd);
+
+ strcat(path, event);
+ strcat(path, "/filter");
+
+ printf("writing %s -> %s\n", fmt, path);
+
+ event_fd = open(path, O_WRONLY, 0);
+ if (event_fd < 0) {
+ printf("failed to open event %s\n", event);
+ return event_fd;
+ }
+
+ err = write(event_fd, fmt, strlen(fmt));
+ (void) err;
+
+ return 0;
+}
+
+static int load_maps(struct bpf_map_def *maps, int len)
+{
+ int i;
+
+ for (i = 0; i < len / sizeof(struct bpf_map_def); i++) {
+
+ map_fd[i] = bpf_create_map(maps[i].type,
+ maps[i].key_size,
+ maps[i].value_size,
+ maps[i].max_entries);
+ if (map_fd[i] < 0)
+ return 1;
+ }
+ return 0;
+}
+
+static int get_sec(Elf *elf, int i, GElf_Ehdr *ehdr, char **shname,
+ GElf_Shdr *shdr, Elf_Data **data)
+{
+ Elf_Scn *scn;
+
+ scn = elf_getscn(elf, i);
+ if (!scn)
+ return 1;
+
+ if (gelf_getshdr(scn, shdr) != shdr)
+ return 2;
+
+ *shname = elf_strptr(elf, ehdr->e_shstrndx, shdr->sh_name);
+ if (!*shname || !shdr->sh_size)
+ return 3;
+
+ *data = elf_getdata(scn, 0);
+ if (!*data || elf_getdata(scn, *data) != NULL)
+ return 4;
+
+ return 0;
+}
+
+static struct bpf_map_fixup fixup[128];
+
+static int parse_relo_into_map_fixup(Elf_Data *data, Elf_Data *symbols,
+ GElf_Shdr *shdr)
+{
+ GElf_Sym sym;
+ GElf_Rel rel;
+ int i, nrels;
+
+ nrels = shdr->sh_size / shdr->sh_entsize;
+
+ for (i = 0; i < nrels; i++) {
+ gelf_getrel(data, i, &rel);
+
+ fixup[i].insn_idx = rel.r_offset / sizeof(struct bpf_insn);
+
+ gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &sym);
+
+ fixup[i].fd = map_fd[sym.st_value / sizeof(struct bpf_map_def)];
+ }
+
+ return nrels;
+}
+
+int load_bpf_file(char *path)
+{
+ int fd, i, nrels;
+ Elf *elf;
+ GElf_Ehdr ehdr;
+ GElf_Shdr shdr, shdr_prog;
+ Elf_Data *data, *data_prog, *symbols = NULL;
+ char *shname, *shname_prog;
+
+ if (elf_version(EV_CURRENT) == EV_NONE)
+ return 1;
+
+ fd = open(path, O_RDONLY, 0);
+ if (fd < 0)
+ return 1;
+
+ elf = elf_begin(fd, ELF_C_READ, NULL);
+
+ if (!elf)
+ return 1;
+
+ if (gelf_getehdr(elf, &ehdr) != &ehdr)
+ return 1;
+
+ /* scan over all elf sections to get license and map info */
+ for (i = 1; i < ehdr.e_shnum; i++) {
+
+ if (get_sec(elf, i, &ehdr, &shname, &shdr, &data))
+ continue;
+
+ if (0)
+ printf("section %d:%s data %p size %zd link %d flags %d\n",
+ i, shname, data->d_buf, data->d_size,
+ shdr.sh_link, (int) shdr.sh_flags);
+
+ if (strcmp(shname, "license") == 0) {
+ processed_sec[i] = true;
+ memcpy(license, data->d_buf, data->d_size);
+ } else if (strcmp(shname, "maps") == 0) {
+ processed_sec[i] = true;
+ if (load_maps(data->d_buf, data->d_size))
+ return 1;
+ } else if (shdr.sh_type == SHT_SYMTAB) {
+ symbols = data;
+ }
+ }
+
+ /* load programs that need map fixup (relocations) */
+ for (i = 1; i < ehdr.e_shnum; i++) {
+
+ if (get_sec(elf, i, &ehdr, &shname, &shdr, &data))
+ continue;
+ if (shdr.sh_type == SHT_REL) {
+
+ if (get_sec(elf, shdr.sh_info, &ehdr, &shname_prog,
+ &shdr_prog, &data_prog))
+ continue;
+
+ if (0)
+ printf("relo %s into %s\n", shname, shname_prog);
+
+ processed_sec[shdr.sh_info] = true;
+ processed_sec[i] = true;
+
+ nrels = parse_relo_into_map_fixup(data, symbols, &shdr);
+
+ if (memcmp(shname_prog, "events/", sizeof("events/") - 1) == 0)
+ load_and_attach(shname_prog,
+ data_prog->d_buf, data_prog->d_size,
+ fixup, nrels * sizeof(fixup[0]));
+ }
+ }
+
+ /* load programs that don't use maps */
+ for (i = 1; i < ehdr.e_shnum; i++) {
+
+ if (processed_sec[i])
+ continue;
+
+ if (get_sec(elf, i, &ehdr, &shname, &shdr, &data))
+ continue;
+
+ if (memcmp(shname, "events/", sizeof("events/") - 1) == 0)
+ load_and_attach(shname, data->d_buf, data->d_size,
+ NULL, 0);
+ }
+
+ close(fd);
+ return 0;
+}
+
+void read_trace_pipe(void)
+{
+ int trace_fd;
+
+ trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
+ if (trace_fd < 0)
+ return;
+
+ while (1) {
+ static char buf[4096];
+ ssize_t sz;
+
+ sz = read(trace_fd, buf, sizeof(buf));
+ if (sz)
+ puts(buf);
+ }
+}
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
new file mode 100644
index 000000000000..d34e61a101d2
--- /dev/null
+++ b/samples/bpf/bpf_load.h
@@ -0,0 +1,18 @@
+#ifndef __BPF_LOAD_H
+#define __BPF_LOAD_H
+
+#define MAX_MAPS 64
+
+extern int map_fd[MAX_MAPS];
+
+/* parses elf file compiled by llvm .c->.o
+ * creates maps and stores FD into map_fd array
+ * loads eBPF programs
+ * returns zero on success
+ */
+int load_bpf_file(char *path);
+
+/* forever reads /sys/.../trace_pipe */
+void read_trace_pipe(void);
+
+#endif
--
1.7.9.5
^ permalink raw reply related
* [PATCH RFC v3 net-next 0/3] eBPF examples in C
From: Alexei Starovoitov @ 2014-07-22 3:45 UTC (permalink / raw)
To: David S. Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Arnaldo Carvalho de Melo, Jiri Olsa, Thomas Gleixner,
H. Peter Anvin, Andrew Morton, Kees Cook, linux-api, netdev,
linux-kernel
Hi,
I've been asked to repost LLVM eBPF backend and examples in C, so here they are.
LLVM backend is 99% the same as it was in Feb. Not resending it to the list,
since I only fixed minor things there. See it in my tree.
ex1 - is the same example I showed in Feb, but now it works through BPF syscall
and Ctrl-C does auto cleanup. This is patch 2/3
ex2 - is a new example that demonstrates key feature of eBPF programs
for kernel debugging/tracing. This is patch 3/3
dtrace/systemtap/ktap approach is to use one script file that should provide
all desired functionality. That architectural decision overcomplicated their
implementations.
eBPF follows split model: everything that needs to process millions of events
per second needs to run in kernel and needs to be short and deterministic,
all other things like aggregation and nice graphs should run in user space.
In the patch 3/3, kfree_skb events are counted by a program written in C,
compiled into eBPF and attached to the event. That's ex2_kern.c file.
The corresponding user space part is ex2_user.c which walks in-kernel map
every second and prints its contents. So user space and kernel are
accessing BPF maps in parallel. Kernel is counting events, user space
prints them.
Patch 1/3 is a parser of .o file generated by LLVM. It looks for pre-defined
ELF sections like 'license', 'maps', 'events' and loads bpf maps/programs
via BPF syscall that I posted earlier.
Alexei Starovoitov (3):
samples: bpf: elf file loader
samples: bpf: eBPF example in C
samples: bpf: eBPF dropmon example in C
samples/bpf/Makefile | 17 +++-
samples/bpf/bpf_helpers.h | 21 +++++
samples/bpf/bpf_load.c | 228 +++++++++++++++++++++++++++++++++++++++++++++
samples/bpf/bpf_load.h | 18 ++++
samples/bpf/ex1_kern.c | 27 ++++++
samples/bpf/ex1_user.c | 11 +++
samples/bpf/ex2_kern.c | 29 ++++++
samples/bpf/ex2_user.c | 28 ++++++
8 files changed, 377 insertions(+), 2 deletions(-)
create mode 100644 samples/bpf/bpf_helpers.h
create mode 100644 samples/bpf/bpf_load.c
create mode 100644 samples/bpf/bpf_load.h
create mode 100644 samples/bpf/ex1_kern.c
create mode 100644 samples/bpf/ex1_user.c
create mode 100644 samples/bpf/ex2_kern.c
create mode 100644 samples/bpf/ex2_user.c
----
The following changes since commit 240524089d7a5c0396656574e299beb3a55461e3:
net: bcmgenet: only update UMAC_CMD if something changed (2014-07-21 19:49:11 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/ast/bpf master
for you to fetch changes up to 27ae0cec12d0aac6f0705b0269ee705a0c599571:
samples: bpf: eBPF dropmon example in C (2014-07-21 20:01:29 -0700)
----------------------------------------------------------------
Alexei Starovoitov (20):
net: filter: split filter.c into two files
bpf: update MAINTAINERS entry
net: filter: rename struct sock_filter_int into bpf_insn
net: filter: split filter.h and expose eBPF to user space
bpf: introduce syscall(BPF, ...) and BPF maps
bpf: enable bpf syscall on x64
bpf: add lookup/update/delete/iterate methods to BPF maps
bpf: add hashtable type of BPF maps
bpf: expand BPF syscall with program load/unload
bpf: add eBPF verifier
bpf: allow eBPF programs to use maps
net: sock: allow eBPF programs to be attached to sockets
tracing: allow eBPF programs to be attached to events
samples: bpf: add mini eBPF library to manipulate maps and programs
samples: bpf: example of stateful socket filtering
samples: bpf: example of tracing filters with eBPF
bpf: llvm backend
samples: bpf: elf file loader
samples: bpf: eBPF example in C
samples: bpf: eBPF dropmon example in C
^ permalink raw reply
* Re: [RFC PATCH 5/5] futex, doc: add a document on how to use the spinning futexes
From: Waiman Long @ 2014-07-22 3:19 UTC (permalink / raw)
To: Randy Dunlap
Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Heiko Carstens, linux-kernel, linux-api,
linux-doc, Jason Low, Scott J Norton
In-Reply-To: <53CD3582.707@infradead.org>
On 07/21/2014 11:45 AM, Randy Dunlap wrote:
> On 07/21/2014 08:24 AM, Waiman Long wrote:
>> This patch adds a new document file on how to use the spinning futexes.
>>
>> Signed-off-by: Waiman Long<Waiman.Long@hp.com>
>> ---
>> Documentation/spinning-futex.txt | 109 ++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 109 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/spinning-futex.txt
>>
>> diff --git a/Documentation/spinning-futex.txt b/Documentation/spinning-futex.txt
>> new file mode 100644
>> index 0000000..e3cb5a2
>> --- /dev/null
>> +++ b/Documentation/spinning-futex.txt
>> @@ -0,0 +1,109 @@
>> +Started by: Waiman Long<waiman.long@hp.com>
>> +
>> +Spinning Futex
>> +--------------
>> +
>> +There are two main problems for a wait-wake futex (FUTEX_WAIT and
>> +FUTEX_WAKE) when used for creating user-space lock primitives:
>> +
>> + 1) With a wait-wake futex, tasks waiting for a lock are put to sleep
>> + in the futex queue to be woken up by the lock owner when it is done
>> + with the lock. Waking up a sleeping task, however, introduces some
>> + additional latency which can be large especially if the critical
>> + section protected by the lock is relatively short. This may cause
>> + a performance bottleneck on large systems with many CPUs running
>> + applications that need a lot of inter-thread synchronization.
>> +
>> + 2) The performance of the wait-wake futex is currently
>> + spinlock-constrained. When many threads are contending for a
>> + futex in a large system with many CPUs, it is not unusual to have
>> + spinlock contention accounting for more than 90% of the total
>> + CPU cycles consumed at various points in time.
>> +
>> +Spinning futex is a solution to both the wakeup latency and spinlock
>> +contention problems by optimistically spinning on a locked futex
>> +when the lock owner is running within the kernel until the lock is
>> +free. This is the same optimistic spinning mechanism used by the kernel
>> +mutex and rw semaphore implementations to improve performance. The
>> +optimistic spinning was done without taking any lock.
> is done
>
>> +
>> +Implementation
>> +--------------
>> +
>> +Like the PI and robust futexes, a lock acquirer has to atomically
>> +put its thread ID (TID) into the lower 30 bits of the 32-bit futex
>> +which should has an original value of 0. If it succeeds, it will be
> have
>
>> +the owner of the futex. Otherwise, it has to call into the kernel
>> +using the new FUTEX_SPIN_LOCK futex(2) syscall.
>> +
>> +The kernel will use the setting of the most significant bit
>> +(FUTEX_WAITERS) in the futex value to indicate one or more waiters
>> +are sleeping and need to be woken up later on.
>> +
>> +When it is time to unlock, the lock owner has to atomically clear
>> +the TID portion of the futex value. If the FUTEX_WAITERS bit is set,
>> +it has to issue a FUTEX_SPIN_UNLOCK futex system call to wake up the
>> +sleeping task.
>> +
>> +A return value of 1 from the FUTEX_SPIN_UNLOCK futex(2) syscall
>> +indicates a task has been woken up. The syscall returns 0 if no
>> +sleeping task is found or spinners are present to take the lock.
>> +
>> +The error number returned by a FUTEX_SPIN_UNLOCK call on an empty
>> +futex can be used to decide if the spinning futex functionality is
>> +implemented in the kernel. If it is present, the returned error number
>> +should be ESRCH. Otherwise it will be ENOSYS.
>> +
>> +Currently, only the first and the second arguments (the futex address
>> +and the opcode) of the futex(2) syscall is used. All the other
> are used.
>
>> +arguments must be set to 0 or NULL to avoid forward compatibility
>> +problem.
>> +
>> +The spinning futex requires the kernel to have support for the cmpxchg
>> +functionality. For architectures that don't support cmpxchg, spinning
>> +futex will not be supported as well.
>> +
>> +Usage Scenario
>> +--------------
>> +
>> +A spinning futex can be used as an exclusive lock to guard a critical
>> +section which are unlikely to go to sleep in the kernel. The spinners
> is
>
>> +in a spinning futex, however, will fall back to sleep in a wait queue
>> +if the lock owner isn't running. Therefore, it can also be used when
>> +the critical section is long and prone to sleeping. However, it may
>> +not have the performance benefit when compared with a wait-wake futex
>> +in this case.
>> +
>> +Sample Code
>> +-----------
>> +
>> +The following are sample code to implement a simple lock and unlock
> is
>
>> +function.
>> +
>> +__thread int tid; /* Thread ID */
>> +
>> +void mutex_lock(int *faddr)
>> +{
>> + if (cmpxchg(faddr, 0, tid) == 0)
>> + return;
>> + for (;;)
>> + if (futex(faddr, FUTEX_SPIN_LOCK, ...) == 0)
>> + break;
>> +}
>> +
>> +void mutex_unlock(int *faddr)
>> +{
>> + int old, fval;
>> +
>> + if ((fval = cmpxchg(faddr, tid, 0)) == tid)
>> + return;
>> + /* Clear only the TID portion of the futex */
>> + for (;;) {
>> + old = fval;
>> + fval = cmpxchg(faddr, old, old& ~FUTEX_TID_MASK);
>> + if (fval == old)
>> + break;
>> + }
>> + if (fval& FUTEX_WAITERS)
>> + futex(faddr, FUTEX_SPIN_UNLOCK, ...);
>> +}
>>
>
Thank for the grammar corrections. Will apply those to the documents.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Davidlohr Bueso @ 2014-07-22 3:06 UTC (permalink / raw)
To: Steven Rostedt
Cc: Thomas Gleixner, Darren Hart, Andy Lutomirski, Peter Zijlstra,
Andi Kleen, Waiman Long, Ingo Molnar, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <20140721213457.46623e2f@gandalf.local.home>
On Mon, 2014-07-21 at 21:34 -0400, Steven Rostedt wrote:
> On Tue, 22 Jul 2014 03:01:00 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Mon, 21 Jul 2014, Darren Hart wrote:
> > > On 7/21/14, 14:47, "Thomas Gleixner" <tglx@linutronix.de> wrote:
> > >
> > > >On Mon, 21 Jul 2014, Andy Lutomirski wrote:
> > > >> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org>
> > > >>wrote:
> > > >> > All this is predicated on the fact that syscalls are 'expensive'.
> > > >> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> > > >> > more expensive due to cacheline misses, which due to the size of the
> > > >> > things is almost guaranteed.
> > > >>
> > > >> 120 - 300 cycles for me, unless tracing happens, and I'm working on
> > > >> reducing the incidence of tracing.
> > > >
> > > >So it's a non issue indeed and definitely not worth the trouble of
> > > >that extra storage, the scheduler overhead, etc.
> > > >
> > > >Summary: Once you can't take it atomically in user space, you've lost
> > > > anyway. And we are better off to do the magic spinning in
> > > > kernel where we have all the information accessible already.
>
> I just want to point out that I was having a very nice conversation
> with Robert Haas (Cc'd) in Napa Valley at Linux Collaboration about
> this very topic. Robert is a PostgeSQL developer who told me that they
> implement their spin locks completely in userspace (no futex, just raw
> spinning on shared memory). This is because the sleep on contention of a
> futex has shown to be very expensive in their benchmarks.
Well sure, hashing, plist handling, wakeups etc. (not to mention
dirtying cachelines). The "fast" in futex is only due to the fact that
there is no jump to kernel space when the lock is uncontended after say,
a successful cmpxchg in user space. Once futex(2) is actually called,
performance is quite a different story.
> His work is
> not a micro benchmark but for a very popular database where locking is
> crucial.
>
> I was telling Robert that if futexes get optimistic spinning, he should
> reconsider their use of userspace spinlocks in favor of this, because
> I'm pretty sure that they will see a great improvement.
Agreed, with the critical regions being correctly sized for spinning
locks.
> Now Robert will be the best one to answer if the system call is indeed
> more expensive than doing full spins in userspace. If the spin is done
> in the kernel and they still get better performance by just spinning
> blindly in userspace even if the owner is asleep, I think we will have
> our answer.
>
> Note, I believe they only care about shared threads, and this
> optimistic spinning does not need to be something done between
> processes.
With the pi_state there would be no such distinction.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Mike Galbraith @ 2014-07-22 2:31 UTC (permalink / raw)
To: Steven Rostedt
Cc: Thomas Gleixner, Darren Hart, Andy Lutomirski, Peter Zijlstra,
Andi Kleen, Waiman Long, Ingo Molnar, Davidlohr Bueso,
Heiko Carstens, linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <20140721213457.46623e2f@gandalf.local.home>
On Mon, 2014-07-21 at 21:34 -0400, Steven Rostedt wrote:
> I was telling Robert that if futexes get optimistic spinning, he should
> reconsider their use of userspace spinlocks in favor of this, because
> I'm pretty sure that they will see a great improvement.
My (dated) experience with pgsql says you're likely right.
Once upon a time, preempting a userspace spinlock owner caused pgsql to
collapse into a quivering heap. The scheduler trying to hand the CPU
back to a preempted task instead of selecting what was strictly speaking
the most deserving task afterward (LAST_BUDDY) let pgsql+oltp scale nice
and flat instead of self destructing, but the root cause of collapse was
spinners lacking knowledge. With spin done in kernel, you can know
when spinning is a waste of perfectly good cycles.
-Mike
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Steven Rostedt @ 2014-07-22 1:34 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Darren Hart, Andy Lutomirski, Peter Zijlstra, Andi Kleen,
Waiman Long, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <alpine.DEB.2.10.1407220047490.20847@nanos>
On Tue, 22 Jul 2014 03:01:00 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 21 Jul 2014, Darren Hart wrote:
> > On 7/21/14, 14:47, "Thomas Gleixner" <tglx@linutronix.de> wrote:
> >
> > >On Mon, 21 Jul 2014, Andy Lutomirski wrote:
> > >> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org>
> > >>wrote:
> > >> > All this is predicated on the fact that syscalls are 'expensive'.
> > >> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> > >> > more expensive due to cacheline misses, which due to the size of the
> > >> > things is almost guaranteed.
> > >>
> > >> 120 - 300 cycles for me, unless tracing happens, and I'm working on
> > >> reducing the incidence of tracing.
> > >
> > >So it's a non issue indeed and definitely not worth the trouble of
> > >that extra storage, the scheduler overhead, etc.
> > >
> > >Summary: Once you can't take it atomically in user space, you've lost
> > > anyway. And we are better off to do the magic spinning in
> > > kernel where we have all the information accessible already.
I just want to point out that I was having a very nice conversation
with Robert Haas (Cc'd) in Napa Valley at Linux Collaboration about
this very topic. Robert is a PostgeSQL developer who told me that they
implement their spin locks completely in userspace (no futex, just raw
spinning on shared memory). This is because the sleep on contention of a
futex has shown to be very expensive in their benchmarks. His work is
not a micro benchmark but for a very popular database where locking is
crucial.
I was telling Robert that if futexes get optimistic spinning, he should
reconsider their use of userspace spinlocks in favor of this, because
I'm pretty sure that they will see a great improvement.
Now Robert will be the best one to answer if the system call is indeed
more expensive than doing full spins in userspace. If the spin is done
in the kernel and they still get better performance by just spinning
blindly in userspace even if the owner is asleep, I think we will have
our answer.
Note, I believe they only care about shared threads, and this
optimistic spinning does not need to be something done between
processes.
> >
> > And we have such an implementation with the FUTEX_LOCK_ADAPTIVE code we
> > discussed back in Oct 2010 (purely kernel, no VDSO), updated with some of
> > your and other's comments:
> >
> > http://git.infradead.org/users/dvhart/linux.git/shortlog/refs/heads/futex/f
> > utex-lock/v7
> >
> >
> > I can work on forward porting this series to current mainline (post recent
> > security fixes) and cleaning up the commentary and such if people are
> > interested in seeing this implementation (based on Peter Z's spinning
> > mutex work iirc) resurrected...
>
> No. We really want to avoid more magic hackery in the futex code.
>
> Lets sit down and think about what we need:
>
> 1) Support for all existing features
>
> 2) More fine grained locking
>
> 3) Optimistic spinning
>
> @1: No discussion about that. Period.
>
> We are not going to introduce some new futex opcode, which is
> optimized for a microbenchmark and ignoring all of the pain we
> went through in the last 10 years. No way, especially after the
> recent security disaster.
>
> @2 and @3: Yes, we want that.
>
> But again, we don't add fine grained locking just for some half
> baken new opcode. No, we are adding it where it makes the most
> sense and lets us reuse most of the code.
>
> I can predict your question, how that should work :)
>
> If we want to have fine grained locking beyond the futex hash
> buckets and that's something we discussed before, you need a state
> representation of the user space futex in the kernel. That's what
> Waiman added as well, just in a way that's beyond repair.
>
> The charm of the futex hash buckets is that they do not create
> kernel state because the futex_q which is enqueued into the bucket
> is on the task stack of the task which is blocked on the futex.
>
> So much for the theory, because that stopped to be true when we
> added support for PI futexes. They already create kernel internal
> state which is not magically removed when the thread exits the
> syscall. It's called pi_state.
>
> Now the question is how is this related to the non PI magic
> spinning optimization? Very much so.
>
> Simply because the handling of pi_state already covers ALL the
> corner cases which come with the extra kernel internal state and
> they provide the full function set of futexes required by the
> various (ab)use cases including the requeue functionality which is
> required for condvars. It even supports caching of the pi_state
> struct in a way which makes sense, rather than blindly slapping a
> cached entry onto each hash bucket.
>
> So it's not a really mind boggling thought experiment to think
> about this magic new feature as a subset of the already existing
> PI futex code. It is a subset, just minus the PI portion plus the
> more fine grained locking.
>
> So the right solution is to rework the existing code.
>
> 1) Make pi_state the entity which gets enqueued into the hash bucket
> and manage the waiters in a pi_state internal queue.
>
> Straight forward problem as it is still protected by the hash
> bucket lock.
>
> 2) Add private locking to the pi_state to reduce the lock contention
> on the hash bucket lock.
>
> Straight forward conversion as well
>
> 3) Make mutex a subset of rt_mutex
+1
>
> That makes a lot of sense, simply because a mutex is the same as a
> rtmutex just without the PI part.
>
> Just for the record before anyone cries murder: The original mutex
> implemention was modeled after the rtmutex one in the first place
> and the optimistic spinning was first introduced on rtmutexes by
> Gregory Haskins et. al. in the context of the RT tree. Back then it
> was too early or we were simply too stupid to see the similarities
> and opted for a complete separate implementation.
I believe it was more about politics to why it was a separate solution.
Technically, I think they should have been one.
>
> Yes, I'm aware that this is a non-trivial problem, but if you look
> at the details, then the non-trivial part is in the slow path were
> stuff actually goes to sleep. The fast path of mutex and rt_mutex
> is simply the same. So there is no reason to keep them separate. An
> extra conditional in the slow path is not going to hurt at all.
>
> Surely, you might say, that I'm an egoistic bastard, who just wants
> to have the benefit of MCS for rtmutex for free. Right you are, but
> there is a whole bunch of reasons why this makes tons of sense:
>
> - As I said above, it's almost the same, except for the slow path
> details, where a few extra conditionals do not matter
>
> - Sharing the code has the obvious benefits. And it's not only
> sharing between rtmutex and mutex, it's also sharing the
> underlying optimizations of MCS for the kernel internal users and
> the user space exposed ones, i.e. futexes.
>
> - Once unified the desired futex functionality just works with a
> minimal impact on the futex code itself.
>
> The futex code does not care whether the underlying primitive has
> PI semantics or not, it just cares that all the required features
> like proxy locking etc. are in place. And that everything like
> timeouts, requeue etc. is just supported.
>
> - The semantics of the new futex functionality has not to be
> defined again. We can simply reuse the rather well defined and
> well tested straight forward semantics of the existing PI
> opcodes.
>
> - That minimizes the risk massively, because all state and error
> handling is common code.
>
> - Any update to the underlying primitives will benefit ALL usage
> sites and reduces the copy and paste induced hell of MCS code
> sprinkled all over the kernel.
>
> 4) Enable user space to make use of it.
>
> Whether this will be a separate opcode or just a new flag to the
> existing PI opcodes is going to be just a bikeshedding question at
> that point. :)
>
> The important point is, that we do NOT grow any new unmaintainable
> warts to the futex code. And the 650+ new lines of hackery which come
> with the proposed patch set are in no way acceptable, especially as
> they only cover the minimalistic functionality set of futexes. Not to
> talk about the other issues I've observed on the first glance.
>
> So before anyone comes up with a "solution" for all of this tomorrow
> afternoon in form of another half baken patch, please sit back mull it
> in your head and lets have a proper discussion about the approach
> first.
>
If we can have that schedule_out() threads_on_cpu, that could be a
helpful solution to see if the system call is indeed expensive. Sounds
like we can have both an in kernel solution and a userspace one as well.
-- Steve
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-22 1:01 UTC (permalink / raw)
To: Darren Hart
Cc: Andy Lutomirski, Peter Zijlstra, Andi Kleen, Waiman Long,
Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton,
Steven Rostedt
In-Reply-To: <CFF2E3D6.9D525%dvhart@linux.intel.com>
On Mon, 21 Jul 2014, Darren Hart wrote:
> On 7/21/14, 14:47, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>
> >On Mon, 21 Jul 2014, Andy Lutomirski wrote:
> >> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org>
> >>wrote:
> >> > All this is predicated on the fact that syscalls are 'expensive'.
> >> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> >> > more expensive due to cacheline misses, which due to the size of the
> >> > things is almost guaranteed.
> >>
> >> 120 - 300 cycles for me, unless tracing happens, and I'm working on
> >> reducing the incidence of tracing.
> >
> >So it's a non issue indeed and definitely not worth the trouble of
> >that extra storage, the scheduler overhead, etc.
> >
> >Summary: Once you can't take it atomically in user space, you've lost
> > anyway. And we are better off to do the magic spinning in
> > kernel where we have all the information accessible already.
>
> And we have such an implementation with the FUTEX_LOCK_ADAPTIVE code we
> discussed back in Oct 2010 (purely kernel, no VDSO), updated with some of
> your and other's comments:
>
> http://git.infradead.org/users/dvhart/linux.git/shortlog/refs/heads/futex/f
> utex-lock/v7
>
>
> I can work on forward porting this series to current mainline (post recent
> security fixes) and cleaning up the commentary and such if people are
> interested in seeing this implementation (based on Peter Z's spinning
> mutex work iirc) resurrected...
No. We really want to avoid more magic hackery in the futex code.
Lets sit down and think about what we need:
1) Support for all existing features
2) More fine grained locking
3) Optimistic spinning
@1: No discussion about that. Period.
We are not going to introduce some new futex opcode, which is
optimized for a microbenchmark and ignoring all of the pain we
went through in the last 10 years. No way, especially after the
recent security disaster.
@2 and @3: Yes, we want that.
But again, we don't add fine grained locking just for some half
baken new opcode. No, we are adding it where it makes the most
sense and lets us reuse most of the code.
I can predict your question, how that should work :)
If we want to have fine grained locking beyond the futex hash
buckets and that's something we discussed before, you need a state
representation of the user space futex in the kernel. That's what
Waiman added as well, just in a way that's beyond repair.
The charm of the futex hash buckets is that they do not create
kernel state because the futex_q which is enqueued into the bucket
is on the task stack of the task which is blocked on the futex.
So much for the theory, because that stopped to be true when we
added support for PI futexes. They already create kernel internal
state which is not magically removed when the thread exits the
syscall. It's called pi_state.
Now the question is how is this related to the non PI magic
spinning optimization? Very much so.
Simply because the handling of pi_state already covers ALL the
corner cases which come with the extra kernel internal state and
they provide the full function set of futexes required by the
various (ab)use cases including the requeue functionality which is
required for condvars. It even supports caching of the pi_state
struct in a way which makes sense, rather than blindly slapping a
cached entry onto each hash bucket.
So it's not a really mind boggling thought experiment to think
about this magic new feature as a subset of the already existing
PI futex code. It is a subset, just minus the PI portion plus the
more fine grained locking.
So the right solution is to rework the existing code.
1) Make pi_state the entity which gets enqueued into the hash bucket
and manage the waiters in a pi_state internal queue.
Straight forward problem as it is still protected by the hash
bucket lock.
2) Add private locking to the pi_state to reduce the lock contention
on the hash bucket lock.
Straight forward conversion as well
3) Make mutex a subset of rt_mutex
That makes a lot of sense, simply because a mutex is the same as a
rtmutex just without the PI part.
Just for the record before anyone cries murder: The original mutex
implemention was modeled after the rtmutex one in the first place
and the optimistic spinning was first introduced on rtmutexes by
Gregory Haskins et. al. in the context of the RT tree. Back then it
was too early or we were simply too stupid to see the similarities
and opted for a complete separate implementation.
Yes, I'm aware that this is a non-trivial problem, but if you look
at the details, then the non-trivial part is in the slow path were
stuff actually goes to sleep. The fast path of mutex and rt_mutex
is simply the same. So there is no reason to keep them separate. An
extra conditional in the slow path is not going to hurt at all.
Surely, you might say, that I'm an egoistic bastard, who just wants
to have the benefit of MCS for rtmutex for free. Right you are, but
there is a whole bunch of reasons why this makes tons of sense:
- As I said above, it's almost the same, except for the slow path
details, where a few extra conditionals do not matter
- Sharing the code has the obvious benefits. And it's not only
sharing between rtmutex and mutex, it's also sharing the
underlying optimizations of MCS for the kernel internal users and
the user space exposed ones, i.e. futexes.
- Once unified the desired futex functionality just works with a
minimal impact on the futex code itself.
The futex code does not care whether the underlying primitive has
PI semantics or not, it just cares that all the required features
like proxy locking etc. are in place. And that everything like
timeouts, requeue etc. is just supported.
- The semantics of the new futex functionality has not to be
defined again. We can simply reuse the rather well defined and
well tested straight forward semantics of the existing PI
opcodes.
- That minimizes the risk massively, because all state and error
handling is common code.
- Any update to the underlying primitives will benefit ALL usage
sites and reduces the copy and paste induced hell of MCS code
sprinkled all over the kernel.
4) Enable user space to make use of it.
Whether this will be a separate opcode or just a new flag to the
existing PI opcodes is going to be just a bikeshedding question at
that point. :)
The important point is, that we do NOT grow any new unmaintainable
warts to the futex code. And the 650+ new lines of hackery which come
with the proposed patch set are in no way acceptable, especially as
they only cover the minimalistic functionality set of futexes. Not to
talk about the other issues I've observed on the first glance.
So before anyone comes up with a "solution" for all of this tomorrow
afternoon in form of another half baken patch, please sit back mull it
in your head and lets have a proper discussion about the approach
first.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Davidlohr Bueso @ 2014-07-22 0:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Zijlstra, Thomas Gleixner, Darren Hart, Andi Kleen,
Waiman Long, Ingo Molnar, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton
In-Reply-To: <CALCETrXSaWM6-nxr4ggtMh5qcfTQg4wRvkW8FTSbri5+_1ZbVg@mail.gmail.com>
On Mon, 2014-07-21 at 14:31 -0700, Andy Lutomirski wrote:
> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > All this is predicated on the fact that syscalls are 'expensive'.
> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> > more expensive due to cacheline misses, which due to the size of the
> > things is almost guaranteed.
>
> 120 - 300 cycles for me, unless tracing happens, and I'm working on
> reducing the incidence of tracing.
fwiw here's what lmbench's lat_ctx says on my system . For 'accuracy', I
kept the runs short.
http://www.stgolabs.net/lat_ctx.png
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Darren Hart @ 2014-07-21 22:41 UTC (permalink / raw)
To: Thomas Gleixner, Andy Lutomirski
Cc: Peter Zijlstra, Andi Kleen, Waiman Long, Ingo Molnar,
Davidlohr Bueso, Heiko Carstens, linux-kernel@vger.kernel.org,
Linux API, linux-doc@vger.kernel.org, Jason Low, Scott J Norton
In-Reply-To: <alpine.DEB.2.10.1407212344130.20847@nanos>
On 7/21/14, 14:47, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Mon, 21 Jul 2014, Andy Lutomirski wrote:
>> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org>
>>wrote:
>> > All this is predicated on the fact that syscalls are 'expensive'.
>> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
>> > more expensive due to cacheline misses, which due to the size of the
>> > things is almost guaranteed.
>>
>> 120 - 300 cycles for me, unless tracing happens, and I'm working on
>> reducing the incidence of tracing.
>
>So it's a non issue indeed and definitely not worth the trouble of
>that extra storage, the scheduler overhead, etc.
>
>Summary: Once you can't take it atomically in user space, you've lost
> anyway. And we are better off to do the magic spinning in
> kernel where we have all the information accessible already.
And we have such an implementation with the FUTEX_LOCK_ADAPTIVE code we
discussed back in Oct 2010 (purely kernel, no VDSO), updated with some of
your and other's comments:
http://git.infradead.org/users/dvhart/linux.git/shortlog/refs/heads/futex/f
utex-lock/v7
I can work on forward porting this series to current mainline (post recent
security fixes) and cleaning up the commentary and such if people are
interested in seeing this implementation (based on Peter Z's spinning
mutex work iirc) resurrected...
--
Darren Hart Open Source Technology Center
darren.hart@intel.com Intel Corporation
^ permalink raw reply
* Re: [PATCH -v4] random: introduce getrandom(2) system call
From: Theodore Ts'o @ 2014-07-21 22:23 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, beck-7YlrpqBBQ3VAfugRpC6u6w,
deraadt-VON6Tr2NNtkmbxgs1yVkuA
In-Reply-To: <20140721202125.GF1158@osiris>
On Mon, Jul 21, 2014 at 10:21:26PM +0200, Till Smejkal wrote:
> Hi,
>
> On Fri, 18 Jul 2014, Theodore Ts'o wrote:
> [...]
> > If the GRND_RANDOM bit is not set, then the /dev/urandom pool
> > will be used. Unlike using read(2) to fetch data from
> > /dev/urandom, if the urandom pool has not been sufficiently
> > initialized, getrandom(2) will block or return -1 with the
> > errno set to EGAIN if the GRND_NONBLOCK bit is set in flags.
> ^^^^^
> Small typo: this should be EAGAIN.
Thanks for pointing this out! Will fix.
- Ted
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Andy Lutomirski @ 2014-07-21 22:16 UTC (permalink / raw)
To: Aditya Kali
Cc: Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, Linux API, Tejun Heo,
Ingo Molnar
In-Reply-To: <CAGr1F2Fd_4=WUm4STPd4kdd5tNLO6aQ1OOQMKnRqyOKZSGvCpg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Jul 21, 2014 at 3:11 PM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Jul 18, 2014 at 11:57 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Fri, Jul 18, 2014 at 11:51 AM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Fri, Jul 18, 2014 at 9:51 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>> On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>
>>>>> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>> > What happens if someone moves a task in a cgroup namespace outside of
>>>>> > the namespace root cgroup?
>>>>> >
>>>>>
>>>>> Attempt to move a task outside of cgroupns root will fail with EPERM.
>>>>> This is true irrespective of the privileges of the process attempting
>>>>> this. Once cgroupns is created, the task will be confined to the
>>>>> cgroup hierarchy under its cgroupns root until it dies.
>>>>
>>>> Can a task in a non-init userns create a cgroupns? If not, that's
>>>> unusual. If so, is it problematic if they can prevent themselves from
>>>> being moved?
>>>>
>>>
>>> Currently, only a task with CAP_SYS_ADMIN in the init-userns can
>>> create cgroupns. It is stricter than for other namespaces, yes.
>>
>> I'm slightly hesitant to have unshare(CLONE_NEWUSER |
>> CLONE_NEWCGROUPNS | ...) start having weird side effects that are
>> visible outside the namespace, especially when those side effects
>> don't happen (because the call fails entirely) if
>> unshare(CLONE_NEWUSER) happens first. I don't see a real problem with
>> it, but it's weird.
>>
>
> I expect this to be only in the initial version of the patch. We can
> make this consistent with other namespaces once we figure out how
> cgroupns can be safely enabled for non-init-userns.
>
>>>
>>>> I hate to say it, but it might be worth requiring explicit permission
>>>> from the cgroup manager for this. For example, there could be a new
>>>> cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
>>>> will fail with -EPERM unless the caller is in a may_share=1 cgroup.
>>>> may_unshare in a parent cgroup would not give child cgroups the
>>>> ability to unshare.
>>>>
>>>
>>> What you suggest can be done. The current patch-set punts the problem
>>> of permission checking by only allowing unshare from a
>>> capable(CAP_SYS_ADMIN) process. This can be implemented as a follow-up
>>> improvement to cgroupns feature if we want to open it to non-init
>>> userns.
>>>
>>> Being said that, I would argue that even if we don't have this
>>> explicit permission and relax the check to non-init userns, it should
>>> be 'OK' to let ns_capable(current_user_ns(), CAP_SYS_ADMIN) tasks to
>>> unshare cgroupns (basically, if you can "create" a cgroup hierarchy,
>>> you should probably be allowed to unshare() it).
>>
>> But non-init-userns tasks can't create cgroup hierarchies, unless I
>> misunderstand the current code. And, if they can, I bet I can find
>> three or four serious security issues in an hour or two. :)
>>
>
> Task running in non-init userns can create cgroup hierarchies if you
> chown/chgrp their cgroup root to the task user:
Won't the systemd people hate you forever for this suggestion? (I do
exactly this myself...)
> This is a powerful feature as it allows non-root tasks to run
> container-management tools and provision their resources properly. But
> this makes implementing your suggestion of having 'cgroup.may_unshare'
> file tricky as the cgroup owner (task) will be able to set it and
> still unshare cgroupns. Instead, may be we could just check if the
> task has appropriate (write?) permissions on the cgroup directory
> before allowing nested cgroupns creation.
I bet that systemd will want to set may_unshare but not give write
access. Who knows?
> [shudder]
> I am surprised that this even works correctly.
>
> Either way, may be checking cgroup directory permissions will work for
> you? i.e., if you "chown" a cgroup directory to the user, it should be
> OK if the user's task unshares cgroupns under that cgroup and you
> don't care about moving tasks from under that cgroup. Without
> ownership of the cgroup directory, creation of cgroupns will be
> disallowed. What do you think?
I think this is *safe* but may not useful for eventual systemd stuff.
Not really sure.
--Andy
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Aditya Kali @ 2014-07-21 22:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, Linux API, Tejun Heo,
Ingo Molnar
In-Reply-To: <CALCETrVeeL71sfVdbzRx0FpGrvQKbviEmUcMEosbUU+UJNQu9w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jul 18, 2014 at 11:57 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Jul 18, 2014 at 11:51 AM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, Jul 18, 2014 at 9:51 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>> > What happens if someone moves a task in a cgroup namespace outside of
>>>> > the namespace root cgroup?
>>>> >
>>>>
>>>> Attempt to move a task outside of cgroupns root will fail with EPERM.
>>>> This is true irrespective of the privileges of the process attempting
>>>> this. Once cgroupns is created, the task will be confined to the
>>>> cgroup hierarchy under its cgroupns root until it dies.
>>>
>>> Can a task in a non-init userns create a cgroupns? If not, that's
>>> unusual. If so, is it problematic if they can prevent themselves from
>>> being moved?
>>>
>>
>> Currently, only a task with CAP_SYS_ADMIN in the init-userns can
>> create cgroupns. It is stricter than for other namespaces, yes.
>
> I'm slightly hesitant to have unshare(CLONE_NEWUSER |
> CLONE_NEWCGROUPNS | ...) start having weird side effects that are
> visible outside the namespace, especially when those side effects
> don't happen (because the call fails entirely) if
> unshare(CLONE_NEWUSER) happens first. I don't see a real problem with
> it, but it's weird.
>
I expect this to be only in the initial version of the patch. We can
make this consistent with other namespaces once we figure out how
cgroupns can be safely enabled for non-init-userns.
>>
>>> I hate to say it, but it might be worth requiring explicit permission
>>> from the cgroup manager for this. For example, there could be a new
>>> cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
>>> will fail with -EPERM unless the caller is in a may_share=1 cgroup.
>>> may_unshare in a parent cgroup would not give child cgroups the
>>> ability to unshare.
>>>
>>
>> What you suggest can be done. The current patch-set punts the problem
>> of permission checking by only allowing unshare from a
>> capable(CAP_SYS_ADMIN) process. This can be implemented as a follow-up
>> improvement to cgroupns feature if we want to open it to non-init
>> userns.
>>
>> Being said that, I would argue that even if we don't have this
>> explicit permission and relax the check to non-init userns, it should
>> be 'OK' to let ns_capable(current_user_ns(), CAP_SYS_ADMIN) tasks to
>> unshare cgroupns (basically, if you can "create" a cgroup hierarchy,
>> you should probably be allowed to unshare() it).
>
> But non-init-userns tasks can't create cgroup hierarchies, unless I
> misunderstand the current code. And, if they can, I bet I can find
> three or four serious security issues in an hour or two. :)
>
Task running in non-init userns can create cgroup hierarchies if you
chown/chgrp their cgroup root to the task user:
# while running as 'root' (uid=0)
$ cd $CGROUP_MOUNT
$ mkdir -p batchjobs/c_job_id1/
# transfer ownership to the user (in this case 'nobody' (uid=99)).
$ chown nobody batchjobs/c_job_id1/
$ chgrp nobody batchjobs/c_job_id1/
$ ls -ld batchjobs/c_job_id1/
drwxr-xr-x 2 nobody nobody 0 2014-07-21 12:47 batchjobs/c_job_id1/
# enter container cgroup
$ echo 0 > batchjobs/c_job_id1/cgroup.procs
# unshare both userns and cgroupns
$ unshare -u -c
# setup uid_map and gid_map and export user '99' in the userns
# $ cat /proc/<pid>/uid_map
# 0 0 1
# 99 99 1
# $ cat /proc/<pid>/gid_map
# 0 0 1
# 99 99 1
# switch to user 'nobody'
$ su nobody
$ id
uid=99(nobody) gid=99(nobody) groups=99(nobody)
# Now user nobody running under non-init userns can create sub-cgroups
# under "batchjobs/c_job_id1/".
# PWD=$CGROUP_MOUNT/batchjobs/c_job_id1
$ mkdir sub_cgroup1
$ ls -ld sub_cgroup1/
drwxr-xr-x 2 nobody nobody 0 2014-07-21 13:11 sub_cgroup1/
$ echo 0 > sub_cgroup1/cgroup.procs
$ cat /proc/self/cgroup
0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/sub_cgroup1
$ ls -l sub_cgroup1/
total 0
-r--r--r-- 1 nobody nobody 0 2014-07-21 13:11 cgroup.controllers
-r--r--r-- 1 nobody nobody 0 2014-07-21 13:11 cgroup.populated
-rw-r--r-- 1 nobody nobody 0 2014-07-21 13:12 cgroup.procs
-rw-r--r-- 1 nobody nobody 0 2014-07-21 13:11 cgroup.subtree_control
This is a powerful feature as it allows non-root tasks to run
container-management tools and provision their resources properly. But
this makes implementing your suggestion of having 'cgroup.may_unshare'
file tricky as the cgroup owner (task) will be able to set it and
still unshare cgroupns. Instead, may be we could just check if the
task has appropriate (write?) permissions on the cgroup directory
before allowing nested cgroupns creation.
>> By unsharing
>> cgroupns, the tasks can only confine themselves further under its
>> cgroupns-root. As long as it cannot escape that hierarchy, it should
>> be fine.
>
> But they can also *lock* their hierarchy.
>
But locking the tasks inside the hierarchy is really what cgroupns
feature is trying to provide. I understand that this is a change in
expectation, but with unified hierarchy, there are already
restrictions on where tasks can be moved (only to leaf cgroups). With
cgroup namespaces, this becomes: "only to leaf cgroups within task's
cgroupns".
>> In my experience, there is seldom a need to move tasks out of their
>> cgroup. At most, we create a sub-cgroup and move the task there (which
>> is allowed in their cgroupns). Even for a cgroup manager, I can't
>> think of a case where it will be useful to move a task from one cgroup
>> hierarchy to another. Such move seems overly complicated (even without
>> cgroup namespaces). The cgroup manager can just modify the settings of
>> the task's cgroup as needed or simply kill & restart the task in a new
>> container.
>>
>
> I do this all the time. Maybe my new systemd overlords will make me
> stop doing it, at which point my current production setup will blow
> up.
>
[shudder]
I am surprised that this even works correctly.
Either way, may be checking cgroup directory permissions will work for
you? i.e., if you "chown" a cgroup directory to the user, it should be
OK if the user's task unshares cgroupns under that cgroup and you
don't care about moving tasks from under that cgroup. Without
ownership of the cgroup directory, creation of cgroupns will be
disallowed. What do you think?
> --Andy
--
Aditya
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-21 21:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Zijlstra, Darren Hart, Andi Kleen, Waiman Long, Ingo Molnar,
Davidlohr Bueso, Heiko Carstens, linux-kernel@vger.kernel.org,
Linux API, linux-doc@vger.kernel.org, Jason Low, Scott J Norton
In-Reply-To: <CALCETrXSaWM6-nxr4ggtMh5qcfTQg4wRvkW8FTSbri5+_1ZbVg@mail.gmail.com>
On Mon, 21 Jul 2014, Andy Lutomirski wrote:
> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > All this is predicated on the fact that syscalls are 'expensive'.
> > Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> > more expensive due to cacheline misses, which due to the size of the
> > things is almost guaranteed.
>
> 120 - 300 cycles for me, unless tracing happens, and I'm working on
> reducing the incidence of tracing.
So it's a non issue indeed and definitely not worth the trouble of
that extra storage, the scheduler overhead, etc.
Summary: Once you can't take it atomically in user space, you've lost
anyway. And we are better off to do the magic spinning in
kernel where we have all the information accessible already.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-21 21:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Darren Hart, Andi Kleen, Waiman Long, Ingo Molnar,
Davidlohr Bueso, Heiko Carstens, linux-kernel, linux-api,
linux-doc, Jason Low, Scott J Norton
In-Reply-To: <20140721212740.GS3935@laptop>
On Mon, 21 Jul 2014, Peter Zijlstra wrote:
> On Mon, Jul 21, 2014 at 10:16:37PM +0200, Thomas Gleixner wrote:
> > On Mon, 21 Jul 2014, Darren Hart wrote:
> > > We observed some significant improvements under some very specific use
> > > cases, but a more thorough dive into performance impact in the other cases
> > > as well as security implications with the vdso is still wanting.
> >
> > The security implication is that the feature can only be available for
> > process private futexes. There is no way to expose information which
> > crosses the process spaces.
> >
> > But the way worse issue is storage.
> >
> > While you can cache the namespace specific TID of a thread in the
> > task_struct, you still need a O(1) zero overhead mechanism to update
> > the thread state (only on/off cpu is interesting) in a per process
> > shared data structure from the guts of schedule()
> >
> > For that you have basically two choices:
> >
> > 1) cpu_thread_id[NR_CPUS]
> >
> > Simple to update from the scheduler, and a halfways moderate
> > storage size (NR_CPUS * 4 bytes) in the worst case, i.e. 16k
> > today. Set to 0 on scheduling out and to the namespace specific TID
> > on scheduling in.
> >
> > But that requires a linear search in the user space spin loop. And
> > that's required for every iteration of the loop. Can you imagine
> > how well that works performance wise?
> >
> > 2) Bitmap threads_on_cpu
> >
> > Again, simple to update from the scheduler, cache line bouncing
> > issues aside. Clear the bit on schedule out and set it on schedule
> > in.
> >
> > But the bitmap needs the size of PID_MAX_LIMIT, which is a whopping
> > 512k per process in the worst case.
> >
> > Anything else would involve search/lookup schemes which are just
> > overkill in both the scheduler and the user space loop.
> >
> > Now for enhanced fun you need immutable pages for that storage, as you
> > can't have pagefaults in the guts of schedule().
> >
> > So once you found a way to make that opt-in as you don't want inflict
> > any of this to all processes by default, it might be a worthwhile
> > optimization. So the probably tolerable impact on schedule() would be
> >
> > schedule_out()
> > if (curr->threads_on_cpu)
> > clear_bit(curr->ns_tid, curr->threads_on_cpu);
> > and
> >
> > schedule_in()
> > if (curr->threads_on_cpu)
> > clear_bit(curr->ns_tid, curr->threads_on_cpu);
> >
> > Anything more complex is just going to defeat the whole purpose.
>
> All this is predicated on the fact that syscalls are 'expensive'.
> Weren't syscalls only 100s of cycles? All this bitmap mucking is far
> more expensive due to cacheline misses, which due to the size of the
> things is almost guaranteed.
I completely agree.
This wants to backed by proper numbers taken from a proper
implementation and not from some randomly cobbled together works for
me hackery.
As I said: It might be a worthwhile optimization....
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-21 21:41 UTC (permalink / raw)
To: Ingo Molnar
Cc: Waiman Long, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
Heiko Carstens, linux-kernel, linux-api, linux-doc, Jason Low,
Scott J Norton
In-Reply-To: <20140721211801.GA12149@gmail.com>
On Mon, 21 Jul 2014, Ingo Molnar wrote:
> * Waiman Long <Waiman.Long@hp.com> wrote:
>
> > Testing done on a 4-socket Westmere-EX boxes with 40 cores (HT off)
> > showed the following performance data (average kops/s) with various
> > load factor (number of pause instructions) used in the critical
> > section using an userspace mutex microbenchmark.
> >
> > Threads Load Waiting Futex Spinning Futex %Change
> > ------- ---- ------------- -------------- -------
> > 256 1 6894 8883 +29%
> > 256 10 3656 4912 +34%
> > 256 50 1332 4358 +227%
> > 256 100 792 2753 +248%
> > 10 1 6382 4838 -24%
> > 10 10 3614 4748 +31%
> > 10 50 1319 3900 +196%
> > 10 100 782 2459 +214%
> > 2 1 7905 7194 -9.0%
> > 2 10 4556 4717 +3.5%
> > 2 50 2191 4167 +90%
> > 2 100 1767 2407 +36%
>
> So the numbers look interesting - but it would be _really_ important
> to provide noise/sttdev figures in a sixth column as well (denoted in
> percentage units, not in benchmark units), so that we know how
> significant a particular speedup (or slowdown) is.
We care about that, once we have something which
- Has a proper design
- Covers all the corner cases of futexes
- Does not introduce a gazillions of new lines of codes in futex.c
- Does not create another set of subtle security issues. I'm so not
interested to do the same exercise again - my brain still suffers.
The numbers provided are completely irrelevant as the implementation
is just the most idiotic approach to avoid all corner cases of
futexes, error handling and the proper treatment of detached kernel
state for the price of adding a completely unreviewable clusterfuck to
futex.c.
So, no. Don't encourage that number wankery any further. It's going
nowhere, period.
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