Linux userland API discussions
 help / color / mirror / Atom feed
* [PATCH v3 5/6] tpm: TPM 2.0 CRB Interface
From: Jarkko Sakkinen @ 2014-10-15 11:35 UTC (permalink / raw)
  To: Peter Huewe, Ashley Lai, Marcel Selhorst
  Cc: tpmdd-devel, linux-kernel, linux-api, josh.triplett,
	christophe.ricard, will.c.arthur, monty.wiseman, Jarkko Sakkinen
In-Reply-To: <1413372916-12091-1-git-send-email-jarkko.sakkinen@linux.intel.com>

tpm_crb is a driver for TPM 2.0 Command Response Buffer (CRB) Interface
as defined in PC Client Platform TPM Profile (PTP) Specification.

Only polling and single locality is supported as these are the limitations
of the available hardware, Platform Trust Techonlogy (PTT) in Haswell
CPUs.

The driver always applies CRB with ACPI start because PTT reports using
only ACPI start as start method but as a result of my testing it requires
also CRB start.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/Kconfig   |   9 ++
 drivers/char/tpm/Makefile  |   1 +
 drivers/char/tpm/tpm_crb.c | 329 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_crb.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index c54cac3..10c9419 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -122,4 +122,13 @@ config TCG_XEN
 	  To compile this driver as a module, choose M here; the module
 	  will be called xen-tpmfront.
 
+config TCG_CRB
+	tristate "TPM 2.0 CRB Interface"
+	depends on X86 && ACPI
+	---help---
+	  If you have a TPM security chip that is compliant with the
+	  TCG CRB 2.0 TPM specification say Yes and it will be accessible
+	  from within Linux.  To compile this driver as a module, choose
+	  M here; the module will be called tpm_crb.
+
 endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index d3cf905..15e3b4c 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
 obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
 obj-$(CONFIG_TCG_ST33_I2C) += tpm_i2c_stm_st33.o
 obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
+obj-$(CONFIG_TCG_CRB) += tpm_crb.o
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
new file mode 100644
index 0000000..0509228
--- /dev/null
+++ b/drivers/char/tpm/tpm_crb.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * Authors:
+ * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
+ *
+ * Maintained by: <tpmdd-devel@lists.sourceforge.net>
+ *
+ * This device driver implements the TPM interface as defined in
+ * the TCG CRB 2.0 TPM specification.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/acpi.h>
+#include <linux/highmem.h>
+#include <linux/rculist.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include "tpm.h"
+
+#define ACPI_SIG_TPM2 "TPM2"
+
+static const u8 CRB_ACPI_START_UUID[] = {
+	/* 0000 */ 0xAB, 0x6C, 0xBF, 0x6B, 0x63, 0x54, 0x14, 0x47,
+	/* 0008 */ 0xB7, 0xCD, 0xF0, 0x20, 0x3C, 0x03, 0x68, 0xD4
+};
+
+enum crb_defaults {
+	CRB_ACPI_START_REVISION_ID = 1,
+	CRB_ACPI_START_INDEX = 1,
+};
+
+enum crb_start_method {
+	CRB_SM_ACPI_START = 2,
+	CRB_SM_CRB = 7,
+	CRB_SM_CRB_WITH_ACPI_START = 8,
+};
+
+struct acpi_tpm2 {
+	struct acpi_table_header hdr;
+	u16 platform_class;
+	u16 reserved;
+	u64 control_area_pa;
+	u32 start_method;
+};
+
+enum crb_ca_request {
+	CRB_CA_REQ_GO_IDLE	= BIT(0),
+	CRB_CA_REQ_CMD_READY	= BIT(1),
+};
+
+enum crb_ca_status {
+	CRB_CA_STS_ERROR	= BIT(0),
+	CRB_CA_STS_TPM_IDLE	= BIT(1),
+};
+
+struct crb_control_area {
+	u32 req;
+	u32 sts;
+	u32 cancel;
+	u32 start;
+	u32 int_enable;
+	u32 int_sts;
+	u32 cmd_size;
+	u64 cmd_pa;
+	u32 rsp_size;
+	u64 rsp_pa;
+} __packed;
+
+enum crb_status {
+	CRB_STS_COMPLETE	= BIT(0),
+};
+
+enum crb_flags {
+	CRB_FL_ACPI_START	= BIT(0),
+	CRB_FL_CRB_START	= BIT(1),
+};
+
+struct crb_priv {
+	unsigned int flags;
+	struct crb_control_area *cca;
+	unsigned long cca_pa;
+	acpi_handle dev_handle;
+};
+
+#ifdef CONFIG_PM_SLEEP
+int crb_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int crb_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	(void) tpm2_do_selftest(chip);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(crb_pm, crb_suspend, crb_resume);
+
+static u8 crb_status(struct tpm_chip *chip)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+	u8 sts = 0;
+
+	if ((le32_to_cpu(priv->cca->start) & 1) != 1)
+		sts |= CRB_STS_COMPLETE;
+
+	return sts;
+}
+
+static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+	struct crb_control_area *cca;
+	unsigned int expected;
+	unsigned long offset;
+	u8 *resp;
+
+	cca = priv->cca;
+	if (le32_to_cpu(cca->sts) & CRB_CA_STS_ERROR)
+		return -EIO;
+
+	offset = le64_to_cpu(cca->rsp_pa) - priv->cca_pa;
+	resp = (u8 *) ((unsigned long) cca + offset);
+	memcpy(buf, resp, 6);
+	expected = be32_to_cpup((__be32 *) &buf[2]);
+
+	if (expected > count)
+		return -EIO;
+
+	memcpy(&buf[6], &resp[6], expected - 6);
+
+	return expected;
+}
+
+static int crb_do_acpi_start(struct tpm_chip *chip)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+	union acpi_object *obj;
+	int rc;
+
+	obj = acpi_evaluate_dsm(priv->dev_handle,
+				CRB_ACPI_START_UUID,
+				CRB_ACPI_START_REVISION_ID,
+				CRB_ACPI_START_INDEX,
+				NULL);
+	if (!obj)
+		return -ENXIO;
+	rc = obj->integer.value == 0 ? 0 : -ENXIO;
+	ACPI_FREE(obj);
+	return rc;
+}
+
+static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+	struct crb_control_area *cca;
+	u8 *cmd;
+	int rc = 0;
+
+	cca = priv->cca;
+
+	if (len > le32_to_cpu(cca->cmd_size)) {
+		dev_err(chip->dev,
+			"invalid command count value %x %zx\n",
+			(unsigned int) len,
+			(size_t) le32_to_cpu(cca->cmd_size));
+		return -E2BIG;
+	}
+
+	cmd = (u8 *) ((unsigned long) cca + le64_to_cpu(cca->cmd_pa) - priv->cca_pa);
+	memcpy(cmd, buf, len);
+	wmb();
+
+	cca->start = cpu_to_le32(1);
+	rc = crb_do_acpi_start(chip);
+	return rc;
+}
+
+static void crb_cancel(struct tpm_chip *chip)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+	struct crb_control_area *cca;
+
+	cca = priv->cca;
+	cca->cancel = cpu_to_le32(1);
+	wmb();
+
+	if (crb_do_acpi_start(chip))
+		dev_err(chip->dev, "ACPI Start failed\n");
+
+	cca->cancel = 0;
+}
+
+static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	struct crb_priv *priv = chip->vendor.priv;
+
+	return (le32_to_cpu(priv->cca->cancel) & 1) == 1;
+}
+
+static const struct tpm_class_ops tpm_crb = {
+	.status = crb_status,
+	.recv = crb_recv,
+	.send = crb_send,
+	.cancel = crb_cancel,
+	.req_canceled = crb_req_canceled,
+	.req_complete_mask = CRB_STS_COMPLETE,
+	.req_complete_val = CRB_STS_COMPLETE,
+};
+
+static int crb_acpi_add(struct acpi_device *device)
+{
+	struct tpm_chip *chip;
+	struct acpi_tpm2 *buf;
+	struct crb_priv *priv;
+	struct device *dev = &device->dev;
+	acpi_status status;
+	u32 sm;
+	int rc;
+
+	chip = tpmm_chip_alloc(dev, &tpm_crb);
+	if (IS_ERR(chip)) {
+		dev_err(dev, "allocating chip failed\n");
+		return PTR_ERR(chip);
+	}
+
+	chip->flags = TPM_CHIP_FLAG_TPM2;
+
+	status = acpi_get_table(ACPI_SIG_TPM2, 1,
+				(struct acpi_table_header **) &buf);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "could not get TPM2 ACPI table\n");
+		return -ENODEV;
+	}
+
+	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
+						GFP_KERNEL);
+	if (!priv) {
+		dev_err(dev, "allocating memory failed\n");
+		return -ENOMEM;
+	}
+
+	sm = le32_to_cpu(buf->start_method);
+
+	if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START)
+		priv->flags |= CRB_FL_CRB_START;
+
+	if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+		priv->flags |= CRB_FL_ACPI_START;
+
+	priv->dev_handle = device->handle;
+	priv->cca_pa = le32_to_cpu(buf->control_area_pa);
+	priv->cca = (struct crb_control_area *)
+		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
+	if (!priv->cca) {
+		dev_err(dev, "allocating memory failed\n");
+		return -ENOMEM;
+	}
+
+	chip->vendor.priv = priv;
+
+	/* Default timeouts and durations */
+	chip->vendor.timeout_a = usecs_to_jiffies(TPM2_TIMEOUT_A);
+	chip->vendor.timeout_b = usecs_to_jiffies(TPM2_TIMEOUT_B);
+	chip->vendor.timeout_c = usecs_to_jiffies(TPM2_TIMEOUT_C);
+	chip->vendor.timeout_d = usecs_to_jiffies(TPM2_TIMEOUT_D);
+	chip->vendor.duration[TPM_SHORT] =
+		usecs_to_jiffies(TPM2_DURATION_SHORT);
+	chip->vendor.duration[TPM_MEDIUM] =
+		usecs_to_jiffies(TPM2_DURATION_MEDIUM);
+	chip->vendor.duration[TPM_LONG] =
+		usecs_to_jiffies(TPM2_DURATION_LONG);
+
+	rc = tpm2_do_selftest(chip);
+	if (rc) {
+		dev_err(dev, "self-test failed\n");
+		return rc;
+	}
+
+	rc = tpm_chip_register(chip);
+	if (rc) {
+		dev_err(dev, "registering the chip failed\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+int crb_acpi_remove(struct acpi_device *device)
+{
+	struct device *dev = &device->dev;
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	tpm_chip_unregister(chip);
+	return 0;
+}
+
+static struct acpi_device_id crb_device_ids[] = {
+	{"MSFT0101", 0},
+	{"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, crb_device_ids);
+
+static struct acpi_driver crb_acpi_driver = {
+	.name = "tpm_crb",
+	.ids = crb_device_ids,
+	.ops = {
+		.add = crb_acpi_add,
+		.remove = crb_acpi_remove,
+	},
+	.drv = {
+		.pm = &crb_pm,
+	},
+};
+
+module_acpi_driver(crb_acpi_driver);
+MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
+MODULE_DESCRIPTION("TPM2 Driver");
+MODULE_VERSION("0.1");
+MODULE_LICENSE("GPL");
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 6/6] tpm: TPM 2.0 FIFO Interface
From: Jarkko Sakkinen @ 2014-10-15 11:35 UTC (permalink / raw)
  To: Peter Huewe, Ashley Lai, Marcel Selhorst
  Cc: tpmdd-devel, linux-kernel, linux-api, josh.triplett,
	christophe.ricard, will.c.arthur, monty.wiseman, Jarkko Sakkinen
In-Reply-To: <1413372916-12091-1-git-send-email-jarkko.sakkinen@linux.intel.com>

From: Will Arthur <will.c.arthur@intel.com>

Detect TPM 2.0 by using the extended STS (STS3) register. For TPM 2.0,
instead of calling tpm_get_timeouts(), assign duration and timeout
values defined in the TPM 2.0 PTP specification.

Signed-off-by: Will Arthur <will.c.arthur@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 75 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e2cb04d..ec89c61d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2005, 2006 IBM Corporation
+ * Copyright (C) 2014 Intel Corporation
  *
  * Authors:
  * Leendert van Doorn <leendert@watson.ibm.com>
@@ -44,6 +45,10 @@ enum tis_status {
 	TPM_STS_DATA_EXPECT = 0x08,
 };
 
+enum tis_status3 {
+	TPM_STS3_TPM2_FAM = 0x04,
+};
+
 enum tis_int_flags {
 	TPM_GLOBAL_INT_ENABLE = 0x80000000,
 	TPM_INTF_BURST_COUNT_STATIC = 0x100,
@@ -70,6 +75,7 @@ enum tis_defaults {
 #define	TPM_INT_STATUS(l)		(0x0010 | ((l) << 12))
 #define	TPM_INTF_CAPS(l)		(0x0014 | ((l) << 12))
 #define	TPM_STS(l)			(0x0018 | ((l) << 12))
+#define	TPM_STS3(l)			(0x001b | ((l) << 12))
 #define	TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
 
 #define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
@@ -344,6 +350,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	int rc;
 	u32 ordinal;
+	unsigned long dur;
 
 	rc = tpm_tis_send_data(chip, buf, len);
 	if (rc < 0)
@@ -355,9 +362,14 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 
 	if (chip->vendor.irq) {
 		ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
+
+		if (chip->flags & TPM_CHIP_FLAG_TPM2)
+			dur = tpm_calc_ordinal_duration(chip, ordinal);
+		else
+			dur = tpm_calc_ordinal_duration(chip, ordinal);
+
 		if (wait_for_tpm_stat
-		    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
-		     tpm_calc_ordinal_duration(chip, ordinal),
+		    (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
 		     &chip->vendor.read_queue, false) < 0) {
 			rc = -ETIME;
 			goto out_err;
@@ -542,6 +554,8 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	u32 vendor, intfcaps, intmask;
 	int rc, i, irq_s, irq_e, probe;
 	struct tpm_chip *chip;
+	u8 sts3;
+	u32 dummy;
 
 	chip = tpmm_chip_alloc(dev, &tpm_tis);
 	if (IS_ERR(chip))
@@ -551,11 +565,28 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	if (!chip->vendor.iobase)
 		return -EIO;
 
-	/* Default timeouts */
-	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
-	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
-	chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
-	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+	sts3 = ioread8(chip->vendor.iobase + TPM_STS3(1));
+	if (sts3 & TPM_STS3_TPM2_FAM)
+		chip->flags = TPM_CHIP_FLAG_TPM2;
+
+        /* Default timeouts */
+        if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+                chip->vendor.timeout_a = usecs_to_jiffies(TPM2_TIMEOUT_A);
+                chip->vendor.timeout_b = usecs_to_jiffies(TPM2_TIMEOUT_B);
+                chip->vendor.timeout_c = usecs_to_jiffies(TPM2_TIMEOUT_C);
+                chip->vendor.timeout_d = usecs_to_jiffies(TPM2_TIMEOUT_D);
+                chip->vendor.duration[TPM_SHORT] =
+                        usecs_to_jiffies(TPM2_DURATION_SHORT);
+                chip->vendor.duration[TPM_MEDIUM] =
+                        usecs_to_jiffies(TPM2_DURATION_MEDIUM);
+                chip->vendor.duration[TPM_LONG] =
+                        usecs_to_jiffies(TPM2_DURATION_LONG);
+        } else {
+                chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+                chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+                chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+                chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+        }
 
 	if (wait_startup(chip, 0) != 0) {
 		rc = -ENODEV;
@@ -570,9 +601,9 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
 	chip->vendor.manufacturer_id = vendor;
 
-	dev_info(dev,
-		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
-		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
+        dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
+                 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",
+                 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
 	if (!itpm) {
 		probe = probe_itpm(chip);
@@ -619,7 +650,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		goto out_err;
 	}
 
-	if (tpm_do_selftest(chip)) {
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = tpm2_do_selftest(chip);
+	else
+		rc = tpm_do_selftest(chip);
+	if (rc) {
 		dev_err(dev, "TPM self test failed\n");
 		rc = -ENODEV;
 		goto out_err;
@@ -680,7 +715,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 			chip->vendor.probed_irq = 0;
 
 			/* Generate Interrupts */
-			tpm_gen_interrupt(chip);
+			if (chip->flags & TPM_CHIP_FLAG_TPM2)
+				rc = tpm2_get_tpm_pt(chip, TPM2_CAP_TPM_PROPERTIES, &dummy,
+						     "attempting to generate an interrupt");
+			else
+				tpm_gen_interrupt(chip);
 
 			chip->vendor.irq = chip->vendor.probed_irq;
 
@@ -756,14 +795,18 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 static int tpm_tis_resume(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	int ret;
+	int ret = 0;
 
 	if (chip->vendor.irq)
 		tpm_tis_reenable_interrupts(chip);
 
-	ret = tpm_pm_resume(dev);
-	if (!ret)
-		tpm_do_selftest(chip);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_do_selftest(chip);
+	else {
+		ret = tpm_pm_resume(dev);
+		if (!ret)
+			tpm_do_selftest(chip);
+	}
 
 	return ret;
 }
-- 
2.1.0

^ permalink raw reply related

* Re: [RFC PATCH v2 3/3] i2c: show and change bus frequency via sysfs
From: Octavian Purdila @ 2014-10-15 11:49 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wolfram Sang, Johan Hovold, linux-i2c,
	linux-api-u79uwXL29TY76Z2rM5mHXA, lkml
In-Reply-To: <20141014154151.GB10067-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>
> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
> > This patch adds three new sysfs files: bus_frequency,
> > bus_min_frequency and bus_max_frequency which allows the user to view
> > or change the bus frequency on a per bus level.
> >

<snip>

> > +
> > +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
> > +                i2c_sysfs_freq_store);
>
> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>

Unfortunately that won't work because we must transform bus_frequency
to a RW entry (via is_visible) if the bus can change the frequency. We
can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
RO entry with is visible is not possible:

fs/sysfs/group.c:

static int create_files(struct kernfs_node *parent, struct kobject *kobj,
...
                        if (grp->is_visible) {
                                mode = grp->is_visible(kobj, *attr, i);
                                if (!mode)
                                        continue;
                        }
                        error = sysfs_add_file_mode_ns(parent, *attr, false,
                                                       (*attr)->mode | mode,
                                                       NULL);

Of course if we only allow a RW frequency entry as you suggest below,
then we can use DEVICE_ATTR_RW.

> > +
> > +
> > +static ssize_t
> > +i2c_sysfs_min_freq_show(struct device *dev, struct device_attribute *attr,
> > +                       char *buf)
> > +{
> > +     struct i2c_adapter *adap = to_i2c_adapter(dev);
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%d\n", adap->min_freq);
> > +}
> > +
> > +static DEVICE_ATTR(bus_min_frequency, S_IRUGO, i2c_sysfs_min_freq_show, NULL);
> > +
>
> Consider using DEVICE_ATTR_RO.
>

OK.

> > +static ssize_t
> > +i2c_sysfs_max_freq_show(struct device *dev, struct device_attribute *attr,
> > +                       char *buf)
> > +{
> > +     struct i2c_adapter *adap = to_i2c_adapter(dev);
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%d\n", adap->max_freq);
> > +}
> > +
> > +static DEVICE_ATTR(bus_max_frequency, S_IRUGO, i2c_sysfs_max_freq_show, NULL);
> > +
> Consider using DEVICE_ATTR_RO.
>

OK.

> Overall, it seems to me that the bus_ in front of the attrribute names
> is really not necessary. The attributes are attached to the adapter, so it
> should be obvious that the attributes describe the adapter (=bus) frequency and
> not some other frequency.
>
> > +umode_t i2c_adapter_visible_attr(struct kobject *kobj,
> > +                              struct attribute *attr, int idx)
>
> static umode_t
>

Oops :)

> > +{
> > +     struct device *dev = container_of(kobj, struct device, kobj);
> > +     struct i2c_adapter *adap = to_i2c_adapter(dev);
> > +     umode_t mode = attr->mode;
> > +
> > +     if (attr == &dev_attr_bus_min_frequency.attr)
> > +             return adap->min_freq ? mode : 0;
> > +
> > +     if (attr == &dev_attr_bus_max_frequency.attr)
> > +             return adap->max_freq ? mode : 0;
> > +
> > +     if (attr == &dev_attr_bus_frequency.attr) {
> > +             if (adap->set_freq)
> > +                     mode |= S_IWUSR;
> > +             return adap->freq ? mode : 0;
>
> Personally, I would make all those attributes only visible if the adapter
> supports setting the frquency, and not bother with other conditions,
> to keep things simple. Not a strong call, though.
>

I don't have a strong opinion either. I think a RO frequency entry
would help with debugging, but I am not sure how useful it is in
practice.

^ permalink raw reply

* Re: [RFC PATCH v2 3/3] i2c: show and change bus frequency via sysfs
From: Guenter Roeck @ 2014-10-15 13:13 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: Wolfram Sang, Johan Hovold, linux-i2c,
	linux-api-u79uwXL29TY76Z2rM5mHXA, lkml
In-Reply-To: <CAE1zotKpgEqAhtQvsFFee8xnX02MmhwrM0YU-6OHVZHcxiAi9g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On 10/15/2014 04:49 AM, Octavian Purdila wrote:
> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>
>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>> This patch adds three new sysfs files: bus_frequency,
>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>> or change the bus frequency on a per bus level.
>>>
>
> <snip>
>
>>> +
>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>> +                i2c_sysfs_freq_store);
>>
>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>
>
> Unfortunately that won't work because we must transform bus_frequency
> to a RW entry (via is_visible) if the bus can change the frequency. We

Ah yes, you are right.

> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
> RO entry with is visible is not possible:
>

Why not ?

is_visible returns the desired mode. Just like you can return mode | S_IWUSR,
you can return mode & ~S_IWUSR.

Am I missing something ?

Guenter

^ permalink raw reply

* Re: [RFC PATCH v2 3/3] i2c: show and change bus frequency via sysfs
From: Octavian Purdila @ 2014-10-15 13:32 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wolfram Sang, Johan Hovold, linux-i2c,
	linux-api-u79uwXL29TY76Z2rM5mHXA, lkml
In-Reply-To: <543E7312.1020104-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

On Wed, Oct 15, 2014 at 4:13 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 10/15/2014 04:49 AM, Octavian Purdila wrote:
>>
>> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>
>>>
>>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>>>
>>>> This patch adds three new sysfs files: bus_frequency,
>>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>>> or change the bus frequency on a per bus level.
>>>>
>>
>> <snip>
>>
>>>> +
>>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>>> +                i2c_sysfs_freq_store);
>>>
>>>
>>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>>
>>
>> Unfortunately that won't work because we must transform bus_frequency
>> to a RW entry (via is_visible) if the bus can change the frequency. We
>
>
> Ah yes, you are right.
>
>> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
>> RO entry with is visible is not possible:
>>
>
> Why not ?
>
> is_visible returns the desired mode. Just like you can return mode |
> S_IWUSR,
> you can return mode & ~S_IWUSR.
>
> Am I missing something ?
>

Here is how sysfs uses is_visible:

static int create_files(struct kernfs_node *parent, struct kobject *kobj,
...
                        if (grp->is_visible) {
                                mode = grp->is_visible(kobj, *attr, i);
                                if (!mode)
                                        continue;
                        }
                        error = sysfs_add_file_mode_ns(parent, *attr, false,
                                                       (*attr)->mode | mode,
                                                       NULL);

so basically is the mode set is the original mode from the attributed
"or-ed" with the mode return by is_visible.

^ permalink raw reply

* Re: [RFC PATCH v2 3/3] i2c: show and change bus frequency via sysfs
From: Guenter Roeck @ 2014-10-15 13:46 UTC (permalink / raw)
  To: Octavian Purdila; +Cc: Wolfram Sang, Johan Hovold, linux-i2c, linux-api, lkml
In-Reply-To: <CAE1zotL4TLpdCeaX4=1YC=9PenscvAMDXJDAdYdVRNmGcjJaeQ@mail.gmail.com>

On 10/15/2014 06:32 AM, Octavian Purdila wrote:
> On Wed, Oct 15, 2014 at 4:13 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 10/15/2014 04:49 AM, Octavian Purdila wrote:
>>>
>>> On Tue, Oct 14, 2014 at 6:41 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>>
>>>> On Tue, Oct 14, 2014 at 05:48:14PM +0300, Octavian Purdila wrote:
>>>>>
>>>>> This patch adds three new sysfs files: bus_frequency,
>>>>> bus_min_frequency and bus_max_frequency which allows the user to view
>>>>> or change the bus frequency on a per bus level.
>>>>>
>>>
>>> <snip>
>>>
>>>>> +
>>>>> +static DEVICE_ATTR(bus_frequency, S_IRUGO, i2c_sysfs_freq_show,
>>>>> +                i2c_sysfs_freq_store);
>>>>
>>>>
>>>> Consider using DEVICE_ATTR_RO here. Also, extra empty line.
>>>>
>>>
>>> Unfortunately that won't work because we must transform bus_frequency
>>> to a RW entry (via is_visible) if the bus can change the frequency. We
>>
>>
>> Ah yes, you are right.
>>
>>> can't use DEVIE_ATTR_RW either, because transforming a RW entry to a
>>> RO entry with is visible is not possible:
>>>
>>
>> Why not ?
>>
>> is_visible returns the desired mode. Just like you can return mode |
>> S_IWUSR,
>> you can return mode & ~S_IWUSR.
>>
>> Am I missing something ?
>>
>
> Here is how sysfs uses is_visible:
>
> static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> ...
>                          if (grp->is_visible) {
>                                  mode = grp->is_visible(kobj, *attr, i);
>                                  if (!mode)
>                                          continue;
>                          }
>                          error = sysfs_add_file_mode_ns(parent, *attr, false,
>                                                         (*attr)->mode | mode,
>                                                         NULL);
>
> so basically is the mode set is the original mode from the attributed
> "or-ed" with the mode return by is_visible.
>
Ah, you are right. That's a new one for me. Thanks, I didn't notice earlier.
Good to know ;-).

Guenter

^ permalink raw reply

* [PATH v3 0/4] i2c: show and change bus frequency via sysfs
From: Octavian Purdila @ 2014-10-15 20:03 UTC (permalink / raw)
  To: wsa-z923LK4zBo2bacvFa/9K2g
  Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, johan-DgEjT+Ai2ygdnm+yROfE0A,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Octavian Purdila

This patch series adds support to show and change the bus frequency
via sysfs, by exposing files to show the minimum, maximum and current
frequency as well as allowing the frequency to be changed. This allows
the user to view or change the bus frequency on a per bus level.

Changes since v3:

 * use DEVICE_ATTR_RO and make i2c_adapter_visible_attr static
 
 * implement sysfs bus frequency support for the diolan u2c bus

Octavian Purdila (4):
  i2c: document the existing i2c sysfs ABI
  i2c: document struct i2c_adapter
  i2c: show and change bus frequency via sysfs
  i2c: i2c-diolan-u2c: sysfs bus frequency support

 Documentation/ABI/testing/sysfs-bus-i2c | 75 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-diolan-u2c.c     | 49 ++++++++++++------
 drivers/i2c/i2c-core.c                  | 90 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 32 ++++++++++--
 4 files changed, 228 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

-- 
1.9.1

^ permalink raw reply

* [PATH v3 1/4] i2c: document the existing i2c sysfs ABI
From: Octavian Purdila @ 2014-10-15 20:03 UTC (permalink / raw)
  To: wsa; +Cc: linux, johan, linux-i2c, linux-api, linux-kernel,
	Octavian Purdila
In-Reply-To: <1413403411-8895-1-git-send-email-octavian.purdila@intel.com>

This patch adds Documentation/ABI/testing/sysfs-bus-i2c which
documents the existing i2c sysfs ABI.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-i2c | 45 +++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
new file mode 100644
index 0000000..8075585
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -0,0 +1,45 @@
+What:		/sys/bus/i2c/devices/i2c-X
+KernelVersion:	since at least 2.6.12
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		This entry represents a registered i2c bus. X is the
+		bus number and its format is "%d".
+
+What:		/sys/bus/i2c/devices/i2c-X/Y
+What:		/sys/bus/i2c/devices/Y
+KernelVersion:	since at least 2.6.12
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		An i2c device attached to bus X. Format of Y is
+		"%d-%04x" where the first number is the bus number (X)
+		and the second number is the device i2c address.
+
+What:		/sys/bus/i2c/devices/i2c-X/new_device
+KernelVersion:	2.6.31
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		Write only entry that allows instantiating a
+		new i2c device on bus X. This is to be used when
+		enumeration mechanism such as ACPI or DT are not
+		present or not used for this device.
+		Format: "%s %hi\n" where the first argument is the
+		device name (no spaces allowed) and the second is the
+		i2c address of the device.
+
+What:		/sys/bus/i2c/devices/i2c-X/delete_device
+KernelVersion:	2.6.31
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		Write only entry that allows the removal of an i2c
+		device from bus X.
+		Format: "%s %hi\n" where the first argument is the
+		device name (no spaces allowed) and the second is the
+		i2c address of the device.
+
+What:		/sys/bus/i2c/devices/i2c-X/i2c-Y
+What:		/sys/bus/i2c/devices/i2c-Y
+KernelVersion:	3.13
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		An i2c device attached to bus X that is enumerated via
+		ACPI. Y is the ACPI device name and its format is "%s".
-- 
1.9.1

^ permalink raw reply related

* [PATH v3 2/4] i2c: document struct i2c_adapter
From: Octavian Purdila @ 2014-10-15 20:03 UTC (permalink / raw)
  To: wsa; +Cc: linux, johan, linux-i2c, linux-api, linux-kernel,
	Octavian Purdila
In-Reply-To: <1413403411-8895-1-git-send-email-octavian.purdila@intel.com>

Document the i2c_adapter fields that must be initialized before
calling i2c_add_adapter().

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 include/linux/i2c.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a95efeb..36041e2 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -418,9 +418,19 @@ int i2c_recover_bus(struct i2c_adapter *adap);
 int i2c_generic_gpio_recovery(struct i2c_adapter *adap);
 int i2c_generic_scl_recovery(struct i2c_adapter *adap);
 
-/*
- * i2c_adapter is the structure used to identify a physical i2c bus along
- * with the access algorithms necessary to access it.
+/**
+ * struct i2c_adapter - represents an I2C physical bus
+ *
+ * The following members must be set by the adapter driver before
+ * calling i2c_add_adapter():
+ *
+ * @owner: the module owner, usually THIS_MODULE
+ * @class: bitmask of I2C_CLASS_*
+ * @algo: see struct i2c_algorithm
+ * @dev.parent: set this to the struct device of the driver (e.g. pci_dev->dev,
+ *	usb->interface->dev, platform_device->dev etc.)
+ * @name: name of this i2c bus
+ * @bus_recovery_info: see struct i2c_bus_recovery_info. Optional.
  */
 struct i2c_adapter {
 	struct module *owner;
-- 
1.9.1

^ permalink raw reply related

* [PATH v3 3/4] i2c: show and change bus frequency via sysfs
From: Octavian Purdila @ 2014-10-15 20:03 UTC (permalink / raw)
  To: wsa; +Cc: linux, johan, linux-i2c, linux-api, linux-kernel,
	Octavian Purdila
In-Reply-To: <1413403411-8895-1-git-send-email-octavian.purdila@intel.com>

This patch adds three new sysfs files: frequency, frequency_min and
frequency_max which allows the user to view or change the bus
frequency on a per bus level.

This is required for the case where multiple busses have the same
adapter driver in which case a module parameter does not allow
controlling the bus speed individually (e.g. USB I2C adapters).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-i2c | 30 +++++++++++
 drivers/i2c/i2c-core.c                  | 90 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 16 ++++++
 3 files changed, 136 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c b/Documentation/ABI/testing/sysfs-bus-i2c
index 8075585..70af81f 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c
+++ b/Documentation/ABI/testing/sysfs-bus-i2c
@@ -43,3 +43,33 @@ Contact:	linux-i2c@vger.kernel.org
 Description:
 		An i2c device attached to bus X that is enumerated via
 		ACPI. Y is the ACPI device name and its format is "%s".
+
+What:		/sys/bus/i2c/devices/i2c-X/frequency
+KernelVersion:	3.19
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		The current bus frequency for bus X. Can be updated if
+		the bus supports it. The unit is HZ and format is
+		"%d\n".
+		If the bus does not support changing the frequency
+		then this entry will be read-only.
+		If the bus does not support showing the frequency than
+		this entry will not be visible.
+		When updating the bus frequency that value must be in
+		the range defined by bus_frequency_min and
+		bus_frequency_max otherwise writing to this entry will
+		fail with -EINVAL.
+		The bus may not support all of the frequencies in the
+		min-max range and may round the frequency to the
+		closest supported one. The user must read the entry
+		after writing it to retrieve the actual set frequency.
+
+What:		/sys/bus/i2c/devices/i2c-X/frequency_min
+What:		/sys/bus/i2c/devices/i2c-X/frequency_max
+KernelVersion:	3.19
+Contact:	linux-i2c@vger.kernel.org
+Description:
+		Minimum and maximum frequencies for bus X. The unit is
+		HZ and format is "%d\n".
+		If the bus does not support changing the frequency
+		these entries will not be visible.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 632057a..27c29eb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -940,15 +940,97 @@ static DEVICE_ATTR(new_device, S_IWUSR, NULL, i2c_sysfs_new_device);
 static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, S_IWUSR, NULL,
 				   i2c_sysfs_delete_device);
 
+static ssize_t
+i2c_sysfs_freq_show(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", adap->freq);
+}
+
+static ssize_t
+i2c_sysfs_freq_store(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+	unsigned int freq;
+	int ret;
+
+	if (kstrtouint(buf, 0, &freq) || freq < adap->min_freq ||
+	    freq > adap->max_freq)
+		return -EINVAL;
+
+	i2c_lock_adapter(adap);
+	ret = adap->set_freq(adap, &freq);
+	i2c_unlock_adapter(adap);
+
+	if (ret)
+		return ret;
+
+	adap->freq = freq;
+
+	return count;
+}
+
+static DEVICE_ATTR(frequency, S_IRUGO, i2c_sysfs_freq_show,
+		   i2c_sysfs_freq_store);
+
+static ssize_t frequency_min_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", adap->min_freq);
+}
+
+static DEVICE_ATTR_RO(frequency_min);
+
+static ssize_t frequency_max_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", adap->max_freq);
+}
+
+static DEVICE_ATTR_RO(frequency_max);
+
+static umode_t i2c_adapter_visible_attr(struct kobject *kobj,
+					struct attribute *attr, int idx)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+	umode_t mode = attr->mode;
+
+	if (attr == &dev_attr_frequency_min.attr)
+		return adap->min_freq ? mode : 0;
+
+	if (attr == &dev_attr_frequency_max.attr)
+		return adap->max_freq ? mode : 0;
+
+	if (attr == &dev_attr_frequency.attr) {
+		if (adap->set_freq)
+			mode |= S_IWUSR;
+		return adap->freq ? mode : 0;
+	}
+
+	return mode;
+}
+
 static struct attribute *i2c_adapter_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_new_device.attr,
 	&dev_attr_delete_device.attr,
+	&dev_attr_frequency.attr,
+	&dev_attr_frequency_min.attr,
+	&dev_attr_frequency_max.attr,
 	NULL
 };
 
 static struct attribute_group i2c_adapter_attr_group = {
 	.attrs		= i2c_adapter_attrs,
+	.is_visible	= i2c_adapter_visible_attr,
 };
 
 static const struct attribute_group *i2c_adapter_attr_groups[] = {
@@ -1262,6 +1344,14 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
 	struct device *dev = &adapter->dev;
 	int id;
 
+	if (adapter->set_freq) {
+		if (!adapter->freq || !adapter->min_freq || !adapter->max_freq)
+			return -EINVAL;
+	} else {
+		if (adapter->min_freq || adapter->max_freq)
+			return -EINVAL;
+	}
+
 	if (dev->of_node) {
 		id = of_alias_get_id(dev->of_node, "i2c");
 		if (id >= 0) {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 36041e2..5d893f5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -431,6 +431,17 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap);
  *	usb->interface->dev, platform_device->dev etc.)
  * @name: name of this i2c bus
  * @bus_recovery_info: see struct i2c_bus_recovery_info. Optional.
+ * @set_freq: set the bus frequency (in HZ) and returns the actual set
+ *	value. Since not all the frequencies in the min - max interval
+ *	may be valid the driver may round the frequency to the closest
+ *	supported one. Optional but must be set if min_freq or
+ *	max_freq is set.
+ * @min_freq: minimum bus frequency. Optional but must be set if
+ *	set_freq is set.
+ * @max_freq: maximum bus frequency. Optional but must be set if
+ *	set_freq is set.
+ * @freq: initial bus frequency. Optional but must be set if set_freq
+ *	is set.
  */
 struct i2c_adapter {
 	struct module *owner;
@@ -438,6 +449,11 @@ struct i2c_adapter {
 	const struct i2c_algorithm *algo; /* the algorithm to access the bus */
 	void *algo_data;
 
+	unsigned int freq;
+	unsigned int min_freq;
+	unsigned int max_freq;
+	int (*set_freq)(struct i2c_adapter *a, unsigned int *freq);
+
 	/* data fields that are valid for all devices	*/
 	struct rt_mutex bus_lock;
 
-- 
1.9.1

^ permalink raw reply related

* [PATH v3 4/4] i2c: i2c-diolan-u2c: sysfs bus frequency support
From: Octavian Purdila @ 2014-10-15 20:03 UTC (permalink / raw)
  To: wsa-z923LK4zBo2bacvFa/9K2g
  Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, johan-DgEjT+Ai2ygdnm+yROfE0A,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Octavian Purdila
In-Reply-To: <1413403411-8895-1-git-send-email-octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Add support for showing and changing the bus frequency via
sysfs.

Tested on a DLN2 adapter run in U2C-12 compatibility mode.

Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-diolan-u2c.c | 49 +++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
index b19a310..ff4e120 100644
--- a/drivers/i2c/busses/i2c-diolan-u2c.c
+++ b/drivers/i2c/busses/i2c-diolan-u2c.c
@@ -71,6 +71,9 @@
 #define U2C_I2C_FREQ_STD	100000
 #define U2C_I2C_FREQ(s)		(1000000 / (2 * (s - 1) + 10))
 
+#define U2C_I2C_MIN_FREQ	U2C_I2C_FREQ(U2C_I2C_SPEED_2KHZ)
+#define U2C_I2C_MAX_FREQ	U2C_I2C_FREQ_FAST
+
 #define DIOLAN_USB_TIMEOUT	100	/* in ms */
 #define DIOLAN_SYNC_TIMEOUT	20	/* in ms */
 
@@ -298,31 +301,24 @@ static void diolan_get_serial(struct i2c_diolan_u2c *dev)
 	}
 }
 
-static int diolan_init(struct i2c_diolan_u2c *dev)
+static int diolan_set_freq(struct i2c_adapter *adapter, unsigned int *frequency)
 {
+	struct i2c_diolan_u2c *dev = i2c_get_adapdata(adapter);
 	int speed, ret;
 
-	if (frequency >= 200000) {
+	if (*frequency >= 200000) {
 		speed = U2C_I2C_SPEED_FAST;
-		frequency = U2C_I2C_FREQ_FAST;
-	} else if (frequency >= 100000 || frequency == 0) {
+		*frequency = U2C_I2C_FREQ_FAST;
+	} else if (*frequency >= 100000 || *frequency == 0) {
 		speed = U2C_I2C_SPEED_STD;
-		frequency = U2C_I2C_FREQ_STD;
+		*frequency = U2C_I2C_FREQ_STD;
 	} else {
-		speed = U2C_I2C_SPEED(frequency);
+		speed = U2C_I2C_SPEED(*frequency);
 		if (speed > U2C_I2C_SPEED_2KHZ)
 			speed = U2C_I2C_SPEED_2KHZ;
-		frequency = U2C_I2C_FREQ(speed);
+		*frequency = U2C_I2C_FREQ(speed);
 	}
 
-	dev_info(&dev->interface->dev,
-		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
-		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
-
-	diolan_flush_input(dev);
-	diolan_fw_version(dev);
-	diolan_get_serial(dev);
-
 	/* Set I2C speed */
 	ret = diolan_set_speed(dev, speed);
 	if (ret < 0)
@@ -336,6 +332,23 @@ static int diolan_init(struct i2c_diolan_u2c *dev)
 	if (speed != U2C_I2C_SPEED_FAST)
 		ret = diolan_set_clock_synch_timeout(dev, DIOLAN_SYNC_TIMEOUT);
 
+	return 0;
+}
+
+static int diolan_init(struct i2c_diolan_u2c *dev)
+{
+	int ret;
+
+	diolan_flush_input(dev);
+	diolan_fw_version(dev);
+	diolan_get_serial(dev);
+
+	ret = diolan_set_freq(&dev->adapter, &frequency);
+
+	dev_info(&dev->interface->dev,
+		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
+		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
+
 	return ret;
 }
 
@@ -471,6 +484,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
 	dev->adapter.owner = THIS_MODULE;
 	dev->adapter.class = I2C_CLASS_HWMON;
 	dev->adapter.algo = &diolan_usb_algorithm;
+	dev->adapter.min_freq = U2C_I2C_MIN_FREQ;
+	dev->adapter.max_freq = U2C_I2C_MAX_FREQ;
+	dev->adapter.set_freq = diolan_set_freq;
 	i2c_set_adapdata(&dev->adapter, dev);
 	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
 		 DRIVER_NAME " at bus %03d device %03d",
@@ -485,6 +501,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
 		goto error_free;
 	}
 
+	/* set the current bus frequency */
+	dev->adapter.freq = frequency;
+
 	/* and finally attach to i2c layer */
 	ret = i2c_add_adapter(&dev->adapter);
 	if (ret < 0) {
-- 
1.9.1

^ permalink raw reply related

* Re: [PATH v3 0/4] i2c: show and change bus frequency via sysfs
From: Wolfram Sang @ 2014-10-16  6:53 UTC (permalink / raw)
  To: Octavian Purdila
  Cc: linux-0h96xk9xTtrk1uMJSBkQmQ, johan-DgEjT+Ai2ygdnm+yROfE0A,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1413403411-8895-1-git-send-email-octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Wed, Oct 15, 2014 at 11:03:27PM +0300, Octavian Purdila wrote:
> This patch series adds support to show and change the bus frequency
> via sysfs, by exposing files to show the minimum, maximum and current
> frequency as well as allowing the frequency to be changed. This allows
> the user to view or change the bus frequency on a per bus level.

To give you some early feedback.

Patches 1-2 are nice, will review them somewhen, no general issues here.
For patch 3, I am not convinced that sysfs is the right mechanism. Maybe
configfs, yet I have never dealt with it, so far. So, looking into this
might take more time since I have other core changes pending before
that.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply

* Re: [PATCH v3 4/6] tpm: TPM 2.0 sysfs attributes
From: Greg KH @ 2014-10-16  9:31 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Peter Huewe, Ashley Lai, Marcel Selhorst,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
	christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
	will.c.arthur-ral2JQCrhuEAvxtiuMwx3w,
	monty.wiseman-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <1413372916-12091-5-git-send-email-jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

On Wed, Oct 15, 2014 at 01:35:14PM +0200, Jarkko Sakkinen wrote:
> +int tpm2_sysfs_add_device(struct tpm_chip *chip)
> +{
> +	struct pcr_bank *pcr_bank;
> +	struct kobject *pcrs_kobj;
> +	struct device *dev = chip->dev;
> +	int rc;
> +
> +	rc = sysfs_create_group(&chip->vendor.miscdev.this_device->kobj,
> +				&tpm_dev_group);
> +	if (rc) {
> +		dev_err(dev, "failed to create sysfs attributes, %d\n", rc);
> +		return rc;
> +	}

You just raced and created sysfs files _after_ userspace saw that your
device was enabled.

> +	pcrs_kobj = kobject_create_and_add("pcrs",
> +					   &chip->vendor.miscdev.this_device->kobj);

Ick, no no no no.  Never create a kobject under a 'struct device'.  You
just lost all libudev support for any attribute you created under here,
not good at all.  Use a "real" device if you really want a sub-device,
not a kobject.

thanks,

greg k-h

^ permalink raw reply

* [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Greg Kroah-Hartman @ 2014-10-16 12:47 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
	linux-api-u79uwXL29TY76Z2rM5mHXA
  Cc: Santosh Shilimkar, John Stultz, Arve Hjønnevåg,
	Sumit Semwal, Rebecca Schultz Zavin, Christoffer Dall, Anup Patel

From: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>

The Android binder code has been "stable" for many years now.  No matter
what comes in the future, we are going to have to support this API, so
might as well move it to the "real" part of the kernel as there's no
real work that needs to be done to the existing code.

Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
---

This was discussed in the Android miniconf at the Plumbers conference.
If anyone has any objections to this, please let me know, otherwise I'm
queueing this up for 3.19-rc1


 drivers/Kconfig                                    |  2 ++
 drivers/Makefile                                   |  1 +
 drivers/android/Kconfig                            | 37 ++++++++++++++++++++++
 drivers/android/Makefile                           |  3 ++
 drivers/{staging => }/android/binder.c             |  0
 drivers/{staging => }/android/binder.h             |  2 +-
 drivers/{staging => }/android/binder_trace.h       |  0
 drivers/staging/android/Kconfig                    | 30 ------------------
 drivers/staging/android/Makefile                   |  1 -
 include/uapi/linux/Kbuild                          |  1 +
 include/uapi/linux/android/Kbuild                  |  2 ++
 .../uapi => include/uapi/linux/android}/binder.h   |  0
 12 files changed, 47 insertions(+), 32 deletions(-)
 create mode 100644 drivers/android/Kconfig
 create mode 100644 drivers/android/Makefile
 rename drivers/{staging => }/android/binder.c (100%)
 rename drivers/{staging => }/android/binder.h (95%)
 rename drivers/{staging => }/android/binder_trace.h (100%)
 create mode 100644 include/uapi/linux/android/Kbuild
 rename {drivers/staging/android/uapi => include/uapi/linux/android}/binder.h (100%)

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 1a693d3f9d51..569ff7886dc3 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -182,4 +182,6 @@ source "drivers/ras/Kconfig"
 
 source "drivers/thunderbolt/Kconfig"
 
+source "drivers/android/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index ebee55537a05..60d19820a4d4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -161,3 +161,4 @@ obj-$(CONFIG_POWERCAP)		+= powercap/
 obj-$(CONFIG_MCB)		+= mcb/
 obj-$(CONFIG_RAS)		+= ras/
 obj-$(CONFIG_THUNDERBOLT)	+= thunderbolt/
+obj-$(CONFIG_ANDROID)		+= android/
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
new file mode 100644
index 000000000000..bdfc6c6f4f5a
--- /dev/null
+++ b/drivers/android/Kconfig
@@ -0,0 +1,37 @@
+menu "Android"
+
+config ANDROID
+	bool "Android Drivers"
+	---help---
+	  Enable support for various drivers needed on the Android platform
+
+if ANDROID
+
+config ANDROID_BINDER_IPC
+	bool "Android Binder IPC Driver"
+	depends on MMU
+	default n
+	---help---
+	  Binder is used in Android for both communication between processes,
+	  and remote method invocation.
+
+	  This means one Android process can call a method/routine in another
+	  Android process, using Binder to identify, invoke and pass arguments
+	  between said processes.
+
+config ANDROID_BINDER_IPC_32BIT
+	bool
+	depends on !64BIT && ANDROID_BINDER_IPC
+	default y
+	---help---
+	  The Binder API has been changed to support both 32 and 64bit
+	  applications in a mixed environment.
+
+	  Enable this to support an old 32-bit Android user-space (v4.4 and
+	  earlier).
+
+	  Note that enabling this will break newer Android user-space.
+
+endif # if ANDROID
+
+endmenu
diff --git a/drivers/android/Makefile b/drivers/android/Makefile
new file mode 100644
index 000000000000..3b7e4b072c58
--- /dev/null
+++ b/drivers/android/Makefile
@@ -0,0 +1,3 @@
+ccflags-y += -I$(src)			# needed for trace events
+
+obj-$(CONFIG_ANDROID_BINDER_IPC)	+= binder.o
diff --git a/drivers/staging/android/binder.c b/drivers/android/binder.c
similarity index 100%
rename from drivers/staging/android/binder.c
rename to drivers/android/binder.c
diff --git a/drivers/staging/android/binder.h b/drivers/android/binder.h
similarity index 95%
rename from drivers/staging/android/binder.h
rename to drivers/android/binder.h
index eb0834656dfe..5dc6a66b0665 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/android/binder.h
@@ -24,7 +24,7 @@
 #define BINDER_IPC_32BIT 1
 #endif
 
-#include "uapi/binder.h"
+#include <uapi/linux/android/binder.h>
 
 #endif /* _LINUX_BINDER_H */
 
diff --git a/drivers/staging/android/binder_trace.h b/drivers/android/binder_trace.h
similarity index 100%
rename from drivers/staging/android/binder_trace.h
rename to drivers/android/binder_trace.h
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 7a0e28852965..7e012f37792b 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -1,37 +1,7 @@
 menu "Android"
 
-config ANDROID
-	bool "Android Drivers"
-	---help---
-	  Enable support for various drivers needed on the Android platform
-
 if ANDROID
 
-config ANDROID_BINDER_IPC
-	bool "Android Binder IPC Driver"
-	depends on MMU
-	default n
-	---help---
-	  Binder is used in Android for both communication between processes,
-	  and remote method invocation.
-
-	  This means one Android process can call a method/routine in another
-	  Android process, using Binder to identify, invoke and pass arguments
-	  between said processes.
-
-config ANDROID_BINDER_IPC_32BIT
-	bool
-	depends on !64BIT && ANDROID_BINDER_IPC
-	default y
-	---help---
-	  The Binder API has been changed to support both 32 and 64bit
-	  applications in a mixed environment.
-
-	  Enable this to support an old 32-bit Android user-space (v4.4 and
-	  earlier).
-
-	  Note that enabling this will break newer Android user-space.
-
 config ASHMEM
 	bool "Enable the Anonymous Shared Memory Subsystem"
 	default n
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 517ad5ffa429..479b2b86f8c8 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -2,7 +2,6 @@ ccflags-y += -I$(src)			# needed for trace events
 
 obj-y					+= ion/
 
-obj-$(CONFIG_ANDROID_BINDER_IPC)	+= binder.o
 obj-$(CONFIG_ASHMEM)			+= ashmem.o
 obj-$(CONFIG_ANDROID_LOGGER)		+= logger.o
 obj-$(CONFIG_ANDROID_TIMED_OUTPUT)	+= timed_output.o
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 70e150ebc6c9..1bbaf6457861 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -1,4 +1,5 @@
 # UAPI Header export list
+header-y += android/
 header-y += byteorder/
 header-y += can/
 header-y += caif/
diff --git a/include/uapi/linux/android/Kbuild b/include/uapi/linux/android/Kbuild
new file mode 100644
index 000000000000..ca011eec252a
--- /dev/null
+++ b/include/uapi/linux/android/Kbuild
@@ -0,0 +1,2 @@
+# UAPI Header export list
+header-y += binder.h
diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.h
similarity index 100%
rename from drivers/staging/android/uapi/binder.h
rename to include/uapi/linux/android/binder.h
-- 
2.1.2

^ permalink raw reply related

* Re: [PATH v3 4/4] i2c: i2c-diolan-u2c: sysfs bus frequency support
From: Guenter Roeck @ 2014-10-16 13:24 UTC (permalink / raw)
  To: Octavian Purdila, wsa-z923LK4zBo2bacvFa/9K2g
  Cc: johan-DgEjT+Ai2ygdnm+yROfE0A, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1413403411-8895-5-git-send-email-octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On 10/15/2014 01:03 PM, Octavian Purdila wrote:
> Add support for showing and changing the bus frequency via
> sysfs.
>
> Tested on a DLN2 adapter run in U2C-12 compatibility mode.
>
> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-diolan-u2c.c | 49 +++++++++++++++++++++++++------------
>   1 file changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
> index b19a310..ff4e120 100644
> --- a/drivers/i2c/busses/i2c-diolan-u2c.c
> +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> @@ -71,6 +71,9 @@
>   #define U2C_I2C_FREQ_STD	100000
>   #define U2C_I2C_FREQ(s)		(1000000 / (2 * (s - 1) + 10))
>
> +#define U2C_I2C_MIN_FREQ	U2C_I2C_FREQ(U2C_I2C_SPEED_2KHZ)
> +#define U2C_I2C_MAX_FREQ	U2C_I2C_FREQ_FAST
> +
>   #define DIOLAN_USB_TIMEOUT	100	/* in ms */
>   #define DIOLAN_SYNC_TIMEOUT	20	/* in ms */
>
> @@ -298,31 +301,24 @@ static void diolan_get_serial(struct i2c_diolan_u2c *dev)
>   	}
>   }
>
> -static int diolan_init(struct i2c_diolan_u2c *dev)
> +static int diolan_set_freq(struct i2c_adapter *adapter, unsigned int *frequency)

I really dislike this kind of side-effect programming, where a function
named as _set changes one of its parameters. Not my call to make here,
though, so if the i2c maintainers are fine with it

Acked-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

>   {
> +	struct i2c_diolan_u2c *dev = i2c_get_adapdata(adapter);
>   	int speed, ret;
>
> -	if (frequency >= 200000) {
> +	if (*frequency >= 200000) {
>   		speed = U2C_I2C_SPEED_FAST;
> -		frequency = U2C_I2C_FREQ_FAST;
> -	} else if (frequency >= 100000 || frequency == 0) {
> +		*frequency = U2C_I2C_FREQ_FAST;
> +	} else if (*frequency >= 100000 || *frequency == 0) {
>   		speed = U2C_I2C_SPEED_STD;
> -		frequency = U2C_I2C_FREQ_STD;
> +		*frequency = U2C_I2C_FREQ_STD;
>   	} else {
> -		speed = U2C_I2C_SPEED(frequency);
> +		speed = U2C_I2C_SPEED(*frequency);
>   		if (speed > U2C_I2C_SPEED_2KHZ)
>   			speed = U2C_I2C_SPEED_2KHZ;
> -		frequency = U2C_I2C_FREQ(speed);
> +		*frequency = U2C_I2C_FREQ(speed);
>   	}
>
> -	dev_info(&dev->interface->dev,
> -		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> -		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> -
> -	diolan_flush_input(dev);
> -	diolan_fw_version(dev);
> -	diolan_get_serial(dev);
> -
>   	/* Set I2C speed */
>   	ret = diolan_set_speed(dev, speed);
>   	if (ret < 0)
> @@ -336,6 +332,23 @@ static int diolan_init(struct i2c_diolan_u2c *dev)
>   	if (speed != U2C_I2C_SPEED_FAST)
>   		ret = diolan_set_clock_synch_timeout(dev, DIOLAN_SYNC_TIMEOUT);
>
> +	return 0;
> +}
> +
> +static int diolan_init(struct i2c_diolan_u2c *dev)
> +{
> +	int ret;
> +
> +	diolan_flush_input(dev);
> +	diolan_fw_version(dev);
> +	diolan_get_serial(dev);
> +
> +	ret = diolan_set_freq(&dev->adapter, &frequency);
> +
> +	dev_info(&dev->interface->dev,
> +		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> +		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> +
>   	return ret;
>   }
>
> @@ -471,6 +484,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
>   	dev->adapter.owner = THIS_MODULE;
>   	dev->adapter.class = I2C_CLASS_HWMON;
>   	dev->adapter.algo = &diolan_usb_algorithm;
> +	dev->adapter.min_freq = U2C_I2C_MIN_FREQ;
> +	dev->adapter.max_freq = U2C_I2C_MAX_FREQ;
> +	dev->adapter.set_freq = diolan_set_freq;
>   	i2c_set_adapdata(&dev->adapter, dev);
>   	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
>   		 DRIVER_NAME " at bus %03d device %03d",
> @@ -485,6 +501,9 @@ static int diolan_u2c_probe(struct usb_interface *interface,
>   		goto error_free;
>   	}
>
> +	/* set the current bus frequency */
> +	dev->adapter.freq = frequency;
> +
>   	/* and finally attach to i2c layer */
>   	ret = i2c_add_adapter(&dev->adapter);
>   	if (ret < 0) {
>

^ permalink raw reply

* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: Michael Kerrisk (man-pages) @ 2014-10-16 14:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: lkml, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Linux API,
	Santosh Shilimkar, John Stultz, Arve Hjønnevåg,
	Sumit Semwal, Rebecca Schultz Zavin, Christoffer Dall, Anup Patel
In-Reply-To: <20141016124741.GA3832-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>

On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> From: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>
> The Android binder code has been "stable" for many years now.  No matter
> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.

Where does one find the canonical documentation of the user-space API?

Thanks,

Michael


> Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> ---
>
> This was discussed in the Android miniconf at the Plumbers conference.
> If anyone has any objections to this, please let me know, otherwise I'm
> queueing this up for 3.19-rc1
>
>
>  drivers/Kconfig                                    |  2 ++
>  drivers/Makefile                                   |  1 +
>  drivers/android/Kconfig                            | 37 ++++++++++++++++++++++
>  drivers/android/Makefile                           |  3 ++
>  drivers/{staging => }/android/binder.c             |  0
>  drivers/{staging => }/android/binder.h             |  2 +-
>  drivers/{staging => }/android/binder_trace.h       |  0
>  drivers/staging/android/Kconfig                    | 30 ------------------
>  drivers/staging/android/Makefile                   |  1 -
>  include/uapi/linux/Kbuild                          |  1 +
>  include/uapi/linux/android/Kbuild                  |  2 ++
>  .../uapi => include/uapi/linux/android}/binder.h   |  0
>  12 files changed, 47 insertions(+), 32 deletions(-)
>  create mode 100644 drivers/android/Kconfig
>  create mode 100644 drivers/android/Makefile
>  rename drivers/{staging => }/android/binder.c (100%)
>  rename drivers/{staging => }/android/binder.h (95%)
>  rename drivers/{staging => }/android/binder_trace.h (100%)
>  create mode 100644 include/uapi/linux/android/Kbuild
>  rename {drivers/staging/android/uapi => include/uapi/linux/android}/binder.h (100%)
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 1a693d3f9d51..569ff7886dc3 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -182,4 +182,6 @@ source "drivers/ras/Kconfig"
>
>  source "drivers/thunderbolt/Kconfig"
>
> +source "drivers/android/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index ebee55537a05..60d19820a4d4 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -161,3 +161,4 @@ obj-$(CONFIG_POWERCAP)              += powercap/
>  obj-$(CONFIG_MCB)              += mcb/
>  obj-$(CONFIG_RAS)              += ras/
>  obj-$(CONFIG_THUNDERBOLT)      += thunderbolt/
> +obj-$(CONFIG_ANDROID)          += android/
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> new file mode 100644
> index 000000000000..bdfc6c6f4f5a
> --- /dev/null
> +++ b/drivers/android/Kconfig
> @@ -0,0 +1,37 @@
> +menu "Android"
> +
> +config ANDROID
> +       bool "Android Drivers"
> +       ---help---
> +         Enable support for various drivers needed on the Android platform
> +
> +if ANDROID
> +
> +config ANDROID_BINDER_IPC
> +       bool "Android Binder IPC Driver"
> +       depends on MMU
> +       default n
> +       ---help---
> +         Binder is used in Android for both communication between processes,
> +         and remote method invocation.
> +
> +         This means one Android process can call a method/routine in another
> +         Android process, using Binder to identify, invoke and pass arguments
> +         between said processes.
> +
> +config ANDROID_BINDER_IPC_32BIT
> +       bool
> +       depends on !64BIT && ANDROID_BINDER_IPC
> +       default y
> +       ---help---
> +         The Binder API has been changed to support both 32 and 64bit
> +         applications in a mixed environment.
> +
> +         Enable this to support an old 32-bit Android user-space (v4.4 and
> +         earlier).
> +
> +         Note that enabling this will break newer Android user-space.
> +
> +endif # if ANDROID
> +
> +endmenu
> diff --git a/drivers/android/Makefile b/drivers/android/Makefile
> new file mode 100644
> index 000000000000..3b7e4b072c58
> --- /dev/null
> +++ b/drivers/android/Makefile
> @@ -0,0 +1,3 @@
> +ccflags-y += -I$(src)                  # needed for trace events
> +
> +obj-$(CONFIG_ANDROID_BINDER_IPC)       += binder.o
> diff --git a/drivers/staging/android/binder.c b/drivers/android/binder.c
> similarity index 100%
> rename from drivers/staging/android/binder.c
> rename to drivers/android/binder.c
> diff --git a/drivers/staging/android/binder.h b/drivers/android/binder.h
> similarity index 95%
> rename from drivers/staging/android/binder.h
> rename to drivers/android/binder.h
> index eb0834656dfe..5dc6a66b0665 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/android/binder.h
> @@ -24,7 +24,7 @@
>  #define BINDER_IPC_32BIT 1
>  #endif
>
> -#include "uapi/binder.h"
> +#include <uapi/linux/android/binder.h>
>
>  #endif /* _LINUX_BINDER_H */
>
> diff --git a/drivers/staging/android/binder_trace.h b/drivers/android/binder_trace.h
> similarity index 100%
> rename from drivers/staging/android/binder_trace.h
> rename to drivers/android/binder_trace.h
> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> index 7a0e28852965..7e012f37792b 100644
> --- a/drivers/staging/android/Kconfig
> +++ b/drivers/staging/android/Kconfig
> @@ -1,37 +1,7 @@
>  menu "Android"
>
> -config ANDROID
> -       bool "Android Drivers"
> -       ---help---
> -         Enable support for various drivers needed on the Android platform
> -
>  if ANDROID
>
> -config ANDROID_BINDER_IPC
> -       bool "Android Binder IPC Driver"
> -       depends on MMU
> -       default n
> -       ---help---
> -         Binder is used in Android for both communication between processes,
> -         and remote method invocation.
> -
> -         This means one Android process can call a method/routine in another
> -         Android process, using Binder to identify, invoke and pass arguments
> -         between said processes.
> -
> -config ANDROID_BINDER_IPC_32BIT
> -       bool
> -       depends on !64BIT && ANDROID_BINDER_IPC
> -       default y
> -       ---help---
> -         The Binder API has been changed to support both 32 and 64bit
> -         applications in a mixed environment.
> -
> -         Enable this to support an old 32-bit Android user-space (v4.4 and
> -         earlier).
> -
> -         Note that enabling this will break newer Android user-space.
> -
>  config ASHMEM
>         bool "Enable the Anonymous Shared Memory Subsystem"
>         default n
> diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
> index 517ad5ffa429..479b2b86f8c8 100644
> --- a/drivers/staging/android/Makefile
> +++ b/drivers/staging/android/Makefile
> @@ -2,7 +2,6 @@ ccflags-y += -I$(src)                   # needed for trace events
>
>  obj-y                                  += ion/
>
> -obj-$(CONFIG_ANDROID_BINDER_IPC)       += binder.o
>  obj-$(CONFIG_ASHMEM)                   += ashmem.o
>  obj-$(CONFIG_ANDROID_LOGGER)           += logger.o
>  obj-$(CONFIG_ANDROID_TIMED_OUTPUT)     += timed_output.o
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 70e150ebc6c9..1bbaf6457861 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -1,4 +1,5 @@
>  # UAPI Header export list
> +header-y += android/
>  header-y += byteorder/
>  header-y += can/
>  header-y += caif/
> diff --git a/include/uapi/linux/android/Kbuild b/include/uapi/linux/android/Kbuild
> new file mode 100644
> index 000000000000..ca011eec252a
> --- /dev/null
> +++ b/include/uapi/linux/android/Kbuild
> @@ -0,0 +1,2 @@
> +# UAPI Header export list
> +header-y += binder.h
> diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.h
> similarity index 100%
> rename from drivers/staging/android/uapi/binder.h
> rename to include/uapi/linux/android/binder.h
> --
> 2.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

^ permalink raw reply

* [RFC PATCH v2 1/4] vfio: platform: add device tree info API and skeleton
From: Antonios Motakis @ 2014-10-16 15:54 UTC (permalink / raw)
  To: kvmarm, iommu, alex.williamson
  Cc: tech, christoffer.dall, eric.auger, kim.phillips,
	Antonios Motakis, open list, open list:VFIO DRIVER,
	open list:ABI/API
In-Reply-To: <1413474876-28544-1-git-send-email-a.motakis@virtualopensystems.com>

This patch introduced the API to return device tree info about
a PLATFORM device (if described by a device tree) and the skeleton
of the implementation for VFIO_PLATFORM. Information about any device
node bound by VFIO_PLATFORM should be queried via the introduced ioctl
VFIO_DEVICE_GET_DEVTREE_INFO.

The proposed API allows to get a list of strings with available property
names, and then allows to query each property. Note that the properties
are not indexed numerically, so they are always accessed by property name.
The user needs to know the data type of the property he is accessing.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/Makefile                |  3 +-
 drivers/vfio/platform/devtree.c               | 70 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_common.c  | 39 +++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  6 +++
 include/uapi/linux/vfio.h                     | 26 ++++++++++
 5 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 drivers/vfio/platform/devtree.c

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 81de144..99f3ba1 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,5 +1,6 @@
 
-vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
+vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o \
+		   devtree.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
 
diff --git a/drivers/vfio/platform/devtree.c b/drivers/vfio/platform/devtree.c
new file mode 100644
index 0000000..c057be3
--- /dev/null
+++ b/drivers/vfio/platform/devtree.c
@@ -0,0 +1,70 @@
+#include <linux/slab.h>
+#include <linux/vfio.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include "vfio_platform_private.h"
+
+static int devtree_get_prop_list(struct device_node *np, unsigned *lenp,
+				 void __user *datap, unsigned long datasz)
+{
+	return -EINVAL;
+}
+
+static int devtree_get_strings(struct device_node *np,
+			       char *name, unsigned *lenp,
+			       void __user *datap, unsigned long datasz)
+{
+	return -EINVAL;
+}
+
+static int devtree_get_uint(struct device_node *np, char *name,
+			    uint32_t type, unsigned *lenp,
+			    void __user *datap, unsigned long datasz)
+{
+	return -EINVAL;
+}
+
+int vfio_platform_devtree_info(struct device_node *np,
+			       uint32_t type, unsigned *lenp,
+			       void __user *datap, unsigned long datasz)
+{
+	char *name;
+	long namesz;
+	int ret;
+
+	if (type == VFIO_DEVTREE_PROP_LIST) {
+		return devtree_get_prop_list(np, lenp, datap, datasz);
+	}
+
+	namesz = strnlen_user(datap, datasz);
+	if (!namesz)
+		return -EFAULT;
+	if (namesz > datasz)
+		return -EINVAL;
+
+	name = kzalloc(namesz, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+	if (strncpy_from_user(name, datap, namesz) <= 0) {
+		kfree(name);
+		return -EFAULT;
+	}
+
+	switch (type) {
+	case VFIO_DEVTREE_TYPE_STRINGS:
+		ret = devtree_get_strings(np, name, lenp, datap, datasz);
+		break;
+
+	case VFIO_DEVTREE_TYPE_U32:
+	case VFIO_DEVTREE_TYPE_U16:
+	case VFIO_DEVTREE_TYPE_U8:
+		ret = devtree_get_uint(np, name, type, lenp, datap, datasz);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	kfree(name);
+	return ret;
+}
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 2a6c665..bfbee2f 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -24,6 +24,7 @@
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #include "vfio_platform_private.h"
 
@@ -244,6 +245,34 @@ static long vfio_platform_ioctl(void *device_data,
 
 		return ret;
 
+	} else if (cmd == VFIO_DEVICE_GET_DEVTREE_INFO) {
+		struct vfio_devtree_info info;
+		void __user *datap;
+		unsigned long datasz;
+		int ret;
+
+		if (!vdev->of_node)
+			return -EINVAL;
+
+		minsz = offsetofend(struct vfio_devtree_info, length);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		datap = (void __user *) arg + minsz;
+		datasz = info.argsz - minsz;
+
+		ret = vfio_platform_devtree_info(vdev->of_node, info.type,
+						 &info.length, datap, datasz);
+
+		if (copy_to_user((void __user *)arg, &info, minsz))
+			ret = -EFAULT;
+
+		return ret;
+
 	} else if (cmd == VFIO_DEVICE_RESET)
 		return -EINVAL;
 
@@ -486,6 +515,11 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
+	/* get device tree node for info if available */
+	vdev->of_node = of_node_get(dev->of_node);
+	if (vdev->of_node)
+		vdev->flags |= VFIO_DEVICE_FLAGS_DEVTREE;
+
 	mutex_init(&vdev->igate);
 
 	return 0;
@@ -500,6 +534,11 @@ int vfio_platform_remove_common(struct device *dev)
 	if (!vdev)
 		return -EINVAL;
 
+	if (vdev->of_node) {
+		of_node_put(vdev->of_node);
+		vdev->of_node = NULL;
+	}
+
 	iommu_group_put(dev->iommu_group);
 	kfree(vdev);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 23ef038..fd40a26 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -60,6 +60,7 @@ struct vfio_platform_device {
 	void		*opaque;
 	const char	*name;
 	uint32_t	flags;
+	struct device_node *of_node;
 	/* callbacks to discover device resources */
 	struct resource*
 		(*get_resource)(struct vfio_platform_device *vdev, int i);
@@ -77,4 +78,9 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 			uint32_t flags, unsigned index, unsigned start,
 			unsigned count, void *data);
 
+/* device tree info support in devtree.c */
+extern int vfio_platform_devtree_info(struct device_node *np,
+				      uint32_t type, unsigned *lenp,
+				      void __user *datap, unsigned long datasz);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 1a5986c..18e6763 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -160,12 +160,38 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PCI	(1 << 1)	/* vfio-pci device */
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
+#define VFIO_DEVICE_FLAGS_DEVTREE (1 << 4)	/* device tree metadata */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
 };
 #define VFIO_DEVICE_GET_INFO		_IO(VFIO_TYPE, VFIO_BASE + 7)
 
 /**
+ * VFIO_DEVICE_GET_DEVTREE_INFO - _IOR(VFIO_TYPE, VFIO_BASE + 16,
+ *						struct vfio_devtree_info)
+ *
+ * Retrieve information from the device's device tree, if available.
+ * Caller will initialize data[] with a single string with the requested
+ * devicetree property name, and type depending on whether a array of strings
+ * or an array of u32 values is expected. On success, data[] will be extended
+ * with the requested information, either as an array of u32, or with a list
+ * of strings sepparated by the NULL terminating character.
+ * Return: 0 on success, -errno on failure.
+ */
+struct vfio_devtree_info {
+	__u32	argsz;
+	__u32	type;
+#define VFIO_DEVTREE_PROP_LIST		0
+#define VFIO_DEVTREE_TYPE_STRINGS	1
+#define VFIO_DEVTREE_TYPE_U8		2
+#define VFIO_DEVTREE_TYPE_U16		3
+#define VFIO_DEVTREE_TYPE_U32		4
+	__u32	length;
+	__u8	data[];
+};
+#define VFIO_DEVICE_GET_DEVTREE_INFO	_IO(VFIO_TYPE, VFIO_BASE + 17)
+
+/**
  * VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
  *				       struct vfio_region_info)
  *
-- 
2.1.1


^ permalink raw reply related

* Re: [PATCH v3 4/6] tpm: TPM 2.0 sysfs attributes
From: Jarkko Sakkinen @ 2014-10-16 16:03 UTC (permalink / raw)
  To: Greg KH
  Cc: Peter Huewe, Ashley Lai, Marcel Selhorst,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	josh.triplett-ral2JQCrhuEAvxtiuMwx3w,
	christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
	will.c.arthur-ral2JQCrhuEAvxtiuMwx3w,
	monty.wiseman-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <20141016093146.GA25070-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>

On Thu, Oct 16, 2014 at 11:31:46AM +0200, Greg KH wrote:
> On Wed, Oct 15, 2014 at 01:35:14PM +0200, Jarkko Sakkinen wrote:
> > +int tpm2_sysfs_add_device(struct tpm_chip *chip)
> > +{
> > +	struct pcr_bank *pcr_bank;
> > +	struct kobject *pcrs_kobj;
> > +	struct device *dev = chip->dev;
> > +	int rc;
> > +
> > +	rc = sysfs_create_group(&chip->vendor.miscdev.this_device->kobj,
> > +				&tpm_dev_group);
> > +	if (rc) {
> > +		dev_err(dev, "failed to create sysfs attributes, %d\n", rc);
> > +		return rc;
> > +	}
> 
> You just raced and created sysfs files _after_ userspace saw that your
> device was enabled.

Here the options are limited because misc_register already spawns
KOBJ_ADD. Not much to do unless we would wipe the current use of 
misc driver completely.

> 
> > +	pcrs_kobj = kobject_create_and_add("pcrs",
> > +					   &chip->vendor.miscdev.this_device->kobj);
> 
> Ick, no no no no.  Never create a kobject under a 'struct device'.  You
> just lost all libudev support for any attribute you created under here,
> not good at all.  Use a "real" device if you really want a sub-device,
> not a kobject.

Ack, will rework this.

> thanks,
> 
> greg k-h

/Jarkko

^ permalink raw reply

* Re: [PATCHv1 1/8] kernfs: Add API to generate relative kernfs path
From: Serge E. Hallyn @ 2014-10-16 16:07 UTC (permalink / raw)
  To: Aditya Kali
  Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA, luto-kltTT9wpgjJwATOyAt5JVQ,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1413235430-22944-2-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> The new function kernfs_path_from_node() generates and returns
> kernfs path of a given kernfs_node relative to a given parent
> kernfs_node.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

(with or without my comment below taken)

> ---
>  fs/kernfs/dir.c        | 53 ++++++++++++++++++++++++++++++++++++++++----------
>  include/linux/kernfs.h |  3 +++
>  2 files changed, 46 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index a693f5b..8655485 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -44,14 +44,24 @@ static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
>  	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
>  }
>  
> -static char * __must_check kernfs_path_locked(struct kernfs_node *kn, char *buf,
> -					      size_t buflen)
> +static char * __must_check kernfs_path_from_node_locked(
> +	struct kernfs_node *kn_root,
> +	struct kernfs_node *kn,
> +	char *buf,
> +	size_t buflen)
>  {
>  	char *p = buf + buflen;
>  	int len;
>  
> +	BUG_ON(!buflen);
> +
>  	*--p = '\0';
>  
> +	if (kn == kn_root) {
> +		*--p = '/';
> +		return p;
> +	}
> +
>  	do {
>  		len = strlen(kn->name);
>  		if (p - buf < len + 1) {
> @@ -63,6 +73,8 @@ static char * __must_check kernfs_path_locked(struct kernfs_node *kn, char *buf,
>  		memcpy(p, kn->name, len);
>  		*--p = '/';
>  		kn = kn->parent;
> +		if (kn == kn_root)
> +			break;

I wonder if it would be clearer if you instead changed the while condition, i.e.

	} while (kn && kn != kn_root && kn_parent);

i.e .it's not a special condition, just a part of the expected flow.

>  	} while (kn && kn->parent);
>  
>  	return p;
> @@ -92,26 +104,47 @@ int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
>  }
>  
>  /**
> - * kernfs_path - build full path of a given node
> + * kernfs_path_from_node - build path of node @kn relative to @kn_root.
> + * @kn_root: parent kernfs_node relative to which we need to build the path
>   * @kn: kernfs_node of interest
> - * @buf: buffer to copy @kn's name into
> + * @buf: buffer to copy @kn's path into
>   * @buflen: size of @buf
>   *
> - * Builds and returns the full path of @kn in @buf of @buflen bytes.  The
> - * path is built from the end of @buf so the returned pointer usually
> + * Builds and returns @kn's path relative to @kn_root. @kn_root is expected to
> + * be parent of @kn at some level. If this is not true or if @kn_root is NULL,
> + * then full path of @kn is returned.
> + * The path is built from the end of @buf so the returned pointer usually
>   * doesn't match @buf.  If @buf isn't long enough, @buf is nul terminated
>   * and %NULL is returned.
>   */
> -char *kernfs_path(struct kernfs_node *kn, char *buf, size_t buflen)
> +char *kernfs_path_from_node(struct kernfs_node *kn_root, struct kernfs_node *kn,
> +			    char *buf, size_t buflen)
>  {
>  	unsigned long flags;
>  	char *p;
>  
>  	spin_lock_irqsave(&kernfs_rename_lock, flags);
> -	p = kernfs_path_locked(kn, buf, buflen);
> +	p = kernfs_path_from_node_locked(kn_root, kn, buf, buflen);
>  	spin_unlock_irqrestore(&kernfs_rename_lock, flags);
>  	return p;
>  }
> +EXPORT_SYMBOL_GPL(kernfs_path_from_node);
> +
> +/**
> + * kernfs_path - build full path of a given node
> + * @kn: kernfs_node of interest
> + * @buf: buffer to copy @kn's name into
> + * @buflen: size of @buf
> + *
> + * Builds and returns the full path of @kn in @buf of @buflen bytes.  The
> + * path is built from the end of @buf so the returned pointer usually
> + * doesn't match @buf.  If @buf isn't long enough, @buf is nul terminated
> + * and %NULL is returned.
> + */
> +char *kernfs_path(struct kernfs_node *kn, char *buf, size_t buflen)
> +{
> +	return kernfs_path_from_node(NULL, kn, buf, buflen);
> +}
>  EXPORT_SYMBOL_GPL(kernfs_path);
>  
>  /**
> @@ -145,8 +178,8 @@ void pr_cont_kernfs_path(struct kernfs_node *kn)
>  
>  	spin_lock_irqsave(&kernfs_rename_lock, flags);
>  
> -	p = kernfs_path_locked(kn, kernfs_pr_cont_buf,
> -			       sizeof(kernfs_pr_cont_buf));
> +	p = kernfs_path_from_node_locked(NULL, kn, kernfs_pr_cont_buf,
> +					 sizeof(kernfs_pr_cont_buf));
>  	if (p)
>  		pr_cont("%s", p);
>  	else
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index 30faf79..3c2be75 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -258,6 +258,9 @@ static inline bool kernfs_ns_enabled(struct kernfs_node *kn)
>  }
>  
>  int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen);
> +char * __must_check kernfs_path_from_node(struct kernfs_node *root_kn,
> +					  struct kernfs_node *kn, char *buf,
> +					  size_t buflen);
>  char * __must_check kernfs_path(struct kernfs_node *kn, char *buf,
>  				size_t buflen);
>  void pr_cont_kernfs_name(struct kernfs_node *kn);
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCHv1 2/8] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace
From: Serge E. Hallyn @ 2014-10-16 16:08 UTC (permalink / raw)
  To: Aditya Kali
  Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA, luto-kltTT9wpgjJwATOyAt5JVQ,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1413235430-22944-3-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> CLONE_NEWCGROUP will be used to create new cgroup namespace.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

> ---
>  include/uapi/linux/sched.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> index 34f9d73..2f90d00 100644
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -21,8 +21,7 @@
>  #define CLONE_DETACHED		0x00400000	/* Unused, ignored */
>  #define CLONE_UNTRACED		0x00800000	/* set if the tracing process can't force CLONE_PTRACE on this clone */
>  #define CLONE_CHILD_SETTID	0x01000000	/* set the TID in the child */
> -/* 0x02000000 was previously the unused CLONE_STOPPED (Start in stopped state)
> -   and is now available for re-use. */
> +#define CLONE_NEWCGROUP		0x02000000	/* New cgroup namespace */
>  #define CLONE_NEWUTS		0x04000000	/* New utsname group? */
>  #define CLONE_NEWIPC		0x08000000	/* New ipcs */
>  #define CLONE_NEWUSER		0x10000000	/* New user namespace */
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCHv1 3/8] cgroup: add function to get task's cgroup on default hierarchy
From: Serge E. Hallyn @ 2014-10-16 16:13 UTC (permalink / raw)
  To: Aditya Kali
  Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA, luto-kltTT9wpgjJwATOyAt5JVQ,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1413235430-22944-4-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> get_task_cgroup() returns the (reference counted) cgroup of the
> given task on the default hierarchy.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

> ---
>  include/linux/cgroup.h |  1 +
>  kernel/cgroup.c        | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 1d51968..80ed6e0 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -579,6 +579,7 @@ static inline void pr_cont_cgroup_path(struct cgroup *cgrp)
>  }
>  
>  char *task_cgroup_path(struct task_struct *task, char *buf, size_t buflen);
> +struct cgroup *get_task_cgroup(struct task_struct *task);
>  
>  int cgroup_add_dfl_cftypes(struct cgroup_subsys *ss, struct cftype *cfts);
>  int cgroup_add_legacy_cftypes(struct cgroup_subsys *ss, struct cftype *cfts);
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index cab7dc4..56d507b 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1916,6 +1916,31 @@ char *task_cgroup_path(struct task_struct *task, char *buf, size_t buflen)
>  }
>  EXPORT_SYMBOL_GPL(task_cgroup_path);
>  
> +/*
> + * get_task_cgroup - returns the cgroup of the task in the default cgroup
> + * hierarchy.
> + *
> + * @task: target task
> + * This function returns the @task's cgroup on the default cgroup hierarchy. The
> + * returned cgroup has its reference incremented (by calling cgroup_get()). So
> + * the caller must cgroup_put() the obtained reference once it is done with it.
> + */
> +struct cgroup *get_task_cgroup(struct task_struct *task)
> +{
> +	struct cgroup *cgrp;
> +
> +	mutex_lock(&cgroup_mutex);
> +	down_read(&css_set_rwsem);
> +
> +	cgrp = task_cgroup_from_root(task, &cgrp_dfl_root);
> +	cgroup_get(cgrp);
> +
> +	up_read(&css_set_rwsem);
> +	mutex_unlock(&cgroup_mutex);
> +	return cgrp;
> +}
> +EXPORT_SYMBOL_GPL(get_task_cgroup);
> +
>  /* used to track tasks and other necessary states during migration */
>  struct cgroup_taskset {
>  	/* the src and dst cset list running through cset->mg_node */
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCHv1 4/8] cgroup: export cgroup_get() and cgroup_put()
From: Serge E. Hallyn @ 2014-10-16 16:14 UTC (permalink / raw)
  To: Aditya Kali
  Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA, luto-kltTT9wpgjJwATOyAt5JVQ,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1413235430-22944-5-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> move cgroup_get() and cgroup_put() into cgroup.h so that
> they can be called from other places.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

> ---
>  include/linux/cgroup.h | 22 ++++++++++++++++++++++
>  kernel/cgroup.c        | 22 ----------------------
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 80ed6e0..4a0eb2d 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -521,6 +521,28 @@ static inline bool cgroup_on_dfl(const struct cgroup *cgrp)
>  	return cgrp->root == &cgrp_dfl_root;
>  }
>  
> +/* convenient tests for these bits */
> +static inline bool cgroup_is_dead(const struct cgroup *cgrp)
> +{
> +	return !(cgrp->self.flags & CSS_ONLINE);
> +}
> +
> +static inline void cgroup_get(struct cgroup *cgrp)
> +{
> +	WARN_ON_ONCE(cgroup_is_dead(cgrp));
> +	css_get(&cgrp->self);
> +}
> +
> +static inline bool cgroup_tryget(struct cgroup *cgrp)
> +{
> +	return css_tryget(&cgrp->self);
> +}
> +
> +static inline void cgroup_put(struct cgroup *cgrp)
> +{
> +	css_put(&cgrp->self);
> +}
> +
>  /* no synchronization, the result can only be used as a hint */
>  static inline bool cgroup_has_tasks(struct cgroup *cgrp)
>  {
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 56d507b..2b3e9f9 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -284,12 +284,6 @@ static struct cgroup_subsys_state *cgroup_e_css(struct cgroup *cgrp,
>  	return cgroup_css(cgrp, ss);
>  }
>  
> -/* convenient tests for these bits */
> -static inline bool cgroup_is_dead(const struct cgroup *cgrp)
> -{
> -	return !(cgrp->self.flags & CSS_ONLINE);
> -}
> -
>  struct cgroup_subsys_state *of_css(struct kernfs_open_file *of)
>  {
>  	struct cgroup *cgrp = of->kn->parent->priv;
> @@ -1002,22 +996,6 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
>  	return mode;
>  }
>  
> -static void cgroup_get(struct cgroup *cgrp)
> -{
> -	WARN_ON_ONCE(cgroup_is_dead(cgrp));
> -	css_get(&cgrp->self);
> -}
> -
> -static bool cgroup_tryget(struct cgroup *cgrp)
> -{
> -	return css_tryget(&cgrp->self);
> -}
> -
> -static void cgroup_put(struct cgroup *cgrp)
> -{
> -	css_put(&cgrp->self);
> -}
> -
>  /**
>   * cgroup_refresh_child_subsys_mask - update child_subsys_mask
>   * @cgrp: the target cgroup
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCHv1 5/8] cgroup: introduce cgroup namespaces
From: Serge E. Hallyn @ 2014-10-16 16:37 UTC (permalink / raw)
  To: Aditya Kali
  Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA, luto-kltTT9wpgjJwATOyAt5JVQ,
	cgroups-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1413235430-22944-6-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> Introduce the ability to create new cgroup namespace. The newly created
> cgroup namespace remembers the 'struct cgroup *root_cgrp' at the point
> of creation of the cgroup namespace. The task that creates the new
> cgroup namespace and all its future children will now be restricted only
> to the cgroup hierarchy under this root_cgrp.
> The main purpose of cgroup namespace is to virtualize the contents
> of /proc/self/cgroup file. Processes inside a cgroup namespace
> are only able to see paths relative to their namespace root.
> This allows container-tools (like libcontainer, lxc, lmctfy, etc.)
> to create completely virtualized containers without leaking system
> level cgroup hierarchy to the task.
> This patch only implements the 'unshare' part of the cgroupns.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

I'm not sure that the CONFIG_CGROUP_NS is worthwhile.  If you already
have cgroups in the kernel this won't add much in the way of memory
usage, right?  And I think the 'experimental' argument has long since
been squashed.  So I'd argue for simplifying this patch by removing
CONFIG_CGROUP_NS.

(more below)

> ---
>  fs/proc/namespaces.c             |   3 +
>  include/linux/cgroup.h           |  18 +++++-
>  include/linux/cgroup_namespace.h |  62 +++++++++++++++++++
>  include/linux/nsproxy.h          |   2 +
>  include/linux/proc_ns.h          |   4 ++
>  init/Kconfig                     |   9 +++
>  kernel/Makefile                  |   1 +
>  kernel/cgroup.c                  |  11 ++++
>  kernel/cgroup_namespace.c        | 128 +++++++++++++++++++++++++++++++++++++++
>  kernel/fork.c                    |   2 +-
>  kernel/nsproxy.c                 |  19 +++++-
>  11 files changed, 255 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
> index 8902609..e04ed4b 100644
> --- a/fs/proc/namespaces.c
> +++ b/fs/proc/namespaces.c
> @@ -32,6 +32,9 @@ static const struct proc_ns_operations *ns_entries[] = {
>  	&userns_operations,
>  #endif
>  	&mntns_operations,
> +#ifdef CONFIG_CGROUP_NS
> +	&cgroupns_operations,
> +#endif
>  };
>  
>  static const struct file_operations ns_file_operations = {
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 4a0eb2d..aa86495 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -22,6 +22,8 @@
>  #include <linux/seq_file.h>
>  #include <linux/kernfs.h>
>  #include <linux/wait.h>
> +#include <linux/nsproxy.h>
> +#include <linux/types.h>
>  
>  #ifdef CONFIG_CGROUPS
>  
> @@ -460,6 +462,13 @@ struct cftype {
>  #endif
>  };
>  
> +struct cgroup_namespace {
> +	atomic_t		count;
> +	unsigned int		proc_inum;
> +	struct user_namespace	*user_ns;
> +	struct cgroup		*root_cgrp;
> +};
> +
>  extern struct cgroup_root cgrp_dfl_root;
>  extern struct css_set init_css_set;
>  
> @@ -584,10 +593,17 @@ static inline int cgroup_name(struct cgroup *cgrp, char *buf, size_t buflen)
>  	return kernfs_name(cgrp->kn, buf, buflen);
>  }
>  
> +static inline char * __must_check cgroup_path_ns(struct cgroup_namespace *ns,
> +						 struct cgroup *cgrp, char *buf,
> +						 size_t buflen)
> +{
> +	return kernfs_path_from_node(ns->root_cgrp->kn, cgrp->kn, buf, buflen);
> +}
> +
>  static inline char * __must_check cgroup_path(struct cgroup *cgrp, char *buf,
>  					      size_t buflen)
>  {
> -	return kernfs_path(cgrp->kn, buf, buflen);
> +	return cgroup_path_ns(current->nsproxy->cgroup_ns, cgrp, buf, buflen);
>  }
>  
>  static inline void pr_cont_cgroup_name(struct cgroup *cgrp)
> diff --git a/include/linux/cgroup_namespace.h b/include/linux/cgroup_namespace.h
> new file mode 100644
> index 0000000..9f637fe
> --- /dev/null
> +++ b/include/linux/cgroup_namespace.h
> @@ -0,0 +1,62 @@
> +#ifndef _LINUX_CGROUP_NAMESPACE_H
> +#define _LINUX_CGROUP_NAMESPACE_H
> +
> +#include <linux/nsproxy.h>
> +#include <linux/cgroup.h>
> +#include <linux/types.h>
> +#include <linux/user_namespace.h>
> +
> +extern struct cgroup_namespace init_cgroup_ns;
> +
> +static inline struct cgroup *task_cgroupns_root(struct task_struct *tsk)
> +{
> +	return tsk->nsproxy->cgroup_ns->root_cgrp;

Per the rules in nsproxy.h, you should be taking the task_lock here.

(If you are making assumptions about tsk then you need to state them
here - I only looked quickly enough that you pass in 'leader')

> +}
> +
> +#ifdef CONFIG_CGROUP_NS
> +
> +extern void free_cgroup_ns(struct cgroup_namespace *ns);
> +
> +static inline struct cgroup_namespace *get_cgroup_ns(
> +		struct cgroup_namespace *ns)
> +{
> +	if (ns)
> +		atomic_inc(&ns->count);
> +	return ns;
> +}
> +
> +static inline void put_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +	if (ns && atomic_dec_and_test(&ns->count))
> +		free_cgroup_ns(ns);
> +}
> +
> +extern struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +					       struct user_namespace *user_ns,
> +					       struct cgroup_namespace *old_ns);
> +
> +#else  /* CONFIG_CGROUP_NS */
> +
> +static inline struct cgroup_namespace *get_cgroup_ns(
> +		struct cgroup_namespace *ns)
> +{
> +	return &init_cgroup_ns;
> +}
> +
> +static inline void put_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +}
> +
> +static inline struct cgroup_namespace *copy_cgroup_ns(
> +		unsigned long flags,
> +		struct user_namespace *user_ns,
> +		struct cgroup_namespace *old_ns) {
> +	if (flags & CLONE_NEWCGROUP)
> +		return ERR_PTR(-EINVAL);
> +
> +	return old_ns;
> +}
> +
> +#endif  /* CONFIG_CGROUP_NS */
> +
> +#endif  /* _LINUX_CGROUP_NAMESPACE_H */
> diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
> index 35fa08f..ac0d65b 100644
> --- a/include/linux/nsproxy.h
> +++ b/include/linux/nsproxy.h
> @@ -8,6 +8,7 @@ struct mnt_namespace;
>  struct uts_namespace;
>  struct ipc_namespace;
>  struct pid_namespace;
> +struct cgroup_namespace;
>  struct fs_struct;
>  
>  /*
> @@ -33,6 +34,7 @@ struct nsproxy {
>  	struct mnt_namespace *mnt_ns;
>  	struct pid_namespace *pid_ns_for_children;
>  	struct net 	     *net_ns;
> +	struct cgroup_namespace *cgroup_ns;
>  };
>  extern struct nsproxy init_nsproxy;
>  
> diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
> index 34a1e10..e56dd73 100644
> --- a/include/linux/proc_ns.h
> +++ b/include/linux/proc_ns.h
> @@ -6,6 +6,8 @@
>  
>  struct pid_namespace;
>  struct nsproxy;
> +struct task_struct;
> +struct inode;
>  
>  struct proc_ns_operations {
>  	const char *name;
> @@ -27,6 +29,7 @@ extern const struct proc_ns_operations ipcns_operations;
>  extern const struct proc_ns_operations pidns_operations;
>  extern const struct proc_ns_operations userns_operations;
>  extern const struct proc_ns_operations mntns_operations;
> +extern const struct proc_ns_operations cgroupns_operations;
>  
>  /*
>   * We always define these enumerators
> @@ -37,6 +40,7 @@ enum {
>  	PROC_UTS_INIT_INO	= 0xEFFFFFFEU,
>  	PROC_USER_INIT_INO	= 0xEFFFFFFDU,
>  	PROC_PID_INIT_INO	= 0xEFFFFFFCU,
> +	PROC_CGROUP_INIT_INO	= 0xEFFFFFFBU,
>  };
>  
>  #ifdef CONFIG_PROC_FS
> diff --git a/init/Kconfig b/init/Kconfig
> index e84c642..c3be001 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1144,6 +1144,15 @@ config DEBUG_BLK_CGROUP
>  	Enable some debugging help. Currently it exports additional stat
>  	files in a cgroup which can be useful for debugging.
>  
> +config CGROUP_NS
> +	bool "CGroup Namespaces"
> +	default n
> +	help
> +	  This options enables CGroup Namespaces which can be used to isolate
> +	  cgroup paths. This feature is only useful when unified cgroup
> +	  hierarchy is in use (i.e. cgroups are mounted with sane_behavior
> +	  option).
> +
>  endif # CGROUPS
>  
>  config CHECKPOINT_RESTORE
> diff --git a/kernel/Makefile b/kernel/Makefile
> index dc5c775..75334f8 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_KEXEC) += kexec.o
>  obj-$(CONFIG_BACKTRACE_SELF_TEST) += backtracetest.o
>  obj-$(CONFIG_COMPAT) += compat.o
>  obj-$(CONFIG_CGROUPS) += cgroup.o
> +obj-$(CONFIG_CGROUP_NS) += cgroup_namespace.o
>  obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
>  obj-$(CONFIG_CPUSETS) += cpuset.o
>  obj-$(CONFIG_UTS_NS) += utsname.o
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 2b3e9f9..f8099b4 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -57,6 +57,8 @@
>  #include <linux/vmalloc.h> /* TODO: replace with more sophisticated array */
>  #include <linux/kthread.h>
>  #include <linux/delay.h>
> +#include <linux/proc_ns.h>
> +#include <linux/cgroup_namespace.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -195,6 +197,15 @@ static void kill_css(struct cgroup_subsys_state *css);
>  static int cgroup_addrm_files(struct cgroup *cgrp, struct cftype cfts[],
>  			      bool is_add);
>  
> +struct cgroup_namespace init_cgroup_ns = {
> +	.count = {
> +		.counter = 1,
> +	},
> +	.proc_inum = PROC_CGROUP_INIT_INO,
> +	.user_ns = &init_user_ns,

This might mean that you should bump the init_user_ns refcount.

> +	.root_cgrp = &cgrp_dfl_root.cgrp,
> +};
> +
>  /* IDR wrappers which synchronize using cgroup_idr_lock */
>  static int cgroup_idr_alloc(struct idr *idr, void *ptr, int start, int end,
>  			    gfp_t gfp_mask)
> diff --git a/kernel/cgroup_namespace.c b/kernel/cgroup_namespace.c
> new file mode 100644
> index 0000000..c16604f
> --- /dev/null
> +++ b/kernel/cgroup_namespace.c
> @@ -0,0 +1,128 @@
> +
> +#include <linux/cgroup.h>
> +#include <linux/cgroup_namespace.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/nsproxy.h>
> +#include <linux/proc_ns.h>
> +
> +static struct cgroup_namespace *alloc_cgroup_ns(void)
> +{
> +	struct cgroup_namespace *new_ns;
> +
> +	new_ns = kmalloc(sizeof(struct cgroup_namespace), GFP_KERNEL);
> +	if (new_ns)
> +		atomic_set(&new_ns->count, 1);
> +	return new_ns;
> +}
> +
> +void free_cgroup_ns(struct cgroup_namespace *ns)
> +{
> +	cgroup_put(ns->root_cgrp);
> +	put_user_ns(ns->user_ns);

This is a problem on error patch in copy_cgroup_ns.  The
alloc_cgroup_ns() doesn't initialize these values, so if
you should fail in proc_alloc_inum() you'll show up here
with fandom values in ns->*.

> +	proc_free_inum(ns->proc_inum);
> +}
> +EXPORT_SYMBOL(free_cgroup_ns);
> +
> +struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
> +					struct user_namespace *user_ns,
> +					struct cgroup_namespace *old_ns)
> +{
> +	struct cgroup_namespace *new_ns = NULL;
> +	struct cgroup *cgrp = NULL;
> +	int err;
> +
> +	BUG_ON(!old_ns);
> +
> +	if (!(flags & CLONE_NEWCGROUP))
> +		return get_cgroup_ns(old_ns);
> +
> +	/* Allow only sysadmin to create cgroup namespace. */
> +	err = -EPERM;
> +	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
> +		goto err_out;
> +
> +	/* Prevent cgroup changes for this task. */
> +	threadgroup_lock(current);
> +
> +	cgrp = get_task_cgroup(current);
> +
> +	/* Creating new CGROUPNS is supported only when unified hierarchy is in
> +	 * use. */

Oh, drat.  Well, I'll take, it, but under protest  :)

> +	err = -EINVAL;
> +	if (!cgroup_on_dfl(cgrp))
> +		goto err_out_unlock;
> +
> +	err = -ENOMEM;
> +	new_ns = alloc_cgroup_ns();
> +	if (!new_ns)
> +		goto err_out_unlock;
> +
> +	err = proc_alloc_inum(&new_ns->proc_inum);
> +	if (err)
> +		goto err_out_unlock;
> +
> +	new_ns->user_ns = get_user_ns(user_ns);
> +	new_ns->root_cgrp = cgrp;
> +
> +	threadgroup_unlock(current);
> +
> +	return new_ns;
> +
> +err_out_unlock:
> +	threadgroup_unlock(current);
> +err_out:
> +	if (cgrp)
> +		cgroup_put(cgrp);
> +	kfree(new_ns);
> +	return ERR_PTR(err);
> +}
> +
> +static int cgroupns_install(struct nsproxy *nsproxy, void *ns)
> +{
> +	pr_info("setns not supported for cgroup namespace");
> +	return -EINVAL;
> +}
> +
> +static void *cgroupns_get(struct task_struct *task)
> +{
> +	struct cgroup_namespace *ns = NULL;
> +	struct nsproxy *nsproxy;
> +
> +	rcu_read_lock();
> +	nsproxy = task->nsproxy;
> +	if (nsproxy) {
> +		ns = nsproxy->cgroup_ns;
> +		get_cgroup_ns(ns);
> +	}
> +	rcu_read_unlock();
> +
> +	return ns;
> +}
> +
> +static void cgroupns_put(void *ns)
> +{
> +	put_cgroup_ns(ns);
> +}
> +
> +static unsigned int cgroupns_inum(void *ns)
> +{
> +	struct cgroup_namespace *cgroup_ns = ns;
> +
> +	return cgroup_ns->proc_inum;
> +}
> +
> +const struct proc_ns_operations cgroupns_operations = {
> +	.name		= "cgroup",
> +	.type		= CLONE_NEWCGROUP,
> +	.get		= cgroupns_get,
> +	.put		= cgroupns_put,
> +	.install	= cgroupns_install,
> +	.inum		= cgroupns_inum,
> +};
> +
> +static __init int cgroup_namespaces_init(void)
> +{
> +	return 0;
> +}
> +subsys_initcall(cgroup_namespaces_init);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 0cf9cdb..cc06851 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1790,7 +1790,7 @@ static int check_unshare_flags(unsigned long unshare_flags)
>  	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
>  				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
>  				CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNET|
> -				CLONE_NEWUSER|CLONE_NEWPID))
> +				CLONE_NEWUSER|CLONE_NEWPID|CLONE_NEWCGROUP))
>  		return -EINVAL;
>  	/*
>  	 * Not implemented, but pretend it works if there is nothing to
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index ef42d0a..a8b1970 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -25,6 +25,7 @@
>  #include <linux/proc_ns.h>
>  #include <linux/file.h>
>  #include <linux/syscalls.h>
> +#include <linux/cgroup_namespace.h>
>  
>  static struct kmem_cache *nsproxy_cachep;
>  
> @@ -39,6 +40,7 @@ struct nsproxy init_nsproxy = {
>  #ifdef CONFIG_NET
>  	.net_ns			= &init_net,
>  #endif
> +	.cgroup_ns		= &init_cgroup_ns,
>  };
>  
>  static inline struct nsproxy *create_nsproxy(void)
> @@ -92,6 +94,13 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
>  		goto out_pid;
>  	}
>  
> +	new_nsp->cgroup_ns = copy_cgroup_ns(flags, user_ns,
> +					    tsk->nsproxy->cgroup_ns);
> +	if (IS_ERR(new_nsp->cgroup_ns)) {
> +		err = PTR_ERR(new_nsp->cgroup_ns);
> +		goto out_cgroup;
> +	}
> +
>  	new_nsp->net_ns = copy_net_ns(flags, user_ns, tsk->nsproxy->net_ns);
>  	if (IS_ERR(new_nsp->net_ns)) {
>  		err = PTR_ERR(new_nsp->net_ns);
> @@ -101,6 +110,9 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
>  	return new_nsp;
>  
>  out_net:
> +	if (new_nsp->cgroup_ns)
> +		put_cgroup_ns(new_nsp->cgroup_ns);
> +out_cgroup:
>  	if (new_nsp->pid_ns_for_children)
>  		put_pid_ns(new_nsp->pid_ns_for_children);
>  out_pid:
> @@ -128,7 +140,8 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
>  	struct nsproxy *new_ns;
>  
>  	if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> -			      CLONE_NEWPID | CLONE_NEWNET)))) {
> +			      CLONE_NEWPID | CLONE_NEWNET |
> +			      CLONE_NEWCGROUP)))) {
>  		get_nsproxy(old_ns);
>  		return 0;
>  	}
> @@ -165,6 +178,8 @@ void free_nsproxy(struct nsproxy *ns)
>  		put_ipc_ns(ns->ipc_ns);
>  	if (ns->pid_ns_for_children)
>  		put_pid_ns(ns->pid_ns_for_children);
> +	if (ns->cgroup_ns)
> +		put_cgroup_ns(ns->cgroup_ns);
>  	put_net(ns->net_ns);
>  	kmem_cache_free(nsproxy_cachep, ns);
>  }
> @@ -180,7 +195,7 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
>  	int err = 0;
>  
>  	if (!(unshare_flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> -			       CLONE_NEWNET | CLONE_NEWPID)))
> +			       CLONE_NEWNET | CLONE_NEWPID | CLONE_NEWCGROUP)))
>  		return 0;
>  
>  	user_ns = new_cred ? new_cred->user_ns : current_user_ns();
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

^ permalink raw reply

* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
From: John Stultz @ 2014-10-16 17:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: lkml, devel, Linux API, Santosh Shilimkar,
	Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
	Christoffer Dall, Anup Patel
In-Reply-To: <20141016124741.GA3832@kroah.com>

On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The Android binder code has been "stable" for many years now.  No matter

Well, ignoring the ABI break that landed in the last year. :)

> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>
> This was discussed in the Android miniconf at the Plumbers conference.
> If anyone has any objections to this, please let me know, otherwise I'm
> queueing this up for 3.19-rc1

So my main concerns/thoughts here are:

Who is going to maintain this? Has Arve agreed?  Are the Android guys
comfortable with the ABI stability rules they'll now face?  Currently
in the android space no one but libbinder should use the kernel
interface. Can we enforce that no one use this interface out-side of
android (To ensure its one of those "if the abi breaks and no one
notices..." edge cases)?

I'm still hopeful that a libbinder over kdbus will eventually pan out.
I still see having two core IPC mechanisms (even if the use cases are
different in style) as a negative, and I worry this might reduce
motivation to unify things at the lower level. Granted, such work can
still continue, but the incentives do change.

thanks
-john

^ permalink raw reply

* Re: [PATCHv1 7/8] cgroup: cgroup namespace setns support
From: Serge E. Hallyn @ 2014-10-16 21:12 UTC (permalink / raw)
  To: Aditya Kali
  Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
	tj-DgEjT+Ai2ygdnm+yROfE0A, cgroups-u79uwXL29TY76Z2rM5mHXA,
	mingo-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <1413235430-22944-8-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> setns on a cgroup namespace is allowed only if
> * task has CAP_SYS_ADMIN in its current user-namespace and
>   over the user-namespace associated with target cgroupns.
> * task's current cgroup is descendent of the target cgroupns-root
>   cgroup.

What is the point of this?

If I'm a user logged into
/lxc/c1/user.slice/user-1000.slice/session-c12.scope and I start
a container which is in
/lxc/c1/user.slice/user-1000.slice/session-c12.scope/x1
then I will want to be able to enter the container's cgroup.
The container's cgroup root is under my own (satisfying the
below condition0 but my cgroup is not a descendent of the
container's cgroup.


> * target cgroupns-root is same as or deeper than task's current
>   cgroupns-root. This is so that the task cannot escape out of its
>   cgroupns-root. This also ensures that setns() only makes the task
>   get restricted to a deeper cgroup hierarchy.
> 
> Signed-off-by: Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  kernel/cgroup_namespace.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup_namespace.c b/kernel/cgroup_namespace.c
> index c16604f..c612946 100644
> --- a/kernel/cgroup_namespace.c
> +++ b/kernel/cgroup_namespace.c
> @@ -80,8 +80,48 @@ err_out:
>  
>  static int cgroupns_install(struct nsproxy *nsproxy, void *ns)
>  {
> -	pr_info("setns not supported for cgroup namespace");
> -	return -EINVAL;
> +	struct cgroup_namespace *cgroup_ns = ns;
> +	struct task_struct *task = current;
> +	struct cgroup *cgrp = NULL;
> +	int err = 0;
> +
> +	if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN) ||
> +	    !ns_capable(cgroup_ns->user_ns, CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	/* Prevent cgroup changes for this task. */
> +	threadgroup_lock(task);
> +
> +	cgrp = get_task_cgroup(task);
> +
> +	err = -EINVAL;
> +	if (!cgroup_on_dfl(cgrp))
> +		goto out_unlock;
> +
> +	/* Allow switch only if the task's current cgroup is descendant of the
> +	 * target cgroup_ns->root_cgrp.
> +	 */
> +	if (!cgroup_is_descendant(cgrp, cgroup_ns->root_cgrp))
> +		goto out_unlock;
> +
> +	/* Only allow setns to a cgroupns root-ed deeper than task's current
> +	 * cgroupns-root. This will make sure that tasks cannot escape their
> +	 * cgroupns by attaching to parent cgroupns.
> +	 */
> +	if (!cgroup_is_descendant(cgroup_ns->root_cgrp,
> +				  task_cgroupns_root(task)))
> +		goto out_unlock;
> +
> +	err = 0;
> +	get_cgroup_ns(cgroup_ns);
> +	put_cgroup_ns(nsproxy->cgroup_ns);
> +	nsproxy->cgroup_ns = cgroup_ns;
> +
> +out_unlock:
> +	threadgroup_unlock(current);
> +	if (cgrp)
> +		cgroup_put(cgrp);
> +	return err;
>  }
>  
>  static void *cgroupns_get(struct task_struct *task)
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox