[PATCH 0/2] *** SUBJECT HERE ***

[PATCH 0/2] *** SUBJECT HERE ***

[Colin Watson]

Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

This short patch series does so by using the efivar and efiboot
libraries directly.

Colin Watson (2):
  Add %X to grub_vsnprintf_real and friends
  Minimise writes to EFI variable storage

 INSTALL                         |   5 +
 Makefile.util.def               |  20 ++
 configure.ac                    |  12 +
 grub-core/kern/misc.c           |  10 +-
 grub-core/osdep/efivar.c        |   3 +
 grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
 grub-core/osdep/unix/platform.c | 100 +------
 include/grub/util/install.h     |   5 +
 util/grub-install.c             |   4 +-
 9 files changed, 565 insertions(+), 97 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

-- 
2.17.1

[PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Colin Watson]

This is needed for UEFI Boot* variables, which the standard says are
named using upper-case hexadecimal.
---
 grub-core/kern/misc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3b633d51f..73f8e0e9e 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
 static inline char *
 grub_lltoa (char *str, int c, unsigned long long n)
 {
-  unsigned base = (c == 'x') ? 16 : 10;
+  unsigned base = (c == 'x' || c == 'X') ? 16 : 10;
   char *p;
 
   if ((long long) n < 0 && c == 'd')
@@ -603,7 +603,10 @@ grub_lltoa (char *str, int c, unsigned long long n)
     do
       {
 	unsigned d = (unsigned) (n & 0xf);
-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
+	*p = (d > 9) ? d + 'a' - 10 : d + '0';
+	if (c == 'X')
+	  *p = grub_toupper (*p);
+	p++;
       }
     while (n >>= 4);
   else
@@ -676,6 +679,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
 	{
 	case 'p':
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	case 'c':
@@ -762,6 +766,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
       switch (c)
 	{
 	case 'x':
+	case 'X':
 	case 'u':
 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
 	  break;
@@ -900,6 +905,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
 	  c = 'x';
 	  /* Fall through. */
 	case 'x':
+	case 'X':
 	case 'u':
 	case 'd':
 	  {
-- 
2.17.1

[PATCH 2/2] Minimise writes to EFI variable storage

[Colin Watson]

Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

Unfortunately, efibootmgr doesn't offer an interface that would let
grub-install do this.  It doesn't in general make very much effort to
minimise writes; it doesn't allow modifying an existing Boot* variable
entry, except in certain limited ways; and current versions don't have a
way to export the expected variable data so that grub-install can
compare it to the current data.  While it would be possible (and perhaps
desirable?) to add at least some of this to efibootmgr, that would still
leave the problem that there isn't a good upstreamable way for
grub-install to guarantee that it has a new enough version of
efibootmgr.  In any case, it's cumbersome and slow for grub-install to
have to fork efibootmgr to get things done.

Fortunately, a few years ago Peter Jones helpfully factored out a
substantial part of efibootmgr to the efivar and efiboot libraries, and
so it's now possible to have grub-install use those directly.  We still
have to use some code from efibootmgr, but much less than would
previously have been necessary.

grub-install now reuses existing boot entries where possible, and avoids
writing to variables when the new contents are the same as the old
contents.  In the common upgrade case where nothing needs to change, it
no longer writes to NVRAM at all.  It's also now slightly faster, since
using libefivar is faster than forking efibootmgr.

Fixes Debian bug #891434.

Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
---
 INSTALL                         |   5 +
 Makefile.util.def               |  20 ++
 configure.ac                    |  12 +
 grub-core/osdep/efivar.c        |   3 +
 grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
 grub-core/osdep/unix/platform.c | 100 +------
 include/grub/util/install.h     |   5 +
 util/grub-install.c             |   4 +-
 8 files changed, 557 insertions(+), 95 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

diff --git a/INSTALL b/INSTALL
index 8acb40902..342c158e9 100644
--- a/INSTALL
+++ b/INSTALL
@@ -41,6 +41,11 @@ configuring the GRUB.
 * Other standard GNU/Unix tools
 * a libc with large file support (e.g. glibc 2.1 or later)
 
+On Unix-based systems, you also need:
+
+* libefivar (recommended)
+* libefiboot (recommended; your OS may ship this together with libefivar)
+
 On GNU/Linux, you also need:
 
 * libdevmapper 1.02.34 or later (recommended)
diff --git a/Makefile.util.def b/Makefile.util.def
index 969d32f00..60ec4d593 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -535,6 +535,8 @@ program = {
   common = grub-core/osdep/compress.c;
   extra_dist = grub-core/osdep/unix/compress.c;
   extra_dist = grub-core/osdep/basic/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -548,12 +550,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 
   condition = COND_HAVE_EXEC;
 };
@@ -582,6 +587,8 @@ program = {
   extra_dist = grub-core/osdep/basic/no_platform.c;
   extra_dist = grub-core/osdep/unix/platform.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -595,12 +602,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 program = {
@@ -622,6 +632,8 @@ program = {
   common = grub-core/osdep/platform.c;
   common = grub-core/osdep/platform_unix.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -634,12 +646,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 program = {
@@ -661,6 +676,8 @@ program = {
   common = grub-core/osdep/platform.c;
   common = grub-core/osdep/platform_unix.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -670,12 +687,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 script = {
diff --git a/configure.ac b/configure.ac
index 05d040cd6..2add2a092 100644
--- a/configure.ac
+++ b/configure.ac
@@ -443,6 +443,18 @@ AC_CHECK_HEADER([util.h], [
 ])
 AC_SUBST([LIBUTIL])
 
+case "$host_os" in
+  cygwin | windows* | mingw32* | aros*)
+    ;;
+  *)
+    # For setting EFI variables in grub-install.
+    PKG_CHECK_MODULES([EFIVAR], [efivar efiboot], [
+      AC_DEFINE([HAVE_EFIVAR], [1],
+                [Define to 1 if you have the efivar and efiboot libraries.])
+    ], [:])
+    ;;
+esac
+
 AC_CACHE_CHECK([whether -Wtrampolines work], [grub_cv_host_cc_wtrampolines], [
   SAVED_CFLAGS="$CFLAGS"
   CFLAGS="$HOST_CFLAGS -Wtrampolines -Werror"
diff --git a/grub-core/osdep/efivar.c b/grub-core/osdep/efivar.c
new file mode 100644
index 000000000..d2750e252
--- /dev/null
+++ b/grub-core/osdep/efivar.c
@@ -0,0 +1,3 @@
+#if !defined (__MINGW32__) && !defined (__CYGWIN__) && !defined (__AROS__)
+#include "unix/efivar.c"
+#endif
diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c
new file mode 100644
index 000000000..1df3e38f1
--- /dev/null
+++ b/grub-core/osdep/unix/efivar.c
@@ -0,0 +1,503 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2013,2019 Free Software Foundation, Inc.
+ *
+ *  GRUB 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, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* Contains portions derived from efibootmgr, licensed as follows:
+ *
+ *  Copyright (C) 2001-2004 Dell, Inc. <Matt_Domsch@dell.com>
+ *  Copyright 2015-2016 Red Hat, Inc. <pjones@redhat.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; either version 2 of the License, or
+ *  (at your option) any later version.
+ */
+
+#include <config.h>
+
+#ifdef HAVE_EFIVAR
+
+#include <grub/util/install.h>
+#include <grub/emu/hostdisk.h>
+#include <grub/util/misc.h>
+#include <grub/list.h>
+#include <grub/misc.h>
+#include <grub/emu/exec.h>
+#include <sys/types.h>
+#include <ctype.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <efiboot.h>
+#include <efivar.h>
+
+struct efi_variable {
+  struct efi_variable *next;
+  struct efi_variable **prev;
+  char *name;
+  efi_guid_t guid;
+  uint8_t *data;
+  size_t data_size;
+  uint32_t attributes;
+  int num;
+};
+
+/* Boot option attributes.  */
+#define LOAD_OPTION_ACTIVE 0x00000001
+
+/* GUIDs.  */
+#define BLKX_UNKNOWN_GUID \
+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
+	    0x72, 0x3b)
+
+/* Log all errors recorded by libefivar/libefiboot.  */
+static void
+show_efi_errors (void)
+{
+  int i;
+  int saved_errno = errno;
+
+  for (i = 0; ; ++i)
+    {
+      char *filename, *function, *message = NULL;
+      int line, error = 0, rc;
+
+      rc = efi_error_get (i, &filename, &function, &line, &message, &error);
+      if (rc < 0)
+	/* Give up.  The caller is going to log an error anyway.  */
+	break;
+      if (rc == 0)
+	/* No more errors.  */
+	break;
+      grub_util_warn ("%s: %s: %s", function, message, strerror (error));
+    }
+
+  efi_error_clear ();
+  errno = saved_errno;
+}
+
+static struct efi_variable *
+new_efi_variable (void)
+{
+  struct efi_variable *new = xmalloc (sizeof (*new));
+  memset (new, 0, sizeof (*new));
+  return new;
+}
+
+static struct efi_variable *
+new_boot_variable (void)
+{
+  struct efi_variable *new = new_efi_variable ();
+  new->guid = EFI_GLOBAL_GUID;
+  new->attributes = EFI_VARIABLE_NON_VOLATILE |
+		    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		    EFI_VARIABLE_RUNTIME_ACCESS;
+  return new;
+}
+
+static void
+free_efi_variable (struct efi_variable *entry)
+{
+  if (entry)
+    {
+      free (entry->name);
+      free (entry->data);
+      free (entry);
+    }
+}
+
+static int
+read_efi_variable (const char *name, struct efi_variable **entry)
+{
+  struct efi_variable *new = new_efi_variable ();
+  int rc;
+
+  rc = efi_get_variable (EFI_GLOBAL_GUID, name,
+			 &new->data, &new->data_size, &new->attributes);
+  if (rc < 0)
+    {
+      free (new);
+      new = NULL;
+    }
+
+  if (new)
+    {
+      /* Latest Apple firmware sets the high bit which appears invalid
+	 to the Linux kernel if we write it back, so let's zero it out if it
+	 is set since it would be invalid to set it anyway.  */
+      new->attributes = new->attributes & ~(1 << 31);
+
+      new->name = xstrdup (name);
+      new->guid = EFI_GLOBAL_GUID;
+    }
+
+  *entry = new;
+  return rc;
+}
+
+/* Set an EFI variable, but only if it differs from the current value.
+   Some firmware implementations are liable to fill up flash space if we set
+   variables unnecessarily, so try to keep write activity to a minimum. */
+static int
+set_efi_variable (const char *name, struct efi_variable *entry)
+{
+  struct efi_variable *old = NULL;
+  int rc = 0;
+
+  read_efi_variable (name, &old);
+  efi_error_clear ();
+  if (old && old->attributes == entry->attributes &&
+      old->data_size == entry->data_size &&
+      memcmp (old->data, entry->data, entry->data_size) == 0)
+    grub_util_info ("skipping unnecessary update of EFI variable %s", name);
+  else
+    {
+      rc = efi_set_variable (EFI_GLOBAL_GUID, name,
+			     entry->data, entry->data_size, entry->attributes,
+			     0644);
+      if (rc < 0)
+	grub_util_warn (_("Cannot set EFI variable %s"), name);
+    }
+  free_efi_variable (old);
+  return rc;
+}
+
+static int
+cmpvarbyname (const void *p1, const void *p2)
+{
+  const struct efi_variable *var1 = *(const struct efi_variable **)p1;
+  const struct efi_variable *var2 = *(const struct efi_variable **)p2;
+  return strcmp (var1->name, var2->name);
+}
+
+static int
+read_boot_variables (struct efi_variable **varlist)
+{
+  int rc;
+  efi_guid_t *guid = NULL;
+  char *name = NULL;
+  struct efi_variable **newlist = NULL;
+  int nentries = 0;
+  int i;
+
+  while ((rc = efi_get_next_variable_name (&guid, &name)) > 0)
+    {
+      const char *snum = name + sizeof ("Boot") - 1;
+      struct efi_variable *var = NULL;
+      unsigned int num;
+
+      if (memcmp (guid, &efi_guid_global, sizeof (efi_guid_global)) != 0 ||
+	  strncmp (name, "Boot", sizeof ("Boot") - 1) != 0 ||
+	  !grub_isxdigit (snum[0]) || !grub_isxdigit (snum[1]) ||
+	  !grub_isxdigit (snum[2]) || !grub_isxdigit (snum[3]))
+	continue;
+
+      rc = read_efi_variable (name, &var);
+      if (rc < 0)
+	break;
+
+      if (sscanf (var->name, "Boot%04X-%*s", &num) == 1 && num < 65536)
+	var->num = num;
+
+      newlist = xrealloc (newlist, (++nentries) * sizeof (*newlist));
+      newlist[nentries - 1] = var;
+    }
+  if (rc == 0 && newlist)
+    {
+      qsort (newlist, nentries, sizeof (*newlist), cmpvarbyname);
+      for (i = nentries - 1; i >= 0; --i)
+	grub_list_push (GRUB_AS_LIST_P (varlist), GRUB_AS_LIST (newlist[i]));
+    }
+  else if (newlist)
+    {
+      for (i = 0; i < nentries; ++i)
+	free (newlist[i]);
+      free (newlist);
+    }
+  return rc;
+}
+
+static void
+remove_from_boot_order (struct efi_variable *order, uint16_t num)
+{
+  uint16_t *data;
+  unsigned int old_i, new_i;
+
+  /* We've got an array (in order->data) of the order.  Squeeze out any
+     instance of the entry we're deleting by shifting the remainder down.  */
+  data = (uint16_t *) order->data;
+
+  for (old_i = 0, new_i = 0;
+       old_i < order->data_size / sizeof (uint16_t);
+       ++old_i)
+    {
+      if (data[old_i] != num) {
+	if (new_i != old_i)
+	  data[new_i] = data[old_i];
+	new_i++;
+      }
+    }
+
+  order->data_size = sizeof (data[0]) * new_i;
+}
+
+static void
+add_to_boot_order (struct efi_variable *order, uint16_t num)
+{
+  uint16_t *data;
+  int i;
+  size_t new_data_size;
+  uint16_t *new_data;
+
+  /* Check whether this entry is already in the boot order.  If it is, leave
+     it alone.  */
+  data = (uint16_t *) order->data;
+  for (i = 0; i < order->data_size / sizeof (uint16_t); ++i)
+    if (data[i] == num)
+      return;
+
+  new_data_size = order->data_size + sizeof (uint16_t);
+  new_data = xmalloc (new_data_size);
+  new_data[0] = num;
+  memcpy (new_data + 1, order->data, order->data_size);
+  free (order->data);
+  order->data = (uint8_t *) new_data;
+  order->data_size = new_data_size;
+}
+
+static int
+find_free_boot_num (struct efi_variable *entries)
+{
+  int num_vars = 0, i;
+  struct efi_variable *entry;
+
+  FOR_LIST_ELEMENTS (entry, entries)
+    ++num_vars;
+
+  if (num_vars == 0)
+    return 0;
+
+  /* O(n^2), but n is small and this is easy. */
+  for (i = 0; i < num_vars; ++i)
+    {
+      int found = 0;
+      FOR_LIST_ELEMENTS (entry, entries)
+	{
+	  if (entry->num == i)
+	    {
+	      found = 1;
+	      break;
+	    }
+	}
+      if (!found)
+	return i;
+    }
+
+  return i;
+}
+
+static int
+get_edd_version (void)
+{
+  efi_guid_t blkx_guid = BLKX_UNKNOWN_GUID;
+  uint8_t *data = NULL;
+  size_t data_size = 0;
+  uint32_t attributes;
+  efidp_header *path;
+  int rc;
+
+  rc = efi_get_variable (blkx_guid, "blk0", &data, &data_size, &attributes);
+  if (rc < 0)
+    return rc;
+
+  path = (efidp_header *) data;
+  if (path->type == 2 && path->subtype == 1)
+    return 3;
+  return 1;
+}
+
+static struct efi_variable *
+make_boot_variable (int num, const char *disk, int part, const char *loader,
+		    const char *label)
+{
+  struct efi_variable *entry = new_boot_variable ();
+  uint32_t options;
+  uint32_t edd10_devicenum;
+  ssize_t dp_needed, loadopt_needed;
+  efidp dp = NULL;
+
+  options = EFIBOOT_ABBREV_HD;
+  switch (get_edd_version ()) {
+    case 1:
+      options = EFIBOOT_ABBREV_EDD10;
+      break;
+    case 3:
+      options = EFIBOOT_ABBREV_NONE;
+      break;
+  }
+
+  /* This may not be the right disk; but it's probably only an issue on very
+     old hardware anyway. */
+  edd10_devicenum = 0x80;
+
+  dp_needed = efi_generate_file_device_path_from_esp (NULL, 0, disk, part,
+						      loader, options,
+						      edd10_devicenum);
+  if (dp_needed < 0)
+    goto err;
+
+  dp = xmalloc (dp_needed);
+  dp_needed = efi_generate_file_device_path_from_esp ((uint8_t *) dp,
+						      dp_needed, disk, part,
+						      loader, options,
+						      edd10_devicenum);
+  if (dp_needed < 0)
+    goto err;
+
+  loadopt_needed = efi_loadopt_create (NULL, 0, LOAD_OPTION_ACTIVE,
+				       dp, dp_needed, (unsigned char *) label,
+				       NULL, 0);
+  if (loadopt_needed < 0)
+    goto err;
+  entry->data_size = loadopt_needed;
+  entry->data = xmalloc (entry->data_size);
+  loadopt_needed = efi_loadopt_create (entry->data, entry->data_size,
+				       LOAD_OPTION_ACTIVE, dp, dp_needed,
+				       (unsigned char *) label, NULL, 0);
+  if (loadopt_needed < 0)
+    goto err;
+
+  entry->name = xasprintf ("Boot%04X", num);
+  entry->num = num;
+
+  return entry;
+
+err:
+  free_efi_variable (entry);
+  free (dp);
+  return NULL;
+}
+
+int
+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
+				  const char *efifile_path,
+				  const char *efi_distributor)
+{
+  const char *efidir_disk;
+  int efidir_part;
+  struct efi_variable *entries = NULL, *entry;
+  struct efi_variable *order;
+  int entry_num = -1;
+  int rc;
+
+  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
+  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
+
+#ifdef __linux__
+  /*
+   * Linux uses efivarfs (mounted on /sys/firmware/efi/efivars) to access the
+   * EFI variable store. Some legacy systems may still use the deprecated
+   * efivars interface (accessed through /sys/firmware/efi/vars). Where both
+   * are present, libefivar will use the former in preference, so attempting
+   * to load efivars will not interfere with later operations.
+   */
+  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
+			       NULL, NULL, "/dev/null");
+#endif
+
+  if (!efi_variables_supported ())
+    {
+      grub_util_warn ("%s",
+		      _("EFI variables are not supported on this system."));
+      /* Let the user continue.  Perhaps they can still arrange to boot GRUB
+         manually.  */
+      return 0;
+    }
+
+  rc = read_boot_variables (&entries);
+  if (rc < 0)
+    {
+      grub_util_warn ("%s", _("Cannot read EFI Boot* variables"));
+      goto err;
+    }
+  rc = read_efi_variable ("BootOrder", &order);
+  if (rc < 0)
+    {
+      order = new_boot_variable ();
+      order->name = xstrdup ("BootOrder");
+      efi_error_clear ();
+    }
+
+  /* Delete old entries from the same distributor.  */
+  FOR_LIST_ELEMENTS (entry, entries)
+    {
+      efi_load_option *load_option = (efi_load_option *) entry->data;
+      const char *label;
+
+      if (entry->num < 0)
+	continue;
+      label = (const char *) efi_loadopt_desc (load_option, entry->data_size);
+      if (strcasecmp (label, efi_distributor) != 0)
+	continue;
+
+      /* To avoid problems with some firmware implementations, reuse the first
+         matching variable we find rather than deleting and recreating it.  */
+      if (entry_num == -1)
+	entry_num = entry->num;
+      else
+	{
+	  grub_util_info ("deleting superfluous EFI variable %s (%s)",
+			  entry->name, label);
+	  rc = efi_del_variable (EFI_GLOBAL_GUID, entry->name);
+	  if (rc < 0)
+	    {
+	      grub_util_warn (_("Cannot delete EFI variable %s"), entry->name);
+	      goto err;
+	    }
+	}
+
+      remove_from_boot_order (order, (uint16_t) entry->num);
+    }
+
+  if (entry_num == -1)
+    entry_num = find_free_boot_num (entries);
+  entry = make_boot_variable (entry_num, efidir_disk, efidir_part,
+			      efifile_path, efi_distributor);
+  if (!entry)
+    goto err;
+
+  grub_util_info ("setting EFI variable %s", entry->name);
+  rc = set_efi_variable (entry->name, entry);
+  if (rc < 0)
+    goto err;
+
+  add_to_boot_order (order, (uint16_t) entry_num);
+
+  grub_util_info ("setting EFI variable BootOrder");
+  rc = set_efi_variable ("BootOrder", order);
+  if (rc < 0)
+    goto err;
+
+  return 0;
+
+err:
+  show_efi_errors ();
+  return errno;
+}
+
+#endif /* HAVE_EFIVAR */
diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
index 55b8f4016..9d4530a38 100644
--- a/grub-core/osdep/unix/platform.c
+++ b/grub-core/osdep/unix/platform.c
@@ -19,15 +19,12 @@
 #include <config.h>
 
 #include <grub/util/install.h>
-#include <grub/emu/hostdisk.h>
 #include <grub/util/misc.h>
 #include <grub/misc.h>
 #include <grub/i18n.h>
 #include <grub/emu/exec.h>
 #include <sys/types.h>
-#include <dirent.h>
 #include <string.h>
-#include <errno.h>
 
 static char *
 get_ofpathname (const char *dev)
@@ -78,102 +75,19 @@ get_ofpathname (const char *dev)
 		   dev);
 }
 
-static int
-grub_install_remove_efi_entries_by_distributor (const char *efi_distributor)
-{
-  int fd;
-  pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd);
-  char *line = NULL;
-  size_t len = 0;
-  int rc = 0;
-
-  if (!pid)
-    {
-      grub_util_warn (_("Unable to open stream from %s: %s"),
-		      "efibootmgr", strerror (errno));
-      return errno;
-    }
-
-  FILE *fp = fdopen (fd, "r");
-  if (!fp)
-    {
-      grub_util_warn (_("Unable to open stream from %s: %s"),
-		      "efibootmgr", strerror (errno));
-      return errno;
-    }
-
-  line = xmalloc (80);
-  len = 80;
-  while (1)
-    {
-      int ret;
-      char *bootnum;
-      ret = getline (&line, &len, fp);
-      if (ret == -1)
-	break;
-      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
-	  || line[sizeof ("Boot") - 1] < '0'
-	  || line[sizeof ("Boot") - 1] > '9')
-	continue;
-      if (!strcasestr (line, efi_distributor))
-	continue;
-      bootnum = line + sizeof ("Boot") - 1;
-      bootnum[4] = '\0';
-      if (!verbosity)
-	rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
-	      "-b", bootnum,  "-B", NULL });
-      else
-	rc = grub_util_exec ((const char * []){ "efibootmgr",
-	      "-b", bootnum, "-B", NULL });
-    }
-
-  free (line);
-  return rc;
-}
-
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
 			   const char *efi_distributor)
 {
-  const char * efidir_disk;
-  int efidir_part;
-  int ret;
-  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
-  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
-
-  if (grub_util_exec_redirect_null ((const char * []){ "efibootmgr", "--version", NULL }))
-    {
-      /* TRANSLATORS: This message is shown when required executable `%s'
-	 isn't found.  */
-      grub_util_error (_("%s: not found"), "efibootmgr");
-    }
-
-  /* On Linux, we need the efivars kernel modules.  */
-#ifdef __linux__
-  grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
+#ifdef HAVE_EFIVAR
+  return grub_install_efivar_register_efi (efidir_grub_dev, efifile_path,
+					   efi_distributor);
+#else
+  grub_util_error ("%s",
+		   _("GRUB was not built with efivar support; "
+		     "cannot register EFI boot entry"));
 #endif
-  /* Delete old entries from the same distributor.  */
-  ret = grub_install_remove_efi_entries_by_distributor (efi_distributor);
-  if (ret)
-    return ret;
-
-  char *efidir_part_str = xasprintf ("%d", efidir_part);
-
-  if (!verbosity)
-    ret = grub_util_exec ((const char * []){ "efibootmgr", "-q",
-	  "-c", "-d", efidir_disk,
-	  "-p", efidir_part_str, "-w",
-	  "-L", efi_distributor, "-l", 
-	  efifile_path, NULL });
-  else
-    ret = grub_util_exec ((const char * []){ "efibootmgr",
-	  "-c", "-d", efidir_disk,
-	  "-p", efidir_part_str, "-w",
-	  "-L", efi_distributor, "-l", 
-	  efifile_path, NULL });
-  free (efidir_part_str);
-  return ret;
 }
 
 void
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 2631b1074..dcad16ac5 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -216,6 +216,11 @@ grub_install_get_default_arm_platform (void);
 const char *
 grub_install_get_default_x86_platform (void);
 
+int
+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
+				  const char *efifile_path,
+				  const char *efi_distributor);
+
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
diff --git a/util/grub-install.c b/util/grub-install.c
index 264f9ecdc..a61df056e 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1885,7 +1885,7 @@ main (int argc, char *argv[])
 					       "\\System\\Library\\CoreServices",
 					       efi_distributor);
 	      if (ret)
-	        grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
+	        grub_util_error (_("failed to register the EFI boot entry: %s"),
 				 strerror (ret));
 	    }
 
@@ -1929,7 +1929,7 @@ main (int argc, char *argv[])
 	  ret = grub_install_register_efi (efidir_grub_dev,
 					   efifile_path, efi_distributor);
 	  if (ret)
-	    grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
+	    grub_util_error (_("failed to register the EFI boot entry: %s"),
 			     strerror (ret));
 	}
       break;
-- 
2.17.1

Re: [PATCH 0/2] Minimise writes to EFI variable storage

[Colin Watson]

Oops, please ignore me failing to drive my email client correctly.
Corrected subject line here ...

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Daniel Kiper]

On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
> This is needed for UEFI Boot* variables, which the standard says are
> named using upper-case hexadecimal.

Missing SOB. I assume that I can add yours. If yes then
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Colin Watson]

On Tue, Mar 12, 2019 at 01:19:09PM +0100, Daniel Kiper wrote:
> On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
> > This is needed for UEFI Boot* variables, which the standard says are
> > named using upper-case hexadecimal.
> 
> Missing SOB. I assume that I can add yours. If yes then
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Sorry, yes - please do.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 1/2] Add %X to grub_vsnprintf_real and friends

[Steve McIntyre]

On Mon, Mar 11, 2019 at 03:05:19PM +0000, Colin Watson wrote:
>This is needed for UEFI Boot* variables, which the standard says are
>named using upper-case hexadecimal.
>---
> grub-core/kern/misc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
>index 3b633d51f..73f8e0e9e 100644
>--- a/grub-core/kern/misc.c
>+++ b/grub-core/kern/misc.c
>@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, grub_uint64_t *r)
> static inline char *
> grub_lltoa (char *str, int c, unsigned long long n)
> {
>-  unsigned base = (c == 'x') ? 16 : 10;
>+  unsigned base = (c == 'x' || c == 'X') ? 16 : 10;
>   char *p;
> 
>   if ((long long) n < 0 && c == 'd')
>@@ -603,7 +603,10 @@ grub_lltoa (char *str, int c, unsigned long long n)
>     do
>       {
> 	unsigned d = (unsigned) (n & 0xf);
>-	*p++ = (d > 9) ? d + 'a' - 10 : d + '0';
>+	*p = (d > 9) ? d + 'a' - 10 : d + '0';
>+	if (c == 'X')
>+	  *p = grub_toupper (*p);
>+	p++;

I'd be more tempted to simply do the upper-case stuff using

        if (c == 'x')
          *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
        else
          *p++ = (d > 9) ? d + 'A' - 10 : d + '0';
        fi

rather than the call out to grub_toupper(), but that's just minor
nit-picking on style.

>       }
>     while (n >>= 4);
>   else
>@@ -676,6 +679,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
> 	{
> 	case 'p':
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	case 'c':
>@@ -762,6 +766,7 @@ parse_printf_args (const char *fmt0, struct printf_args *args,
>       switch (c)
> 	{
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	  args->ptr[curn].type = UNSIGNED_INT + longfmt;
> 	  break;
>@@ -900,6 +905,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
> 	  c = 'x';
> 	  /* Fall through. */
> 	case 'x':
>+	case 'X':
> 	case 'u':
> 	case 'd':
> 	  {
>-- 
>2.17.1

Reviewed-by: Steve McIntyre <93sam@debian.org>

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"Because heaters aren't purple!" -- Catherine Pitt

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Steve McIntyre]

First of all, thanks very much for doing this!

(For other people: I came up with a previous way of doing this years
ago, using efibootmgr to ask about the state of a variable before
maybe deleting and re-setting it.)

On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
>Some UEFI firmware is easily provoked into running out of space in its
>variable storage.  This is usually due to certain kernel drivers (e.g.
>pstore), but regardless of the cause it can cause grub-install to fail
>because it currently asks efibootmgr to delete and re-add entries, and
>the deletion often doesn't result in an immediate garbage collection.
>Writing variables frequently also increases wear on the NVRAM which may
>have limited write cycles.  For these reasons, it's desirable to find a
>way to minimise writes while still allowing grub-install to ensure that
>a suitable boot entry exists.
>
>Unfortunately, efibootmgr doesn't offer an interface that would let
>grub-install do this.  It doesn't in general make very much effort to
>minimise writes; it doesn't allow modifying an existing Boot* variable
>entry, except in certain limited ways; and current versions don't have a
>way to export the expected variable data so that grub-install can
>compare it to the current data.  While it would be possible (and perhaps
>desirable?) to add at least some of this to efibootmgr, that would still
>leave the problem that there isn't a good upstreamable way for
>grub-install to guarantee that it has a new enough version of
>efibootmgr.  In any case, it's cumbersome and slow for grub-install to
>have to fork efibootmgr to get things done.
>
>Fortunately, a few years ago Peter Jones helpfully factored out a
>substantial part of efibootmgr to the efivar and efiboot libraries, and
>so it's now possible to have grub-install use those directly.  We still
>have to use some code from efibootmgr, but much less than would
>previously have been necessary.
>
>grub-install now reuses existing boot entries where possible, and avoids
>writing to variables when the new contents are the same as the old
>contents.  In the common upgrade case where nothing needs to change, it
>no longer writes to NVRAM at all.  It's also now slightly faster, since
>using libefivar is faster than forking efibootmgr.

Awesome!

Some detailed review stuff inline below, with (maybe?) one error noted
in read_efi_variable().

I've built this code but not tested it directly on real hardware yet.

>Fixes Debian bug #891434.
>
>Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
>---
> INSTALL                         |   5 +
> Makefile.util.def               |  20 ++
> configure.ac                    |  12 +
> grub-core/osdep/efivar.c        |   3 +
> grub-core/osdep/unix/efivar.c   | 503 ++++++++++++++++++++++++++++++++
> grub-core/osdep/unix/platform.c | 100 +------
> include/grub/util/install.h     |   5 +
> util/grub-install.c             |   4 +-
> 8 files changed, 557 insertions(+), 95 deletions(-)
> create mode 100644 grub-core/osdep/efivar.c
> create mode 100644 grub-core/osdep/unix/efivar.c

...

>diff --git a/grub-core/osdep/efivar.c b/grub-core/osdep/efivar.c
>new file mode 100644
>index 000000000..d2750e252
>--- /dev/null
>+++ b/grub-core/osdep/efivar.c
>@@ -0,0 +1,3 @@
>+#if !defined (__MINGW32__) && !defined (__CYGWIN__) && !defined (__AROS__)
>+#include "unix/efivar.c"
>+#endif
>diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c
>new file mode 100644
>index 000000000..1df3e38f1
>--- /dev/null
>+++ b/grub-core/osdep/unix/efivar.c
>@@ -0,0 +1,503 @@
>+/*
>+ *  GRUB  --  GRand Unified Bootloader
>+ *  Copyright (C) 2013,2019 Free Software Foundation, Inc.
>+ *
>+ *  GRUB 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, either version 3 of the License, or
>+ *  (at your option) any later version.
>+ *
>+ *  GRUB is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ *  GNU General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU General Public License
>+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>+ */
>+
>+/* Contains portions derived from efibootmgr, licensed as follows:
>+ *
>+ *  Copyright (C) 2001-2004 Dell, Inc. <Matt_Domsch@dell.com>
>+ *  Copyright 2015-2016 Red Hat, Inc. <pjones@redhat.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; either version 2 of the License, or
>+ *  (at your option) any later version.
>+ */
>+
>+#include <config.h>
>+
>+#ifdef HAVE_EFIVAR
>+
>+#include <grub/util/install.h>
>+#include <grub/emu/hostdisk.h>
>+#include <grub/util/misc.h>
>+#include <grub/list.h>
>+#include <grub/misc.h>
>+#include <grub/emu/exec.h>
>+#include <sys/types.h>
>+#include <ctype.h>
>+#include <errno.h>
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#include <efiboot.h>
>+#include <efivar.h>
>+
>+struct efi_variable {
>+  struct efi_variable *next;
>+  struct efi_variable **prev;
>+  char *name;
>+  efi_guid_t guid;
>+  uint8_t *data;
>+  size_t data_size;
>+  uint32_t attributes;
>+  int num;
>+};
>+
>+/* Boot option attributes.  */
>+#define LOAD_OPTION_ACTIVE 0x00000001
>+
>+/* GUIDs.  */
>+#define BLKX_UNKNOWN_GUID \
>+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
>+	    0x72, 0x3b)

Ugh. I'm assuming the mahic numbers here are not exposed usefully by
efivar or efiboot?

>+/* Log all errors recorded by libefivar/libefiboot.  */
>+static void
>+show_efi_errors (void)
>+{
>+  int i;
>+  int saved_errno = errno;
>+
>+  for (i = 0; ; ++i)
>+    {
>+      char *filename, *function, *message = NULL;
>+      int line, error = 0, rc;
>+
>+      rc = efi_error_get (i, &filename, &function, &line, &message, &error);
>+      if (rc < 0)
>+	/* Give up.  The caller is going to log an error anyway.  */
>+	break;
>+      if (rc == 0)
>+	/* No more errors.  */
>+	break;
>+      grub_util_warn ("%s: %s: %s", function, message, strerror (error));
>+    }
>+
>+  efi_error_clear ();
>+  errno = saved_errno;
>+}
>+
>+static struct efi_variable *
>+new_efi_variable (void)
>+{
>+  struct efi_variable *new = xmalloc (sizeof (*new));
>+  memset (new, 0, sizeof (*new));
>+  return new;
>+}
>+
>+static struct efi_variable *
>+new_boot_variable (void)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  new->guid = EFI_GLOBAL_GUID;
>+  new->attributes = EFI_VARIABLE_NON_VOLATILE |
>+		    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>+		    EFI_VARIABLE_RUNTIME_ACCESS;
>+  return new;
>+}
>+
>+static void
>+free_efi_variable (struct efi_variable *entry)
>+{
>+  if (entry)
>+    {
>+      free (entry->name);
>+      free (entry->data);
>+      free (entry);
>+    }
>+}
>+
>+static int
>+read_efi_variable (const char *name, struct efi_variable **entry)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  int rc;
>+
>+  rc = efi_get_variable (EFI_GLOBAL_GUID, name,
>+			 &new->data, &new->data_size, &new->attributes);
>+  if (rc < 0)
>+    {
>+      free (new);
>+      new = NULL;
>+    }

So new_efi_variable() is using xmalloc() so it's safe if the
allocation fails. But what happens if efi_get_variable() fails - do
you need to free all the members by calling free_efi_variable() rather
than simply free() here?

>+
>+  if (new)
>+    {
>+      /* Latest Apple firmware sets the high bit which appears invalid
>+	 to the Linux kernel if we write it back, so let's zero it out if it
>+	 is set since it would be invalid to set it anyway.  */
>+      new->attributes = new->attributes & ~(1 << 31);
>+
>+      new->name = xstrdup (name);
>+      new->guid = EFI_GLOBAL_GUID;
>+    }
>+
>+  *entry = new;
>+  return rc;
>+}
>+
>+/* Set an EFI variable, but only if it differs from the current value.
>+   Some firmware implementations are liable to fill up flash space if we set
>+   variables unnecessarily, so try to keep write activity to a minimum. */
>+static int
>+set_efi_variable (const char *name, struct efi_variable *entry)
>+{
>+  struct efi_variable *old = NULL;
>+  int rc = 0;
>+
>+  read_efi_variable (name, &old);
>+  efi_error_clear ();
>+  if (old && old->attributes == entry->attributes &&
>+      old->data_size == entry->data_size &&
>+      memcmp (old->data, entry->data, entry->data_size) == 0)
>+    grub_util_info ("skipping unnecessary update of EFI variable %s", name);
>+  else
>+    {
>+      rc = efi_set_variable (EFI_GLOBAL_GUID, name,
>+			     entry->data, entry->data_size, entry->attributes,
>+			     0644);
>+      if (rc < 0)
>+	grub_util_warn (_("Cannot set EFI variable %s"), name);
>+    }
>+  free_efi_variable (old);
>+  return rc;
>+}
>+
>+static int
>+cmpvarbyname (const void *p1, const void *p2)
>+{
>+  const struct efi_variable *var1 = *(const struct efi_variable **)p1;
>+  const struct efi_variable *var2 = *(const struct efi_variable **)p2;
>+  return strcmp (var1->name, var2->name);
>+}
>+
>+static int
>+read_boot_variables (struct efi_variable **varlist)
>+{
>+  int rc;
>+  efi_guid_t *guid = NULL;
>+  char *name = NULL;
>+  struct efi_variable **newlist = NULL;
>+  int nentries = 0;
>+  int i;
>+
>+  while ((rc = efi_get_next_variable_name (&guid, &name)) > 0)
>+    {
>+      const char *snum = name + sizeof ("Boot") - 1;
>+      struct efi_variable *var = NULL;
>+      unsigned int num;
>+
>+      if (memcmp (guid, &efi_guid_global, sizeof (efi_guid_global)) != 0 ||
>+	  strncmp (name, "Boot", sizeof ("Boot") - 1) != 0 ||
>+	  !grub_isxdigit (snum[0]) || !grub_isxdigit (snum[1]) ||
>+	  !grub_isxdigit (snum[2]) || !grub_isxdigit (snum[3]))
>+	continue;
>+
>+      rc = read_efi_variable (name, &var);
>+      if (rc < 0)
>+	break;
>+
>+      if (sscanf (var->name, "Boot%04X-%*s", &num) == 1 && num < 65536)
>+	var->num = num;
>+
>+      newlist = xrealloc (newlist, (++nentries) * sizeof (*newlist));
>+      newlist[nentries - 1] = var;
>+    }
>+  if (rc == 0 && newlist)
>+    {
>+      qsort (newlist, nentries, sizeof (*newlist), cmpvarbyname);
>+      for (i = nentries - 1; i >= 0; --i)
>+	grub_list_push (GRUB_AS_LIST_P (varlist), GRUB_AS_LIST (newlist[i]));
>+    }
>+  else if (newlist)
>+    {
>+      for (i = 0; i < nentries; ++i)
>+	free (newlist[i]);
>+      free (newlist);
>+    }
>+  return rc;
>+}
>+
>+static void
>+remove_from_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  uint16_t *data;
>+  unsigned int old_i, new_i;
>+
>+  /* We've got an array (in order->data) of the order.  Squeeze out any
>+     instance of the entry we're deleting by shifting the remainder down.  */
>+  data = (uint16_t *) order->data;
>+
>+  for (old_i = 0, new_i = 0;
>+       old_i < order->data_size / sizeof (uint16_t);
>+       ++old_i)
>+    {
>+      if (data[old_i] != num) {
>+	if (new_i != old_i)
>+	  data[new_i] = data[old_i];
>+	new_i++;
>+      }
>+    }
>+
>+  order->data_size = sizeof (data[0]) * new_i;
>+}
>+
>+static void
>+add_to_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  uint16_t *data;
>+  int i;
>+  size_t new_data_size;
>+  uint16_t *new_data;
>+
>+  /* Check whether this entry is already in the boot order.  If it is, leave
>+     it alone.  */
>+  data = (uint16_t *) order->data;
>+  for (i = 0; i < order->data_size / sizeof (uint16_t); ++i)
>+    if (data[i] == num)
>+      return;
>+
>+  new_data_size = order->data_size + sizeof (uint16_t);
>+  new_data = xmalloc (new_data_size);
>+  new_data[0] = num;
>+  memcpy (new_data + 1, order->data, order->data_size);
>+  free (order->data);
>+  order->data = (uint8_t *) new_data;
>+  order->data_size = new_data_size;
>+}
>+
>+static int
>+find_free_boot_num (struct efi_variable *entries)
>+{
>+  int num_vars = 0, i;
>+  struct efi_variable *entry;
>+
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    ++num_vars;
>+
>+  if (num_vars == 0)
>+    return 0;
>+
>+  /* O(n^2), but n is small and this is easy. */

Nod, fair point. Never seen more than a dozen or so boot entries on a
real system.

>+  for (i = 0; i < num_vars; ++i)
>+    {
>+      int found = 0;
>+      FOR_LIST_ELEMENTS (entry, entries)
>+	{
>+	  if (entry->num == i)
>+	    {
>+	      found = 1;
>+	      break;
>+	    }
>+	}
>+      if (!found)
>+	return i;
>+    }
>+
>+  return i;
>+}
>+
>+static int
>+get_edd_version (void)
>+{
>+  efi_guid_t blkx_guid = BLKX_UNKNOWN_GUID;
>+  uint8_t *data = NULL;
>+  size_t data_size = 0;
>+  uint32_t attributes;
>+  efidp_header *path;
>+  int rc;
>+
>+  rc = efi_get_variable (blkx_guid, "blk0", &data, &data_size, &attributes);
>+  if (rc < 0)
>+    return rc;
>+
>+  path = (efidp_header *) data;
>+  if (path->type == 2 && path->subtype == 1)
>+    return 3;
>+  return 1;
>+}
>+
>+static struct efi_variable *
>+make_boot_variable (int num, const char *disk, int part, const char *loader,
>+		    const char *label)
>+{
>+  struct efi_variable *entry = new_boot_variable ();
>+  uint32_t options;
>+  uint32_t edd10_devicenum;
>+  ssize_t dp_needed, loadopt_needed;
>+  efidp dp = NULL;
>+
>+  options = EFIBOOT_ABBREV_HD;
>+  switch (get_edd_version ()) {
>+    case 1:
>+      options = EFIBOOT_ABBREV_EDD10;
>+      break;
>+    case 3:
>+      options = EFIBOOT_ABBREV_NONE;
>+      break;
>+  }
>+
>+  /* This may not be the right disk; but it's probably only an issue on very
>+     old hardware anyway. */
>+  edd10_devicenum = 0x80;
>+
>+  dp_needed = efi_generate_file_device_path_from_esp (NULL, 0, disk, part,
>+						      loader, options,
>+						      edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  dp = xmalloc (dp_needed);
>+  dp_needed = efi_generate_file_device_path_from_esp ((uint8_t *) dp,
>+						      dp_needed, disk, part,
>+						      loader, options,
>+						      edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  loadopt_needed = efi_loadopt_create (NULL, 0, LOAD_OPTION_ACTIVE,
>+				       dp, dp_needed, (unsigned char *) label,
>+				       NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+  entry->data_size = loadopt_needed;
>+  entry->data = xmalloc (entry->data_size);
>+  loadopt_needed = efi_loadopt_create (entry->data, entry->data_size,
>+				       LOAD_OPTION_ACTIVE, dp, dp_needed,
>+				       (unsigned char *) label, NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+
>+  entry->name = xasprintf ("Boot%04X", num);
>+  entry->num = num;
>+
>+  return entry;
>+
>+err:
>+  free_efi_variable (entry);
>+  free (dp);
>+  return NULL;
>+}
>+
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+				  const char *efifile_path,
>+				  const char *efi_distributor)
>+{
>+  const char *efidir_disk;
>+  int efidir_part;
>+  struct efi_variable *entries = NULL, *entry;
>+  struct efi_variable *order;
>+  int entry_num = -1;
>+  int rc;
>+
>+  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
>+  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
>+
>+#ifdef __linux__
>+  /*
>+   * Linux uses efivarfs (mounted on /sys/firmware/efi/efivars) to access the
>+   * EFI variable store. Some legacy systems may still use the deprecated
>+   * efivars interface (accessed through /sys/firmware/efi/vars). Where both
>+   * are present, libefivar will use the former in preference, so attempting
>+   * to load efivars will not interfere with later operations.
>+   */
>+  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL },
>+			       NULL, NULL, "/dev/null");
>+#endif
>+
>+  if (!efi_variables_supported ())
>+    {
>+      grub_util_warn ("%s",
>+		      _("EFI variables are not supported on this system."));
>+      /* Let the user continue.  Perhaps they can still arrange to boot GRUB
>+         manually.  */
>+      return 0;
>+    }
>+
>+  rc = read_boot_variables (&entries);
>+  if (rc < 0)
>+    {
>+      grub_util_warn ("%s", _("Cannot read EFI Boot* variables"));
>+      goto err;
>+    }
>+  rc = read_efi_variable ("BootOrder", &order);
>+  if (rc < 0)
>+    {
>+      order = new_boot_variable ();
>+      order->name = xstrdup ("BootOrder");
>+      efi_error_clear ();
>+    }
>+
>+  /* Delete old entries from the same distributor.  */
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    {
>+      efi_load_option *load_option = (efi_load_option *) entry->data;
>+      const char *label;
>+
>+      if (entry->num < 0)
>+	continue;
>+      label = (const char *) efi_loadopt_desc (load_option, entry->data_size);
>+      if (strcasecmp (label, efi_distributor) != 0)
>+	continue;
>+
>+      /* To avoid problems with some firmware implementations, reuse the first
>+         matching variable we find rather than deleting and recreating it.  */
>+      if (entry_num == -1)
>+	entry_num = entry->num;
>+      else
>+	{
>+	  grub_util_info ("deleting superfluous EFI variable %s (%s)",
>+			  entry->name, label);
>+	  rc = efi_del_variable (EFI_GLOBAL_GUID, entry->name);
>+	  if (rc < 0)
>+	    {
>+	      grub_util_warn (_("Cannot delete EFI variable %s"), entry->name);
>+	      goto err;
>+	    }
>+	}
>+
>+      remove_from_boot_order (order, (uint16_t) entry->num);
>+    }
>+
>+  if (entry_num == -1)
>+    entry_num = find_free_boot_num (entries);
>+  entry = make_boot_variable (entry_num, efidir_disk, efidir_part,
>+			      efifile_path, efi_distributor);
>+  if (!entry)
>+    goto err;
>+
>+  grub_util_info ("setting EFI variable %s", entry->name);
>+  rc = set_efi_variable (entry->name, entry);
>+  if (rc < 0)
>+    goto err;
>+
>+  add_to_boot_order (order, (uint16_t) entry_num);
>+
>+  grub_util_info ("setting EFI variable BootOrder");
>+  rc = set_efi_variable ("BootOrder", order);
>+  if (rc < 0)
>+    goto err;
>+
>+  return 0;
>+
>+err:
>+  show_efi_errors ();
>+  return errno;
>+}
>+
>+#endif /* HAVE_EFIVAR */
>diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
>index 55b8f4016..9d4530a38 100644
>--- a/grub-core/osdep/unix/platform.c
>+++ b/grub-core/osdep/unix/platform.c
>@@ -19,15 +19,12 @@
> #include <config.h>
> 
> #include <grub/util/install.h>
>-#include <grub/emu/hostdisk.h>
> #include <grub/util/misc.h>
> #include <grub/misc.h>
> #include <grub/i18n.h>
> #include <grub/emu/exec.h>
> #include <sys/types.h>
>-#include <dirent.h>
> #include <string.h>
>-#include <errno.h>
> 
> static char *
> get_ofpathname (const char *dev)
>@@ -78,102 +75,19 @@ get_ofpathname (const char *dev)
> 		   dev);
> }
> 
>-static int
>-grub_install_remove_efi_entries_by_distributor (const char *efi_distributor)
>-{
>-  int fd;
>-  pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd);
>-  char *line = NULL;
>-  size_t len = 0;
>-  int rc = 0;
>-
>-  if (!pid)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-		      "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  FILE *fp = fdopen (fd, "r");
>-  if (!fp)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-		      "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  line = xmalloc (80);
>-  len = 80;
>-  while (1)
>-    {
>-      int ret;
>-      char *bootnum;
>-      ret = getline (&line, &len, fp);
>-      if (ret == -1)
>-	break;
>-      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>-	  || line[sizeof ("Boot") - 1] < '0'
>-	  || line[sizeof ("Boot") - 1] > '9')
>-	continue;
>-      if (!strcasestr (line, efi_distributor))
>-	continue;
>-      bootnum = line + sizeof ("Boot") - 1;
>-      bootnum[4] = '\0';
>-      if (!verbosity)
>-	rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-	      "-b", bootnum,  "-B", NULL });
>-      else
>-	rc = grub_util_exec ((const char * []){ "efibootmgr",
>-	      "-b", bootnum, "-B", NULL });
>-    }
>-
>-  free (line);
>-  return rc;
>-}
>-
> int
> grub_install_register_efi (grub_device_t efidir_grub_dev,
> 			   const char *efifile_path,
> 			   const char *efi_distributor)
> {
>-  const char * efidir_disk;
>-  int efidir_part;
>-  int ret;
>-  efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk);
>-  efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1;
>-
>-  if (grub_util_exec_redirect_null ((const char * []){ "efibootmgr", "--version", NULL }))
>-    {
>-      /* TRANSLATORS: This message is shown when required executable `%s'
>-	 isn't found.  */
>-      grub_util_error (_("%s: not found"), "efibootmgr");
>-    }
>-
>-  /* On Linux, we need the efivars kernel modules.  */
>-#ifdef __linux__
>-  grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>+#ifdef HAVE_EFIVAR
>+  return grub_install_efivar_register_efi (efidir_grub_dev, efifile_path,
>+					   efi_distributor);
>+#else
>+  grub_util_error ("%s",
>+		   _("GRUB was not built with efivar support; "
>+		     "cannot register EFI boot entry"));
> #endif
>-  /* Delete old entries from the same distributor.  */
>-  ret = grub_install_remove_efi_entries_by_distributor (efi_distributor);
>-  if (ret)
>-    return ret;
>-
>-  char *efidir_part_str = xasprintf ("%d", efidir_part);
>-
>-  if (!verbosity)
>-    ret = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-	  "-c", "-d", efidir_disk,
>-	  "-p", efidir_part_str, "-w",
>-	  "-L", efi_distributor, "-l", 
>-	  efifile_path, NULL });
>-  else
>-    ret = grub_util_exec ((const char * []){ "efibootmgr",
>-	  "-c", "-d", efidir_disk,
>-	  "-p", efidir_part_str, "-w",
>-	  "-L", efi_distributor, "-l", 
>-	  efifile_path, NULL });
>-  free (efidir_part_str);
>-  return ret;
> }
> 
> void
>diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>index 2631b1074..dcad16ac5 100644
>--- a/include/grub/util/install.h
>+++ b/include/grub/util/install.h
>@@ -216,6 +216,11 @@ grub_install_get_default_arm_platform (void);
> const char *
> grub_install_get_default_x86_platform (void);
> 
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+				  const char *efifile_path,
>+				  const char *efi_distributor);
>+
> int
> grub_install_register_efi (grub_device_t efidir_grub_dev,
> 			   const char *efifile_path,
>diff --git a/util/grub-install.c b/util/grub-install.c
>index 264f9ecdc..a61df056e 100644
>--- a/util/grub-install.c
>+++ b/util/grub-install.c
>@@ -1885,7 +1885,7 @@ main (int argc, char *argv[])
> 					       "\\System\\Library\\CoreServices",
> 					       efi_distributor);
> 	      if (ret)
>-	        grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
>+	        grub_util_error (_("failed to register the EFI boot entry: %s"),
> 				 strerror (ret));
> 	    }
> 
>@@ -1929,7 +1929,7 @@ main (int argc, char *argv[])
> 	  ret = grub_install_register_efi (efidir_grub_dev,
> 					   efifile_path, efi_distributor);
> 	  if (ret)
>-	    grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
>+	    grub_util_error (_("failed to register the EFI boot entry: %s"),
> 			     strerror (ret));
> 	}
>       break;
>-- 
>2.17.1
>
-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"This dress doesn't reverse." -- Alden Spiess

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Daniel Kiper]

On Mon, Mar 11, 2019 at 03:04:49PM +0000, Colin Watson wrote:
> Some UEFI firmware is easily provoked into running out of space in its
> variable storage.  This is usually due to certain kernel drivers (e.g.
> pstore), but regardless of the cause it can cause grub-install to fail
> because it currently asks efibootmgr to delete and re-add entries, and
> the deletion often doesn't result in an immediate garbage collection.
> Writing variables frequently also increases wear on the NVRAM which may
> have limited write cycles.  For these reasons, it's desirable to find a
> way to minimise writes while still allowing grub-install to ensure that
> a suitable boot entry exists.
>
> This short patch series does so by using the efivar and efiboot
> libraries directly.

This looks like something for longer discussion. So, I am going to take
the patchset after the release if you do not convince me that it should
land in the GRUB 2.04.

And I agree with Steve comments.

Daniel

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Colin Watson]

On Wed, Mar 13, 2019 at 10:56:47AM +0100, Daniel Kiper wrote:
> This looks like something for longer discussion. So, I am going to take
> the patchset after the release if you do not convince me that it should
> land in the GRUB 2.04.

While I think it's important and I expect to be applying some variation
of it to Debian for our upcoming release, it's also not in any way a new
issue, so I think it's more important to get more timely and frequent
GRUB releases happening than to push for this particular patch set to be
in 2.04.

> And I agree with Steve comments.

Yep - I'll deal with those soon and post a v2.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Daniel Kiper]

On Wed, Mar 13, 2019 at 10:12:26AM +0000, Colin Watson wrote:
> On Wed, Mar 13, 2019 at 10:56:47AM +0100, Daniel Kiper wrote:
> > This looks like something for longer discussion. So, I am going to take
> > the patchset after the release if you do not convince me that it should
> > land in the GRUB 2.04.
>
> While I think it's important and I expect to be applying some variation
> of it to Debian for our upcoming release, it's also not in any way a new
> issue, so I think it's more important to get more timely and frequent
> GRUB releases happening than to push for this particular patch set to be
> in 2.04.

So, after the release. Granted!

> > And I agree with Steve comments.
>
> Yep - I'll deal with those soon and post a v2.

Thanks!

Daniel

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Colin Watson]

On Wed, Mar 13, 2019 at 01:07:20AM +0000, Steve McIntyre wrote:
> On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
> >+/* Boot option attributes.  */
> >+#define LOAD_OPTION_ACTIVE 0x00000001
> >+
> >+/* GUIDs.  */
> >+#define BLKX_UNKNOWN_GUID \
> >+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
> >+	    0x72, 0x3b)
> 
> Ugh. I'm assuming the mahic numbers here are not exposed usefully by
> efivar or efiboot?

Sadly not, as far as I can see.

> So new_efi_variable() is using xmalloc() so it's safe if the
> allocation fails. But what happens if efi_get_variable() fails - do
> you need to free all the members by calling free_efi_variable() rather
> than simply free() here?

Quite right, and there was a similar bug in another place.  I'll send a
v2 fixing this and your comment about my grub_lltoa change.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 2/2] Minimise writes to EFI variable storage

[Steve McIntyre]

On Fri, Mar 22, 2019 at 11:29:15PM +0000, Colin Watson wrote:
>On Wed, Mar 13, 2019 at 01:07:20AM +0000, Steve McIntyre wrote:
>> On Mon, Mar 11, 2019 at 03:05:46PM +0000, Colin Watson wrote:
>> >+/* Boot option attributes.  */
>> >+#define LOAD_OPTION_ACTIVE 0x00000001
>> >+
>> >+/* GUIDs.  */
>> >+#define BLKX_UNKNOWN_GUID \
>> >+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
>> >+	    0x72, 0x3b)
>> 
>> Ugh. I'm assuming the mahic numbers here are not exposed usefully by
>> efivar or efiboot?
>
>Sadly not, as far as I can see.

:-( That sounds like a clear bug, then. IMHO it's clearly part of the
interface that they should be providing.

>> So new_efi_variable() is using xmalloc() so it's safe if the
>> allocation fails. But what happens if efi_get_variable() fails - do
>> you need to free all the members by calling free_efi_variable() rather
>> than simply free() here?
>
>Quite right, and there was a similar bug in another place.  I'll send a
>v2 fixing this and your comment about my grub_lltoa change.

ACK, and then a v3. Will look again shortly...

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
Welcome my son, welcome to the machine.

[PATCH 0/2] *** SUBJECT HERE ***

[lixiaoyong]

*** BLURB HERE ***

lixiaoyong (2):
  utils.bbclass: enhance readelf command call with llvm
  oe/package.py: enhance objdump command call with llvm

 meta/classes-global/utils.bbclass | 4 ++--
 meta/lib/oe/package.py            | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 0/2] *** SUBJECT HERE ***

[Sergey Lisov]

DesignWare MMC cores have a configurable data bus width of either 16, 32, or 64
bytes. It is possible, and some vendors actually do it, to ship a DW MMC core
configured for 32-bit data bus within a 64-bit SoC. In this case the kernel
will attempt 64-bit (readq) accesses to certain 64-bit MMIO registers, while
the core will expect pairs of 32-bit accesses.

It seems that currently the only register for which the kernel performs 64-bit
accesses is the FIFO. The symptom is that the DW MMC core never receives a read
on the second half of the register, does not register the datum as being read,
and thus not advancing its internal FIFO pointer, breaking further reads. It
also seems that this FIFO is only used for small (less than 16 bytes)
transfers, which probably means that only some SDIO cards are affected.

Sergey Lisov (2):
  devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property
  dw_mmc: add an option to force 32-bit accesses to 64-bit device
    registers

 .../bindings/mmc/synopsys-dw-mshc-common.yaml |   6 +
 drivers/mmc/host/dw_mmc.c                     | 125 +++++++++++++++++-
 drivers/mmc/host/dw_mmc.h                     |   2 +
 3 files changed, 131 insertions(+), 2 deletions(-)

-- 
2.38.3

[PATCH 0/2] *** SUBJECT HERE ***

[Sergey Lisov]

DesignWare MMC cores have a configurable data bus width of either 16, 32, or 64
bytes. It is possible, and some vendors actually do it, to ship a DW MMC core
configured for 32-bit data bus within a 64-bit SoC. In this case the kernel
will attempt 64-bit (readq) accesses to certain 64-bit MMIO registers, while
the core will expect pairs of 32-bit accesses.

It seems that currently the only register for which the kernel performs 64-bit
accesses is the FIFO. The symptom is that the DW MMC core never receives a read
on the second half of the register, does not register the datum as being read,
and thus not advancing its internal FIFO pointer, breaking further reads. It
also seems that this FIFO is only used for small (less than 16 bytes)
transfers, which probably means that only some SDIO cards are affected.

Sergey Lisov (2):
  devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property
  dw_mmc: add an option to force 32-bit accesses to 64-bit device
    registers

 .../bindings/mmc/synopsys-dw-mshc-common.yaml |   6 +
 drivers/mmc/host/dw_mmc.c                     | 125 +++++++++++++++++-
 drivers/mmc/host/dw_mmc.h                     |   2 +
 3 files changed, 131 insertions(+), 2 deletions(-)

-- 
2.38.3

[PATCH 0/2] *** SUBJECT HERE ***

[Sergey Lisov]

DesignWare MMC cores have a configurable data bus width of either 16, 32, or 64
bytes. It is possible, and some vendors actually do it, to ship a DW MMC core
configured for 32-bit data bus within a 64-bit SoC. In this case the kernel
will attempt 64-bit (readq) accesses to certain 64-bit MMIO registers, while
the core will expect pairs of 32-bit accesses.

It seems that currently the only register for which the kernel performs 64-bit
accesses is the FIFO. The symptom is that the DW MMC core never receives a read
on the second half of the register, does not register the datum as being read,
and thus not advancing its internal FIFO pointer, breaking further reads. It
also seems that this FIFO is only used for small (less than 16 bytes)
transfers, which probably means that only some SDIO cards are affected.

Sergey Lisov (2):
  devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property
  dw_mmc: add an option to force 32-bit accesses to 64-bit device
    registers

 .../bindings/mmc/synopsys-dw-mshc-common.yaml |   6 +
 drivers/mmc/host/dw_mmc.c                     | 125 +++++++++++++++++-
 drivers/mmc/host/dw_mmc.h                     |   2 +
 3 files changed, 131 insertions(+), 2 deletions(-)

-- 
2.38.3

[PATCH 0/2] *** SUBJECT HERE ***

[sieu.mun.tang]

From: Sieu Mun Tang <sieu.mun.tang@intel.com>

*** BLURB HERE ***

Tien Fong Chee (2):
  arch: arm: mach-socfpga: Use custom header target buffer in SPL
  arch: arm: mach-socfpga: Add SPL fitImage config match

 arch/arm/mach-socfpga/spl_a10.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

-- 
2.25.1

[PATCH 0/2] *** SUBJECT HERE ***

[Jit Loon Lim]

*** BLURB HERE ***

Chee Hong Ang (2):
  arm: socfpga: soc64: Enable L2 reset on S10
  arm: socfpga: soc64: Perform warm reset after L2 reset in SPL on S10

 .../include/mach/reset_manager_soc64.h        |  1 +
 arch/arm/mach-socfpga/lowlevel_init_soc64.S   | 24 ++++++++
 drivers/sysreset/sysreset_socfpga_soc64.c     | 58 ++++++++++++++++++-
 include/configs/socfpga_soc64_common.h        |  7 +++
 4 files changed, 88 insertions(+), 2 deletions(-)

-- 
2.26.2

Re: [PATCH 0/2] *** SUBJECT HERE ***

[SAURAV GIREPUNJE]

On Fri, May 07, 2021 at 08:00:29AM +0200, Greg KH wrote:
> On Fri, May 07, 2021 at 10:06:17AM +0530, Saurav Girepunje wrote:
> > *** BLURB HERE ***
>
> No subject or blurb?
>
> >
> > Saurav Girepunje (2):
> >   usb: musb: remove unused function argument
> >   usb: musb: Remove unused function argument
>
> Again, these have the same subject line, which is not allowed.
>
> Please fix up and resend them all (we only saw 1 on the mailing list.)
>
> thanks,
>
> greg k-h


Plese ignore this mail.

I was trying to learn how to send pathes with cover letter from below
https://kernelnewbies.org/FirstKernelPatch

and did not realized that on sample command set the cc list
git format-patch -o /tmp/ --cover-letter -n --thread=shallow --cc="linux-usb@vger.kernel.org" 3b12c21^..b7ca36a

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg KH]

On Fri, May 07, 2021 at 10:06:17AM +0530, Saurav Girepunje wrote:
> *** BLURB HERE ***

No subject or blurb?

> 
> Saurav Girepunje (2):
>   usb: musb: remove unused function argument
>   usb: musb: Remove unused function argument

Again, these have the same subject line, which is not allowed.

Please fix up and resend them all (we only saw 1 on the mailing list.)

thanks,

greg k-h

[PATCH 0/2] *** SUBJECT HERE ***

[Saurav Girepunje]

*** BLURB HERE ***

Saurav Girepunje (2):
  usb: musb: remove unused function argument
  usb: musb: Remove unused function argument

 drivers/usb/musb/musb_host.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

--
2.25.1

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg Kroah-Hartman]

On Fri, Apr 16, 2021 at 04:07:54PM +0800, Tao Zhang wrote:
> *** BLURB HERE ***

Where is the blurb?

And your subject is not ok :(

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg Kroah-Hartman]

On Fri, Apr 16, 2021 at 04:07:54PM +0800, Tao Zhang wrote:
> *** BLURB HERE ***

Where is the blurb?

And your subject is not ok :(


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/2] *** SUBJECT HERE ***

[Tao Zhang]

*** BLURB HERE ***

Tao Zhang (2):
  coresight: Add support for device names
  dt-bindings: arm: add property for coresight component name

 Documentation/devicetree/bindings/arm/coresight.txt | 2 ++
 drivers/hwtracing/coresight/coresight-core.c        | 6 ++++++
 2 files changed, 8 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 0/2] *** SUBJECT HERE ***

[Andrew Pinski]

*** BLURB HERE ***

Andrew Pinski (2):
  ARM64:VDSO: Improve gettimeofday, don't use udiv
  ARM64:VDSO: Improve __do_get_tspec, don't use udiv

 arch/arm64/kernel/vdso/gettimeofday.S |   47 ++++++++++++++++++++++++--------
 1 files changed, 35 insertions(+), 12 deletions(-)

-- 
1.7.2.5

[PATCH 0/2] *** SUBJECT HERE ***

[Andrew Pinski]

*** BLURB HERE ***

Andrew Pinski (2):
  ARM64:VDSO: Improve gettimeofday, don't use udiv
  ARM64:VDSO: Improve __do_get_tspec, don't use udiv

 arch/arm64/kernel/vdso/gettimeofday.S |   47 ++++++++++++++++++++++++--------
 1 files changed, 35 insertions(+), 12 deletions(-)

-- 
1.7.2.5

[PATCH 0/2] *** SUBJECT HERE ***

[Claire Murphy]

*** BLURB HERE ***

Claire Murphy (2):
  Patch for Qemu wrapper for US-VHost to ensure Qemu process ends when
    VM is shutdown.
  Patch to allow live migration of a VM with US-VHost.

 examples/vhost/libvirt/qemu-wrap.py |   31 +++++++++++++++++++++++++++----
 examples/vhost/vhost-net-cdev.c     |   18 ++++++++++++++++++
 examples/vhost/virtio-net.c         |    8 +++++++-
 3 files changed, 52 insertions(+), 5 deletions(-)

[PATCH 0/2] *** SUBJECT HERE ***

[Chris Zankel]

Hi,

These two patches fix two of the broken builds for Xtensa:
- coreutils
- cdrkit

The first patch enables valloc for recent versions of uClibc (snapshot), which
is still used by cdrkit. The second patch disables libnsrp for Xtensa
(!BR_xtensa), which is used by libnss, and consequently libnss and ecryptfs.

Thanks,
-Chris

Chris Zankel (2):
  uclibc-snapshot: enable option UCLIBC_SUSV2_LEGACY
  libnspr: Add dependency on !BR2_xtensa

 package/ecryptfs-utils/Config.in      | 1 +
 package/libnspr/Config.in             | 3 ++-
 package/libnss/Config.in              | 1 +
 package/uclibc/uClibc-snapshot.config | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

-- 
1.8.1.2

[PATCH 0/2] *** SUBJECT HERE ***

[Damien Millescamps]

*** BLURB HERE ***

Damien Millescamps (2):
  eal: add flag to force unbind device
  eal: load libraries before creating threads

 lib/librte_eal/common/include/rte_pci.h |    2 ++
 lib/librte_eal/linuxapp/eal/eal.c       |   24 ++++++++++++------------
 lib/librte_eal/linuxapp/eal/eal_pci.c   |    5 +++++
 3 files changed, 19 insertions(+), 12 deletions(-)

-- 
1.7.2.5

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Johan Hovold]

On Tue, Jul 02, 2013 at 01:22:01AM +0200, Anders Hammarquist wrote:
> In a message of Fri, 28 Jun 2013 12:23:33 +0200, Johan Hovold writes:
> >> I did a quick check of adding the device id though sysfs, and although
> >> it partly works, it doesn't find the correct firmware (it ends up trying
> >> to load 5052 firmware for a 3410 device. Looking at the code it seems
> >> (struct ti_device) td_is_3410 isn't set properly.)
> >
> >Turns out that the drivers device-type detection has never worked with
> >the dynamic id interface (all devices were detected as 2-port devices).
> >
> >I'm responding to this mail with a fix. Care to give it a try?
> 
> Yes, this works fine. 

Thanks for testing.

Johan

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

In a message of Fri, 28 Jun 2013 12:23:33 +0200, Johan Hovold writes:
>> I did a quick check of adding the device id though sysfs, and although
>> it partly works, it doesn't find the correct firmware (it ends up trying
>> to load 5052 firmware for a 3410 device. Looking at the code it seems
>> (struct ti_device) td_is_3410 isn't set properly.)
>
>Turns out that the drivers device-type detection has never worked with
>the dynamic id interface (all devices were detected as 2-port devices).
>
>I'm responding to this mail with a fix. Care to give it a try?

Yes, this works fine. 

/Anders

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Johan Hovold]

On Thu, Jun 27, 2013 at 11:50:52PM +0200, Anders Hammarquist wrote:
> In a message of Wed, 26 Jun 2013 12:39:24 +0200, Johan Hovold writes:
> >On Wed, Jun 26, 2013 at 10:29:59AM +0200, Anders Hammarquist wrote:
> >> In a message of Tue, 25 Jun 2013 16:39:11 -0700, Greg KH writes:
> >> >> Indeed. I'd already had some (failed) thoughts about how to handle it
> >> >> nicely. Now I've had another think through, and I have something which
> >> >> deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
> >> >> without changing the initializer. Patch 2/2
> >> >
> >> >Why don't we just drop the extra id thing entirely?  The usb-serial
> >> >subsystem handles new device ids being added dynamically from sysfs for
> >> >a long time now.  Removing this module option would clean up the code a
> >> >lot, and prevent these errors from ever happening again.
> >> 
> >> Aha, yes, I'm all for that (had I only known I'd have done that to start
> >> with). I'll look in to it.
> >
> >I already have a few patches here (part of a larger 3.11 clean-up series)
> >which removes the vid/pid module parameters from all usb-serial modules
> >including ti_usb_3410_5052.
> >
> >I hope to be able to submit the whole series a later tonight, but here's
> >the ti_usb_3410_5052 part if anyone's interested.
> 
> I did a quick check of adding the device id though sysfs, and although
> it partly works, it doesn't find the correct firmware (it ends up trying
> to load 5052 firmware for a 3410 device. Looking at the code it seems
> (struct ti_device) td_is_3410 isn't set properly.)

Turns out that the drivers device-type detection has never worked with
the dynamic id interface (all devices were detected as 2-port devices).

I'm responding to this mail with a fix. Care to give it a try?

Thanks,
Johan

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

In a message of Wed, 26 Jun 2013 12:39:24 +0200, Johan Hovold writes:
>On Wed, Jun 26, 2013 at 10:29:59AM +0200, Anders Hammarquist wrote:
>> In a message of Tue, 25 Jun 2013 16:39:11 -0700, Greg KH writes:
>> >> Indeed. I'd already had some (failed) thoughts about how to handle it
>> >> nicely. Now I've had another think through, and I have something which
>> >> deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
>> >> without changing the initializer. Patch 2/2
>> >
>> >Why don't we just drop the extra id thing entirely?  The usb-serial
>> >subsystem handles new device ids being added dynamically from sysfs for
>> >a long time now.  Removing this module option would clean up the code a
>> >lot, and prevent these errors from ever happening again.
>> 
>> Aha, yes, I'm all for that (had I only known I'd have done that to start
>> with). I'll look in to it.
>
>I already have a few patches here (part of a larger 3.11 clean-up series)
>which removes the vid/pid module parameters from all usb-serial modules
>including ti_usb_3410_5052.
>
>I hope to be able to submit the whole series a later tonight, but here's
>the ti_usb_3410_5052 part if anyone's interested.

I did a quick check of adding the device id though sysfs, and although
it partly works, it doesn't find the correct firmware (it ends up trying
to load 5052 firmware for a 3410 device. Looking at the code it seems
(struct ti_device) td_is_3410 isn't set properly.)

I can take a stab at fixing it in the next few days.

/Anders

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Johan Hovold]

On Wed, Jun 26, 2013 at 10:29:59AM +0200, Anders Hammarquist wrote:
> In a message of Tue, 25 Jun 2013 16:39:11 -0700, Greg KH writes:
> >> Indeed. I'd already had some (failed) thoughts about how to handle it
> >> nicely. Now I've had another think through, and I have something which
> >> deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
> >> without changing the initializer. Patch 2/2
> >
> >Why don't we just drop the extra id thing entirely?  The usb-serial
> >subsystem handles new device ids being added dynamically from sysfs for
> >a long time now.  Removing this module option would clean up the code a
> >lot, and prevent these errors from ever happening again.
> 
> Aha, yes, I'm all for that (had I only known I'd have done that to start
> with). I'll look in to it.

I already have a few patches here (part of a larger 3.11 clean-up series)
which removes the vid/pid module parameters from all usb-serial modules
including ti_usb_3410_5052.

I hope to be able to submit the whole series a later tonight, but here's
the ti_usb_3410_5052 part if anyone's interested.

Thanks,
Johan


From: Johan Hovold <jhovold@gmail.com>
Subject: [PATCH] USB: ti_usb_3410_5052: remove vendor/product module parameters

Remove the vendor and product module parameters which were added a long
time ago when we did not have the dynamic sysfs interface to add
new device ids (and which isn't limited to five new vid/pid pair).

A vid/pid pair can be added dynamically using sysfs, for example:

  echo 0451 1234 >/sys/bus/usb-serial/drivers/ti_usb_3410_5052_1/new_id

for 1-port adapters, or

  echo 0451 1234 >/sys/bus/usb-serial/drivers/ti_usb_3410_5052_2/new_id

for 2-port adapters.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 72 ++++-------------------------------
 1 file changed, 7 insertions(+), 65 deletions(-)

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index f3e21f5..5585b20 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -141,20 +141,9 @@ static int ti_download_firmware(struct ti_device *tdev);
 
 /* module parameters */
 static int closing_wait = TI_DEFAULT_CLOSING_WAIT;
-static ushort vendor_3410[TI_EXTRA_VID_PID_COUNT];
-static unsigned int vendor_3410_count;
-static ushort product_3410[TI_EXTRA_VID_PID_COUNT];
-static unsigned int product_3410_count;
-static ushort vendor_5052[TI_EXTRA_VID_PID_COUNT];
-static unsigned int vendor_5052_count;
-static ushort product_5052[TI_EXTRA_VID_PID_COUNT];
-static unsigned int product_5052_count;
 
 /* supported devices */
-/* the array dimension is the number of default entries plus */
-/* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
-/* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },
@@ -171,16 +160,18 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
+	{ }	/* terminator */
 };
 
-static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_5052[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5152_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_EEPROM_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
+	{ }	/* terminator */
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },
@@ -200,7 +191,7 @@ static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
-	{ }
+	{ }	/* terminator */
 };
 
 static struct usb_serial_driver ti_1port_device = {
@@ -289,61 +280,12 @@ module_param(closing_wait, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(closing_wait,
     "Maximum wait for data to drain in close, in .01 secs, default is 4000");
 
-module_param_array(vendor_3410, ushort, &vendor_3410_count, S_IRUGO);
-MODULE_PARM_DESC(vendor_3410,
-		"Vendor ids for 3410 based devices, 1-5 short integers");
-module_param_array(product_3410, ushort, &product_3410_count, S_IRUGO);
-MODULE_PARM_DESC(product_3410,
-		"Product ids for 3410 based devices, 1-5 short integers");
-module_param_array(vendor_5052, ushort, &vendor_5052_count, S_IRUGO);
-MODULE_PARM_DESC(vendor_5052,
-		"Vendor ids for 5052 based devices, 1-5 short integers");
-module_param_array(product_5052, ushort, &product_5052_count, S_IRUGO);
-MODULE_PARM_DESC(product_5052,
-		"Product ids for 5052 based devices, 1-5 short integers");
-
 MODULE_DEVICE_TABLE(usb, ti_id_table_combined);
 
+module_usb_serial_driver(serial_drivers, ti_id_table_combined);
 
 /* Functions */
 
-static int __init ti_init(void)
-{
-	int i, j, c;
-
-	/* insert extra vendor and product ids */
-	c = ARRAY_SIZE(ti_id_table_combined) - 2 * TI_EXTRA_VID_PID_COUNT - 1;
-	j = ARRAY_SIZE(ti_id_table_3410) - TI_EXTRA_VID_PID_COUNT - 1;
-	for (i = 0; i < min(vendor_3410_count, product_3410_count); i++, j++, c++) {
-		ti_id_table_3410[j].idVendor = vendor_3410[i];
-		ti_id_table_3410[j].idProduct = product_3410[i];
-		ti_id_table_3410[j].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-		ti_id_table_combined[c].idVendor = vendor_3410[i];
-		ti_id_table_combined[c].idProduct = product_3410[i];
-		ti_id_table_combined[c].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-	}
-	j = ARRAY_SIZE(ti_id_table_5052) - TI_EXTRA_VID_PID_COUNT - 1;
-	for (i = 0; i < min(vendor_5052_count, product_5052_count); i++, j++, c++) {
-		ti_id_table_5052[j].idVendor = vendor_5052[i];
-		ti_id_table_5052[j].idProduct = product_5052[i];
-		ti_id_table_5052[j].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-		ti_id_table_combined[c].idVendor = vendor_5052[i];
-		ti_id_table_combined[c].idProduct = product_5052[i];
-		ti_id_table_combined[c].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-	}
-
-	return usb_serial_register_drivers(serial_drivers, KBUILD_MODNAME, ti_id_table_combined);
-}
-
-static void __exit ti_exit(void)
-{
-	usb_serial_deregister_drivers(serial_drivers);
-}
-
-module_init(ti_init);
-module_exit(ti_exit);
-
-
 static int ti_startup(struct usb_serial *serial)
 {
 	struct ti_device *tdev;
-- 
1.8.2.1

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

In a message of Tue, 25 Jun 2013 16:39:11 -0700, Greg KH writes:
>> Indeed. I'd already had some (failed) thoughts about how to handle it
>> nicely. Now I've had another think through, and I have something which
>> deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
>> without changing the initializer. Patch 2/2
>
>Why don't we just drop the extra id thing entirely?  The usb-serial
>subsystem handles new device ids being added dynamically from sysfs for
>a long time now.  Removing this module option would clean up the code a
>lot, and prevent these errors from ever happening again.

Aha, yes, I'm all for that (had I only known I'd have done that to start
with). I'll look in to it.

/Anders

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg KH]

On Sat, Jun 22, 2013 at 08:54:43PM +0200, Anders Hammarquist wrote:
> In a message of Fri, 21 Jun 2013 16:56:03 -0700, Greg KH writes:
> >Please resend this in a format that I can apply it in (i.e. one that
> >does not require me to edit it by hand...)
> 
> After more fighting with git, I belive I now made it spit out what I
> wanted. Patch 1/2 ahead.
> 
> >> -static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
> >> +static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
> >
> >That's a mess, why have it be a static array at all?  Just include an
> >empty one at the end.
> 
> Indeed. I'd already had some (failed) thoughts about how to handle it
> nicely. Now I've had another think through, and I have something which
> deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
> without changing the initializer. Patch 2/2

Why don't we just drop the extra id thing entirely?  The usb-serial
subsystem handles new device ids being added dynamically from sysfs for
a long time now.  Removing this module option would clean up the code a
lot, and prevent these errors from ever happening again.

thanks,

greg k-h

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

In a message of Fri, 21 Jun 2013 16:56:03 -0700, Greg KH writes:
>Please resend this in a format that I can apply it in (i.e. one that
>does not require me to edit it by hand...)

After more fighting with git, I belive I now made it spit out what I
wanted. Patch 1/2 ahead.

>> -static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
>> +static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
>
>That's a mess, why have it be a static array at all?  Just include an
>empty one at the end.

Indeed. I'd already had some (failed) thoughts about how to handle it
nicely. Now I've had another think through, and I have something which
deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed
without changing the initializer. Patch 2/2

/Anders

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg KH]

On Sat, Jun 22, 2013 at 01:08:12AM +0200, Anders Hammarquist wrote:
> In a message of Wed, 19 Jun 2013 15:53:15 -0700, Greg KH writes:
> >I think you forgot a Subject :)
> 
> Indeed. I blame it on my first fight with git format-patch ;-)
> 
> >On Wed, Jun 19, 2013 at 02:05:07AM +0200, Anders Hammarquist wrote:
> >> This patch set adds the product id to the driver, and makes the
> >> product type more explicit. Arguably, the ABBOTT_PRODUCT_ID
> >> define could be removed, but I left it on the off chance that
> >> someone other that the TI 3410 driver uses it.
> >
> >No, it doesn't, please fix up that last patch and resend it.
> 
> Right, and in removing it I actually found that it was used in
> two places in the driver. I lost my second fight with format-patch
> so I'll just enclose the fixed patch below.
> 
> /Anders
> 
> ---8<---
> ti_usb_3410_5052:
> * Remove unspecific ABBOTT_PRODUCT_ID
> * Fix size of statically sized arrays
> 
> Signed-off-by: Anders Hammarquist <iko@iko.pp.se>

Please resend this in a format that I can apply it in (i.e. one that
does not require me to edit it by hand...)

Also, please cc: the linux-usb@vger.kernel.org mailing list for usb
patches.

> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index e581c25..26c1161 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -158,7 +158,7 @@ static unsigned int product_5052_count;
>  /* the array dimension is the number of default entries plus */
>  /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
>  /* null entry */
> -static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
> +static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {

That's a mess, why have it be a static array at all?  Just include an
empty one at the end.


>  	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
>  	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
>  	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },
> @@ -172,8 +172,8 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
>  	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
>  	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
>  	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
> -	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
> -	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
> +	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
> +	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
>  	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
>  };
>  
> @@ -184,7 +184,7 @@ static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
>  	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
>  };
>  
> -static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
> +static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {

Same here.  Although that might take another patch, to handle it all
correctly, separate from this "change the device id names" patch.

thanks,

greg k-h

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

In a message of Wed, 19 Jun 2013 15:53:15 -0700, Greg KH writes:
>I think you forgot a Subject :)

Indeed. I blame it on my first fight with git format-patch ;-)

>On Wed, Jun 19, 2013 at 02:05:07AM +0200, Anders Hammarquist wrote:
>> This patch set adds the product id to the driver, and makes the
>> product type more explicit. Arguably, the ABBOTT_PRODUCT_ID
>> define could be removed, but I left it on the off chance that
>> someone other that the TI 3410 driver uses it.
>
>No, it doesn't, please fix up that last patch and resend it.

Right, and in removing it I actually found that it was used in
two places in the driver. I lost my second fight with format-patch
so I'll just enclose the fixed patch below.

/Anders

---8<---
ti_usb_3410_5052:
* Remove unspecific ABBOTT_PRODUCT_ID
* Fix size of statically sized arrays

Signed-off-by: Anders Hammarquist <iko@iko.pp.se>

diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index e581c25..26c1161 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -158,7 +158,7 @@ static unsigned int product_5052_count;
 /* the array dimension is the number of default entries plus */
 /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
 /* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },
@@ -172,8 +172,8 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 };
 
@@ -184,7 +184,7 @@ static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },
@@ -202,7 +202,8 @@ static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 	{ }
 };
diff --git a/drivers/usb/serial/ti_usb_3410_5052.h b/drivers/usb/serial/ti_usb_3410_5052.h
index 4a2423e..d3ff470 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.h
+++ b/drivers/usb/serial/ti_usb_3410_5052.h
@@ -52,9 +52,8 @@
 
 /* Abbott Diabetics vendor and product ids */
 #define ABBOTT_VENDOR_ID		0x1a61
-#define ABBOTT_STEREO_PLUG_ID		0x3410
-#define ABBOTT_PRODUCT_ID		ABBOTT_STEREO_PLUG_ID
-#define ABBOTT_STRIP_PORT_ID		0x3420
+#define ABBOTT_STEREO_PLUG_PRODUCT_ID	0x3410
+#define ABBOTT_STRIP_PORT_PRODUCT_ID	0x3420
 
 /* Commands */
 #define TI_GET_VERSION			0x01

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg KH]

I think you forgot a Subject :)

On Wed, Jun 19, 2013 at 02:05:07AM +0200, Anders Hammarquist wrote:
> The USB cable to read out data from the Abbott FreeStyle Precision
> meters, known as the Abbott stip port cable, uses the TI 3410 chip,
> just as the already added stereo port cable. They are essestially
> the same cable, just with different connectors at the end.
> 
> This patch set adds the product id to the driver, and makes the
> product type more explicit. Arguably, the ABBOTT_PRODUCT_ID
> define could be removed, but I left it on the off chance that
> someone other that the TI 3410 driver uses it.

No, it doesn't, please fix up that last patch and resend it.

thanks,

greg k-h

[PATCH 0/2] *** SUBJECT HERE ***

[Anders Hammarquist]

The USB cable to read out data from the Abbott FreeStyle Precision
meters, known as the Abbott stip port cable, uses the TI 3410 chip,
just as the already added stereo port cable. They are essestially
the same cable, just with different connectors at the end.

This patch set adds the product id to the driver, and makes the
product type more explicit. Arguably, the ABBOTT_PRODUCT_ID
define could be removed, but I left it on the off chance that
someone other that the TI 3410 driver uses it.

/Anders

Anders Hammarquist (2):
  Add product id for Abbott strip port cable for Precision meter    
    which uses the TI 3410 chip.
  Be explicit about the Abbott product ids being product ids.

 drivers/usb/serial/ti_usb_3410_5052.c |    3 ++-
 drivers/usb/serial/ti_usb_3410_5052.h |    4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.7.10.4

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Greg KH]

On Tue, May 03, 2011 at 12:00:55AM -0700, sukeshs@broadcom.com wrote:
> From: Sukesh Srikakula <sukeshs@xl-sj1-20.sj.broadcom.com>
> 
> *** BLURB HERE ***

I think you forgot the subject and the BLURB :(

care to retry?

[PATCH 0/2] *** SUBJECT HERE ***

[sukeshs]

From: Sukesh Srikakula <sukeshs@xl-sj1-20.sj.broadcom.com>

*** BLURB HERE ***

Sukesh Srikakula (2):
  brcm80211: FIX for TKIP GTK bug
  brcm80211: Fix for suspend/resume bug

 drivers/staging/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    4 ----
 drivers/staging/brcm80211/brcmfmac/dhd.h          |    3 +++
 drivers/staging/brcm80211/brcmfmac/dhd_linux.c    |    6 ++++--
 drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c  |   12 ++++++++++++
 4 files changed, 19 insertions(+), 6 deletions(-)

-- 
1.7.3.2

[PATCH 0/2] *** SUBJECT HERE ***

[Arnaldo Carvalho de Melo]

*** BLURB HERE ***

Arnaldo Carvalho de Melo (1):
  perf symbols: Fix kallsyms kernel/module map splitting

Ming Lei (1):
  perf symbols: Figure out start address of kernel map from kallsyms

 tools/perf/util/symbol.c |   59 +++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 53 insertions(+), 6 deletions(-)

Re: [PATCH 0/2] *** SUBJECT HERE ***

[Bill Gatliff]

Bill Gatliff wrote:
> *** BLURB HERE ***
>   

Dangit, sometimes I really hate it when emacs leaves its backup files
around...  :(

Like now, for example.  Please disregard the noise generated by my
careless use of filename wildcards...


b.g.

-- 
Bill Gatliff
Embedded systems training and consulting
http://billgatliff.com
bgat@billgatliff.com

[PATCH 0/2] *** SUBJECT HERE ***

[Bill Gatliff]

*** BLURB HERE ***

Bill Gatliff (2):
  Use struct of_i2c_gpio_chip instead of raw struct gpio_chip
  Reorder initialization to better support device tree data

 drivers/gpio/pca953x.c |  168 +++++++++++++++++++++++++-----------------------
 1 files changed, 88 insertions(+), 80 deletions(-)

[PATCH 0/2] *** SUBJECT HERE ***

[Izik Eidus]

RFC: move the dirty page tracking to use dirty bit

Well, i was bored this morning and had this idea for a while, didnt test it to
much..., first i want to hear what ppl think?

Thanks.

Izik Eidus (2):
  kvm: fix dirty bit tracking for slots with large pages
  kvm: change the dirty page tracking to work with dirty bit instead of
    page fault

 arch/ia64/kvm/kvm-ia64.c        |    4 ++++
 arch/powerpc/kvm/powerpc.c      |    4 ++++
 arch/s390/kvm/kvm-s390.c        |    4 ++++
 arch/x86/include/asm/kvm_host.h |    3 +++
 arch/x86/kvm/mmu.c              |   32 +++++++++++++++++++++++++++++---
 arch/x86/kvm/svm.c              |    7 +++++++
 arch/x86/kvm/vmx.c              |    7 +++++++
 arch/x86/kvm/x86.c              |   21 ++++++++++++++++++---
 include/linux/kvm_host.h        |    1 +
 virt/kvm/kvm_main.c             |   17 ++++++++++++-----
 10 files changed, 89 insertions(+), 11 deletions(-)

[PATCH 0/2] *** SUBJECT HERE ***

[Junio C Hamano]

*** BLURB HERE ***

Junio C Hamano (1):
  Makefile: test-parse-options depends on parse-options.h

Kjetil Barvik (1):
  symlinks.c: small style cleanup

 Makefile   |    2 ++
 symlinks.c |    6 ++----
 2 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 0/2] *** SUBJECT HERE ***

[Thomas Rast]

Eric Wong wrote.
> > So should we just change all "unknown foo" tests to verify that 'git
> > svn info' errors out too?
>
> Yes, I see no reason to differ from plain svn here.

This starts getting more complicated at every turn.  The included
mini-series (probably textually depends on the other 6 patches though)
"fixes" this.

HOWEVER: Subversion itself broke compatibility here.  In 1.4:

  $ svn info new; echo $?
  new:  (Not a versioned resource)

  0

Note the extra linebreak and successful exit.  Current git-svn
precisely matches this output.  In 1.5, it's different:

  $ svn info new; echo $?
  svn: 'new' is not under version control
  1

While it is of course up to you what you would like to do (and modulo
test_must_fail, 2/2 can still be used to fix the tests if you decide
to reject 1/2), I suggest changing to 1.5 behaviour.  exit(1) is the
sane thing to do in this case, and that is already breaking
bit-for-bit compatibility with SVN 1.4, so we might as well adopt the
new error message.  Of course this prevents us from comparing the
output literally in the tests, so I settled for a slightly weaker
check: failure status and mention of the filename.

Unfortunately this does raise the question whether the URL-encoding
issue treated in the other series is in fact a similar incompatibility
between 1.4 and 1.5, not a (minor but long-standing) bug in git-svn.

- Thomas


 git-svn.perl            |    4 +-
 t/t9119-git-svn-info.sh |   73 ++++++++++++++++-------------------------------
 2 files changed, 27 insertions(+), 50 deletions(-)