All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] bcb: Android bootloader control block driver
@ 2012-06-29 19:36 Andrew Boie
  2012-06-29 21:25 ` NeilBrown
  2012-06-30  3:23 ` Greg KH
  0 siblings, 2 replies; 13+ messages in thread
From: Andrew Boie @ 2012-06-29 19:36 UTC (permalink / raw)
  To: gregkh, ccross, arve, rebecca; +Cc: devel, linux-kernel, Andrew Boie

Android userspace tells the kernel that it wants to boot into recovery
or some other non-default OS environment by passing a string argument
to reboot(). It is left to the OEM to hook this up to their specific
bootloader.

This driver uses the bootloader control block (BCB) in the 'misc'
partition, which is the AOSP mechanism used to communicate with
the bootloader. Writing 'bootonce-NNN' to the command field
will cause the bootloader to do a oneshot boot into an alternate
boot label NNN if it exists. The device and partition number are
passed in via kernel command line.

It is the bootloader's job to guard against this area being uninitialzed
or containing an invalid boot label, and just boot normally if that
is the case. The bootloader will always populate a magic signature in
the BCB; the driver will refuse to update it if not present.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
---
 drivers/staging/android/Kconfig  |   11 ++
 drivers/staging/android/Makefile |    1 +
 drivers/staging/android/bcb.c    |  232 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 244 insertions(+)
 create mode 100644 drivers/staging/android/bcb.c

diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 9a99238..c30fd20 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
 	  elapsed realtime, and a non-wakeup alarm on the monotonic clock.
 	  Also exports the alarm interface to user-space.
 
+config BOOTLOADER_CONTROL
+	tristate "Bootloader Control Block module"
+	default n
+	help
+	  This driver installs a reboot hook, such that if reboot() is invoked
+	  with a string argument NNN, "bootonce-NNN" is copied to the command
+	  field in the Bootloader Control Block on the /misc partition, to be
+	  read by the bootloader. If the string matches one of the boot labels
+	  defined in its configuration, it will boot once into that label. The
+	  device and partition number are specified on the kernel command line.
+
 endif # if ANDROID
 
 endmenu
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 8e18d4e..a27707f 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
 obj-$(CONFIG_ANDROID_SWITCH)		+= switch/
 obj-$(CONFIG_ANDROID_INTF_ALARM_DEV)	+= alarm-dev.o
 obj-$(CONFIG_PERSISTENT_TRACER)		+= trace_persistent.o
+obj-$(CONFIG_BOOTLOADER_CONTROL)	+= bcb.o
 
 CFLAGS_REMOVE_trace_persistent.o = -pg
diff --git a/drivers/staging/android/bcb.c b/drivers/staging/android/bcb.c
new file mode 100644
index 0000000..af75257
--- /dev/null
+++ b/drivers/staging/android/bcb.c
@@ -0,0 +1,232 @@
+/*
+ * bcb.c: Reboot hook to write bootloader commands to
+ *        the Android bootloader control block
+ *
+ * (C) Copyright 2012 Intel Corporation
+ * Author: Andrew Boie <andrew.p.boie@intel.com>
+ *
+ * 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/module.h>
+#include <linux/moduleparam.h>
+#include <linux/blkdev.h>
+#include <linux/reboot.h>
+
+#define BCB_MAGIC	0xFEEDFACE
+
+/* Persistent area written by Android recovery console and Linux bcb driver
+ * reboot hook for communication with the bootloader. Bootloader must
+ * gracefully handle this area being unitinitailzed */
+struct bootloader_message {
+	/* Directive to the bootloader on what it needs to do next.
+	 * Possible values:
+	 *   boot-NNN - Automatically boot into label NNN
+	 *   bootonce-NNN - Automatically boot into label NNN, clearing this
+	 *     field afterwards
+	 *   anything else / garbage - Boot default label */
+	char command[32];
+
+	/* Storage area for error codes when using the BCB to boot into special
+	 * boot targets, e.g. for baseband update. Not used here. */
+	char status[32];
+
+	/* Area for recovery console to stash its command line arguments
+	 * in case it is reset and the cache command file is erased.
+	 * Not used here. */
+	char recovery[1024];
+
+	/* Magic sentinel value written by the bootloader; don't update this
+	 * if not equalto to BCB_MAGIC */
+	uint32_t magic;
+};
+
+/* TODO: device names/partition numbers are unstable. Add support for looking
+ * by GPT partition UUIDs */
+static char *bootdev = "sda";
+module_param(bootdev, charp, S_IRUGO);
+MODULE_PARM_DESC(bootdev, "Block device for bootloader communication");
+
+static int partno;
+module_param(partno, int, S_IRUGO);
+MODULE_PARM_DESC(partno, "Partition number for bootloader communication");
+
+static int device_match(struct device *dev, void *data)
+{
+	if (strcmp(dev_name(dev), bootdev) == 0)
+		return 1;
+	return 0;
+}
+
+static struct block_device *get_emmc_bdev(void)
+{
+	struct block_device *bdev;
+	struct device *disk_device;
+
+	disk_device = class_find_device(&block_class, NULL, NULL, device_match);
+	if (!disk_device) {
+		pr_err("bcb: device %s not found!\n", bootdev);
+		return NULL;
+	}
+	bdev = bdget_disk(dev_to_disk(disk_device), partno);
+	if (!bdev) {
+		dev_err(disk_device, "bcb: unable to get disk (%s,%d)\n",
+				bootdev, partno);
+		return NULL;
+	}
+	/* Note: this bdev ref will be freed after first
+	   bdev_get/bdev_put cycle */
+	return bdev;
+}
+
+static u64 last_lba(struct block_device *bdev)
+{
+	if (!bdev || !bdev->bd_inode)
+		return 0;
+	return div_u64(bdev->bd_inode->i_size,
+		       bdev_logical_block_size(bdev)) - 1ULL;
+}
+
+static size_t read_lba(struct block_device *bdev,
+		       u64 lba, u8 *buffer, size_t count)
+{
+	size_t totalreadcount = 0;
+	sector_t n = lba * (bdev_logical_block_size(bdev) / 512);
+
+	if (!buffer || lba > last_lba(bdev))
+		return 0;
+
+	while (count) {
+		int copied = 512;
+		Sector sect;
+		unsigned char *data = read_dev_sector(bdev, n++, &sect);
+		if (!data)
+			break;
+		if (copied > count)
+			copied = count;
+		memcpy(buffer, data, copied);
+		put_dev_sector(sect);
+		buffer += copied;
+		totalreadcount += copied;
+		count -= copied;
+	}
+	return totalreadcount;
+}
+
+static size_t write_lba(struct block_device *bdev,
+		       u64 lba, u8 *buffer, size_t count)
+{
+	size_t totalwritecount = 0;
+	sector_t n = lba * (bdev_logical_block_size(bdev) / 512);
+
+	if (!buffer || lba > last_lba(bdev))
+		return 0;
+
+	while (count) {
+		int copied = 512;
+		Sector sect;
+		unsigned char *data = read_dev_sector(bdev, n++, &sect);
+		if (!data)
+			break;
+		if (copied > count)
+			copied = count;
+		memcpy(data, buffer, copied);
+		set_page_dirty(sect.v);
+		unlock_page(sect.v);
+		put_dev_sector(sect);
+		buffer += copied;
+		totalwritecount += copied;
+		count -= copied;
+	}
+	sync_blockdev(bdev);
+	return totalwritecount;
+}
+
+static int bcb_reboot_notifier_call(
+		struct notifier_block *notifier,
+		unsigned long what, void *data)
+{
+	int ret = NOTIFY_DONE;
+	char *cmd = (char *)data;
+	struct block_device *bdev = NULL;
+	struct bootloader_message *bcb = NULL;
+
+	if (what != SYS_RESTART || !data)
+		goto out;
+
+	bdev = get_emmc_bdev();
+	if (!bdev)
+		goto out;
+
+	/* make sure the block device is open rw */
+	if (blkdev_get(bdev, FMODE_READ | FMODE_WRITE, NULL) < 0) {
+		pr_err("bcb: blk_dev_get failed!\n");
+		goto out;
+	}
+
+	bcb = kmalloc(sizeof(*bcb), GFP_KERNEL);
+	if (!bcb) {
+		pr_err("bcb: out of memory\n");
+		goto out;
+	}
+
+	if (read_lba(bdev, 0, (u8 *)bcb, sizeof(*bcb)) != sizeof(*bcb)) {
+		pr_err("bcb: couldn't read bootloader control block\n");
+		goto out;
+	}
+
+	if (bcb->magic != BCB_MAGIC) {
+		pr_err("bcb: bootloader control block corrupted, not writing\n");
+		goto out;
+	}
+
+	/* When the bootloader reads this area, it will null-terminate it
+	* and check if it matches any existing boot labels */
+	snprintf(bcb->command, sizeof(bcb->command), "bootonce-%s", cmd);
+
+	if (write_lba(bdev, 0, (u8 *)bcb, sizeof(*bcb)) != sizeof(*bcb)) {
+		pr_err("bcb: couldn't write bootloader control block\n");
+		goto out;
+	}
+
+	ret = NOTIFY_OK;
+out:
+	kfree(bcb);
+	if (bdev)
+		blkdev_put(bdev, FMODE_READ | FMODE_WRITE);
+
+	return ret;
+}
+
+static struct notifier_block bcb_reboot_notifier = {
+	.notifier_call = bcb_reboot_notifier_call,
+};
+
+static int __init bcb_init(void)
+{
+	if (partno < 1) {
+		pr_err("bcb: partition number not specified\n");
+		return -1;
+	}
+	if (register_reboot_notifier(&bcb_reboot_notifier)) {
+		pr_err("bcb: unable to register reboot notifier\n");
+		return -1;
+	}
+	pr_info("bcb: writing commands to (%s,%d)\n",
+			bootdev, partno);
+	return 0;
+}
+module_init(bcb_init);
+
+static void __exit bcb_exit(void)
+{
+	unregister_reboot_notifier(&bcb_reboot_notifier);
+}
+module_exit(bcb_exit);
+
+MODULE_AUTHOR("Andrew Boie <andrew.p.boie@intel.com>");
+MODULE_DESCRIPTION("bootloader communication module");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-29 19:36 [PATCH] bcb: Android bootloader control block driver Andrew Boie
@ 2012-06-29 21:25 ` NeilBrown
  2012-06-29 21:56   ` Boie, Andrew P
  2012-06-30  3:23 ` Greg KH
  1 sibling, 1 reply; 13+ messages in thread
From: NeilBrown @ 2012-06-29 21:25 UTC (permalink / raw)
  To: Andrew Boie; +Cc: gregkh, ccross, arve, rebecca, devel, linux-kernel

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

On Fri, 29 Jun 2012 12:36:30 -0700 Andrew Boie <andrew.p.boie@intel.com>
wrote:

> Android userspace tells the kernel that it wants to boot into recovery
> or some other non-default OS environment by passing a string argument
> to reboot(). It is left to the OEM to hook this up to their specific
> bootloader.
> 
> This driver uses the bootloader control block (BCB) in the 'misc'
> partition, which is the AOSP mechanism used to communicate with
> the bootloader. Writing 'bootonce-NNN' to the command field
> will cause the bootloader to do a oneshot boot into an alternate
> boot label NNN if it exists. The device and partition number are
> passed in via kernel command line.
> 
> It is the bootloader's job to guard against this area being uninitialzed
> or containing an invalid boot label, and just boot normally if that
> is the case. The bootloader will always populate a magic signature in
> the BCB; the driver will refuse to update it if not present.
> 
> Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

Maybe I'm missing something obvious, but why does this need to be implemented
in the kernel?  Can't some user-space process just write to that partition
using open/read/seek/write/close before calling 'reboot'.

Thanks,
NeilBrown



> ---
>  drivers/staging/android/Kconfig  |   11 ++
>  drivers/staging/android/Makefile |    1 +
>  drivers/staging/android/bcb.c    |  232 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 244 insertions(+)
>  create mode 100644 drivers/staging/android/bcb.c
> 
> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> index 9a99238..c30fd20 100644
> --- a/drivers/staging/android/Kconfig
> +++ b/drivers/staging/android/Kconfig
> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
>  	  elapsed realtime, and a non-wakeup alarm on the monotonic clock.
>  	  Also exports the alarm interface to user-space.
>  
> +config BOOTLOADER_CONTROL
> +	tristate "Bootloader Control Block module"
> +	default n
> +	help
> +	  This driver installs a reboot hook, such that if reboot() is invoked
> +	  with a string argument NNN, "bootonce-NNN" is copied to the command
> +	  field in the Bootloader Control Block on the /misc partition, to be
> +	  read by the bootloader. If the string matches one of the boot labels
> +	  defined in its configuration, it will boot once into that label. The
> +	  device and partition number are specified on the kernel command line.
> +
>  endif # if ANDROID
>  
>  endmenu
> diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
> index 8e18d4e..a27707f 100644
> --- a/drivers/staging/android/Makefile
> +++ b/drivers/staging/android/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
>  obj-$(CONFIG_ANDROID_SWITCH)		+= switch/
>  obj-$(CONFIG_ANDROID_INTF_ALARM_DEV)	+= alarm-dev.o
>  obj-$(CONFIG_PERSISTENT_TRACER)		+= trace_persistent.o
> +obj-$(CONFIG_BOOTLOADER_CONTROL)	+= bcb.o
>  
>  CFLAGS_REMOVE_trace_persistent.o = -pg
> diff --git a/drivers/staging/android/bcb.c b/drivers/staging/android/bcb.c
> new file mode 100644
> index 0000000..af75257
> --- /dev/null
> +++ b/drivers/staging/android/bcb.c
> @@ -0,0 +1,232 @@
> +/*
> + * bcb.c: Reboot hook to write bootloader commands to
> + *        the Android bootloader control block
> + *
> + * (C) Copyright 2012 Intel Corporation
> + * Author: Andrew Boie <andrew.p.boie@intel.com>
> + *
> + * 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/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/blkdev.h>
> +#include <linux/reboot.h>
> +
> +#define BCB_MAGIC	0xFEEDFACE
> +
> +/* Persistent area written by Android recovery console and Linux bcb driver
> + * reboot hook for communication with the bootloader. Bootloader must
> + * gracefully handle this area being unitinitailzed */
> +struct bootloader_message {
> +	/* Directive to the bootloader on what it needs to do next.
> +	 * Possible values:
> +	 *   boot-NNN - Automatically boot into label NNN
> +	 *   bootonce-NNN - Automatically boot into label NNN, clearing this
> +	 *     field afterwards
> +	 *   anything else / garbage - Boot default label */
> +	char command[32];
> +
> +	/* Storage area for error codes when using the BCB to boot into special
> +	 * boot targets, e.g. for baseband update. Not used here. */
> +	char status[32];
> +
> +	/* Area for recovery console to stash its command line arguments
> +	 * in case it is reset and the cache command file is erased.
> +	 * Not used here. */
> +	char recovery[1024];
> +
> +	/* Magic sentinel value written by the bootloader; don't update this
> +	 * if not equalto to BCB_MAGIC */
> +	uint32_t magic;
> +};
> +
> +/* TODO: device names/partition numbers are unstable. Add support for looking
> + * by GPT partition UUIDs */
> +static char *bootdev = "sda";
> +module_param(bootdev, charp, S_IRUGO);
> +MODULE_PARM_DESC(bootdev, "Block device for bootloader communication");
> +
> +static int partno;
> +module_param(partno, int, S_IRUGO);
> +MODULE_PARM_DESC(partno, "Partition number for bootloader communication");
> +
> +static int device_match(struct device *dev, void *data)
> +{
> +	if (strcmp(dev_name(dev), bootdev) == 0)
> +		return 1;
> +	return 0;
> +}
> +
> +static struct block_device *get_emmc_bdev(void)
> +{
> +	struct block_device *bdev;
> +	struct device *disk_device;
> +
> +	disk_device = class_find_device(&block_class, NULL, NULL, device_match);
> +	if (!disk_device) {
> +		pr_err("bcb: device %s not found!\n", bootdev);
> +		return NULL;
> +	}
> +	bdev = bdget_disk(dev_to_disk(disk_device), partno);
> +	if (!bdev) {
> +		dev_err(disk_device, "bcb: unable to get disk (%s,%d)\n",
> +				bootdev, partno);
> +		return NULL;
> +	}
> +	/* Note: this bdev ref will be freed after first
> +	   bdev_get/bdev_put cycle */
> +	return bdev;
> +}
> +
> +static u64 last_lba(struct block_device *bdev)
> +{
> +	if (!bdev || !bdev->bd_inode)
> +		return 0;
> +	return div_u64(bdev->bd_inode->i_size,
> +		       bdev_logical_block_size(bdev)) - 1ULL;
> +}
> +
> +static size_t read_lba(struct block_device *bdev,
> +		       u64 lba, u8 *buffer, size_t count)
> +{
> +	size_t totalreadcount = 0;
> +	sector_t n = lba * (bdev_logical_block_size(bdev) / 512);
> +
> +	if (!buffer || lba > last_lba(bdev))
> +		return 0;
> +
> +	while (count) {
> +		int copied = 512;
> +		Sector sect;
> +		unsigned char *data = read_dev_sector(bdev, n++, &sect);
> +		if (!data)
> +			break;
> +		if (copied > count)
> +			copied = count;
> +		memcpy(buffer, data, copied);
> +		put_dev_sector(sect);
> +		buffer += copied;
> +		totalreadcount += copied;
> +		count -= copied;
> +	}
> +	return totalreadcount;
> +}
> +
> +static size_t write_lba(struct block_device *bdev,
> +		       u64 lba, u8 *buffer, size_t count)
> +{
> +	size_t totalwritecount = 0;
> +	sector_t n = lba * (bdev_logical_block_size(bdev) / 512);
> +
> +	if (!buffer || lba > last_lba(bdev))
> +		return 0;
> +
> +	while (count) {
> +		int copied = 512;
> +		Sector sect;
> +		unsigned char *data = read_dev_sector(bdev, n++, &sect);
> +		if (!data)
> +			break;
> +		if (copied > count)
> +			copied = count;
> +		memcpy(data, buffer, copied);
> +		set_page_dirty(sect.v);
> +		unlock_page(sect.v);
> +		put_dev_sector(sect);
> +		buffer += copied;
> +		totalwritecount += copied;
> +		count -= copied;
> +	}
> +	sync_blockdev(bdev);
> +	return totalwritecount;
> +}
> +
> +static int bcb_reboot_notifier_call(
> +		struct notifier_block *notifier,
> +		unsigned long what, void *data)
> +{
> +	int ret = NOTIFY_DONE;
> +	char *cmd = (char *)data;
> +	struct block_device *bdev = NULL;
> +	struct bootloader_message *bcb = NULL;
> +
> +	if (what != SYS_RESTART || !data)
> +		goto out;
> +
> +	bdev = get_emmc_bdev();
> +	if (!bdev)
> +		goto out;
> +
> +	/* make sure the block device is open rw */
> +	if (blkdev_get(bdev, FMODE_READ | FMODE_WRITE, NULL) < 0) {
> +		pr_err("bcb: blk_dev_get failed!\n");
> +		goto out;
> +	}
> +
> +	bcb = kmalloc(sizeof(*bcb), GFP_KERNEL);
> +	if (!bcb) {
> +		pr_err("bcb: out of memory\n");
> +		goto out;
> +	}
> +
> +	if (read_lba(bdev, 0, (u8 *)bcb, sizeof(*bcb)) != sizeof(*bcb)) {
> +		pr_err("bcb: couldn't read bootloader control block\n");
> +		goto out;
> +	}
> +
> +	if (bcb->magic != BCB_MAGIC) {
> +		pr_err("bcb: bootloader control block corrupted, not writing\n");
> +		goto out;
> +	}
> +
> +	/* When the bootloader reads this area, it will null-terminate it
> +	* and check if it matches any existing boot labels */
> +	snprintf(bcb->command, sizeof(bcb->command), "bootonce-%s", cmd);
> +
> +	if (write_lba(bdev, 0, (u8 *)bcb, sizeof(*bcb)) != sizeof(*bcb)) {
> +		pr_err("bcb: couldn't write bootloader control block\n");
> +		goto out;
> +	}
> +
> +	ret = NOTIFY_OK;
> +out:
> +	kfree(bcb);
> +	if (bdev)
> +		blkdev_put(bdev, FMODE_READ | FMODE_WRITE);
> +
> +	return ret;
> +}
> +
> +static struct notifier_block bcb_reboot_notifier = {
> +	.notifier_call = bcb_reboot_notifier_call,
> +};
> +
> +static int __init bcb_init(void)
> +{
> +	if (partno < 1) {
> +		pr_err("bcb: partition number not specified\n");
> +		return -1;
> +	}
> +	if (register_reboot_notifier(&bcb_reboot_notifier)) {
> +		pr_err("bcb: unable to register reboot notifier\n");
> +		return -1;
> +	}
> +	pr_info("bcb: writing commands to (%s,%d)\n",
> +			bootdev, partno);
> +	return 0;
> +}
> +module_init(bcb_init);
> +
> +static void __exit bcb_exit(void)
> +{
> +	unregister_reboot_notifier(&bcb_reboot_notifier);
> +}
> +module_exit(bcb_exit);
> +
> +MODULE_AUTHOR("Andrew Boie <andrew.p.boie@intel.com>");
> +MODULE_DESCRIPTION("bootloader communication module");
> +MODULE_LICENSE("GPL v2");


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] bcb: Android bootloader control block driver
  2012-06-29 21:25 ` NeilBrown
@ 2012-06-29 21:56   ` Boie, Andrew P
  2012-06-30  3:08     ` gregkh
  0 siblings, 1 reply; 13+ messages in thread
From: Boie, Andrew P @ 2012-06-29 21:56 UTC (permalink / raw)
  To: NeilBrown; +Cc: gregkh, ccross, arve, rebecca, devel, linux-kernel

> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Friday, June 29, 2012 2:25 PM
> 
> On Fri, 29 Jun 2012 12:36:30 -0700 Andrew Boie <andrew.p.boie@intel.com>
> wrote:
> 
> > Android userspace tells the kernel that it wants to boot into recovery
> > or some other non-default OS environment by passing a string argument
> > to reboot(). It is left to the OEM to hook this up to their specific
> > bootloader.

> Maybe I'm missing something obvious, but why does this need to be
> implemented
> in the kernel?  Can't some user-space process just write to that partition
> using open/read/seek/write/close before calling 'reboot'.

Thanks for reviewing. The way Android is currently designed, all calls to reboot the system into an alternate target go into android_reboot() in libcutils which then make the reboot() system call with a string argument. How this is actually done on a particular board is not specified in the Android Open Source Project as far as I can see. The particular bootloader is not specified either, many different ones are being used in practice.

Not  every architecture is going to be using the Bootloader Control Block to handle these boots into alternate targets. For example, I worked on one Android-based device that didn't have a traditional bootloader at all and the reboot hook in the kernel was radically different. In this case the BCB ended up only being used by the recovery console to stash its command line arguments.

 If this were all done in userspace, then I think there would have to be separate code paths in libcutils for different board implementations of this policy. As of right now libcutils doesn't have any hardware-specific stuff in it and the mechanism to effect these policies is left to the kernel, libcutils works everywhere without modification.

Regards,
Andrew

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-29 21:56   ` Boie, Andrew P
@ 2012-06-30  3:08     ` gregkh
  0 siblings, 0 replies; 13+ messages in thread
From: gregkh @ 2012-06-30  3:08 UTC (permalink / raw)
  To: Boie, Andrew P; +Cc: NeilBrown, devel, linux-kernel, arve, ccross, rebecca

On Fri, Jun 29, 2012 at 09:56:36PM +0000, Boie, Andrew P wrote:
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Friday, June 29, 2012 2:25 PM
> > 
> > On Fri, 29 Jun 2012 12:36:30 -0700 Andrew Boie <andrew.p.boie@intel.com>
> > wrote:
> > 
> > > Android userspace tells the kernel that it wants to boot into recovery
> > > or some other non-default OS environment by passing a string argument
> > > to reboot(). It is left to the OEM to hook this up to their specific
> > > bootloader.
> 
> > Maybe I'm missing something obvious, but why does this need to be
> > implemented
> > in the kernel?  Can't some user-space process just write to that partition
> > using open/read/seek/write/close before calling 'reboot'.
> 
> Thanks for reviewing. The way Android is currently designed, all calls
> to reboot the system into an alternate target go into android_reboot()
> in libcutils which then make the reboot() system call with a string
> argument. How this is actually done on a particular board is not
> specified in the Android Open Source Project as far as I can see. The
> particular bootloader is not specified either, many different ones are
> being used in practice.

Which is fine, the kernel doesn't, and shouldn't care about this.

> Not  every architecture is going to be using the Bootloader Control
> Block to handle these boots into alternate targets. For example, I
> worked on one Android-based device that didn't have a traditional
> bootloader at all and the reboot hook in the kernel was radically
> different. In this case the BCB ended up only being used by the
> recovery console to stash its command line arguments.

So what architectures are going to be using this?

> If this were all done in userspace, then I think there would have to
> be separate code paths in libcutils for different board
> implementations of this policy. As of right now libcutils doesn't have
> any hardware-specific stuff in it and the mechanism to effect these
> policies is left to the kernel, libcutils works everywhere without
> modification.

So, without that, the kernel is setting the policy here instead?  That's
generally the opposite of what we want to do, it's up to userspace to
determine things like this.

confused,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-29 19:36 [PATCH] bcb: Android bootloader control block driver Andrew Boie
  2012-06-29 21:25 ` NeilBrown
@ 2012-06-30  3:23 ` Greg KH
  2012-06-30  3:43   ` Colin Cross
  1 sibling, 1 reply; 13+ messages in thread
From: Greg KH @ 2012-06-30  3:23 UTC (permalink / raw)
  To: Andrew Boie; +Cc: ccross, arve, rebecca, devel, linux-kernel

On Fri, Jun 29, 2012 at 12:36:30PM -0700, Andrew Boie wrote:
> Android userspace tells the kernel that it wants to boot into recovery
> or some other non-default OS environment by passing a string argument
> to reboot(). It is left to the OEM to hook this up to their specific
> bootloader.

How does userspace "tell" the kernel this?  You are creating a new
user/kernel api here, and haven't documented it at all.  That's not
generally a wise idea.

> This driver uses the bootloader control block (BCB) in the 'misc'
> partition, which is the AOSP mechanism used to communicate with
> the bootloader. Writing 'bootonce-NNN' to the command field
> will cause the bootloader to do a oneshot boot into an alternate
> boot label NNN if it exists. The device and partition number are
> passed in via kernel command line.

I don't understand still, why userspace can't just do this as it is the
one that knws where the device and partition is, and it knows what it
wants to have written in this area, why have the kernel do this?  Why
not just modify userspace to do it instead?

> It is the bootloader's job to guard against this area being uninitialzed
> or containing an invalid boot label, and just boot normally if that
> is the case. The bootloader will always populate a magic signature in
> the BCB; the driver will refuse to update it if not present.

That sounds like something that userspace should be doing, not the
kernel.

> Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
> ---
>  drivers/staging/android/Kconfig  |   11 ++
>  drivers/staging/android/Makefile |    1 +
>  drivers/staging/android/bcb.c    |  232 ++++++++++++++++++++++++++++++++++++++

Why is this being proposed for drivers/staging/?  What is wrong with the
code that is keeping it out of the "real" part of the kernel (well,
except for the above questions that is)?

What would need to be done to it to get it out of staging, and why
hasn't that been done to the code already?


> 
> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> index 9a99238..c30fd20 100644
> --- a/drivers/staging/android/Kconfig
> +++ b/drivers/staging/android/Kconfig
> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
>  	  elapsed realtime, and a non-wakeup alarm on the monotonic clock.
>  	  Also exports the alarm interface to user-space.
>  
> +config BOOTLOADER_CONTROL
> +	tristate "Bootloader Control Block module"
> +	default n
> +	help
> +	  This driver installs a reboot hook, such that if reboot() is invoked
> +	  with a string argument NNN, "bootonce-NNN" is copied to the command
> +	  field in the Bootloader Control Block on the /misc partition, to be
> +	  read by the bootloader. If the string matches one of the boot labels
> +	  defined in its configuration, it will boot once into that label. The
> +	  device and partition number are specified on the kernel command line.
> +
>  endif # if ANDROID
>  
>  endmenu
> diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
> index 8e18d4e..a27707f 100644
> --- a/drivers/staging/android/Makefile
> +++ b/drivers/staging/android/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
>  obj-$(CONFIG_ANDROID_SWITCH)		+= switch/
>  obj-$(CONFIG_ANDROID_INTF_ALARM_DEV)	+= alarm-dev.o
>  obj-$(CONFIG_PERSISTENT_TRACER)		+= trace_persistent.o
> +obj-$(CONFIG_BOOTLOADER_CONTROL)	+= bcb.o
>  
>  CFLAGS_REMOVE_trace_persistent.o = -pg
> diff --git a/drivers/staging/android/bcb.c b/drivers/staging/android/bcb.c
> new file mode 100644
> index 0000000..af75257
> --- /dev/null
> +++ b/drivers/staging/android/bcb.c
> @@ -0,0 +1,232 @@
> +/*
> + * bcb.c: Reboot hook to write bootloader commands to
> + *        the Android bootloader control block
> + *
> + * (C) Copyright 2012 Intel Corporation
> + * Author: Andrew Boie <andrew.p.boie@intel.com>
> + *
> + * 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/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/blkdev.h>
> +#include <linux/reboot.h>
> +
> +#define BCB_MAGIC	0xFEEDFACE
> +
> +/* Persistent area written by Android recovery console and Linux bcb driver
> + * reboot hook for communication with the bootloader. Bootloader must
> + * gracefully handle this area being unitinitailzed */
> +struct bootloader_message {
> +	/* Directive to the bootloader on what it needs to do next.
> +	 * Possible values:
> +	 *   boot-NNN - Automatically boot into label NNN
> +	 *   bootonce-NNN - Automatically boot into label NNN, clearing this
> +	 *     field afterwards
> +	 *   anything else / garbage - Boot default label */
> +	char command[32];
> +
> +	/* Storage area for error codes when using the BCB to boot into special
> +	 * boot targets, e.g. for baseband update. Not used here. */
> +	char status[32];
> +
> +	/* Area for recovery console to stash its command line arguments
> +	 * in case it is reset and the cache command file is erased.
> +	 * Not used here. */
> +	char recovery[1024];
> +
> +	/* Magic sentinel value written by the bootloader; don't update this
> +	 * if not equalto to BCB_MAGIC */
> +	uint32_t magic;
> +};

Ick, you are tacking this onto the reboot() syscall?  And doing so
without setting any of those fields to 0?  How do you handle null
terminating the command line arguments, or the command or the status
fields properly?  Pad them out with spaces?  You better document the
heck out of this as you just essencially are creating a whole new
syscall, and we can't do that without lots of documentation and review.

Actually, why not just create a new syscall?  That's what you really
want here, right?

> +/* TODO: device names/partition numbers are unstable. Add support for looking
> + * by GPT partition UUIDs */

They are more than just "unstable" they are not stable at all and should
not be used at all for something like this.

> +static char *bootdev = "sda";
> +module_param(bootdev, charp, S_IRUGO);
> +MODULE_PARM_DESC(bootdev, "Block device for bootloader communication");
> +
> +static int partno;
> +module_param(partno, int, S_IRUGO);
> +MODULE_PARM_DESC(partno, "Partition number for bootloader communication");

So, by default, you write over sda.  Not very nice.  And a module
parameter?  Really?  That's not a good way to talk to a module, who is
going to set these options and how are they going to know how to do it?
And who is going to give them the permission to set these options?

Why are these module parameters, and the other information tacked on in
a string to the reboot syscall?  Wouldn't it make more sense to put it
all in the syscall?  That way you could build the code into the
kernel...

Note, none of this review should be construed as me saying this is a
valid option to be putting into the kernel at all.  I still think it
should be done in userspace and see no reason why it can not be done
that way.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-30  3:23 ` Greg KH
@ 2012-06-30  3:43   ` Colin Cross
  2012-06-30  4:19     ` Greg KH
  0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-06-30  3:43 UTC (permalink / raw)
  To: Greg KH; +Cc: Andrew Boie, arve, rebecca, devel, linux-kernel

On Fri, Jun 29, 2012 at 8:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 29, 2012 at 12:36:30PM -0700, Andrew Boie wrote:
>> Android userspace tells the kernel that it wants to boot into recovery
>> or some other non-default OS environment by passing a string argument
>> to reboot(). It is left to the OEM to hook this up to their specific
>> bootloader.
>
> How does userspace "tell" the kernel this?  You are creating a new
> user/kernel api here, and haven't documented it at all.  That's not
> generally a wise idea.

Android's libcutils uses the existing reboot syscall with
reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2,
LINUX_REBOOT_CMD_MAGIC2, <string>).  According to "man 2 reboot":
       LINUX_REBOOT_CMD_RESTART2
              (0xa1b2c3d4; since 2.1.30).  The message "Restarting system with
              command  '%s'"  is  printed,  and  a  restart (using the command
              string given in arg) is performed immediately.  If not  preceded
              by a sync(2), data will be lost.

>> This driver uses the bootloader control block (BCB) in the 'misc'
>> partition, which is the AOSP mechanism used to communicate with
>> the bootloader. Writing 'bootonce-NNN' to the command field
>> will cause the bootloader to do a oneshot boot into an alternate
>> boot label NNN if it exists. The device and partition number are
>> passed in via kernel command line.
>
> I don't understand still, why userspace can't just do this as it is the
> one that knws where the device and partition is, and it knows what it
> wants to have written in this area, why have the kernel do this?  Why
> not just modify userspace to do it instead?

In this particular case, userspace could handle writing the data.  On
many SoCs, reboot mode is written to a register or to internal IO
mapped memory.  Supporting the REBOOT2 argument to the syscall
requires something in the kernel to save that message, this driver
seems to commonize saving that to a disk partition, which is the least
common denominator way to handle it.

<snip>

>>
>> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
>> index 9a99238..c30fd20 100644
>> --- a/drivers/staging/android/Kconfig
>> +++ b/drivers/staging/android/Kconfig
>> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
>>         elapsed realtime, and a non-wakeup alarm on the monotonic clock.
>>         Also exports the alarm interface to user-space.
>>
>> +config BOOTLOADER_CONTROL
>> +     tristate "Bootloader Control Block module"
>> +     default n
>> +     help
>> +       This driver installs a reboot hook, such that if reboot() is invoked
>> +       with a string argument NNN, "bootonce-NNN" is copied to the command
>> +       field in the Bootloader Control Block on the /misc partition, to be
>> +       read by the bootloader. If the string matches one of the boot labels
>> +       defined in its configuration, it will boot once into that label. The
>> +       device and partition number are specified on the kernel command line.
>> +
>>  endif # if ANDROID
>>
>>  endmenu

Most of this driver is not unique to Android.  If you remove the
status and recovery fields in the partition and the list of possible
command values, it becomes a neutral way to pass the REBOOT2 argument
from userspace to the bootloader.  status and recovery are a separate
contract between userspace and the bootloader, and could go into a
separate block in the same partition for systems that need it.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-30  3:43   ` Colin Cross
@ 2012-06-30  4:19     ` Greg KH
  2012-06-30  4:37       ` Colin Cross
  0 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2012-06-30  4:19 UTC (permalink / raw)
  To: Colin Cross; +Cc: Andrew Boie, arve, rebecca, devel, linux-kernel

On Fri, Jun 29, 2012 at 08:43:34PM -0700, Colin Cross wrote:
> On Fri, Jun 29, 2012 at 8:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jun 29, 2012 at 12:36:30PM -0700, Andrew Boie wrote:
> >> Android userspace tells the kernel that it wants to boot into recovery
> >> or some other non-default OS environment by passing a string argument
> >> to reboot(). It is left to the OEM to hook this up to their specific
> >> bootloader.
> >
> > How does userspace "tell" the kernel this?  You are creating a new
> > user/kernel api here, and haven't documented it at all.  That's not
> > generally a wise idea.
> 
> Android's libcutils uses the existing reboot syscall with
> reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2,
> LINUX_REBOOT_CMD_MAGIC2, <string>).  According to "man 2 reboot":
>        LINUX_REBOOT_CMD_RESTART2
>               (0xa1b2c3d4; since 2.1.30).  The message "Restarting system with
>               command  '%s'"  is  printed,  and  a  restart (using the command
>               string given in arg) is performed immediately.  If not  preceded
>               by a sync(2), data will be lost.

Yes, but now you have given the <string> field here a "magic" value and
data which causes the kernel to write something to a random disk block.
And that <string> is now not just a string, but it is really a structure
that the kernel is interpreting.

In other words, you have just created a new syscall :)

> >> This driver uses the bootloader control block (BCB) in the 'misc'
> >> partition, which is the AOSP mechanism used to communicate with
> >> the bootloader. Writing 'bootonce-NNN' to the command field
> >> will cause the bootloader to do a oneshot boot into an alternate
> >> boot label NNN if it exists. The device and partition number are
> >> passed in via kernel command line.
> >
> > I don't understand still, why userspace can't just do this as it is the
> > one that knws where the device and partition is, and it knows what it
> > wants to have written in this area, why have the kernel do this?  Why
> > not just modify userspace to do it instead?
> 
> In this particular case, userspace could handle writing the data.  On
> many SoCs, reboot mode is written to a register or to internal IO
> mapped memory.  Supporting the REBOOT2 argument to the syscall
> requires something in the kernel to save that message, this driver
> seems to commonize saving that to a disk partition, which is the least
> common denominator way to handle it.

Ok, but as that is what this driver is doing, userspace should be doing
it instead as there is no writing to magic registers or internal IO
mapped memory happening.

> <snip>
> 
> >>
> >> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> >> index 9a99238..c30fd20 100644
> >> --- a/drivers/staging/android/Kconfig
> >> +++ b/drivers/staging/android/Kconfig
> >> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
> >>         elapsed realtime, and a non-wakeup alarm on the monotonic clock.
> >>         Also exports the alarm interface to user-space.
> >>
> >> +config BOOTLOADER_CONTROL
> >> +     tristate "Bootloader Control Block module"
> >> +     default n
> >> +     help
> >> +       This driver installs a reboot hook, such that if reboot() is invoked
> >> +       with a string argument NNN, "bootonce-NNN" is copied to the command
> >> +       field in the Bootloader Control Block on the /misc partition, to be
> >> +       read by the bootloader. If the string matches one of the boot labels
> >> +       defined in its configuration, it will boot once into that label. The
> >> +       device and partition number are specified on the kernel command line.
> >> +
> >>  endif # if ANDROID
> >>
> >>  endmenu
> 
> Most of this driver is not unique to Android.

Do any other systems use it?

> If you remove the status and recovery fields in the partition and the
> list of possible command values, it becomes a neutral way to pass the
> REBOOT2 argument from userspace to the bootloader.  status and
> recovery are a separate contract between userspace and the bootloader,
> and could go into a separate block in the same partition for systems
> that need it.

Possibly, but again, writing to a specific block should be something
that userspace does, not the kernel, as userspace just told the kernel
to write this data to the disk already, why not use the standard method
instead of creating a new way?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-30  4:19     ` Greg KH
@ 2012-06-30  4:37       ` Colin Cross
  2012-07-02  0:10         ` NeilBrown
  0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-06-30  4:37 UTC (permalink / raw)
  To: Greg KH; +Cc: Andrew Boie, arve, rebecca, devel, linux-kernel

On Fri, Jun 29, 2012 at 9:19 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 29, 2012 at 08:43:34PM -0700, Colin Cross wrote:
>> On Fri, Jun 29, 2012 at 8:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Jun 29, 2012 at 12:36:30PM -0700, Andrew Boie wrote:
>> >> Android userspace tells the kernel that it wants to boot into recovery
>> >> or some other non-default OS environment by passing a string argument
>> >> to reboot(). It is left to the OEM to hook this up to their specific
>> >> bootloader.
>> >
>> > How does userspace "tell" the kernel this?  You are creating a new
>> > user/kernel api here, and haven't documented it at all.  That's not
>> > generally a wise idea.
>>
>> Android's libcutils uses the existing reboot syscall with
>> reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2,
>> LINUX_REBOOT_CMD_MAGIC2, <string>).  According to "man 2 reboot":
>>        LINUX_REBOOT_CMD_RESTART2
>>               (0xa1b2c3d4; since 2.1.30).  The message "Restarting system with
>>               command  '%s'"  is  printed,  and  a  restart (using the command
>>               string given in arg) is performed immediately.  If not  preceded
>>               by a sync(2), data will be lost.
>
> Yes, but now you have given the <string> field here a "magic" value and
> data which causes the kernel to write something to a random disk block.
> And that <string> is now not just a string, but it is really a structure
> that the kernel is interpreting.
>
> In other words, you have just created a new syscall :)

I don't think so, the structure is the format on disk, not through the
reboot syscall.  The magic in the driver refers to a magic value on
disk, which if not present will prevent writing over other data.  The
string from the reboot syscall is written to the command field on
disk.  This driver slightly munges it, and mixes it with some other
fields, but if it passed it straight through it would have nothing
magic and no policy.

>> >> This driver uses the bootloader control block (BCB) in the 'misc'
>> >> partition, which is the AOSP mechanism used to communicate with
>> >> the bootloader. Writing 'bootonce-NNN' to the command field
>> >> will cause the bootloader to do a oneshot boot into an alternate
>> >> boot label NNN if it exists. The device and partition number are
>> >> passed in via kernel command line.
>> >
>> > I don't understand still, why userspace can't just do this as it is the
>> > one that knws where the device and partition is, and it knows what it
>> > wants to have written in this area, why have the kernel do this?  Why
>> > not just modify userspace to do it instead?
>>
>> In this particular case, userspace could handle writing the data.  On
>> many SoCs, reboot mode is written to a register or to internal IO
>> mapped memory.  Supporting the REBOOT2 argument to the syscall
>> requires something in the kernel to save that message, this driver
>> seems to commonize saving that to a disk partition, which is the least
>> common denominator way to handle it.
>
> Ok, but as that is what this driver is doing, userspace should be doing
> it instead as there is no writing to magic registers or internal IO
> mapped memory happening.

What's the point of the existing syscall option if it doesn't work on
all platforms, or at least all platforms that want to support it?  It
doesn't make sense to me to use REBOOT2 on some SoCs because they
happen to use something that userspace cannot access, but use direct
access from userspace and a different reboot syscall option on other
SoCs.

>> <snip>
>>
>> >>
>> >> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
>> >> index 9a99238..c30fd20 100644
>> >> --- a/drivers/staging/android/Kconfig
>> >> +++ b/drivers/staging/android/Kconfig
>> >> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
>> >>         elapsed realtime, and a non-wakeup alarm on the monotonic clock.
>> >>         Also exports the alarm interface to user-space.
>> >>
>> >> +config BOOTLOADER_CONTROL
>> >> +     tristate "Bootloader Control Block module"
>> >> +     default n
>> >> +     help
>> >> +       This driver installs a reboot hook, such that if reboot() is invoked
>> >> +       with a string argument NNN, "bootonce-NNN" is copied to the command
>> >> +       field in the Bootloader Control Block on the /misc partition, to be
>> >> +       read by the bootloader. If the string matches one of the boot labels
>> >> +       defined in its configuration, it will boot once into that label. The
>> >> +       device and partition number are specified on the kernel command line.
>> >> +
>> >>  endif # if ANDROID
>> >>
>> >>  endmenu
>>
>> Most of this driver is not unique to Android.
>
> Do any other systems use it?

None that I'm aware of, but REBOOT2 existed long before Android, so I
assume something must have used it.

>> If you remove the status and recovery fields in the partition and the
>> list of possible command values, it becomes a neutral way to pass the
>> REBOOT2 argument from userspace to the bootloader.  status and
>> recovery are a separate contract between userspace and the bootloader,
>> and could go into a separate block in the same partition for systems
>> that need it.
>
> Possibly, but again, writing to a specific block should be something
> that userspace does, not the kernel, as userspace just told the kernel
> to write this data to the disk already, why not use the standard method
> instead of creating a new way?

Depends which standard method you are using.  I am assuming that
REBOOT2 with a string is a standard method, and something like this
driver can be used to provide a shared implementation so every
platform that wants to use it doesn't have to implement it in their
board file.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-06-30  4:37       ` Colin Cross
@ 2012-07-02  0:10         ` NeilBrown
  2012-07-07 22:39           ` Colin Cross
  0 siblings, 1 reply; 13+ messages in thread
From: NeilBrown @ 2012-07-02  0:10 UTC (permalink / raw)
  To: Colin Cross; +Cc: Greg KH, Andrew Boie, arve, rebecca, devel, linux-kernel

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

On Fri, 29 Jun 2012 21:37:36 -0700 Colin Cross <ccross@android.com> wrote:

> What's the point of the existing syscall option if it doesn't work on
> all platforms, or at least all platforms that want to support it?  It
> doesn't make sense to me to use REBOOT2 on some SoCs because they
> happen to use something that userspace cannot access, but use direct
> access from userspace and a different reboot syscall option on other
> SoCs.
> 
> >> <snip>
> >>
> >> >>
> >> >> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> >> >> index 9a99238..c30fd20 100644
> >> >> --- a/drivers/staging/android/Kconfig
> >> >> +++ b/drivers/staging/android/Kconfig
> >> >> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
> >> >>         elapsed realtime, and a non-wakeup alarm on the monotonic clock.
> >> >>         Also exports the alarm interface to user-space.
> >> >>
> >> >> +config BOOTLOADER_CONTROL
> >> >> +     tristate "Bootloader Control Block module"
> >> >> +     default n
> >> >> +     help
> >> >> +       This driver installs a reboot hook, such that if reboot() is invoked
> >> >> +       with a string argument NNN, "bootonce-NNN" is copied to the command
> >> >> +       field in the Bootloader Control Block on the /misc partition, to be
> >> >> +       read by the bootloader. If the string matches one of the boot labels
> >> >> +       defined in its configuration, it will boot once into that label. The
> >> >> +       device and partition number are specified on the kernel command line.
> >> >> +
> >> >>  endif # if ANDROID
> >> >>
> >> >>  endmenu
> >>
> >> Most of this driver is not unique to Android.
> >
> > Do any other systems use it?
> 
> None that I'm aware of, but REBOOT2 existed long before Android, so I
> assume something must have used it.

Dangerous that - making assumptions.

I've just spent a while hunting though the code and the history.
The LINUX_REBOOT_CMD_RESTART2 option to sys_reboot - which is the only one
that uses the the 'arg' option - was added in 2.1.30.  This was the same time
that reboot notifiers were added and there seem to be steps towards a more
generic "machine_restart" call.
No code actually *used* the arg.

Looking through current code is rather time consuming as you have to follow
several levels of indirection to find code that might actually use the arg,
but I spend a while looking, trying to cover samples for all archs and driver
classes (there are lots of watchdogs -  I didn't check them all).
I only found *1* instance of code which actually used the arg.
This is arch/alpha/kernel/process.c which tests if the arg is NULL, and
selects between a "cold bootstrap" and a "warm bootstrap".

I think we would be well served by a patch that just removes it.  Or at
least, that ignores the value of the arg and just passes NULL or (void*)1.
And definitely don't pass it to the reboot notifiers - no code uses it there.

I can certainly see value in having a standard interface to say "the next
reboot should boot 'foo' rather than the default".  I don't think there is
any real need for the kernel to provide that interface.

I would suggest that you create
    /sbin/next-boot-image
(or some better name), which gets a different program installed depending on
what boot loader is in use.  Then your libcutils can just

  system("/sbin/next-boot-image foo")
  reboot(LINUX_REBOOT_CMD_RESTART);

and work on all platforms.
(Or use a loadable module, or a binder RPC to some service, or dbus or smoke
signals or whatever).

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-07-02  0:10         ` NeilBrown
@ 2012-07-07 22:39           ` Colin Cross
  2012-07-07 23:05             ` Greg KH
  0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-07-07 22:39 UTC (permalink / raw)
  To: NeilBrown; +Cc: Greg KH, Andrew Boie, arve, rebecca, devel, linux-kernel

On Sun, Jul 1, 2012 at 5:10 PM, NeilBrown <neilb@suse.de> wrote:
> On Fri, 29 Jun 2012 21:37:36 -0700 Colin Cross <ccross@android.com> wrote:
>
>> What's the point of the existing syscall option if it doesn't work on
>> all platforms, or at least all platforms that want to support it?  It
>> doesn't make sense to me to use REBOOT2 on some SoCs because they
>> happen to use something that userspace cannot access, but use direct
>> access from userspace and a different reboot syscall option on other
>> SoCs.

You missed this part of my message.

>> >> <snip>
>> >>
>> >> >>
>> >> >> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
>> >> >> index 9a99238..c30fd20 100644
>> >> >> --- a/drivers/staging/android/Kconfig
>> >> >> +++ b/drivers/staging/android/Kconfig
>> >> >> @@ -78,6 +78,17 @@ config ANDROID_INTF_ALARM_DEV
>> >> >>         elapsed realtime, and a non-wakeup alarm on the monotonic clock.
>> >> >>         Also exports the alarm interface to user-space.
>> >> >>
>> >> >> +config BOOTLOADER_CONTROL
>> >> >> +     tristate "Bootloader Control Block module"
>> >> >> +     default n
>> >> >> +     help
>> >> >> +       This driver installs a reboot hook, such that if reboot() is invoked
>> >> >> +       with a string argument NNN, "bootonce-NNN" is copied to the command
>> >> >> +       field in the Bootloader Control Block on the /misc partition, to be
>> >> >> +       read by the bootloader. If the string matches one of the boot labels
>> >> >> +       defined in its configuration, it will boot once into that label. The
>> >> >> +       device and partition number are specified on the kernel command line.
>> >> >> +
>> >> >>  endif # if ANDROID
>> >> >>
>> >> >>  endmenu
>> >>
>> >> Most of this driver is not unique to Android.
>> >
>> > Do any other systems use it?
>>
>> None that I'm aware of, but REBOOT2 existed long before Android, so I
>> assume something must have used it.
>
> Dangerous that - making assumptions.
>
> I've just spent a while hunting though the code and the history.
> The LINUX_REBOOT_CMD_RESTART2 option to sys_reboot - which is the only one
> that uses the the 'arg' option - was added in 2.1.30.  This was the same time
> that reboot notifiers were added and there seem to be steps towards a more
> generic "machine_restart" call.
> No code actually *used* the arg.
>
> Looking through current code is rather time consuming as you have to follow
> several levels of indirection to find code that might actually use the arg,
> but I spend a while looking, trying to cover samples for all archs and driver
> classes (there are lots of watchdogs -  I didn't check them all).
> I only found *1* instance of code which actually used the arg.
> This is arch/alpha/kernel/process.c which tests if the arg is NULL, and
> selects between a "cold bootstrap" and a "warm bootstrap".
>
> I think we would be well served by a patch that just removes it.  Or at
> least, that ignores the value of the arg and just passes NULL or (void*)1.
> And definitely don't pass it to the reboot notifiers - no code uses it there.
>
> I can certainly see value in having a standard interface to say "the next
> reboot should boot 'foo' rather than the default".  I don't think there is
> any real need for the kernel to provide that interface.
>
> I would suggest that you create
>     /sbin/next-boot-image
> (or some better name), which gets a different program installed depending on
> what boot loader is in use.  Then your libcutils can just
>
>   system("/sbin/next-boot-image foo")
>   reboot(LINUX_REBOOT_CMD_RESTART);
>
> and work on all platforms.
> (Or use a loadable module, or a binder RPC to some service, or dbus or smoke
> signals or whatever).
>

On many (most?) ARM SoCs, the reboot flag is not stored on disk, or
anywhere else userspace can access.  It is stored in a power
management controller scratch register that survives resets, or a
register in an external I2C PMIC, or in internal SoC RAM (I've seen
all of these).  For your proposal, every SoC would need a custom API
to save the reboot flag somewhere.  For these devices, it clearly
makes more sense to use something like the REBOOT2 API, and if we have
to use it on some devices, it makes no sense to me to not use it on
other devices just because userspace could theoretically handle it.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-07-07 22:39           ` Colin Cross
@ 2012-07-07 23:05             ` Greg KH
  2012-07-08  0:25               ` Colin Cross
  0 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2012-07-07 23:05 UTC (permalink / raw)
  To: Colin Cross; +Cc: NeilBrown, Andrew Boie, arve, rebecca, devel, linux-kernel

On Sat, Jul 07, 2012 at 03:39:09PM -0700, Colin Cross wrote:
> On many (most?) ARM SoCs, the reboot flag is not stored on disk, or
> anywhere else userspace can access.  It is stored in a power
> management controller scratch register that survives resets, or a
> register in an external I2C PMIC, or in internal SoC RAM (I've seen
> all of these).  For your proposal, every SoC would need a custom API
> to save the reboot flag somewhere.  For these devices, it clearly
> makes more sense to use something like the REBOOT2 API, and if we have
> to use it on some devices, it makes no sense to me to not use it on
> other devices just because userspace could theoretically handle it.

That's fine, but that is not what this patch did at all.

Propose a patch that handles writing to those types of registers and we
will be glad to review it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-07-07 23:05             ` Greg KH
@ 2012-07-08  0:25               ` Colin Cross
  2012-07-08  0:35                 ` Boie, Andrew P
  0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-07-08  0:25 UTC (permalink / raw)
  To: Greg KH; +Cc: NeilBrown, Andrew Boie, arve, rebecca, devel, linux-kernel

On Sat, Jul 7, 2012 at 4:05 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Jul 07, 2012 at 03:39:09PM -0700, Colin Cross wrote:
>> On many (most?) ARM SoCs, the reboot flag is not stored on disk, or
>> anywhere else userspace can access.  It is stored in a power
>> management controller scratch register that survives resets, or a
>> register in an external I2C PMIC, or in internal SoC RAM (I've seen
>> all of these).  For your proposal, every SoC would need a custom API
>> to save the reboot flag somewhere.  For these devices, it clearly
>> makes more sense to use something like the REBOOT2 API, and if we have
>> to use it on some devices, it makes no sense to me to not use it on
>> other devices just because userspace could theoretically handle it.
>
> That's fine, but that is not what this patch did at all.

Yes it is, this patch is exactly what I was referring to in the last
sentence.  This patch implements the REBOOT2 API a device where there
is no available persistent storage inside the SoC and the disk must be
used instead.

> Propose a patch that handles writing to those types of registers and we
> will be glad to review it.

This isn't getting anywhere.  Let me lay out my logic why a driver
that saves reboot reason on disk is useful, and you can tell me where
you disagree.  I'll use specific examples where I can to try to be
less vague.

OMAP4 Panda boards require kernel support for reboot reason, it is
stored in internal RAM ("SAR" RAM) that is also used for
suspend/resume.

Some Tegra boards (Motorola Xoom) require kernel support for reboot
reason, it is stored in the PMIC registers over I2C.

A common API is needed for Panda, Xoom, and a variety of other boards
that use unique storage locations, and the existing REBOOT2 API seems
to me to be designed for this purpose.

Other boards (don't have a specific example here, perhaps Andrew can
provide one) need to store the reboot reason on disk.

Because the REBOOT2 API exists and is required on Panda and Xoom, it
should work on Andrew's board as well.

This patch is an attempt to make the REBOOT2 API work on Andrew's board.

Where do you disagree?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bcb: Android bootloader control block driver
  2012-07-08  0:25               ` Colin Cross
@ 2012-07-08  0:35                 ` Boie, Andrew P
  0 siblings, 0 replies; 13+ messages in thread
From: Boie, Andrew P @ 2012-07-08  0:35 UTC (permalink / raw)
  To: Colin Cross; +Cc: Greg KH, NeilBrown, arve, rebecca, devel, linux-kernel

> 
> Other boards (don't have a specific example here, perhaps Andrew can
> provide one) need to store the reboot reason on disk.

For this case, I'm trying to get the alternate boot logic to work on PC compatible devices. I have a patch against SYSLINUX which reads this area and selects an alternate boot target appropriately.

Andrew

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-07-08  0:35 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-29 19:36 [PATCH] bcb: Android bootloader control block driver Andrew Boie
2012-06-29 21:25 ` NeilBrown
2012-06-29 21:56   ` Boie, Andrew P
2012-06-30  3:08     ` gregkh
2012-06-30  3:23 ` Greg KH
2012-06-30  3:43   ` Colin Cross
2012-06-30  4:19     ` Greg KH
2012-06-30  4:37       ` Colin Cross
2012-07-02  0:10         ` NeilBrown
2012-07-07 22:39           ` Colin Cross
2012-07-07 23:05             ` Greg KH
2012-07-08  0:25               ` Colin Cross
2012-07-08  0:35                 ` Boie, Andrew P

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.