* [PATCH v6 7/8] devicetree: mfd: max14577: Add device tree bindings document
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-1-git-send-email-k.kozlowski@samsung.com>
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@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: devicetree@vger.kernel.org
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
Acked-by: Andrew Morton <akpm@linux-foundation.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
* [PATCH v6 6/8] power: max17040: Add ID for MAX77836 Fuel Gauge block
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-1-git-send-email-k.kozlowski@samsung.com>
MAX77836 has the same Fuel Gauge as MAX17040/17048. The max17040 driver
can be safely re-used. The patch adds MAX77836 device to the array of
i2c_device_id. Additionally it removes the id associated with MAX17040
device as the value is not used.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/power/max17040_battery.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/power/max17040_battery.c b/drivers/power/max17040_battery.c
index 0fbac861080d..14d44706327b 100644
--- a/drivers/power/max17040_battery.c
+++ b/drivers/power/max17040_battery.c
@@ -277,7 +277,8 @@ static SIMPLE_DEV_PM_OPS(max17040_pm_ops, max17040_suspend, max17040_resume);
#endif /* CONFIG_PM_SLEEP */
static const struct i2c_device_id max17040_id[] = {
- { "max17040", 0 },
+ { "max17040" },
+ { "max77836-battery" },
{ }
};
MODULE_DEVICE_TABLE(i2c, max17040_id);
--
1.9.1
^ permalink raw reply related
* [PATCH v6 5/8] charger: max14577: Configure battery-dependent settings from DTS and sysfs
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-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.
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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/power/Kconfig | 1 +
drivers/power/max14577_charger.c | 311 +++++++++++++++++++++++++++++++----
include/linux/mfd/max14577-private.h | 19 +++
include/linux/mfd/max14577.h | 7 +
4 files changed, 310 insertions(+), 28 deletions(-)
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 69fa8a9ef7a6..04e1d2fe2201 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -327,6 +327,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..c2e87c41d853 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 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 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 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 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
* [PATCH v6 4/8] regulator/mfd: max14577: Export symbols for calculating charger current
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-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>
Acked-by: Andrew Morton <akpm@linux-foundation.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 6599407b5624..b8af263be594 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",
@@ -466,6 +547,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
* [PATCH v6 3/8] charger: max14577: Add support for MAX77836 charger
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-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>
Acked-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/power/Kconfig | 4 +--
drivers/power/max14577_charger.c | 77 ++++++++++++++++++++++++++++++++--------
2 files changed, 64 insertions(+), 17 deletions(-)
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 73cfcdf28a36..69fa8a9ef7a6 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -325,11 +325,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");
--
1.9.1
^ permalink raw reply related
* [PATCH v6 2/8] mfd: max14577: Map charger device to its own of_node
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-1-git-send-email-k.kozlowski@samsung.com>
Add a "maxim,max14577-charger" of_compatible to the mfd_cell so the
MFD child device (the charger) will have its own of_node set. This will
be used by the max14577 charger driver in next patches to obtain battery
configuration from DTS.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/mfd/max14577.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c
index 4a5e885383f8..6599407b5624 100644
--- a/drivers/mfd/max14577.c
+++ b/drivers/mfd/max14577.c
@@ -35,7 +35,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[] = {
--
1.9.1
^ permalink raw reply related
* [PATCH v6 1/8] mfd: max14577: Add defines for MAX77836 charger
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel, linux-api, linux-pm
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree, Andrew Morton,
Krzysztof Kozlowski
In-Reply-To: <1410504840-23543-1-git-send-email-k.kozlowski@samsung.com>
Prepare for adding support for MAX77836 charger to the max14577 charger
driver by adding necessary new defines and prefixes to existing ones.
The MAX77836 uses slightly different values for ChgTyp field of STATUS2
register. On the MAX14577 value of 0x6 is reserved and 0x7 dead battery.
On the MAX77836 the opposite:
- 0x6 means special charger,
- 0x7 is reserved.
Regardless of these differences use one common enum
max14577_muic_charger_type.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Andrew Morton <akpm@linux-foundation.org>
---
include/linux/mfd/max14577-private.h | 54 ++++++++++++++++++++++++++----------
1 file changed, 40 insertions(+), 14 deletions(-)
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
* [PATCH v6 0/8] charger/mfd: max14577: Add support for MAX77836
From: Krzysztof Kozlowski @ 2014-09-12 6:53 UTC (permalink / raw)
To: Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Samuel Ortiz, Lee Jones, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA
Cc: Liam Girdwood, Mark Brown, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, devicetree-u79uwXL29TY76Z2rM5mHXA,
Andrew Morton, Krzysztof Kozlowski
Hi,
This is a sixth version of patches adding support for
MAX77836 device to the max14577 drivers.
This patchset has been on the lists for quite long time and was already
reviewed by all necessary maintainers except power supply.
I need acks from power supply subsystem (patches: 3, 5-8).
Generally this should be pulled at once but some parts may be split
and pulled separately in following batches:
- Patches 1 and 2: as MFD prerequisites,
- Patches 3-5: adding actual charger code, depends on 1 and 2,
- Patch 6: fuel-gauge, independent,
- Patches 7 and 8: only documentation.
Lee Jones said he can take the set through his tree. Still I need acks
from power subsystem.
Everything rebased on Linus' tree (v3.17-rc2-9-g68e370289c29).
Changes since v5
================
1. 5/8: Remove 'inline' from functions (suggested by Andrew Morton).
2. Add Andrew Morton's acks.
Changes since v4
================
1. Add Lee Jones' acks to patch 1 and 2 (mfd).
2. power max17040 6/8: Remove duplicated (and not used) value of id
in array of i2c_device_id. (suggested by Lee Jones)
Changes since v3
================
1. Split MFD changes into separate new patches:
1/8: mfd: max14577: Add defines for MAX77836 charger
2/8: Map charger device to its own of_node
Suggested by Lee Jones.
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 the drivers
(merged):
https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg628696.html
Best regards,
Krzysztof Kozlowski
Krzysztof Kozlowski (8):
mfd: max14577: Add defines for MAX77836 charger
mfd: max14577: Map charger device to its own of_node
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 | 3 +-
drivers/regulator/max14577.c | 80 +----
include/linux/mfd/max14577-private.h | 95 ++++--
include/linux/mfd/max14577.h | 30 ++
9 files changed, 704 insertions(+), 139 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/max14577.txt
--
1.9.1
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-12 1:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CALCETrXqwd=dp31fckMPruQMwVw+UAjaf=SSWp8wr_Cdz_tQdw@mail.gmail.com>
On Thu, Sep 11, 2014 at 6:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Sep 11, 2014 at 3:29 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Thu, Sep 11, 2014 at 2:54 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>> the verifier log contains full trace. Last unsafe instruction + error
>>>> in many cases is useless. What we found empirically from using
>>>> it over last 2 years is that developers have different learning curve
>>>> to adjust to 'safe' style of C. Pretty much everyone couldn't
>>>> figure out why program is rejected based on last error. Therefore
>>>> verifier emits full log. From the 1st insn all the way till the last
>>>> 'unsafe' instruction. So the log is multiline output.
>>>> 'Understanding eBPF verifier messages' section of
>>>> Documentation/networking/filter.txt provides few trivial
>>>> examples of these multiline messages.
>>>> Like for the program:
>>>> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>>>> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>>>> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>>>> BPF_LD_MAP_FD(BPF_REG_1, 0),
>>>> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
>>>> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
>>>> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
>>>> BPF_EXIT_INSN(),
>>>> the verifier log_buf is:
>>>> 0: (7a) *(u64 *)(r10 -8) = 0
>>>> 1: (bf) r2 = r10
>>>> 2: (07) r2 += -8
>>>> 3: (b7) r1 = 0
>>>> 4: (85) call 1
>>>> 5: (15) if r0 == 0x0 goto pc+1
>>>> R0=map_ptr R10=fp
>>>> 6: (7a) *(u64 *)(r0 +4) = 0
>>>> misaligned access off 4 size 8
>>>>
>>>> It will surely change over time as verifier becomes smarter,
>>>> supports new types, optimizations and so on.
>>>> So this log is not an ABI. It's for humans to read.
>>>> The log explains _how_ verifier came to conclusion
>>>> that the program is unsafe.
>>>
>>> Given that you've already arranged (I think) for the verifier to be
>>> compilable in the kernel and in userspace, would it make more sense to
>>> have the kernel version just say yes or no and to make it easy for
>>> user code to retry verification in userspace if they want a full
>>> explanation?
>>
>> Good memory :) Long ago I had a hack where I compiled
>> verifier.o for kernel and linked it with userspace wrappers to
>> have the same verifier for userspace. It was very fragile.
>> and maps were not separate objects and there were no fds.
>> It's not feasible anymore, since different subsystems
>> will configure different bpf_context and helper functions and
>> verifier output is dynamic based on maps that were created.
>> For example, if user's samples/bpf/sock_example.c does
>> bpf_create_map(HASH, sizeof(key) * 2, ...);
>> instead of
>> bpf_create_map(HASH, sizeof(key), ...);
>> the same program will be rejected in first case and will be
>> accepted in the second, because map sizes and ebpf
>> program expectations are mismatching.
>
> Hmm.
>
> This actually furthers my thought that the relocations should be a
> real relocation table. Then you could encode the types of the
> referenced objects in the table, and a program could be verified
> without looking up the fds. The only extra step would be to confirm
> that the actual types referenced match those in the table.
It's not the type is being checked, but one particular map instance
with user specified key/value sizes. type is not helpful. type is not
even used during verification. Only key_size and value_size of
elements are meaningful and they're looked up dynamically by fd.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Andy Lutomirski @ 2014-09-12 1:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuyzOgCZutgsAXs60BO0=0WJGpVAeTYN2hf0Bh1sZ5PMVg@mail.gmail.com>
On Thu, Sep 11, 2014 at 3:29 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Thu, Sep 11, 2014 at 2:54 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>> the verifier log contains full trace. Last unsafe instruction + error
>>> in many cases is useless. What we found empirically from using
>>> it over last 2 years is that developers have different learning curve
>>> to adjust to 'safe' style of C. Pretty much everyone couldn't
>>> figure out why program is rejected based on last error. Therefore
>>> verifier emits full log. From the 1st insn all the way till the last
>>> 'unsafe' instruction. So the log is multiline output.
>>> 'Understanding eBPF verifier messages' section of
>>> Documentation/networking/filter.txt provides few trivial
>>> examples of these multiline messages.
>>> Like for the program:
>>> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>>> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>>> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>>> BPF_LD_MAP_FD(BPF_REG_1, 0),
>>> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
>>> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
>>> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
>>> BPF_EXIT_INSN(),
>>> the verifier log_buf is:
>>> 0: (7a) *(u64 *)(r10 -8) = 0
>>> 1: (bf) r2 = r10
>>> 2: (07) r2 += -8
>>> 3: (b7) r1 = 0
>>> 4: (85) call 1
>>> 5: (15) if r0 == 0x0 goto pc+1
>>> R0=map_ptr R10=fp
>>> 6: (7a) *(u64 *)(r0 +4) = 0
>>> misaligned access off 4 size 8
>>>
>>> It will surely change over time as verifier becomes smarter,
>>> supports new types, optimizations and so on.
>>> So this log is not an ABI. It's for humans to read.
>>> The log explains _how_ verifier came to conclusion
>>> that the program is unsafe.
>>
>> Given that you've already arranged (I think) for the verifier to be
>> compilable in the kernel and in userspace, would it make more sense to
>> have the kernel version just say yes or no and to make it easy for
>> user code to retry verification in userspace if they want a full
>> explanation?
>
> Good memory :) Long ago I had a hack where I compiled
> verifier.o for kernel and linked it with userspace wrappers to
> have the same verifier for userspace. It was very fragile.
> and maps were not separate objects and there were no fds.
> It's not feasible anymore, since different subsystems
> will configure different bpf_context and helper functions and
> verifier output is dynamic based on maps that were created.
> For example, if user's samples/bpf/sock_example.c does
> bpf_create_map(HASH, sizeof(key) * 2, ...);
> instead of
> bpf_create_map(HASH, sizeof(key), ...);
> the same program will be rejected in first case and will be
> accepted in the second, because map sizes and ebpf
> program expectations are mismatching.
Hmm.
This actually furthers my thought that the relocations should be a
real relocation table. Then you could encode the types of the
referenced objects in the table, and a program could be verified
without looking up the fds. The only extra step would be to confirm
that the actual types referenced match those in the table.
--Andy
^ permalink raw reply
* Re: [PATCH v16 0/7] MADV_FREE support
From: Minchan Kim @ 2014-09-11 23:10 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Michael Kerrisk,
linux-api-u79uwXL29TY76Z2rM5mHXA, Hugh Dickins, Johannes Weiner,
Rik van Riel, KOSAKI Motohiro, Mel Gorman, Jason Evans,
zhangyanfei-BthXqXjhjHXQFUHtdCDX3A, Kirill A. Shutemov
In-Reply-To: <1409556048-5045-1-git-send-email-minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hello Andrew,
This patchset have been pended for a long time.
Is there any problem to proceed?
On Mon, Sep 01, 2014 at 04:20:41PM +0900, Minchan Kim wrote:
> This patch enable MADV_FREE hint for madvise syscall, which have
> been supported by other OSes. [PATCH 1] includes the details.
>
> [1] support MADVISE_FREE for !THP page so if VM encounter
> THP page in syscall context, it splits THP page.
> [2-6] is to preparing to call madvise syscall without THP plitting
> [7] enable THP page support for MADV_FREE.
>
> * from v15
> * Add more Acked-by - Rik van Riel
> * Rebased on mmotom-08-29-15-15
>
> * from v14
> * Add more Ackedy-by from arch people(sparc, arm64 and arm)
> * Drop s390 since pmd_dirty/clean was merged
>
> * from v13
> * Add more Ackedy-by from arch people(arm, arm64 and ppc)
> * Rebased on mmotm 2014-08-13-14-29
>
> * from v12
> * Fix - skip to mark free pte on try_to_free_swap failed page - Kirill
> * Add more Acked-by from arch maintainers and Kirill
>
> * From v11
> * Fix arm build - Steve
> * Separate patch for arm and arm64 - Steve
> * Remove unnecessary check - Kirill
> * Skip non-vm_normal page - Kirill
> * Add Acked-by - Zhang
> * Sparc64 build fix
> * Pagetable walker THP handling fix
>
> * From v10
> * Add Acked-by from arch stuff(x86, s390)
> * Pagewalker based pagetable working - Kirill
> * Fix try_to_unmap_one broken with hwpoison - Kirill
> * Use VM_BUG_ON_PAGE in madvise_free_pmd - Kirill
> * Fix pgtable-3level.h for arm - Steve
>
> * From v9
> * Add Acked-by - Rik
> * Add THP page support - Kirill
>
> * From v8
> * Rebased-on v3.16-rc2-mmotm-2014-06-25-16-44
>
> * From v7
> * Rebased-on next-20140613
>
> * From v6
> * Remove page from swapcache in syscal time
> * Move utility functions from memory.c to madvise.c - Johannes
> * Rename untilify functtions - Johannes
> * Remove unnecessary checks from vmscan.c - Johannes
> * Rebased-on v3.15-rc5-mmotm-2014-05-16-16-56
> * Drop Reviewe-by because there was some changes since then.
>
> * From v5
> * Fix PPC problem which don't flush TLB - Rik
> * Remove unnecessary lazyfree_range stub function - Rik
> * Rebased on v3.15-rc5
>
> * From v4
> * Add Reviewed-by: Zhang Yanfei
> * Rebase on v3.15-rc1-mmotm-2014-04-15-16-14
>
> * From v3
> * Add "how to work part" in description - Zhang
> * Add page_discardable utility function - Zhang
> * Clean up
>
> * From v2
> * Remove forceful dirty marking of swap-readed page - Johannes
> * Remove deactivation logic of lazyfreed page
> * Rebased on 3.14
> * Remove RFC tag
>
> * From v1
> * Use custom page table walker for madvise_free - Johannes
> * Remove PG_lazypage flag - Johannes
> * Do madvise_dontneed instead of madvise_freein swapless system
>
> Minchan Kim (7):
> mm: support madvise(MADV_FREE)
> x86: add pmd_[dirty|mkclean] for THP
> sparc: add pmd_[dirty|mkclean] for THP
> powerpc: add pmd_[dirty|mkclean] for THP
> arm: add pmd_mkclean for THP
> arm64: add pmd_[dirty|mkclean] for THP
> mm: Don't split THP page when syscall is called
>
> arch/arm/include/asm/pgtable-3level.h | 1 +
> arch/arm64/include/asm/pgtable.h | 2 +
> arch/powerpc/include/asm/pgtable-ppc64.h | 2 +
> arch/sparc/include/asm/pgtable_64.h | 16 ++++
> arch/x86/include/asm/pgtable.h | 10 ++
> include/linux/huge_mm.h | 4 +
> include/linux/rmap.h | 9 +-
> include/linux/vm_event_item.h | 1 +
> include/uapi/asm-generic/mman-common.h | 1 +
> mm/huge_memory.c | 35 +++++++
> mm/madvise.c | 159 +++++++++++++++++++++++++++++++
> mm/rmap.c | 46 ++++++++-
> mm/vmscan.c | 64 +++++++++----
> mm/vmstat.c | 1 +
> 14 files changed, 331 insertions(+), 20 deletions(-)
>
> --
> 2.0.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo-Bw31MaZKKs0EbZ0PF+XxCw@public.gmane.org For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org"> email-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org </a>
--
Kind regards,
Minchan Kim
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 22:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CALCETrWCEwscbbfX7wAW-A+VQ5Y92igD36BmTXqFra04Qdwk0Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Sep 11, 2014 at 2:54 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>
>> the verifier log contains full trace. Last unsafe instruction + error
>> in many cases is useless. What we found empirically from using
>> it over last 2 years is that developers have different learning curve
>> to adjust to 'safe' style of C. Pretty much everyone couldn't
>> figure out why program is rejected based on last error. Therefore
>> verifier emits full log. From the 1st insn all the way till the last
>> 'unsafe' instruction. So the log is multiline output.
>> 'Understanding eBPF verifier messages' section of
>> Documentation/networking/filter.txt provides few trivial
>> examples of these multiline messages.
>> Like for the program:
>> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> BPF_LD_MAP_FD(BPF_REG_1, 0),
>> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
>> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
>> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
>> BPF_EXIT_INSN(),
>> the verifier log_buf is:
>> 0: (7a) *(u64 *)(r10 -8) = 0
>> 1: (bf) r2 = r10
>> 2: (07) r2 += -8
>> 3: (b7) r1 = 0
>> 4: (85) call 1
>> 5: (15) if r0 == 0x0 goto pc+1
>> R0=map_ptr R10=fp
>> 6: (7a) *(u64 *)(r0 +4) = 0
>> misaligned access off 4 size 8
>>
>> It will surely change over time as verifier becomes smarter,
>> supports new types, optimizations and so on.
>> So this log is not an ABI. It's for humans to read.
>> The log explains _how_ verifier came to conclusion
>> that the program is unsafe.
>
> Given that you've already arranged (I think) for the verifier to be
> compilable in the kernel and in userspace, would it make more sense to
> have the kernel version just say yes or no and to make it easy for
> user code to retry verification in userspace if they want a full
> explanation?
Good memory :) Long ago I had a hack where I compiled
verifier.o for kernel and linked it with userspace wrappers to
have the same verifier for userspace. It was very fragile.
and maps were not separate objects and there were no fds.
It's not feasible anymore, since different subsystems
will configure different bpf_context and helper functions and
verifier output is dynamic based on maps that were created.
For example, if user's samples/bpf/sock_example.c does
bpf_create_map(HASH, sizeof(key) * 2, ...);
instead of
bpf_create_map(HASH, sizeof(key), ...);
the same program will be rejected in first case and will be
accepted in the second, because map sizes and ebpf
program expectations are mismatching.
For the 1st case verifier will complain that program is
trying to pass uninitialized stack into bpf_lookup(key,...)
method or stack may be out of bounds.
Human insight is needed to understand what is going on.
I think more important is that source of truth needs to be
in one place == kernel. If we have two verifiers they will
diverge sooner or later and cause confusion for users.
I think as long as we document that verifier log
messages are not cast in stone and will change, we're fine.
I consider them as continuation of compiler warnings/errors.
They are meant for humans and do change from time to time.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Andy Lutomirski @ 2014-09-11 21:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuziPptHxtw_7fkOdR-paB+8BatNmRPoo3txP8wOp9D6Tw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Sep 11, 2014 at 1:33 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Thu, Sep 11, 2014 at 12:47 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 09/10/2014 07:32 PM, Alexei Starovoitov wrote:
>>>
>>> On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> wrote:
>>>>>
>>>>> struct { /* anonymous struct used by BPF_PROG_LOAD command
>>>>> */
>>>>> enum bpf_prog_type prog_type;
>>>>> __u32 insn_cnt;
>>>>> const struct bpf_insn *insns;
>>>>> const char *license;
>>>>> __u32 log_level; /* verbosity level of
>>>>> eBPF verifier */
>>>>> __u32 log_size; /* size of user buffer
>>>>> */
>>>>> void *log_buf; /* user supplied
>>>>> buffer
>>>>> */
>>>>
>>>>
>>>>
>>>> What is log buffer? Would that mean the verifier will return an error
>>>> string if the program will not pass it, or if not, what other data?
>>>> I think the man page is missing how to examine the returned verifier
>>>> log buffer data.
>>>
>>>
>>> yes. it's an error log (as text string for humans) from verifier.
>>
>> I was confused due to the void pointer. But that also means that the text
>
> ahh. ok. will change it to 'char *' then.
>
>> string becomes part of the ABI; aren't eBPF specific error codes (perhaps
>> a tuple of [line + error code]), though ugly as well, but perhaps the better
>> solution to this [which user space can then map to an actual string]?
>
> the verifier log contains full trace. Last unsafe instruction + error
> in many cases is useless. What we found empirically from using
> it over last 2 years is that developers have different learning curve
> to adjust to 'safe' style of C. Pretty much everyone couldn't
> figure out why program is rejected based on last error. Therefore
> verifier emits full log. From the 1st insn all the way till the last
> 'unsafe' instruction. So the log is multiline output.
> 'Understanding eBPF verifier messages' section of
> Documentation/networking/filter.txt provides few trivial
> examples of these multiline messages.
> Like for the program:
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1, 0),
> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
> BPF_EXIT_INSN(),
> the verifier log_buf is:
> 0: (7a) *(u64 *)(r10 -8) = 0
> 1: (bf) r2 = r10
> 2: (07) r2 += -8
> 3: (b7) r1 = 0
> 4: (85) call 1
> 5: (15) if r0 == 0x0 goto pc+1
> R0=map_ptr R10=fp
> 6: (7a) *(u64 *)(r0 +4) = 0
> misaligned access off 4 size 8
>
> It will surely change over time as verifier becomes smarter,
> supports new types, optimizations and so on.
> So this log is not an ABI. It's for humans to read.
> The log explains _how_ verifier came to conclusion
> that the program is unsafe.
Given that you've already arranged (I think) for the verifier to be
compilable in the kernel and in userspace, would it make more sense to
have the kernel version just say yes or no and to make it easy for
user code to retry verification in userspace if they want a full
explanation?
--Andy
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 20:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andy Lutomirski, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <5411FDE1.3060302-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Sep 11, 2014 at 12:54 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/10/2014 10:21 PM, Alexei Starovoitov wrote:
> ...
>>>>
>>>> char bpf_log_buf[LOG_BUF_SIZE];
>>>
>>>
>>> What happens if the size isn't LOG_BUF_SIZE?
>>
>>
>> would do you mean?
>> LOG_BUF_SIZE is just a user defined macro.
>> Can be anything.
>
> I believe, Andy means, what would happen if log_level > 0 but
> the log_size is not big enough so that the human-readable error
> text string from the verifier will fit into it? I presume that
> will be truncated then ... hm.
Correct. It will be truncated and ENOSPC returned from syscall.
Just noticed that I didn't mention that in the manpage... will do.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-11 20:33 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <5411FC42.3070505-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Sep 11, 2014 at 12:47 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/10/2014 07:32 PM, Alexei Starovoitov wrote:
>>
>> On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> wrote:
>>>>
>>>> struct { /* anonymous struct used by BPF_PROG_LOAD command
>>>> */
>>>> enum bpf_prog_type prog_type;
>>>> __u32 insn_cnt;
>>>> const struct bpf_insn *insns;
>>>> const char *license;
>>>> __u32 log_level; /* verbosity level of
>>>> eBPF verifier */
>>>> __u32 log_size; /* size of user buffer
>>>> */
>>>> void *log_buf; /* user supplied
>>>> buffer
>>>> */
>>>
>>>
>>>
>>> What is log buffer? Would that mean the verifier will return an error
>>> string if the program will not pass it, or if not, what other data?
>>> I think the man page is missing how to examine the returned verifier
>>> log buffer data.
>>
>>
>> yes. it's an error log (as text string for humans) from verifier.
>
> I was confused due to the void pointer. But that also means that the text
ahh. ok. will change it to 'char *' then.
> string becomes part of the ABI; aren't eBPF specific error codes (perhaps
> a tuple of [line + error code]), though ugly as well, but perhaps the better
> solution to this [which user space can then map to an actual string]?
the verifier log contains full trace. Last unsafe instruction + error
in many cases is useless. What we found empirically from using
it over last 2 years is that developers have different learning curve
to adjust to 'safe' style of C. Pretty much everyone couldn't
figure out why program is rejected based on last error. Therefore
verifier emits full log. From the 1st insn all the way till the last
'unsafe' instruction. So the log is multiline output.
'Understanding eBPF verifier messages' section of
Documentation/networking/filter.txt provides few trivial
examples of these multiline messages.
Like for the program:
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
BPF_LD_MAP_FD(BPF_REG_1, 0),
BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
BPF_EXIT_INSN(),
the verifier log_buf is:
0: (7a) *(u64 *)(r10 -8) = 0
1: (bf) r2 = r10
2: (07) r2 += -8
3: (b7) r1 = 0
4: (85) call 1
5: (15) if r0 == 0x0 goto pc+1
R0=map_ptr R10=fp
6: (7a) *(u64 *)(r0 +4) = 0
misaligned access off 4 size 8
It will surely change over time as verifier becomes smarter,
supports new types, optimizations and so on.
So this log is not an ABI. It's for humans to read.
The log explains _how_ verifier came to conclusion
that the program is unsafe.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Daniel Borkmann @ 2014-09-11 19:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, David S. Miller, Ingo Molnar, Linus Torvalds,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMEtUuyY0L2=tz5mSvQqR_=_oN18nA6V9fdbR8OiYTPbQth8AQ@mail.gmail.com>
On 09/10/2014 10:21 PM, Alexei Starovoitov wrote:
...
>>> char bpf_log_buf[LOG_BUF_SIZE];
>>
>> What happens if the size isn't LOG_BUF_SIZE?
>
> would do you mean?
> LOG_BUF_SIZE is just a user defined macro.
> Can be anything.
I believe, Andy means, what would happen if log_level > 0 but
the log_size is not big enough so that the human-readable error
text string from the verifier will fit into it? I presume that
will be truncated then ... hm.
> it's passed along with pointer:
> .log_buf = bpf_log_buf,
> .log_size = LOG_BUF_SIZE,
> .log_level = 1,
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Daniel Borkmann @ 2014-09-11 19:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuwrHX4ENK9cZ0C+XVB=wkMz1=wLphX_GVLvd8pyJKMXeQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 09/10/2014 07:32 PM, Alexei Starovoitov wrote:
> On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> struct { /* anonymous struct used by BPF_PROG_LOAD command */
>>> enum bpf_prog_type prog_type;
>>> __u32 insn_cnt;
>>> const struct bpf_insn *insns;
>>> const char *license;
>>> __u32 log_level; /* verbosity level of
>>> eBPF verifier */
>>> __u32 log_size; /* size of user buffer */
>>> void *log_buf; /* user supplied buffer
>>> */
>>
>>
>> What is log buffer? Would that mean the verifier will return an error
>> string if the program will not pass it, or if not, what other data?
>> I think the man page is missing how to examine the returned verifier
>> log buffer data.
>
> yes. it's an error log (as text string for humans) from verifier.
I was confused due to the void pointer. But that also means that the text
string becomes part of the ABI; aren't eBPF specific error codes (perhaps
a tuple of [line + error code]), though ugly as well, but perhaps the better
solution to this [which user space can then map to an actual string]?
^ permalink raw reply
* Re: [PATCH v11 net-next 11/12] net: filter: move eBPF instruction macros
From: Alexei Starovoitov @ 2014-09-11 6:45 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <5411415D.4060103@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 711 bytes --]
On Wed, Sep 10, 2014 at 11:29 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
>>
>> Interesting idea. Are you saying just copy paste these
>> 226 lines into user libbpf.h for now to use them in testsuite
>> and examples ? Hmm.
>> Indeed the less lines we add to uapi the better. I'll do that.
>
>
> Yes, that should be just fine, the existing ABI is not allowed to
> change anyway and we're running into less pain if we decide to change
> existing or add new macros internally.
ok. will do that in the next set.
btw, preliminary compat support in the attached patch.
It works fine, but Andy's concern about enum is starting
to get on me. May be we should replace
enum bpf_.._type with u32 just to be safe...
[-- Attachment #2: 0001-bpf-add-compat_sys_bpf-wrapper.patch --]
[-- Type: application/octet-stream, Size: 4218 bytes --]
From 9c05cc1bd18e278f0cfd5a490644105aa7cbbf52 Mon Sep 17 00:00:00 2001
From: Alexei Starovoitov <ast@plumgrid.com>
Date: Wed, 10 Sep 2014 19:41:31 -0700
Subject: [PATCH] bpf: add compat_sys_bpf wrapper
to support 32-bit user space with 64-bit kernel
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/x86/syscalls/syscall_32.tbl | 2 +-
include/uapi/asm-generic/unistd.h | 2 +-
kernel/bpf/syscall.c | 95 +++++++++++++++++++++++++++++++++++++
3 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 9fe1b5d002f0..2b1ddf1ddb5e 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -363,4 +363,4 @@
354 i386 seccomp sys_seccomp
355 i386 getrandom sys_getrandom
356 i386 memfd_create sys_memfd_create
-357 i386 bpf sys_bpf
+357 i386 bpf sys_bpf compat_sys_bpf
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 22749c134117..94afed93f3cc 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -706,7 +706,7 @@ __SYSCALL(__NR_getrandom, sys_getrandom)
#define __NR_memfd_create 279
__SYSCALL(__NR_memfd_create, sys_memfd_create)
#define __NR_bpf 280
-__SYSCALL(__NR_bpf, sys_bpf)
+__SC_COMP(__NR_bpf, sys_bpf, compat_sys_bpf)
#undef __NR_syscalls
#define __NR_syscalls 281
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6cdfe388c789..4f2c5441e175 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -16,6 +16,7 @@
#include <linux/file.h>
#include <linux/license.h>
#include <linux/filter.h>
+#include <linux/compat.h>
static LIST_HEAD(bpf_map_types);
@@ -576,6 +577,100 @@ free_attr:
return err;
}
+#ifdef CONFIG_COMPAT
+union compat_bpf_attr {
+ struct {
+ enum bpf_map_type map_type;
+ __u32 key_size;
+ __u32 value_size;
+ __u32 max_entries;
+ };
+
+ struct {
+ int map_fd;
+ compat_uptr_t key;
+ union {
+ compat_uptr_t value;
+ compat_uptr_t next_key;
+ };
+ };
+
+ struct {
+ enum bpf_prog_type prog_type;
+ __u32 insn_cnt;
+ compat_uptr_t insns;
+ compat_uptr_t license;
+ __u32 log_level;
+ __u32 log_size;
+ compat_uptr_t log_buf;
+ };
+};
+COMPAT_SYSCALL_DEFINE3(bpf, int, cmd, union compat_bpf_attr __user *, uattr,
+ unsigned int, size)
+{
+ union compat_bpf_attr compat_attr = {};
+ union bpf_attr attr = {};
+ int err;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (size > sizeof(compat_attr))
+ return -EINVAL;
+
+ if (copy_from_user(&compat_attr, uattr, size) != 0)
+ return -EFAULT;
+
+ switch (cmd) {
+ case BPF_MAP_CREATE:
+ attr.map_type = compat_attr.map_type;
+ attr.key_size = compat_attr.key_size;
+ attr.value_size = compat_attr.value_size;
+ attr.max_entries = compat_attr.max_entries;
+ err = map_create(&attr);
+ break;
+ case BPF_MAP_LOOKUP_ELEM:
+ attr.map_fd = compat_attr.map_fd;
+ attr.key = compat_ptr(compat_attr.key);
+ attr.value = compat_ptr(compat_attr.value);
+ err = map_lookup_elem(&attr);
+ break;
+ case BPF_MAP_UPDATE_ELEM:
+ attr.map_fd = compat_attr.map_fd;
+ attr.key = compat_ptr(compat_attr.key);
+ attr.value = compat_ptr(compat_attr.value);
+ err = map_update_elem(&attr);
+ break;
+ case BPF_MAP_DELETE_ELEM:
+ attr.map_fd = compat_attr.map_fd;
+ attr.key = compat_ptr(compat_attr.key);
+ err = map_delete_elem(&attr);
+ break;
+ case BPF_MAP_GET_NEXT_KEY:
+ attr.map_fd = compat_attr.map_fd;
+ attr.key = compat_ptr(compat_attr.key);
+ attr.next_key = compat_ptr(compat_attr.next_key);
+ err = map_get_next_key(&attr);
+ break;
+ case BPF_PROG_LOAD:
+ attr.prog_type = compat_attr.prog_type;
+ attr.insn_cnt = compat_attr.insn_cnt;
+ attr.insns = compat_ptr(compat_attr.insns);
+ attr.license = compat_ptr(compat_attr.license);
+ attr.log_level = compat_attr.log_level;
+ attr.log_size = compat_attr.log_size;
+ attr.log_buf = compat_ptr(compat_attr.log_buf);
+ err = bpf_prog_load(&attr);
+ break;
+ default:
+ err = -EINVAL;
+ break;
+ }
+
+ return err;
+}
+#endif
+
/* called from eBPF program under rcu lock
*
* if kernel subsystem is allowing eBPF programs to call this function,
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v11 net-next 11/12] net: filter: move eBPF instruction macros
From: Daniel Borkmann @ 2014-09-11 6:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuwQXj=oWzyBkogTSfOAhjhx0oyB5gC4+HoccTGWg8Obfg@mail.gmail.com>
On 09/10/2014 08:16 PM, Alexei Starovoitov wrote:
> On Wed, Sep 10, 2014 at 4:24 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> On 09/10/2014 07:10 AM, Alexei Starovoitov wrote:
>>>
>>> move instruction macros (like BPF_MOV64_REG or BPF_ALU32_IMM)
>>> from linux/filter.h into uapi/linux/bpf.h
>>> so that userspace programs can use them.
>>>
>>> verifier testsuite (in later patches) will be using them.
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> I don't think we need this commit at all. These macros are not really
>> part of the ABI and we should expose __as less as possible__, otherwise
>> we won't be able to alter them anymore. Plus, based on your previous
>> argumentation regarding the header file expose, we also won't be able
>> to add any new macros anymore since you could run into name collisions.
>>
>> Anyway, I don't see a reason why a e.g. central user space eBPF library
>> cannot live with, for example, a private copy of these helper macros or
>> whatever it wants to define by itself.
>>
>> I know in classic BPF there's BPF_STMT() and BPF_JUMP() but it's much
>> less complicated than eBPF and less likely to changes.
>
> Interesting idea. Are you saying just copy paste these
> 226 lines into user libbpf.h for now to use them in testsuite
> and examples ? Hmm.
> Indeed the less lines we add to uapi the better. I'll do that.
Yes, that should be just fine, the existing ABI is not allowed to
change anyway and we're running into less pain if we decide to change
existing or add new macros internally.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-10 20:21 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CALCETrVBb_uQZpodv67XSTGVU40y6+9Ktw=3zAJgGV2mNJ=_0A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Sep 10, 2014 at 11:22 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>
>> attr is a pointer to a union of type bpf_attr as defined below.
>>
>> size is the size of the union.
>
> I find this strange. Why not just make attr be a pointer to the
> relevant struct for the operation being invoked?
you mean change attr to be 'void *' and type cast it to particular
struct type based on cmd ? Possible, but I tried to avoid all
typecasts as Dave doesn't like them.
>> union bpf_attr {
>> struct { /* anonymous struct used by BPF_MAP_CREATE command */
>> enum bpf_map_type map_type;
>
> Does this reliably generate the same type on compat systems? C++11
> has a fix for enum ABI compatibility, but this is plain C :(
enum is int on both 32 and 64-bit. What was the concern?
anonymous struct ?
I've checked that with gcc 4.2 - 4.9 and clang. All was fine
and it's part of C standard now.
>> struct { /* anonymous struct used by BPF_PROG_LOAD command */
>> enum bpf_prog_type prog_type;
>> __u32 insn_cnt;
>> const struct bpf_insn *insns;
>> const char *license;
>> __u32 log_level; /* verbosity level of eBPF verifier */
>> __u32 log_size; /* size of user buffer */
>> void *log_buf; /* user supplied buffer */
>> };
>> };
>
> It might be a bit nicer to have separate in and out arguments.
would do you mean specifically?
const pointer is obviously 'in' argument.
'void *' is 'out'.
>> int bpf_create_map(enum bpf_map_type map_type, int key_size,
>> int value_size, int max_entries)
>> {
>> union bpf_attr attr = {
>> .map_type = map_type,
>> .key_size = key_size,
>> .value_size = value_size,
>> .max_entries = max_entries
>> };
>
> I feel like this is asking for trouble, or at least bizarre namespace
> collisions in the anonymous struct members. At least please give the
> structs names. (Also, the first time I read this, I assumed that
> those were union members, which would have made the code be nonsense.)
if inner struct types had names they would need to
have field names as well, so the syscall wrapper above
would become much more verbose and uglier.
Also naming structs may give wrong ideas to some users,
since they might think it's ok to init struct only and type cast
it to pass into syscall. When inner structs don't have names,
the user space is forced to always use 'union bpf_attr' and
initialize relevant fields.
>> char bpf_log_buf[LOG_BUF_SIZE];
>
> What happens if the size isn't LOG_BUF_SIZE?
would do you mean?
LOG_BUF_SIZE is just a user defined macro.
Can be anything.
it's passed along with pointer:
.log_buf = bpf_log_buf,
.log_size = LOG_BUF_SIZE,
.log_level = 1,
>> enum bpf_prog_type {
>> BPF_PROG_TYPE_UNSPEC,
>> BPF_PROG_TYPE_SOCKET_FILTER,
>> BPF_PROG_TYPE_TRACING_FILTER,
>> };
>
> Why does the type matter?
type is way to tell eBPF infra what this type of programs
is allowed to do. Different kernel subsystems
configure different types.
Like patch 12 configures TYPE_UNSPEC for testing.
This type allows one dummy function call and
bpf_context of two u64 fields.
tracing subsystem will configure TYPE_TRACING
to do different set of helper functions and different
body of 'bpf_context'.
PROG_TYPE and MAP_TYPE are two main ways
to configure eBPF infra for different use cases.
> This (the .imm thing) is sufficiently weird that I think it needs to
> be mentioned in the main docs, not just in an example. It's
> especially odd since AFAIK essentially every other object format in
> the world uses a separate relocation table instead of inline magic
> opcodes like this.
we discussed relocations before, right? ;)
I believe relocations are ugly. elf has no other way to deal
with it, since .text has valid cpu instructions and generic
loader has to adjust them without knowing hw encoding.
Here we have pseudo instructions that are much easier
to check/track in verifier than relocations.
As you remember in previous series I've tried relocation
style and it was ugly. Both as user interface and as extra
complexity for verifier.
Does commit log of patch 8 explain map_fd conversion
well enough or not?
If not, I'll add more info, but please read it first.
>> ENOENT For BPF_MAP_LOOKUP_ELEM or BPF_MAP_DELETE_ELEM, indicates that
>> element with given key was not found.
>
> What does it return? (The same question goes for a bunch of the map ops.)
...
> Shouldn't delete return different values depending on whether anything
> was deleted?
...
> Ah, here it is. Please document this with the ops.
I believe it's a standard manpage style to document
return values at the end in 'return value' section.
I can duplicate it with ops, but is it really necessary?
>> These commands may be used only by a privileged process (one having the
>> CAP_SYS_ADMIN capability).
>
> I hope this goes away :)
hehe.
I think folks obsessed with security will say it should stay
this way for looooong time :)
My immediate goal is tracing and there this restriction is
necessary anyway.
> I can't shake the feeling that the whole syscall map API is wrong and
> that, instead, there should be a more general concept of objects
> provided by the eBPF runtime. Those objects could have methods that
> are callable by the syscall and callable from eBPF code.
'concept of objects'... sounds abstractly good :)
Something concrete you have in mind?
Theoretically I can see how we can add a 'stream' object
which user space can read and programs feed stuff to.
In other words an 'abstract' trace buffer, but imo it's
overdesign. When we need a trace buffer, we'll just
add a helper function that pushes stuff to it.
That will be the time when you see how handy pseudo
instructions are. In this patch the only '.imm thing', as you
say, is map_fd. I'm working on per-cpu local buffer via
the same pseudo stuff. Seem to work quite nicely.
Let's not get carried on with future cool stuff, basics first :)
Thanks for the feedback!
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Andy Lutomirski @ 2014-09-10 18:22 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development,
linux-kernel@vger.kernel.org
In-Reply-To: <1410325808-3657-1-git-send-email-ast@plumgrid.com>
On Tue, Sep 9, 2014 at 10:09 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> Hi David,
>
> I've managed to reduce this set to 12:
> Patches 1-4 establish BPF syscall shell for maps and programs.
> Patches 5-10 add verifier step by step
> Patch 11 exposes existing instruction macros to user space
> Patch 12 adds test stubs and verifier testsuite from user space
>
> I don't know how to reduce it further. Drop verifier and
> have programs loaded without verification? Sounds wrong.
> If anyone has other ideas, I'll gladly reduce it further.
>
> Note that patches 1,3,4,7 add commands and attributes to the syscall
> while being backwards compatible from each other, which should demonstrate
> how other commands can be added in the future.
>
> Daniel,
> bpf_common.h patch (that we discussed earlier) I didn't include here
> to reduce the number of patches. It can come next.
>
> For those who have looked at the last set of 28 patches, the difference is:
> - moved attaching to tracing and sockets to future patches
> - moved hash table map type implementation to future
> - split verifier further and moved LD_ABS checks and state prunning to future
> - instead of running verifier testsuite on real tracing programs added
> test_stub.c with fake maps, context and helper functions to test verifier only
> - rebased
>
> Note, after this set the programs can be loaded for testing only. They cannot
> be attached to any events. This will come in the next set.
>
> As requested by Andy and others, here is the man page:
>
> BPF(2) Linux Programmer's Manual BPF(2)
>
>
>
> NAME
> bpf - perform a command on eBPF map or program
>
> SYNOPSIS
> #include <linux/bpf.h>
>
> int bpf(int cmd, union bpf_attr *attr, unsigned int size);
>
>
> DESCRIPTION
> bpf() syscall is a multiplexor for a range of different operations on
> eBPF which can be characterized as "universal in-kernel virtual
> machine". eBPF is similar to original Berkeley Packet Filter (or "clas-
> sic BPF") used to filter network packets. Both statically analyze the
> programs before loading them into the kernel to ensure that programs
> cannot harm the running system.
>
> eBPF extends classic BPF in multiple ways including ability to call in-
> kernel helper functions and access shared data structures like eBPF
> maps. The programs can be written in a restricted C that is compiled
> into eBPF bytecode and executed on the eBPF virtual machine or JITed
> into native instruction set.
>
> eBPF Design/Architecture
> eBPF maps is a generic storage of different types. User process can
> create multiple maps (with key/value being opaque bytes of data) and
> access them via file descriptor. In parallel eBPF programs can access
> maps from inside the kernel. It's up to user process and eBPF program
> to decide what they store inside maps.
>
> eBPF programs are similar to kernel modules. They are loaded by the
> user process and automatically unloaded when process exits. Each eBPF
> program is a safe run-to-completion set of instructions. eBPF verifier
> statically determines that the program terminates and is safe to exe-
> cute. During verification the program takes a hold of maps that it
> intends to use, so selected maps cannot be removed until the program is
> unloaded. The program can be attached to different events. These events
> can be packets, tracepoint events and other types in the future. A new
> event triggers execution of the program which may store information
> about the event in the maps. Beyond storing data the programs may call
> into in-kernel helper functions which may, for example, dump stack, do
> trace_printk or other forms of live kernel debugging. The same program
> can be attached to multiple events. Different programs can access the
> same map:
> tracepoint tracepoint tracepoint sk_buff sk_buff
> event A event B event C on eth0 on eth1
> | | | | |
> | | | | |
> --> tracing <-- tracing socket socket
> prog_1 prog_2 prog_3 prog_4
> | | | |
> |--- -----| |-------| map_3
> map_1 map_2
>
> Syscall Arguments
> bpf() syscall operation is determined by cmd which can be one of the
> following:
>
> BPF_MAP_CREATE
> Create a map with given type and attributes and return map FD
>
> BPF_MAP_LOOKUP_ELEM
> Lookup element by key in a given map and return its value
>
> BPF_MAP_UPDATE_ELEM
> Create or update element (key/value pair) in a given map
>
> BPF_MAP_DELETE_ELEM
> Lookup and delete element by key in a given map
>
> BPF_MAP_GET_NEXT_KEY
> Lookup element by key in a given map and return key of next ele-
> ment
>
> BPF_PROG_LOAD
> Verify and load eBPF program
>
> attr is a pointer to a union of type bpf_attr as defined below.
>
> size is the size of the union.
I find this strange. Why not just make attr be a pointer to the
relevant struct for the operation being invoked?
>
> union bpf_attr {
> struct { /* anonymous struct used by BPF_MAP_CREATE command */
> enum bpf_map_type map_type;
Does this reliably generate the same type on compat systems? C++11
has a fix for enum ABI compatibility, but this is plain C :(
> struct { /* anonymous struct used by BPF_PROG_LOAD command */
> enum bpf_prog_type prog_type;
> __u32 insn_cnt;
> const struct bpf_insn *insns;
> const char *license;
> __u32 log_level; /* verbosity level of eBPF verifier */
> __u32 log_size; /* size of user buffer */
> void *log_buf; /* user supplied buffer */
> };
> };
It might be a bit nicer to have separate in and out arguments.
>
> BPF_MAP_CREATE
> int bpf_create_map(enum bpf_map_type map_type, int key_size,
> int value_size, int max_entries)
> {
> union bpf_attr attr = {
> .map_type = map_type,
> .key_size = key_size,
> .value_size = value_size,
> .max_entries = max_entries
> };
I feel like this is asking for trouble, or at least bizarre namespace
collisions in the anonymous struct members. At least please give the
structs names. (Also, the first time I read this, I assumed that
those were union members, which would have made the code be nonsense.)
>
> BPF_MAP_DELETE_ELEM
> int bpf_delete_elem(int fd, void *key)
> {
> union bpf_attr attr = {
> .map_fd = fd,
> .key = key,
> };
>
> return bpf(BPF_MAP_DELETE_ELEM, &attr, sizeof(attr));
> }
> The call deletes an element in a map fd with given key.
What does it return? (The same question goes for a bunch of the map ops.)
>
> eBPF programs
> BPF_PROG_LOAD
> This cmd is used to load eBPF program into the kernel.
>
> char bpf_log_buf[LOG_BUF_SIZE];
What happens if the size isn't LOG_BUF_SIZE?
>
> int bpf_prog_load(enum bpf_prog_type prog_type,
> const struct bpf_insn *insns, int insn_cnt,
> const char *license)
> {
> union bpf_attr attr = {
> .prog_type = prog_type,
> .insns = insns,
> .insn_cnt = insn_cnt,
> .license = license,
> .log_buf = bpf_log_buf,
> .log_size = LOG_BUF_SIZE,
> .log_level = 1,
> };
>
> return bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
> }
> prog_type one of the available program types:
> enum bpf_prog_type {
> BPF_PROG_TYPE_UNSPEC,
> BPF_PROG_TYPE_SOCKET_FILTER,
> BPF_PROG_TYPE_TRACING_FILTER,
> };
Why does the type matter?
> {
> static struct bpf_insn prog[] = {
> BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
> BPF_LD_ABS(BPF_B, 14 + 9 /* R0 = ip->proto */),
> BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp - 4) = r0 */
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp - 4 */
> BPF_LD_MAP_FD(BPF_REG_1, 0),
> BPF_CALL_FUNC(BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
> BPF_XADD(BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
> BPF_MOV64_IMM(BPF_REG_0, 0), /* r0 = 0 */
> BPF_EXIT_INSN(),
> };
> int sock, map_fd, prog_fd, key;
> long long value = 0, tcp_cnt, udp_cnt;
>
> map_fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value), 2);
> if (map_fd < 0) {
> printf("failed to create map '%s'\n", strerror(errno));
> /* likely not run as root */
> return 1;
> }
>
> key = 6; /* tcp */
> assert(bpf_update_elem(map_fd, &key, &value) == 0);
>
> key = 17; /* udp */
> assert(bpf_update_elem(map_fd, &key, &value) == 0);
>
> prog[5].imm = map_fd;
This (the .imm thing) is sufficiently weird that I think it needs to
be mentioned in the main docs, not just in an example. It's
especially odd since AFAIK essentially every other object format in
the world uses a separate relocation table instead of inline magic
opcodes like this.
>
> All other commands
> Zero.
Shouldn't delete return different values depending on whether anything
was deleted?
>
> ENOENT For BPF_MAP_LOOKUP_ELEM or BPF_MAP_DELETE_ELEM, indicates that
> element with given key was not found.
Ah, here it is. Please document this with the ops.
>
> E2BIG program is too large.
>
> NOTES
> These commands may be used only by a privileged process (one having the
> CAP_SYS_ADMIN capability).
I hope this goes away :)
I can't shake the feeling that the whole syscall map API is wrong and
that, instead, there should be a more general concept of objects
provided by the eBPF runtime. Those objects could have methods that
are callable by the syscall and callable from eBPF code.
--Andy
^ permalink raw reply
* Re: [PATCH v11 net-next 11/12] net: filter: move eBPF instruction macros
From: Alexei Starovoitov @ 2014-09-10 18:16 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <54103506.1030501-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Sep 10, 2014 at 4:24 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/10/2014 07:10 AM, Alexei Starovoitov wrote:
>>
>> move instruction macros (like BPF_MOV64_REG or BPF_ALU32_IMM)
>> from linux/filter.h into uapi/linux/bpf.h
>> so that userspace programs can use them.
>>
>> verifier testsuite (in later patches) will be using them.
>>
>> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>
>
> I don't think we need this commit at all. These macros are not really
> part of the ABI and we should expose __as less as possible__, otherwise
> we won't be able to alter them anymore. Plus, based on your previous
> argumentation regarding the header file expose, we also won't be able
> to add any new macros anymore since you could run into name collisions.
>
> Anyway, I don't see a reason why a e.g. central user space eBPF library
> cannot live with, for example, a private copy of these helper macros or
> whatever it wants to define by itself.
>
> I know in classic BPF there's BPF_STMT() and BPF_JUMP() but it's much
> less complicated than eBPF and less likely to changes.
Interesting idea. Are you saying just copy paste these
226 lines into user libbpf.h for now to use them in testsuite
and examples ? Hmm.
Indeed the less lines we add to uapi the better. I'll do that.
^ permalink raw reply
* Re: [PATCH v11 net-next 12/12] bpf: mini eBPF library, test stubs and verifier testsuite
From: Alexei Starovoitov @ 2014-09-10 18:08 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <54103776.3080706-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Sep 10, 2014 at 4:35 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Since we already have an extensive BPF test suite, that is, lib/test_bpf.c,
> which currently also does sanity checks for the classic BPF verifier, is
> there a reason these verifier test cases cannot be extended/integrated there
> as well but have to go to kernel/bpf/test_stub.c resp.
> samples/bpf/test_verifier.c ?
> I don't like that we put testing code into kernel/bpf/ whereas we already
> have a BPF test infrastructure in the kernel elsewhere.
yes. there is a reason. Verifier needs to be tested from user space,
since it works on fds. Process local map_fd are part of the eBPF
programs. Therefore one is testing things from kernel and
another from userspace. We definitely need both.
Currently there is no use case to call verifier from inside
the kernel. I'm not sure there will be one. Verifier's main
purpose is to check user supplied programs and provide
humans an understandable error messages of what
is 'unsafe' in particular program.
Eventually we will integrate this verifier messages with
program compilation. Like, the user would write a program
in C then invoke a wrapper of compiler and verifier, which
will point to lines in C code which are doing something
wrong like loops or out of bounds access. Currently verifier
complains about particular 'unsafe' instruction, but
humans have hard time correlating asm to C.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-10 17:48 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <541017FD.1030308@redhat.com>
On Wed, Sep 10, 2014 at 2:21 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
>
> When you pass in these structs with pointers in it to other user space
> buffers, how do you handle this with mixed 32/64 bit user/kernel space?
...
> Perhaps I'm missing something, but I think, that would currently break in
> your syscall handler, no?
yes. compat stuff is not part of this diff.
I was thinking to handle it similar to the way signalfd deals with it:
add compat syscall, compat struct and convert it before going into
main entry point.
^ permalink raw reply
* Re: [PATCH v11 net-next 00/12] eBPF syscall, verifier, testsuite
From: Alexei Starovoitov @ 2014-09-10 17:32 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Hannes Frederic Sowa, Chema Gonzalez,
Eric Dumazet, Peter Zijlstra, Pablo Neira Ayuso, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <541013CE.6020307-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Sep 10, 2014 at 2:03 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> struct { /* anonymous struct used by BPF_PROG_LOAD command */
>> enum bpf_prog_type prog_type;
>> __u32 insn_cnt;
>> const struct bpf_insn *insns;
>> const char *license;
>> __u32 log_level; /* verbosity level of
>> eBPF verifier */
>> __u32 log_size; /* size of user buffer */
>> void *log_buf; /* user supplied buffer
>> */
>
>
> What is log buffer? Would that mean the verifier will return an error
> string if the program will not pass it, or if not, what other data?
> I think the man page is missing how to examine the returned verifier
> log buffer data.
yes. it's an error log (as text string for humans) from verifier.
It is briefly explained in the man page:
EACCES For BPF_PROG_LOAD, though program has valid instructions, it was
rejected, since it was deemed unsafe (may access disallowed mem-
ory region or uninitialized stack/register or function con-
straints don't match actual types or misaligned access). In such
case it is recommended to call bpf() again with log_level = 1
and examine log_buf for specific reason provided by verifier.
and there is a whole section about it in
Documentation/networking/filter.txt
called "Understanding eBPF verifier messages".
^ 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