[RFC/PATCH 1/2] kconfig: add KBUILD_USERCONFIG option

[Felipe Contreras]

This allows us to specify a user defconfig which is added on top of the
default defconfig, which makes it easier for users to keep a simpler
defconfig, without duplicating all the options already present in the
arch defconfig.

Here a very simple defconfig I've been using for several years on top of
x86_64_defconfig. It works because I can rely on the fact that
x86_64_defconfig is properly maintained.

CONFIG_KERNEL_LZO=y
CONFIG_OPROFILE=m
CONFIG_MCORE2=y
CONFIG_COMPAT_VDSO=y

CONFIG_PATA_ACPI=y
CONFIG_ATA_GENERIC=y

CONFIG_LOGO=n

CONFIG_BTRFS_FS=m
CONFIG_FUSE_FS=m
CONFIG_NFS_FS=m
CONFIG_NFSD=m
CONFIG_CIFS=m

CONFIG_KVM=m
CONFIG_KVM_INTEL=m
CONFIG_KSM=y
CONFIG_PARAVIRT=y

CONFIG_USB_XHCI_HCD=y
CONFIG_USB_USBNET=m
CONFIG_USB_NET_CDC_EEM=m
CONFIG_USB_UAS=m

CONFIG_SND_HDA_INPUT_JACK=y
CONFIG_SND_HDA_CODEC_REALTEK=y
CONFIG_SND_HDA_CODEC_HDMI=y
CONFIG_SND_HDA_CODEC_CMEDIA=y
CONFIG_SND_HDA_CODEC_SI3054=y

CONFIG_DM_CRYPT=m
CONFIG_CRYPTO_SHA256=y
CONFIG_BLK_DEV_CRYPTOLOOP=m
CONFIG_BRIDGE=m
CONFIG_TUN=m

CONFIG_ISO9660_FS=m
CONFIG_UDF_FS=m

CONFIG_ENCLOSURE_SERVICES=m
CONFIG_SCSI_ENCLOSURE=m

CONFIG_DEBUG_KERNEL=n
CONFIG_X86_VERBOSE_BOOTUP=n
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_PM_RUNTIME=y

CONFIG_SCHED_AUTOGROUP=y

CONFIG_UEVENT_HELPER_PATH=""

CONFIG_FHANDLE=y

CONFIG_EFI_STUB=y
CONFIG_EFIVAR_FS=m

CONFIG_HYPERVISOR_GUEST=y
CONFIG_DRM_CIRRUS_QEMU=y

CONFIG_CFG80211=m
CONFIG_MAC80211=m
CONFIG_IWLWIFI=m
CONFIG_CFG80211_DEFAULT_PS=n

Signed-off-by: Felipe Contreras 
---
 scripts/kconfig/Makefile | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
index d9b1fef..0d55ad1 100644
--- a/scripts/kconfig/Makefile
+++ b/scripts/kconfig/Makefile
@@ -97,9 +97,15 @@ defconfig: $(obj)/conf
 ifeq ($(KBUILD_DEFCONFIG),)
$< $(silent) --defconfig $(Kconfig)
 else
+ifneq ($(KBUILD_USERCONFIG),)
+   @$(kecho) "*** Default configuration is based on '$(KBUILD_DEFCONFIG)' 
and '$(KBUILD_USERCONFIG)'"
+   $(Q)cat arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG) 
$(KBUILD_USERCONFIG) > defconfig
+   $(Q)$< $(silent) --defconfig=defconfig $(Kconfig)
+else
@$(kecho) "*** Default configuration is based on '$(KBUILD_DEFCONFIG)'"
$(Q)$< $(silent) 
--defconfig=arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG) $(Kconfig)
 endif
+endif
 
 %_defconfig: $(obj)/conf
$(Q)$< $(silent) --defconfig=arch/$(SRCARCH)/configs/$@ $(Kconfig)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC/PATCH 0/2] A simpler way to maintain custom defconfigs

[Felipe Contreras]

Hi,

For several years I've used a trick to be able to maintain a simple defconfig
that works across many versions, and requires little maintenance from my
part:

% cat arch/x86/configs/x86_64_defconfig ~/my-config > .config && make 
olddefconfig

I'm sending a proposal to integrate it on the build system so that many people
can do the same in a simple manner.

The interesting part is how to generate this simplified defconfig. In a
nutshell; you want to take your .config, remove everything that is the default
in the Kconfig files (what savedefconfig does), but also removes anything that
is in the default defconfig (e.g. x86_64_defconfig)

I've been doing this by hand, but today I gave it a shot to automate this. The
result is a bit crude, but it works.

Thoughts?

Felipe Contreras (2):
  kconfig: add KBUILD_USERCONFIG option
  kconfig: add KCONFIG_BASECONFIG option to savedefconfig

 scripts/kconfig/Makefile|  6 +++
 scripts/kconfig/conf.c  |  3 ++
 scripts/kconfig/confdata.c  | 89 +
 scripts/kconfig/lkc_proto.h |  1 +
 4 files changed, 99 insertions(+)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC/PATCH 2/2] kconfig: add KCONFIG_BASECONFIG option to savedefconfig

[Felipe Contreras]

This option parses a defconfig file, and sets all the values as default
ones. The result is a much simplified defconfig.

This defconfig can be used as a KBUILD_USERCONFIG, which added to the
default defconfig generates exactly the same config file.

Signed-off-by: Felipe Contreras 
---
 scripts/kconfig/conf.c  |  3 ++
 scripts/kconfig/confdata.c  | 89 +
 scripts/kconfig/lkc_proto.h |  1 +
 3 files changed, 93 insertions(+)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 6c20431..382151c 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -698,6 +698,9 @@ int main(int ac, char **av)
return 1;
}
} else if (input_mode == savedefconfig) {
+   name = getenv("KCONFIG_BASECONFIG");
+   if (name)
+   conf_read_def(name);
if (conf_write_defconfig(defconfig_file)) {
fprintf(stderr, _("n*** Error while saving defconfig 
to: %s\n\n"),
defconfig_file);
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c814f57..af96042 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -476,6 +476,95 @@ int conf_read(const char *name)
return 0;
 }
 
+static void conf_set_sym_default(struct symbol *sym, char *p)
+{
+   struct property *prop, **propp;
+
+   prop = xmalloc(sizeof(*prop));
+   memset(prop, 0, sizeof(*prop));
+   prop->type = P_DEFAULT;
+   prop->sym = sym;
+   prop->file = current_file;
+   prop->lineno = zconf_lineno();
+
+   for (propp = >prop; *propp; propp = &(*propp)->next) {
+   if ((*propp)->type == P_DEFAULT) {
+   prop->next = *propp;
+   break;
+   }
+   }
+   *propp = prop;
+
+   prop->expr = expr_alloc_symbol(sym_lookup(p, SYMBOL_CONST));
+}
+
+int conf_read_def(const char *name)
+{
+   FILE *in = NULL;
+   char *line = NULL;
+   size_t line_asize = 0;
+   char *p, *p2;
+   struct symbol *sym;
+
+   in = zconf_fopen(name);
+   if (!in)
+   return 1;
+
+   conf_filename = name;
+   conf_lineno = 0;
+   conf_warnings = 0;
+
+   while (compat_getline(, _asize, in) != -1) {
+   conf_lineno++;
+   sym = NULL;
+   if (line[0] == '#') {
+   if (memcmp(line + 2, CONFIG_, strlen(CONFIG_)))
+   continue;
+   p = strchr(line + 2 + strlen(CONFIG_), ' ');
+   if (!p)
+   continue;
+   *p++ = 0;
+   if (strncmp(p, "is not set", 10))
+   continue;
+   sym = sym_find(line + 2 + strlen(CONFIG_));
+   if (!sym)
+   continue;
+   switch (sym->type) {
+   case S_BOOLEAN:
+   case S_TRISTATE:
+   conf_set_sym_default(sym, "n");
+   break;
+   default:
+   ;
+   }
+   } else if (memcmp(line, CONFIG_, strlen(CONFIG_)) == 0) {
+   p = strchr(line + strlen(CONFIG_), '=');
+   if (!p)
+   continue;
+   *p++ = 0;
+   p2 = strchr(p, '\n');
+   if (p2) {
+   *p2-- = 0;
+   if (*p2 == '\r')
+   *p2 = 0;
+   }
+
+   sym = sym_find(line + strlen(CONFIG_));
+   if (!sym)
+   continue;
+   conf_set_sym_default(sym, p);
+   } else {
+   if (line[0] != '\r' && line[0] != '\n')
+   conf_warning("unexpected data");
+   continue;
+   }
+   }
+   free(line);
+   fclose(in);
+
+   return 0;
+}
+
 /*
  * Kconfig configuration printer
  *
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index d539871..69c3785 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -3,6 +3,7 @@
 /* confdata.c */
 void conf_parse(const char *name);
 int conf_read(const char *name);
+int conf_read_def(const char *name);
 int conf_read_simple(const char *name, int);
 int conf_write_defconfig(const char *name);
 int conf_write(const char *name);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@

[RFC/PATCH 2/2] kconfig: add KCONFIG_BASECONFIG option to savedefconfig

[Felipe Contreras]

This option parses a defconfig file, and sets all the values as default
ones. The result is a much simplified defconfig.

This defconfig can be used as a KBUILD_USERCONFIG, which added to the
default defconfig generates exactly the same config file.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 scripts/kconfig/conf.c  |  3 ++
 scripts/kconfig/confdata.c  | 89 +
 scripts/kconfig/lkc_proto.h |  1 +
 3 files changed, 93 insertions(+)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 6c20431..382151c 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -698,6 +698,9 @@ int main(int ac, char **av)
return 1;
}
} else if (input_mode == savedefconfig) {
+   name = getenv(KCONFIG_BASECONFIG);
+   if (name)
+   conf_read_def(name);
if (conf_write_defconfig(defconfig_file)) {
fprintf(stderr, _(n*** Error while saving defconfig 
to: %s\n\n),
defconfig_file);
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c814f57..af96042 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -476,6 +476,95 @@ int conf_read(const char *name)
return 0;
 }
 
+static void conf_set_sym_default(struct symbol *sym, char *p)
+{
+   struct property *prop, **propp;
+
+   prop = xmalloc(sizeof(*prop));
+   memset(prop, 0, sizeof(*prop));
+   prop-type = P_DEFAULT;
+   prop-sym = sym;
+   prop-file = current_file;
+   prop-lineno = zconf_lineno();
+
+   for (propp = sym-prop; *propp; propp = (*propp)-next) {
+   if ((*propp)-type == P_DEFAULT) {
+   prop-next = *propp;
+   break;
+   }
+   }
+   *propp = prop;
+
+   prop-expr = expr_alloc_symbol(sym_lookup(p, SYMBOL_CONST));
+}
+
+int conf_read_def(const char *name)
+{
+   FILE *in = NULL;
+   char *line = NULL;
+   size_t line_asize = 0;
+   char *p, *p2;
+   struct symbol *sym;
+
+   in = zconf_fopen(name);
+   if (!in)
+   return 1;
+
+   conf_filename = name;
+   conf_lineno = 0;
+   conf_warnings = 0;
+
+   while (compat_getline(line, line_asize, in) != -1) {
+   conf_lineno++;
+   sym = NULL;
+   if (line[0] == '#') {
+   if (memcmp(line + 2, CONFIG_, strlen(CONFIG_)))
+   continue;
+   p = strchr(line + 2 + strlen(CONFIG_), ' ');
+   if (!p)
+   continue;
+   *p++ = 0;
+   if (strncmp(p, is not set, 10))
+   continue;
+   sym = sym_find(line + 2 + strlen(CONFIG_));
+   if (!sym)
+   continue;
+   switch (sym-type) {
+   case S_BOOLEAN:
+   case S_TRISTATE:
+   conf_set_sym_default(sym, n);
+   break;
+   default:
+   ;
+   }
+   } else if (memcmp(line, CONFIG_, strlen(CONFIG_)) == 0) {
+   p = strchr(line + strlen(CONFIG_), '=');
+   if (!p)
+   continue;
+   *p++ = 0;
+   p2 = strchr(p, '\n');
+   if (p2) {
+   *p2-- = 0;
+   if (*p2 == '\r')
+   *p2 = 0;
+   }
+
+   sym = sym_find(line + strlen(CONFIG_));
+   if (!sym)
+   continue;
+   conf_set_sym_default(sym, p);
+   } else {
+   if (line[0] != '\r'  line[0] != '\n')
+   conf_warning(unexpected data);
+   continue;
+   }
+   }
+   free(line);
+   fclose(in);
+
+   return 0;
+}
+
 /*
  * Kconfig configuration printer
  *
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index d539871..69c3785 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -3,6 +3,7 @@
 /* confdata.c */
 void conf_parse(const char *name);
 int conf_read(const char *name);
+int conf_read_def(const char *name);
 int conf_read_simple(const char *name, int);
 int conf_write_defconfig(const char *name);
 int conf_write(const char *name);
-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please

[RFC/PATCH 1/2] kconfig: add KBUILD_USERCONFIG option

[Felipe Contreras]

This allows us to specify a user defconfig which is added on top of the
default defconfig, which makes it easier for users to keep a simpler
defconfig, without duplicating all the options already present in the
arch defconfig.

Here a very simple defconfig I've been using for several years on top of
x86_64_defconfig. It works because I can rely on the fact that
x86_64_defconfig is properly maintained.

CONFIG_KERNEL_LZO=y
CONFIG_OPROFILE=m
CONFIG_MCORE2=y
CONFIG_COMPAT_VDSO=y

CONFIG_PATA_ACPI=y
CONFIG_ATA_GENERIC=y

CONFIG_LOGO=n

CONFIG_BTRFS_FS=m
CONFIG_FUSE_FS=m
CONFIG_NFS_FS=m
CONFIG_NFSD=m
CONFIG_CIFS=m

CONFIG_KVM=m
CONFIG_KVM_INTEL=m
CONFIG_KSM=y
CONFIG_PARAVIRT=y

CONFIG_USB_XHCI_HCD=y
CONFIG_USB_USBNET=m
CONFIG_USB_NET_CDC_EEM=m
CONFIG_USB_UAS=m

CONFIG_SND_HDA_INPUT_JACK=y
CONFIG_SND_HDA_CODEC_REALTEK=y
CONFIG_SND_HDA_CODEC_HDMI=y
CONFIG_SND_HDA_CODEC_CMEDIA=y
CONFIG_SND_HDA_CODEC_SI3054=y

CONFIG_DM_CRYPT=m
CONFIG_CRYPTO_SHA256=y
CONFIG_BLK_DEV_CRYPTOLOOP=m
CONFIG_BRIDGE=m
CONFIG_TUN=m

CONFIG_ISO9660_FS=m
CONFIG_UDF_FS=m

CONFIG_ENCLOSURE_SERVICES=m
CONFIG_SCSI_ENCLOSURE=m

CONFIG_DEBUG_KERNEL=n
CONFIG_X86_VERBOSE_BOOTUP=n
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_PM_RUNTIME=y

CONFIG_SCHED_AUTOGROUP=y

CONFIG_UEVENT_HELPER_PATH=

CONFIG_FHANDLE=y

CONFIG_EFI_STUB=y
CONFIG_EFIVAR_FS=m

CONFIG_HYPERVISOR_GUEST=y
CONFIG_DRM_CIRRUS_QEMU=y

CONFIG_CFG80211=m
CONFIG_MAC80211=m
CONFIG_IWLWIFI=m
CONFIG_CFG80211_DEFAULT_PS=n

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 scripts/kconfig/Makefile | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
index d9b1fef..0d55ad1 100644
--- a/scripts/kconfig/Makefile
+++ b/scripts/kconfig/Makefile
@@ -97,9 +97,15 @@ defconfig: $(obj)/conf
 ifeq ($(KBUILD_DEFCONFIG),)
$ $(silent) --defconfig $(Kconfig)
 else
+ifneq ($(KBUILD_USERCONFIG),)
+   @$(kecho) *** Default configuration is based on '$(KBUILD_DEFCONFIG)' 
and '$(KBUILD_USERCONFIG)'
+   $(Q)cat arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG) 
$(KBUILD_USERCONFIG)  defconfig
+   $(Q)$ $(silent) --defconfig=defconfig $(Kconfig)
+else
@$(kecho) *** Default configuration is based on '$(KBUILD_DEFCONFIG)'
$(Q)$ $(silent) 
--defconfig=arch/$(SRCARCH)/configs/$(KBUILD_DEFCONFIG) $(Kconfig)
 endif
+endif
 
 %_defconfig: $(obj)/conf
$(Q)$ $(silent) --defconfig=arch/$(SRCARCH)/configs/$@ $(Kconfig)
-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC/PATCH 0/2] A simpler way to maintain custom defconfigs

[Felipe Contreras]

Hi,

For several years I've used a trick to be able to maintain a simple defconfig
that works across many versions, and requires little maintenance from my
part:

% cat arch/x86/configs/x86_64_defconfig ~/my-config  .config  make 
olddefconfig

I'm sending a proposal to integrate it on the build system so that many people
can do the same in a simple manner.

The interesting part is how to generate this simplified defconfig. In a
nutshell; you want to take your .config, remove everything that is the default
in the Kconfig files (what savedefconfig does), but also removes anything that
is in the default defconfig (e.g. x86_64_defconfig)

I've been doing this by hand, but today I gave it a shot to automate this. The
result is a bit crude, but it works.

Thoughts?

Felipe Contreras (2):
  kconfig: add KBUILD_USERCONFIG option
  kconfig: add KCONFIG_BASECONFIG option to savedefconfig

 scripts/kconfig/Makefile|  6 +++
 scripts/kconfig/conf.c  |  3 ++
 scripts/kconfig/confdata.c  | 89 +
 scripts/kconfig/lkc_proto.h |  1 +
 4 files changed, 99 insertions(+)

-- 
2.5.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

Michal Hocko wrote:
> On Fri 06-06-14 18:11:14, Felipe Contreras wrote:
> > On Fri, Jun 6, 2014 at 5:33 AM, Felipe Contreras
> >  wrote:
> > > On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko  wrote:
> > >
> > >> Mel has a nice systemtap script (attached) to watch for stalls. Maybe
> > >> you can give it a try?
> > >
> > > Is there any special configurations I should enable?
> > >
> > > I get this:
> > > semantic error: unresolved arity-1 global array name, missing global
> > > declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
> > > source: name[t]=execname()
> > > ^
> > >
> > > Pass 2: analysis failed.  [man error::pass2]
> > > Number of similar error messages suppressed: 71.
> > > Rerun with -v to see them.
> > > Unexpected exit of STAP script at
> > > /home/felipec/Downloads/watch-dstate-new.pl line 320.
> > 
> > Actually I debugged the problem, and it's that the format of the
> > script is DOS, not UNIX. After changing the format the script works.
> 
> Ups, I've downloaded it from our bugzilla so maybe it just did some
> tricks with the script.
> 
> > However, it's not returning anything. It's running, but doesn't seem
> > to find any stalls.
> 
> Intereting. It was quite good at pointing at stalls. How are you
> measuring those stalls during your testing?

I'm not measuring them, I simply grab a GUI window and move it around
while the big file is being copied, when the issue happens the window
stops moving, and the mouse, everything hangs for a time, then it
resumes, then hangs again... the interactivity is bad.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

Michal Hocko wrote:
 On Fri 06-06-14 18:11:14, Felipe Contreras wrote:
  On Fri, Jun 6, 2014 at 5:33 AM, Felipe Contreras
  felipe.contre...@gmail.com wrote:
   On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko mho...@suse.cz wrote:
  
   Mel has a nice systemtap script (attached) to watch for stalls. Maybe
   you can give it a try?
  
   Is there any special configurations I should enable?
  
   I get this:
   semantic error: unresolved arity-1 global array name, missing global
   declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
   source: name[t]=execname()
   ^
  
   Pass 2: analysis failed.  [man error::pass2]
   Number of similar error messages suppressed: 71.
   Rerun with -v to see them.
   Unexpected exit of STAP script at
   /home/felipec/Downloads/watch-dstate-new.pl line 320.
  
  Actually I debugged the problem, and it's that the format of the
  script is DOS, not UNIX. After changing the format the script works.
 
 Ups, I've downloaded it from our bugzilla so maybe it just did some
 tricks with the script.
 
  However, it's not returning anything. It's running, but doesn't seem
  to find any stalls.
 
 Intereting. It was quite good at pointing at stalls. How are you
 measuring those stalls during your testing?

I'm not measuring them, I simply grab a GUI window and move it around
while the big file is being copied, when the issue happens the window
stops moving, and the mouse, everything hangs for a time, then it
resumes, then hangs again... the interactivity is bad.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Sat, Jun 7, 2014 at 7:35 AM,   wrote:

> Would you please try again based only on comment [1](based on v3.15-rc8)?
> thanks
> Hillf
>
> --- a/mm/vmscan.c   Sat Jun  7 18:38:08 2014
> +++ b/mm/vmscan.c   Sat Jun  7 20:08:36 2014
> @@ -1566,7 +1566,7 @@ shrink_inactive_list(unsigned long nr_to
>  * implies that pages are cycling through the LRU faster than
>  * they are written so also forcibly stall.
>  */
> -   if (nr_unqueued_dirty == nr_taken || nr_immediate)
> +   if (nr_immediate)
> congestion_wait(BLK_RW_ASYNC, HZ/10);
> }

That actually seems to work correctly on v3.15-rc8.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Sat, Jun 7, 2014 at 7:35 AM,  zhd...@sina.com wrote:

 Would you please try again based only on comment [1](based on v3.15-rc8)?
 thanks
 Hillf

 --- a/mm/vmscan.c   Sat Jun  7 18:38:08 2014
 +++ b/mm/vmscan.c   Sat Jun  7 20:08:36 2014
 @@ -1566,7 +1566,7 @@ shrink_inactive_list(unsigned long nr_to
  * implies that pages are cycling through the LRU faster than
  * they are written so also forcibly stall.
  */
 -   if (nr_unqueued_dirty == nr_taken || nr_immediate)
 +   if (nr_immediate)
 congestion_wait(BLK_RW_ASYNC, HZ/10);
 }

That actually seems to work correctly on v3.15-rc8.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 5:33 AM, Felipe Contreras
 wrote:
> On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko  wrote:
>
>> Mel has a nice systemtap script (attached) to watch for stalls. Maybe
>> you can give it a try?
>
> Is there any special configurations I should enable?
>
> I get this:
> semantic error: unresolved arity-1 global array name, missing global
> declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
> source: name[t]=execname()
> ^
>
> Pass 2: analysis failed.  [man error::pass2]
> Number of similar error messages suppressed: 71.
> Rerun with -v to see them.
> Unexpected exit of STAP script at
> /home/felipec/Downloads/watch-dstate-new.pl line 320.

Actually I debugged the problem, and it's that the format of the
script is DOS, not UNIX. After changing the format the script works.

However, it's not returning anything. It's running, but doesn't seem
to find any stalls.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 6:03 AM, Michal Hocko  wrote:
> On Fri 06-06-14 05:33:28, Felipe Contreras wrote:
>> On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko  wrote:
>>
>> > Mel has a nice systemtap script (attached) to watch for stalls. Maybe
>> > you can give it a try?
>>
>> Is there any special configurations I should enable?
>
> You need debuginfo and systemtap AFAIK. I haven't used this script
> myself.

I have both.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Thu, Jun 5, 2014 at 8:37 AM, Michal Hocko  wrote:

> We had a similar report for opensuse. The common part was that there was
> an IO to a slow USB device going on.

Actually I've managed to narrow down my synthetic test, and all I need
is to copy a big file, and it even happens reading and writing to the
SSD (although the stall is less severe).

Here's the test:
http://pastie.org/9264124

Just pass a big file as the first argument.

I don't have much memory in this machine, so I guess running out of
memory is the trigger.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko  wrote:

> Mel has a nice systemtap script (attached) to watch for stalls. Maybe
> you can give it a try?

Is there any special configurations I should enable?

I get this:
semantic error: unresolved arity-1 global array name, missing global
declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
source: name[t]=execname()
^

Pass 2: analysis failed.  [man error::pass2]
Number of similar error messages suppressed: 71.
Rerun with -v to see them.
Unexpected exit of STAP script at
/home/felipec/Downloads/watch-dstate-new.pl line 320.



-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 4:58 AM,   wrote:

> Alternatively can we try wait_iff_congested(zone, BLK_RW_ASYNC, HZ/10) ?

I see the same problem with that code.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 4:58 AM,  zhd...@sina.com wrote:

 Alternatively can we try wait_iff_congested(zone, BLK_RW_ASYNC, HZ/10) ?

I see the same problem with that code.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko mho...@suse.cz wrote:

 Mel has a nice systemtap script (attached) to watch for stalls. Maybe
 you can give it a try?

Is there any special configurations I should enable?

I get this:
semantic error: unresolved arity-1 global array name, missing global
declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
source: name[t]=execname()
^

Pass 2: analysis failed.  [man error::pass2]
Number of similar error messages suppressed: 71.
Rerun with -v to see them.
Unexpected exit of STAP script at
/home/felipec/Downloads/watch-dstate-new.pl line 320.



-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Thu, Jun 5, 2014 at 8:37 AM, Michal Hocko mho...@suse.cz wrote:

 We had a similar report for opensuse. The common part was that there was
 an IO to a slow USB device going on.

Actually I've managed to narrow down my synthetic test, and all I need
is to copy a big file, and it even happens reading and writing to the
SSD (although the stall is less severe).

Here's the test:
http://pastie.org/9264124

Just pass a big file as the first argument.

I don't have much memory in this machine, so I guess running out of
memory is the trigger.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 6:03 AM, Michal Hocko mho...@suse.cz wrote:
 On Fri 06-06-14 05:33:28, Felipe Contreras wrote:
 On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko mho...@suse.cz wrote:

  Mel has a nice systemtap script (attached) to watch for stalls. Maybe
  you can give it a try?

 Is there any special configurations I should enable?

 You need debuginfo and systemtap AFAIK. I haven't used this script
 myself.

I have both.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Fri, Jun 6, 2014 at 5:33 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Fri, Jun 6, 2014 at 4:16 AM, Michal Hocko mho...@suse.cz wrote:

 Mel has a nice systemtap script (attached) to watch for stalls. Maybe
 you can give it a try?

 Is there any special configurations I should enable?

 I get this:
 semantic error: unresolved arity-1 global array name, missing global
 declaration?: identifier 'name' at /tmp/stapd6pu9A:4:2
 source: name[t]=execname()
 ^

 Pass 2: analysis failed.  [man error::pass2]
 Number of similar error messages suppressed: 71.
 Rerun with -v to see them.
 Unexpected exit of STAP script at
 /home/felipec/Downloads/watch-dstate-new.pl line 320.

Actually I debugged the problem, and it's that the format of the
script is DOS, not UNIX. After changing the format the script works.

However, it's not returning anything. It's running, but doesn't seem
to find any stalls.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Thu, Jun 5, 2014 at 8:37 AM, Michal Hocko  wrote:
> On Thu 05-06-14 06:33:40, Felipe Contreras wrote:

>> For a while I've noticed that my machine bogs down in certain
>> situations, usually while doing heavy I/O operations, it is not just the
>> I/O operations, but everything, including the graphical interface, even
>> the mouse pointer.
>>
>> As far as I can recall this did not happen in the past.
>>
>> I noticed this specially on certain operations, for example updating a
>> a game on Steam (to an exteranl USB 3.0 device), or copying TV episodes
>> to a USB memory stick (probably flash-based).
>
> We had a similar report for opensuse. The common part was that there was
> an IO to a slow USB device going on.

Well, it's a USB 3.0 device, I can write at 250 MB/s, so it's not
really that slow.

And in fact, when I read and write to and from the same USB 3.0
device, I don't see the issue.

>> Then I went back to the latest stable version (v3.14.5), and commented
>> out the line I think is causing the slow down:
>>
>>   if (nr_unqueued_dirty == nr_taken || nr_immediate)
>> congestion_wait(BLK_RW_ASYNC, HZ/10);
>
> Yes, I came to the same check. I didn't have any confirmation yet so
> thanks for your confirmation. I've suggested to reduce this
> congestion_wait only to kswapd:
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 32c661d66a45..ef6a1c0e788c 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1566,7 +1566,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct 
> lruvec *lruvec,
>  * implies that pages are cycling through the LRU faster than
>  * they are written so also forcibly stall.
>  */
> -   if (nr_unqueued_dirty == nr_taken || nr_immediate)
> +   if ((nr_unqueued_dirty == nr_taken || nr_immediate) && 
> current_is_kswapd())
> congestion_wait(BLK_RW_ASYNC, HZ/10);
> }

Unfortunately that doesn't fix the issue for me.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

Hi,

For a while I've noticed that my machine bogs down in certain
situations, usually while doing heavy I/O operations, it is not just the
I/O operations, but everything, including the graphical interface, even
the mouse pointer.

As far as I can recall this did not happen in the past.

I noticed this specially on certain operations, for example updating a
a game on Steam (to an exteranl USB 3.0 device), or copying TV episodes
to a USB memory stick (probably flash-based).

Today I decided to finally hunt down the problem, so I created a
synthetic test that basically consists on copying a bunch of files from
one drive to another (from an SSD to an external USB 3.0). This is
pretty similar to what I noticed; the graphical interface slows down.

Then I bisected the issue and it turns out that indeed it wasn't
happening in the past, it started happening in v3.11, and it was
triggered by this commit:

  e2be15f (mm: vmscan: stall page reclaim and writeback pages based on
  dirty/writepage pages encountered)

Then I went back to the latest stable version (v3.14.5), and commented
out the line I think is causing the slow down:

  if (nr_unqueued_dirty == nr_taken || nr_immediate)
  congestion_wait(BLK_RW_ASYNC, HZ/10);

After that I don't notice the slow down any more.

Anybody has any ideas how to fix the issue properly?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

Hi,

For a while I've noticed that my machine bogs down in certain
situations, usually while doing heavy I/O operations, it is not just the
I/O operations, but everything, including the graphical interface, even
the mouse pointer.

As far as I can recall this did not happen in the past.

I noticed this specially on certain operations, for example updating a
a game on Steam (to an exteranl USB 3.0 device), or copying TV episodes
to a USB memory stick (probably flash-based).

Today I decided to finally hunt down the problem, so I created a
synthetic test that basically consists on copying a bunch of files from
one drive to another (from an SSD to an external USB 3.0). This is
pretty similar to what I noticed; the graphical interface slows down.

Then I bisected the issue and it turns out that indeed it wasn't
happening in the past, it started happening in v3.11, and it was
triggered by this commit:

  e2be15f (mm: vmscan: stall page reclaim and writeback pages based on
  dirty/writepage pages encountered)

Then I went back to the latest stable version (v3.14.5), and commented
out the line I think is causing the slow down:

  if (nr_unqueued_dirty == nr_taken || nr_immediate)
  congestion_wait(BLK_RW_ASYNC, HZ/10);

After that I don't notice the slow down any more.

Anybody has any ideas how to fix the issue properly?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Interactivity regression since v3.11 in mm/vmscan.c

[Felipe Contreras]

On Thu, Jun 5, 2014 at 8:37 AM, Michal Hocko mho...@suse.cz wrote:
 On Thu 05-06-14 06:33:40, Felipe Contreras wrote:

 For a while I've noticed that my machine bogs down in certain
 situations, usually while doing heavy I/O operations, it is not just the
 I/O operations, but everything, including the graphical interface, even
 the mouse pointer.

 As far as I can recall this did not happen in the past.

 I noticed this specially on certain operations, for example updating a
 a game on Steam (to an exteranl USB 3.0 device), or copying TV episodes
 to a USB memory stick (probably flash-based).

 We had a similar report for opensuse. The common part was that there was
 an IO to a slow USB device going on.

Well, it's a USB 3.0 device, I can write at 250 MB/s, so it's not
really that slow.

And in fact, when I read and write to and from the same USB 3.0
device, I don't see the issue.

 Then I went back to the latest stable version (v3.14.5), and commented
 out the line I think is causing the slow down:

   if (nr_unqueued_dirty == nr_taken || nr_immediate)
 congestion_wait(BLK_RW_ASYNC, HZ/10);

 Yes, I came to the same check. I didn't have any confirmation yet so
 thanks for your confirmation. I've suggested to reduce this
 congestion_wait only to kswapd:
 diff --git a/mm/vmscan.c b/mm/vmscan.c
 index 32c661d66a45..ef6a1c0e788c 100644
 --- a/mm/vmscan.c
 +++ b/mm/vmscan.c
 @@ -1566,7 +1566,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct 
 lruvec *lruvec,
  * implies that pages are cycling through the LRU faster than
  * they are written so also forcibly stall.
  */
 -   if (nr_unqueued_dirty == nr_taken || nr_immediate)
 +   if ((nr_unqueued_dirty == nr_taken || nr_immediate)  
 current_is_kswapd())
 congestion_wait(BLK_RW_ASYNC, HZ/10);
 }

Unfortunately that doesn't fix the issue for me.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ANNOUNCE] Git v2.0.0-rc4

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras  writes:
> 
> > Junio C Hamano wrote:
> >
> >>  * The remote-helper interface to fast-import/fast-export via the
> >>transport-helper has been tightened to avoid leaving the import
> >>marks file from a failed/crashed run, as such a file that is out-of-
> >>sync with reality confuses a later invocation of itself.
> >
> > Really? Where are the patches for that?
> >
> > I think it's fair to say the way the remote-helpers and transport-helper
> > has been handled for v2.0 has been a total disaster.
> 
> Thanks for noticing.  The last-minute change of plans in the morning
> on the -rc release day did not help.  Will remove.

But this changed before that.

> Anything else I missed?

Not as far as I can see.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ANNOUNCE] Git v2.0.0-rc4

[Felipe Contreras]

Junio C Hamano wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:
 
  Junio C Hamano wrote:
 
   * The remote-helper interface to fast-import/fast-export via the
 transport-helper has been tightened to avoid leaving the import
 marks file from a failed/crashed run, as such a file that is out-of-
 sync with reality confuses a later invocation of itself.
 
  Really? Where are the patches for that?
 
  I think it's fair to say the way the remote-helpers and transport-helper
  has been handled for v2.0 has been a total disaster.
 
 Thanks for noticing.  The last-minute change of plans in the morning
 on the -rc release day did not help.  Will remove.

But this changed before that.

 Anything else I missed?

Not as far as I can see.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [ANNOUNCE] Git v2.0.0-rc4

[Felipe Contreras]

Junio C Hamano wrote:

>  * The remote-helper interface to fast-import/fast-export via the
>transport-helper has been tightened to avoid leaving the import
>marks file from a failed/crashed run, as such a file that is out-of-
>sync with reality confuses a later invocation of itself.

Really? Where are the patches for that?

I think it's fair to say the way the remote-helpers and transport-helper
has been handled for v2.0 has been a total disaster.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [ANNOUNCE] Git v2.0.0-rc4

[Felipe Contreras]

Junio C Hamano wrote:

  * The remote-helper interface to fast-import/fast-export via the
transport-helper has been tightened to avoid leaving the import
marks file from a failed/crashed run, as such a file that is out-of-
sync with reality confuses a later invocation of itself.

Really? Where are the patches for that?

I think it's fair to say the way the remote-helpers and transport-helper
has been handled for v2.0 has been a total disaster.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide "debug" from /proc/cmdline

[Felipe Contreras]

On Thu, Apr 3, 2014 at 5:03 AM, Borislav Petkov  wrote:
> On Thu, Apr 03, 2014 at 11:34:15AM +0100, Måns Rullgård wrote:
>> Once is an accident.  Twice is incompetence.  Three times is malice.
>
> Yeah, maybe it is time Linus started his own init daemon project, like
> that other thing, git, he did start a while ago. We can put it in
> tools/. I'm sure it can get off the ground pretty quickly, judging by
> other projects kernel people have jumped on in the past.

I bet he could do it in a week and it would be better than systemd in
at least one regard: actually booting no matter what.

Maybe the next time systemd developers break things again, which is
just a matter of time.

In the meantime you want to take a look at my little init script that
implements the basics of what an init system should do[1]. You would
probably need to hack it to your needs, but since it's 230 lines of
*readable* code, that shouldn't be a problem. I also wrote a blog post
explaining the process that lad me to that code[2].

Cheers.

[1] https://github.com/felipec/finit
[2] http://felipec.wordpress.com/2013/11/04/init/

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide "debug" from /proc/cmdline

[Felipe Contreras]

On Sun, Apr 6, 2014 at 3:49 PM, David Timothy Strauss
 wrote:
> On Fri, Apr 4, 2014 at 11:57 AM, Linus Torvalds
>  wrote:
>> Since I haven't even heard a "my bad" from the systemd people, I'd be
>> inclined to say that a bit of protection for future issues would be a
>> good idea.
>
> Just coming back to this thread now, I'll say something. I'm a systemd
> maintainer, and I'm sorry systemd's assertion bug, combined with
> aliasing the debug option with the kernel's, broke a bunch of
> workflows (including boot) and caused a bunch of unnecessary pain.

I think the real deeper bug in systemd is Kay Sievers and his
development practices.

> As a few other people have already said, this assertion should now be
> fixed. The question is now about the debug option, and I've given my
> +1 to Greg's patch for that. I think it's justified to put a condom on
> systemd's debug mode given how badly the assertion bug cascaded into
> affecting non-systemd work.

Unfortunately it seems your +1 (and everybody else's in that thread)
didn't matter as the patch was not applied.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide "debug" from /proc/cmdline

[Felipe Contreras]

On Thu, Apr 3, 2014 at 12:06 PM, Greg Kroah-Hartman
 wrote:
> On Thu, Apr 03, 2014 at 08:17:33AM -0700, Tim Bird wrote:
>>
>> I had no idea systemd was so verbose and was abusing the kernel
>> log buffers so badly.  I'm not a big fan of the rate-limiting, as this just
>> seems to encourage this kind of abuse.
>
> That was a bug in systemd, and has been fixed up in the latest versions,
> so it shouldn't happen anymore, even with debugging enabled.

Excellent. Did Kay Sievers accept it was a bug? From what I can see
the bug report is still open[1], and your patch to rename the
configuration to systemd.debug received unanimous consensus in the
mailing list, yet it wasn't applied.

So I bet the answer is no, he never accepted it, and we should expect
these issues to continue... it's only a matter of time before we hit
the next breakage.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=76935

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide debug from /proc/cmdline

[Felipe Contreras]

On Thu, Apr 3, 2014 at 12:06 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
 On Thu, Apr 03, 2014 at 08:17:33AM -0700, Tim Bird wrote:

 I had no idea systemd was so verbose and was abusing the kernel
 log buffers so badly.  I'm not a big fan of the rate-limiting, as this just
 seems to encourage this kind of abuse.

 That was a bug in systemd, and has been fixed up in the latest versions,
 so it shouldn't happen anymore, even with debugging enabled.

Excellent. Did Kay Sievers accept it was a bug? From what I can see
the bug report is still open[1], and your patch to rename the
configuration to systemd.debug received unanimous consensus in the
mailing list, yet it wasn't applied.

So I bet the answer is no, he never accepted it, and we should expect
these issues to continue... it's only a matter of time before we hit
the next breakage.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=76935

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide debug from /proc/cmdline

[Felipe Contreras]

On Sun, Apr 6, 2014 at 3:49 PM, David Timothy Strauss
da...@davidstrauss.net wrote:
 On Fri, Apr 4, 2014 at 11:57 AM, Linus Torvalds
 torva...@linux-foundation.org wrote:
 Since I haven't even heard a my bad from the systemd people, I'd be
 inclined to say that a bit of protection for future issues would be a
 good idea.

 Just coming back to this thread now, I'll say something. I'm a systemd
 maintainer, and I'm sorry systemd's assertion bug, combined with
 aliasing the debug option with the kernel's, broke a bunch of
 workflows (including boot) and caused a bunch of unnecessary pain.

I think the real deeper bug in systemd is Kay Sievers and his
development practices.

 As a few other people have already said, this assertion should now be
 fixed. The question is now about the debug option, and I've given my
 +1 to Greg's patch for that. I think it's justified to put a condom on
 systemd's debug mode given how badly the assertion bug cascaded into
 affecting non-systemd work.

Unfortunately it seems your +1 (and everybody else's in that thread)
didn't matter as the patch was not applied.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] cmdline: Hide debug from /proc/cmdline

[Felipe Contreras]

On Thu, Apr 3, 2014 at 5:03 AM, Borislav Petkov b...@alien8.de wrote:
 On Thu, Apr 03, 2014 at 11:34:15AM +0100, Måns Rullgård wrote:
 Once is an accident.  Twice is incompetence.  Three times is malice.

 Yeah, maybe it is time Linus started his own init daemon project, like
 that other thing, git, he did start a while ago. We can put it in
 tools/. I'm sure it can get off the ground pretty quickly, judging by
 other projects kernel people have jumped on in the past.

I bet he could do it in a week and it would be better than systemd in
at least one regard: actually booting no matter what.

Maybe the next time systemd developers break things again, which is
just a matter of time.

In the meantime you want to take a look at my little init script that
implements the basics of what an init system should do[1]. You would
probably need to hack it to your needs, but since it's 230 lines of
*readable* code, that shouldn't be a problem. I also wrote a blog post
explaining the process that lad me to that code[2].

Cheers.

[1] https://github.com/felipec/finit
[2] http://felipec.wordpress.com/2013/11/04/init/

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [ANNOUNCE] Git v2.0.0-rc0

[Felipe Contreras]

Junio C Hamano wrote:
>  * transport-helper, fast-import and fast-export have been updated to
>allow the ref mapping and ref deletion in a way similar to the
>natively supported transports.

Have they?

% git --version
git version 2.0.0.rc0
% git push origin origin master:foo 
   /tmp/test[master] nysa
searching for changes
no changes found
fatal: remote-helpers do not support old:new syntax
ERROR: unhandled export command:

I think you are missing the patches which I just resent[1].

[1] http://article.gmane.org/gmane.comp.version-control.git/246558

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [ANNOUNCE] Git v2.0.0-rc0

[Felipe Contreras]

Junio C Hamano wrote:
  * transport-helper, fast-import and fast-export have been updated to
allow the ref mapping and ref deletion in a way similar to the
natively supported transports.

Have they?

% git --version
git version 2.0.0.rc0
% git push origin origin master:foo 
   /tmp/test[master] nysa
searching for changes
no changes found
fatal: remote-helpers do not support old:new syntax
ERROR: unhandled export command:

I think you are missing the patches which I just resent[1].

[1] http://article.gmane.org/gmane.comp.version-control.git/246558

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] panic: setup panic_timeout early

[Felipe Contreras]

On Wed, Nov 27, 2013 at 9:47 AM, Jason Baron  wrote:
> On 11/27/2013 01:17 AM, Felipe Contreras wrote:
>> Otherwise we might not reboot when the user needs it the most (early
>> on).
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>  kernel/panic.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..3456652 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -468,9 +468,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>
>>  #endif
>>
>> -core_param(panic, panic_timeout, int, 0644);
>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>> +static int __init set_panic_timeout(char *val)
>> +{
>> + return kstrtoint(val, 0, _timeout);
>> +}
>> +early_param("panic_timeout", set_panic_timeout);
>> +
>>  static int __init oops_setup(char *s)
>>  {
>>   if (!s)
>
> hmmso this changes the comand-line parameter panic=x  to:
> panic_timeout=x. The naming might not be the best, but we are
> really stuck with it at this point.

Hmm, right, it should be "panic".

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/2] panic: setup panic_timeout early

[Felipe Contreras]

On Wed, Nov 27, 2013 at 9:47 AM, Jason Baron jba...@akamai.com wrote:
 On 11/27/2013 01:17 AM, Felipe Contreras wrote:
 Otherwise we might not reboot when the user needs it the most (early
 on).

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
  kernel/panic.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

 diff --git a/kernel/panic.c b/kernel/panic.c
 index b6c482c..3456652 100644
 --- a/kernel/panic.c
 +++ b/kernel/panic.c
 @@ -468,9 +468,14 @@ EXPORT_SYMBOL(__stack_chk_fail);

  #endif

 -core_param(panic, panic_timeout, int, 0644);
  core_param(pause_on_oops, pause_on_oops, int, 0644);

 +static int __init set_panic_timeout(char *val)
 +{
 + return kstrtoint(val, 0, panic_timeout);
 +}
 +early_param(panic_timeout, set_panic_timeout);
 +
  static int __init oops_setup(char *s)
  {
   if (!s)

 hmmso this changes the comand-line parameter panic=x  to:
 panic_timeout=x. The naming might not be the best, but we are
 really stuck with it at this point.

Hmm, right, it should be panic.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 0/2] Setup panic params early

[Felipe Contreras]

Exactly the same as v3, but with an unlrelated patch on top as well.

Felipe Contreras (2):
  panic: setup panic_timeout early
  panic: setup panic_on_oops early

 kernel/panic.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 2/2] panic: setup panic_on_oops early

[Felipe Contreras]

For consistency.

Signed-off-by: Felipe Contreras 
---
 kernel/panic.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 3456652..2256838 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,7 +468,11 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(pause_on_oops, pause_on_oops, int, 0644);
+static int __init set_panic_on_oops(char *val)
+{
+   return kstrtoint(val, 0, _on_oops);
+}
+early_param("panic_on_oops", set_panic_on_oops);
 
 static int __init set_panic_timeout(char *val)
 {
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 1/2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras 
---
 kernel/panic.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..3456652 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   return kstrtoint(val, 0, _timeout);
+}
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: Make panic_timeout configurable

[Felipe Contreras]

On Tue, Nov 19, 2013 at 1:15 AM, Ingo Molnar  wrote:
>
> * Andrew Morton  wrote:
>
>> On Mon, 18 Nov 2013 21:04:36 + (GMT) Jason Baron  
>> wrote:
>>
>> > The panic_timeout value can be set via the command line option 'panic=x', 
>> > or via
>> > /proc/sys/kernel/panic, however that is not sufficient when the panic 
>> > occurs
>> > before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>> > so that we can set the desired value from the .config.
>> >
>> > The default panic_timeout value continues to be 0 - wait forever, except 
>> > for
>> > powerpc and mips, which have been defaulted to 180 and 5 respectively. This
>> > is in keeping with the fact that these arches already set panic_timeout in
>> > their arch init code. However, I found three exceptions- two in mips and 
>> > one in
>> > powerpc where the settings didn't match these default values. In those 
>> > cases, I
>> > left the arch code so it continues to override, in case the user has not 
>> > changed
>> > from the default. It would nice if these arches had one default value, or 
>> > if we
>> > could determine the correct setting at compile-time.
>>
>> Felipe is proposing a simpler patch ("panic: setup panic_timeout
>> early") which switches to early_param().  Is that sufficient for the
>> (undescribed!) failure which you are presumably observing?
>
> Also note that that patch is still incomplete: if panic_timeout is
> switched to early_param() then closely related functionality such as
> pause_on_oops should be moved early as well...

If that was the case, then whomever made the "oops" param generic sent
an incomplete patch because it's also early_param, and it's
interesting why you didn't have that problem with that patch, but you
do with this one.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3 v3] build in panic_timeout value

[Felipe Contreras]

On Tue, Nov 26, 2013 at 5:17 AM, Ingo Molnar  wrote:
> * Jason Baron  wrote:

>> I've now separated out the arch bits into separate patches.
>> Hopefully, it makes review easier. I also didn't address moving the
>> 'panic_timeout' command-line parameter up as an 'early_param()'. I
>> think it might make sense to move it up, especially for distro
>> kernels, but its not a need here, so I didn't want to just shove it
>> in. If needed, I think it can come in separately, as it shoudn't
>> affect this series.
>
> The series looks good to me, I've applied the patches to
> tip:core/debug.
>
> If Felipe Contreras's fix patch looks good to you then it would also
> be nice if you could send me that as well, on top of your patches.

Why? Because for ad hominem reasons you wouldn't take the patch itself
even though it's obviously good?

> That fix patch had only one remaining bug/problem, last I checked: if
> panic_timeout is turned into an early_param() then pause_on_oops
> should obviously also be turned into an early param.

That is not a "problem" with the patch itself, and it's the first time
this "problem" has been mentioned at all.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3 v3] build in panic_timeout value

[Felipe Contreras]

On Tue, Nov 26, 2013 at 5:17 AM, Ingo Molnar mi...@kernel.org wrote:
 * Jason Baron jba...@akamai.com wrote:

 I've now separated out the arch bits into separate patches.
 Hopefully, it makes review easier. I also didn't address moving the
 'panic_timeout' command-line parameter up as an 'early_param()'. I
 think it might make sense to move it up, especially for distro
 kernels, but its not a need here, so I didn't want to just shove it
 in. If needed, I think it can come in separately, as it shoudn't
 affect this series.

 The series looks good to me, I've applied the patches to
 tip:core/debug.

 If Felipe Contreras's fix patch looks good to you then it would also
 be nice if you could send me that as well, on top of your patches.

Why? Because for ad hominem reasons you wouldn't take the patch itself
even though it's obviously good?

 That fix patch had only one remaining bug/problem, last I checked: if
 panic_timeout is turned into an early_param() then pause_on_oops
 should obviously also be turned into an early param.

That is not a problem with the patch itself, and it's the first time
this problem has been mentioned at all.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: Make panic_timeout configurable

[Felipe Contreras]

On Tue, Nov 19, 2013 at 1:15 AM, Ingo Molnar mi...@kernel.org wrote:

 * Andrew Morton a...@linux-foundation.org wrote:

 On Mon, 18 Nov 2013 21:04:36 + (GMT) Jason Baron jba...@akamai.com 
 wrote:

  The panic_timeout value can be set via the command line option 'panic=x', 
  or via
  /proc/sys/kernel/panic, however that is not sufficient when the panic 
  occurs
  before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
  so that we can set the desired value from the .config.
 
  The default panic_timeout value continues to be 0 - wait forever, except 
  for
  powerpc and mips, which have been defaulted to 180 and 5 respectively. This
  is in keeping with the fact that these arches already set panic_timeout in
  their arch init code. However, I found three exceptions- two in mips and 
  one in
  powerpc where the settings didn't match these default values. In those 
  cases, I
  left the arch code so it continues to override, in case the user has not 
  changed
  from the default. It would nice if these arches had one default value, or 
  if we
  could determine the correct setting at compile-time.

 Felipe is proposing a simpler patch (panic: setup panic_timeout
 early) which switches to early_param().  Is that sufficient for the
 (undescribed!) failure which you are presumably observing?

 Also note that that patch is still incomplete: if panic_timeout is
 switched to early_param() then closely related functionality such as
 pause_on_oops should be moved early as well...

If that was the case, then whomever made the oops param generic sent
an incomplete patch because it's also early_param, and it's
interesting why you didn't have that problem with that patch, but you
do with this one.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 2/2] panic: setup panic_on_oops early

[Felipe Contreras]

For consistency.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 kernel/panic.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 3456652..2256838 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,7 +468,11 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(pause_on_oops, pause_on_oops, int, 0644);
+static int __init set_panic_on_oops(char *val)
+{
+   return kstrtoint(val, 0, panic_on_oops);
+}
+early_param(panic_on_oops, set_panic_on_oops);
 
 static int __init set_panic_timeout(char *val)
 {
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 1/2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 kernel/panic.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..3456652 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   return kstrtoint(val, 0, panic_timeout);
+}
+early_param(panic_timeout, set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 0/2] Setup panic params early

[Felipe Contreras]

Exactly the same as v3, but with an unlrelated patch on top as well.

Felipe Contreras (2):
  panic: setup panic_timeout early
  panic: setup panic_on_oops early

 kernel/panic.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

On Sat, Nov 16, 2013 at 2:21 PM, Levente Kurusa  wrote:
> 2013-11-16 18:32 keltezéssel, Felipe Contreras írta:
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(memparse);
>>
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(get_option);
>>
>> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>> +EXPORT_SYMBOL(get_options);
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>  lib/cmdline.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/cmdline.c b/lib/cmdline.c
>> index 5466333..d4932f7 100644
>> --- a/lib/cmdline.c
>> +++ b/lib/cmdline.c
>> @@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
>>
>>   return 1;
>>  }
>> +EXPORT_SYMBOL(get_option);
>>
>>  /**
>>   *   get_options - Parse a string into a list of integers
>> @@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
>>   ints[0] = i - 1;
>>   return (char *)str;
>>  }
>> +EXPORT_SYMBOL(get_options);
>>
>>  /**
>>   *   memparse - parse a string with mem suffixes into a number
>> @@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char 
>> **retptr)
>>
>>   return ret;
>>  }
>> -
>>  EXPORT_SYMBOL(memparse);
>> -EXPORT_SYMBOL(get_option);
>> -EXPORT_SYMBOL(get_options);
>>
>
> I don't know about this one, but I have seen lots of files where 
> EXPORT_SYMBOLs were
> listed at the end of the file. To avoid misunderstanding, I still think that 
> having the
> exports after the function is more appropriate.

If that was appropriate then checkpatch should be updated to remove
that warning, but presumably it's desirable to have them one next to
the other.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar  wrote:
> * Felipe Contreras  wrote:

> Anyway, the fact that you are argumentative even with Linus gives me
> little hope that you will improve your communication patterns with
> _me_, so I'm personally done arguing with you.

How am I being argumentative? I avoided all arguments.

>> You don't want to argue? Then don't argue. Apply the patch. [...]
>
> *Plonk*.

This is exactly the opposite of what is conducive to less argumentation.

If you are not going to comment on the patch, then don't say anything.

The patch is good, and the fact that both you and Linus are avoiding
any comments in the patch itself doesn't speak well for your
intentions to avoid argumentation.

So I ask you again. Do you see something wrong with the patch?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/5] kstrtox: remove redundant casts

[Felipe Contreras]

The temporary variable is of the same type as the cast, so it's
redundant.

Signed-off-by: Felipe Contreras 
---
 lib/kstrtox.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index ec8da78..649b74b 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -176,7 +176,7 @@ int _kstrtoul(const char *s, unsigned int base, unsigned 
long *res)
rv = kstrtoull(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (unsigned long long)(unsigned long)tmp)
+   if (tmp != (unsigned long)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -192,7 +192,7 @@ int _kstrtol(const char *s, unsigned int base, long *res)
rv = kstrtoll(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (long long)(long)tmp)
+   if (tmp != (long)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -223,7 +223,7 @@ int kstrtouint(const char *s, unsigned int base, unsigned 
int *res)
rv = kstrtoull(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (unsigned long long)(unsigned int)tmp)
+   if (tmp != (unsigned int)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -254,7 +254,7 @@ int kstrtoint(const char *s, unsigned int base, int *res)
rv = kstrtoll(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (long long)(int)tmp)
+   if (tmp != (int)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -269,7 +269,7 @@ int kstrtou16(const char *s, unsigned int base, u16 *res)
rv = kstrtoull(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (unsigned long long)(u16)tmp)
+   if (tmp != (u16)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -284,7 +284,7 @@ int kstrtos16(const char *s, unsigned int base, s16 *res)
rv = kstrtoll(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (long long)(s16)tmp)
+   if (tmp != (s16)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -299,7 +299,7 @@ int kstrtou8(const char *s, unsigned int base, u8 *res)
rv = kstrtoull(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (unsigned long long)(u8)tmp)
+   if (tmp != (u8)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -314,7 +314,7 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
rv = kstrtoll(s, base, );
if (rv < 0)
return rv;
-   if (tmp != (long long)(s8)tmp)
+   if (tmp != (s8)tmp)
return -ERANGE;
*res = tmp;
return 0;
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/5] cmdline: fix style issues

[Felipe Contreras]

WARNING: space prohibited between function name and open parenthesis '('
+int get_option (char **str, int *pint)

WARNING: space prohibited between function name and open parenthesis '('
+   *pint = simple_strtol (cur, str, 0);

ERROR: trailing whitespace
+ $

WARNING: please, no spaces at the start of a line
+ $

WARNING: space prohibited between function name and open parenthesis '('
+   res = get_option ((char **), ints + i);

Signed-off-by: Felipe Contreras 
---
 lib/cmdline.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index eb67911..5466333 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -49,13 +49,13 @@ static int get_range(char **str, int *pint)
  * 3 - hyphen found to denote a range
  */
 
-int get_option (char **str, int *pint)
+int get_option(char **str, int *pint)
 {
char *cur = *str;
 
if (!cur || !(*cur))
return 0;
-   *pint = simple_strtol (cur, str, 0);
+   *pint = simple_strtol(cur, str, 0);
if (cur == *str)
return 0;
if (**str == ',') {
@@ -84,13 +84,13 @@ int get_option (char **str, int *pint)
  * the parse to end (typically a null terminator, if @str is
  * completely parseable).
  */
- 
+
 char *get_options(const char *str, int nints, int *ints)
 {
int res, i = 1;
 
while (i < nints) {
-   res = get_option ((char **), ints + i);
+   res = get_option((char **), ints + i);
if (res == 0)
break;
if (res == 3) {
@@ -153,7 +153,6 @@ unsigned long long memparse(const char *ptr, char **retptr)
return ret;
 }
 
-
 EXPORT_SYMBOL(memparse);
 EXPORT_SYMBOL(get_option);
 EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(memparse);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_option);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_options);

Signed-off-by: Felipe Contreras 
---
 lib/cmdline.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5466333..d4932f7 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
 
return 1;
 }
+EXPORT_SYMBOL(get_option);
 
 /**
  * get_options - Parse a string into a list of integers
@@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
ints[0] = i - 1;
return (char *)str;
 }
+EXPORT_SYMBOL(get_options);
 
 /**
  * memparse - parse a string with mem suffixes into a number
@@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char **retptr)
 
return ret;
 }
-
 EXPORT_SYMBOL(memparse);
-EXPORT_SYMBOL(get_option);
-EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/5] params: improve standard definitions

[Felipe Contreras]

We are repeating the functionality of kstrtol in param_set_long, and the
same for kstrtoint. We can get rid of the extra code by using the right
functions.

Signed-off-by: Felipe Contreras 
---
 kernel/params.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index c00d5b5..48e1a81 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -227,17 +227,10 @@ int parse_args(const char *doing,
 }
 
 /* Lazy bastard, eh? */
-#define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)  
\
+#define STANDARD_PARAM_DEF(name, type, format, strtolfn)   \
int param_set_##name(const char *val, const struct kernel_param *kp) \
{   \
-   tmptype l;  \
-   int ret;\
-   \
-   ret = strtolfn(val, 0, ); \
-   if (ret < 0 || ((type)l != l))  \
-   return ret < 0 ? ret : -EINVAL; \
-   *((type *)kp->arg) = l; \
-   return 0;   \
+   return strtolfn(val, 0, (type *)kp->arg);   \
}   \
int param_get_##name(char *buffer, const struct kernel_param *kp) \
{   \
@@ -253,13 +246,13 @@ int parse_args(const char *doing,
EXPORT_SYMBOL(param_ops_##name)
 
 
-STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(short, short, "%hi", long, kstrtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
-STANDARD_PARAM_DEF(uint, unsigned int, "%u", unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(long, long, "%li", long, kstrtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, kstrtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
+STANDARD_PARAM_DEF(short, short, "%hi", kstrtos16);
+STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", kstrtou16);
+STANDARD_PARAM_DEF(int, int, "%i", kstrtoint);
+STANDARD_PARAM_DEF(uint, unsigned int, "%u", kstrtouint);
+STANDARD_PARAM_DEF(long, long, "%li", kstrtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", kstrtoul);
 
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/5] kstrtox: remove redundant cleanup

[Felipe Contreras]

We can't reach the cleanup code unless the flag KSTRTOX_OVERFLOW is not
set, so there's not no point in clearing a bit that we know is not set.

Signed-off-by: Felipe Contreras 
---
 lib/kstrtox.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index f78ae0c..ec8da78 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -92,7 +92,6 @@ static int _kstrtoull(const char *s, unsigned int base, 
unsigned long long *res)
rv = _parse_integer(s, base, &_res);
if (rv & KSTRTOX_OVERFLOW)
return -ERANGE;
-   rv &= ~KSTRTOX_OVERFLOW;
if (rv == 0)
return -EINVAL;
s += rv;
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/5] Command line related cleanups

[Felipe Contreras]

Hi,

These became apparent in the review process of a new command line parameter.

Felipe Contreras (5):
  kstrtox: remove redundant cleanup
  cmdline: fix style issues
  cmdline: declare exported symbols immediately
  kstrtox: remove redundant casts
  params: improve standard definitions

 kernel/params.c | 25 +
 lib/cmdline.c   | 14 ++
 lib/kstrtox.c   | 17 -
 3 files changed, 23 insertions(+), 33 deletions(-)

-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/5] kstrtox: remove redundant cleanup

[Felipe Contreras]

We can't reach the cleanup code unless the flag KSTRTOX_OVERFLOW is not
set, so there's not no point in clearing a bit that we know is not set.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 lib/kstrtox.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index f78ae0c..ec8da78 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -92,7 +92,6 @@ static int _kstrtoull(const char *s, unsigned int base, 
unsigned long long *res)
rv = _parse_integer(s, base, _res);
if (rv  KSTRTOX_OVERFLOW)
return -ERANGE;
-   rv = ~KSTRTOX_OVERFLOW;
if (rv == 0)
return -EINVAL;
s += rv;
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/5] Command line related cleanups

[Felipe Contreras]

Hi,

These became apparent in the review process of a new command line parameter.

Felipe Contreras (5):
  kstrtox: remove redundant cleanup
  cmdline: fix style issues
  cmdline: declare exported symbols immediately
  kstrtox: remove redundant casts
  params: improve standard definitions

 kernel/params.c | 25 +
 lib/cmdline.c   | 14 ++
 lib/kstrtox.c   | 17 -
 3 files changed, 23 insertions(+), 33 deletions(-)

-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(memparse);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_option);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
+EXPORT_SYMBOL(get_options);

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 lib/cmdline.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5466333..d4932f7 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -67,6 +67,7 @@ int get_option(char **str, int *pint)
 
return 1;
 }
+EXPORT_SYMBOL(get_option);
 
 /**
  * get_options - Parse a string into a list of integers
@@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
ints[0] = i - 1;
return (char *)str;
 }
+EXPORT_SYMBOL(get_options);
 
 /**
  * memparse - parse a string with mem suffixes into a number
@@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char **retptr)
 
return ret;
 }
-
 EXPORT_SYMBOL(memparse);
-EXPORT_SYMBOL(get_option);
-EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/5] params: improve standard definitions

[Felipe Contreras]

We are repeating the functionality of kstrtol in param_set_long, and the
same for kstrtoint. We can get rid of the extra code by using the right
functions.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 kernel/params.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index c00d5b5..48e1a81 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -227,17 +227,10 @@ int parse_args(const char *doing,
 }
 
 /* Lazy bastard, eh? */
-#define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)  
\
+#define STANDARD_PARAM_DEF(name, type, format, strtolfn)   \
int param_set_##name(const char *val, const struct kernel_param *kp) \
{   \
-   tmptype l;  \
-   int ret;\
-   \
-   ret = strtolfn(val, 0, l); \
-   if (ret  0 || ((type)l != l))  \
-   return ret  0 ? ret : -EINVAL; \
-   *((type *)kp-arg) = l; \
-   return 0;   \
+   return strtolfn(val, 0, (type *)kp-arg);   \
}   \
int param_get_##name(char *buffer, const struct kernel_param *kp) \
{   \
@@ -253,13 +246,13 @@ int parse_args(const char *doing,
EXPORT_SYMBOL(param_ops_##name)
 
 
-STANDARD_PARAM_DEF(byte, unsigned char, %hhu, unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(short, short, %hi, long, kstrtol);
-STANDARD_PARAM_DEF(ushort, unsigned short, %hu, unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(int, int, %i, long, kstrtol);
-STANDARD_PARAM_DEF(uint, unsigned int, %u, unsigned long, kstrtoul);
-STANDARD_PARAM_DEF(long, long, %li, long, kstrtol);
-STANDARD_PARAM_DEF(ulong, unsigned long, %lu, unsigned long, kstrtoul);
+STANDARD_PARAM_DEF(byte, unsigned char, %hhu, kstrtou8);
+STANDARD_PARAM_DEF(short, short, %hi, kstrtos16);
+STANDARD_PARAM_DEF(ushort, unsigned short, %hu, kstrtou16);
+STANDARD_PARAM_DEF(int, int, %i, kstrtoint);
+STANDARD_PARAM_DEF(uint, unsigned int, %u, kstrtouint);
+STANDARD_PARAM_DEF(long, long, %li, kstrtol);
+STANDARD_PARAM_DEF(ulong, unsigned long, %lu, kstrtoul);
 
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/5] kstrtox: remove redundant casts

[Felipe Contreras]

The temporary variable is of the same type as the cast, so it's
redundant.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 lib/kstrtox.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index ec8da78..649b74b 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -176,7 +176,7 @@ int _kstrtoul(const char *s, unsigned int base, unsigned 
long *res)
rv = kstrtoull(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (unsigned long long)(unsigned long)tmp)
+   if (tmp != (unsigned long)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -192,7 +192,7 @@ int _kstrtol(const char *s, unsigned int base, long *res)
rv = kstrtoll(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (long long)(long)tmp)
+   if (tmp != (long)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -223,7 +223,7 @@ int kstrtouint(const char *s, unsigned int base, unsigned 
int *res)
rv = kstrtoull(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (unsigned long long)(unsigned int)tmp)
+   if (tmp != (unsigned int)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -254,7 +254,7 @@ int kstrtoint(const char *s, unsigned int base, int *res)
rv = kstrtoll(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (long long)(int)tmp)
+   if (tmp != (int)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -269,7 +269,7 @@ int kstrtou16(const char *s, unsigned int base, u16 *res)
rv = kstrtoull(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (unsigned long long)(u16)tmp)
+   if (tmp != (u16)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -284,7 +284,7 @@ int kstrtos16(const char *s, unsigned int base, s16 *res)
rv = kstrtoll(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (long long)(s16)tmp)
+   if (tmp != (s16)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -299,7 +299,7 @@ int kstrtou8(const char *s, unsigned int base, u8 *res)
rv = kstrtoull(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (unsigned long long)(u8)tmp)
+   if (tmp != (u8)tmp)
return -ERANGE;
*res = tmp;
return 0;
@@ -314,7 +314,7 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
rv = kstrtoll(s, base, tmp);
if (rv  0)
return rv;
-   if (tmp != (long long)(s8)tmp)
+   if (tmp != (s8)tmp)
return -ERANGE;
*res = tmp;
return 0;
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/5] cmdline: fix style issues

[Felipe Contreras]

WARNING: space prohibited between function name and open parenthesis '('
+int get_option (char **str, int *pint)

WARNING: space prohibited between function name and open parenthesis '('
+   *pint = simple_strtol (cur, str, 0);

ERROR: trailing whitespace
+ $

WARNING: please, no spaces at the start of a line
+ $

WARNING: space prohibited between function name and open parenthesis '('
+   res = get_option ((char **)str, ints + i);

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 lib/cmdline.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index eb67911..5466333 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -49,13 +49,13 @@ static int get_range(char **str, int *pint)
  * 3 - hyphen found to denote a range
  */
 
-int get_option (char **str, int *pint)
+int get_option(char **str, int *pint)
 {
char *cur = *str;
 
if (!cur || !(*cur))
return 0;
-   *pint = simple_strtol (cur, str, 0);
+   *pint = simple_strtol(cur, str, 0);
if (cur == *str)
return 0;
if (**str == ',') {
@@ -84,13 +84,13 @@ int get_option (char **str, int *pint)
  * the parse to end (typically a null terminator, if @str is
  * completely parseable).
  */
- 
+
 char *get_options(const char *str, int nints, int *ints)
 {
int res, i = 1;
 
while (i  nints) {
-   res = get_option ((char **)str, ints + i);
+   res = get_option((char **)str, ints + i);
if (res == 0)
break;
if (res == 3) {
@@ -153,7 +153,6 @@ unsigned long long memparse(const char *ptr, char **retptr)
return ret;
 }
 
-
 EXPORT_SYMBOL(memparse);
 EXPORT_SYMBOL(get_option);
 EXPORT_SYMBOL(get_options);
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Sat, Nov 16, 2013 at 11:45 AM, Ingo Molnar mi...@kernel.org wrote:
 * Felipe Contreras felipe.contre...@gmail.com wrote:

 Anyway, the fact that you are argumentative even with Linus gives me
 little hope that you will improve your communication patterns with
 _me_, so I'm personally done arguing with you.

How am I being argumentative? I avoided all arguments.

 You don't want to argue? Then don't argue. Apply the patch. [...]

 *Plonk*.

This is exactly the opposite of what is conducive to less argumentation.

If you are not going to comment on the patch, then don't say anything.

The patch is good, and the fact that both you and Linus are avoiding
any comments in the patch itself doesn't speak well for your
intentions to avoid argumentation.

So I ask you again. Do you see something wrong with the patch?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] cmdline: declare exported symbols immediately

[Felipe Contreras]

On Sat, Nov 16, 2013 at 2:21 PM, Levente Kurusa le...@linux.com wrote:
 2013-11-16 18:32 keltezéssel, Felipe Contreras írta:
 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
 +EXPORT_SYMBOL(memparse);

 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
 +EXPORT_SYMBOL(get_option);

 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
 +EXPORT_SYMBOL(get_options);

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
  lib/cmdline.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

 diff --git a/lib/cmdline.c b/lib/cmdline.c
 index 5466333..d4932f7 100644
 --- a/lib/cmdline.c
 +++ b/lib/cmdline.c
 @@ -67,6 +67,7 @@ int get_option(char **str, int *pint)

   return 1;
  }
 +EXPORT_SYMBOL(get_option);

  /**
   *   get_options - Parse a string into a list of integers
 @@ -112,6 +113,7 @@ char *get_options(const char *str, int nints, int *ints)
   ints[0] = i - 1;
   return (char *)str;
  }
 +EXPORT_SYMBOL(get_options);

  /**
   *   memparse - parse a string with mem suffixes into a number
 @@ -152,7 +154,4 @@ unsigned long long memparse(const char *ptr, char 
 **retptr)

   return ret;
  }
 -
  EXPORT_SYMBOL(memparse);
 -EXPORT_SYMBOL(get_option);
 -EXPORT_SYMBOL(get_options);


 I don't know about this one, but I have seen lots of files where 
 EXPORT_SYMBOLs were
 listed at the end of the file. To avoid misunderstanding, I still think that 
 having the
 exports after the function is more appropriate.

If that was appropriate then checkpatch should be updated to remove
that warning, but presumably it's desirable to have them one next to
the other.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
 wrote:
> On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
>  wrote:
>>
>> I haven't seen a single complaint about this commit message, so I
>> don't see what is your point.
>
> My point is that I have sixteen pointless messages in my mbox, half of
> which are due to just your argumentative nature.

This is clearly not the point you were making, but I won't argue why.

You don't want to argue? Then don't argue. Apply the patch. Unless you
see something wrong with the patch. Do you see something wrong with
the patch?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 1:55 PM, Linus Torvalds
 wrote:

> It's not just code. You need to also explain *why* people should apply
> it, and stop the f*cking idiotic arguing every time somebody comments
> about your patches.

Doesn't this explain it?

--
panic: setup panic_timeout early

Otherwise we might not reboot when the user needs it the most (early
on).
--

I haven't seen a single complaint about this commit message, so I
don't see what is your point.

You didn't address what I said at all. If the code is technically
correct *and* it is clear there's a reason why the patch should be
applied, who sent the patch should be irrelevant, because even if that
person is problematic, and there's something lacking in the patch,
somebody else can take it from there and fix the remaining issues,
because if there's a reason the patch should be applied, it should be
applied.

In this particular case it is clear we want panic_timeout to work
early on, therefore a patch should be applied to achieve that, and you
might as well simply apply the patch I sent, even though *I* sent it,
because it's technically correct, and the need is explained. Why
wouldn't you?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras 
---

Since v2:

There's no need for a temporary variable because kstrtoint() is already taking
care of that (if there's an error panic_timeout is not updated).

diff --git a/kernel/panic.c b/kernel/panic.c
index d865263..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -472,15 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
 
 static int __init set_panic_timeout(char *val)
 {
-   long timeout;
-   int ret;
-
-   ret = kstrtol(val, 0, );
-   if (ret < 0)
-   return ret;
-
-   panic_timeout = timeout;
-   return 0;
+   return kstrtoint(val, 0, _timeout);
 }
 
 early_param("panic_timeout", set_panic_timeout);

 kernel/panic.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,15 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   return kstrtoint(val, 0, _timeout);
+}
+
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 7:12 AM, Levente Kurusa  wrote:

> No, you don't want timeout to be a long, and instead of kstrtol, you should 
> use
> kstrtoint(char *s, int base, int *res)
> which is defined in lib/ktstrtox.c

And if you look into kstrtoint() you would see that it's doing
*exactly* the same I'm doing; using a temporary bigger integer, same
as STANDARD_PARAM_DEF.

> Also, a bit of advice: Calm down. If you continue to act childish nobody will 
> take
> you seriously. Before acting like you are the king here, please at least take 
> the time
> to research other options.

I am irrelevant, the code is what matters, and panic_timeout should be
set early on, regardless of who is sending the patch.

Moreover, if you are going to claim that I didn't research other
options, then the guy that did this:

  STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);

Didn't research other options either, because it should be:

  STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Presumably it doesn't matter much, but it seems it does matter when
*I* am the one sending the patch.

And what exactly is childish about asking this question?

> So you would rather have this?
>
>   kstrtol(val, 0, (long *));

The answer should be: no, use kstrtoint(), but that wasn't your
answer, was it? Your answer basically repeated what Ingo said, so I
had to go step by step to the conclusion of 'kstrtol(val, 0, (long
*));'. And then I asked you a question: "What else do you
suggest to fix the problem that kstrtol() expects a long?" *Now* your
answer is useful. Why is that childish?

Now, in this process a few things are clear to me:

1) loglevel should use kstrtoint() as well, not get_option
2) get_option() should use kstrtoint(); it's using simple_strtol() and
it's not even checking for overflows
3) It should be STANDARD_PARAM_DEF(int, int, "%i", int, kstrtoint);

Don't you think it's little bit of a stretch to say that *I* didn't do
my research, when in fact I did, and none of the parameters code is
using kstrtoint() as it should, even Ingo himself told me I should use
simple_strtol(), which is actually deprecated.

My patch is simply doing what similar code is already doing. Why don't
you take a step back and accept the possibility that a) I did do my
research, and b) I wasn't being childish in asking how to make kstrtol
work (given that Ingo suggested to use simple_strtol()). I think you
should take your own advice and calm down, maybe even take back what
you said. Maybe I was combative, and maybe I made the wrong
assumptions, but at least I didn't throw insults, nor called anybody
names like "childish".

Anyway, I will update the patch to sue kstrtoint(), and I would gladly
fix the existing code for issues 1) 2) and 3) *if* you or anybody
agrees they should be using kstrtoint() as well, I'm not in the mood
of going through Ingo's review process again for something so trivial.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 7:12 AM, Levente Kurusa le...@linux.com wrote:

 No, you don't want timeout to be a long, and instead of kstrtol, you should 
 use
 kstrtoint(char *s, int base, int *res)
 which is defined in lib/ktstrtox.c

And if you look into kstrtoint() you would see that it's doing
*exactly* the same I'm doing; using a temporary bigger integer, same
as STANDARD_PARAM_DEF.

 Also, a bit of advice: Calm down. If you continue to act childish nobody will 
 take
 you seriously. Before acting like you are the king here, please at least take 
 the time
 to research other options.

I am irrelevant, the code is what matters, and panic_timeout should be
set early on, regardless of who is sending the patch.

Moreover, if you are going to claim that I didn't research other
options, then the guy that did this:

  STANDARD_PARAM_DEF(int, int, %i, long, kstrtol);

Didn't research other options either, because it should be:

  STANDARD_PARAM_DEF(int, int, %i, int, kstrtoint);

Presumably it doesn't matter much, but it seems it does matter when
*I* am the one sending the patch.

And what exactly is childish about asking this question?

 So you would rather have this?

   kstrtol(val, 0, (long *)timeout);

The answer should be: no, use kstrtoint(), but that wasn't your
answer, was it? Your answer basically repeated what Ingo said, so I
had to go step by step to the conclusion of 'kstrtol(val, 0, (long
*)timeout);'. And then I asked you a question: What else do you
suggest to fix the problem that kstrtol() expects a long? *Now* your
answer is useful. Why is that childish?

Now, in this process a few things are clear to me:

1) loglevel should use kstrtoint() as well, not get_option
2) get_option() should use kstrtoint(); it's using simple_strtol() and
it's not even checking for overflows
3) It should be STANDARD_PARAM_DEF(int, int, %i, int, kstrtoint);

Don't you think it's little bit of a stretch to say that *I* didn't do
my research, when in fact I did, and none of the parameters code is
using kstrtoint() as it should, even Ingo himself told me I should use
simple_strtol(), which is actually deprecated.

My patch is simply doing what similar code is already doing. Why don't
you take a step back and accept the possibility that a) I did do my
research, and b) I wasn't being childish in asking how to make kstrtol
work (given that Ingo suggested to use simple_strtol()). I think you
should take your own advice and calm down, maybe even take back what
you said. Maybe I was combative, and maybe I made the wrong
assumptions, but at least I didn't throw insults, nor called anybody
names like childish.

Anyway, I will update the patch to sue kstrtoint(), and I would gladly
fix the existing code for issues 1) 2) and 3) *if* you or anybody
agrees they should be using kstrtoint() as well, I'm not in the mood
of going through Ingo's review process again for something so trivial.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---

Since v2:

There's no need for a temporary variable because kstrtoint() is already taking
care of that (if there's an error panic_timeout is not updated).

diff --git a/kernel/panic.c b/kernel/panic.c
index d865263..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -472,15 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
 
 static int __init set_panic_timeout(char *val)
 {
-   long timeout;
-   int ret;
-
-   ret = kstrtol(val, 0, timeout);
-   if (ret  0)
-   return ret;
-
-   panic_timeout = timeout;
-   return 0;
+   return kstrtoint(val, 0, panic_timeout);
 }
 
 early_param(panic_timeout, set_panic_timeout);

 kernel/panic.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..fa3c187 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,15 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   return kstrtoint(val, 0, panic_timeout);
+}
+
+early_param(panic_timeout, set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 1:55 PM, Linus Torvalds
torva...@linux-foundation.org wrote:

 It's not just code. You need to also explain *why* people should apply
 it, and stop the f*cking idiotic arguing every time somebody comments
 about your patches.

Doesn't this explain it?

--
panic: setup panic_timeout early

Otherwise we might not reboot when the user needs it the most (early
on).
--

I haven't seen a single complaint about this commit message, so I
don't see what is your point.

You didn't address what I said at all. If the code is technically
correct *and* it is clear there's a reason why the patch should be
applied, who sent the patch should be irrelevant, because even if that
person is problematic, and there's something lacking in the patch,
somebody else can take it from there and fix the remaining issues,
because if there's a reason the patch should be applied, it should be
applied.

In this particular case it is clear we want panic_timeout to work
early on, therefore a patch should be applied to achieve that, and you
might as well simply apply the patch I sent, even though *I* sent it,
because it's technically correct, and the need is explained. Why
wouldn't you?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Fri, Nov 15, 2013 at 2:15 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Fri, Nov 15, 2013 at 12:10 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:

 I haven't seen a single complaint about this commit message, so I
 don't see what is your point.

 My point is that I have sixteen pointless messages in my mbox, half of
 which are due to just your argumentative nature.

This is clearly not the point you were making, but I won't argue why.

You don't want to argue? Then don't argue. Apply the patch. Unless you
see something wrong with the patch. Do you see something wrong with
the patch?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Hi,

Sending again since the previous one was rejected by the server.

On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
 wrote:
> Levente Kurusa wrote:
>> 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
>> > On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar  wrote:
>> >>
>> >> * Felipe Contreras  wrote:
>> >>
>> >>> Otherwise we might not reboot when the user needs it the most (early
>> >>> on).
>> >>>
>> >>> Signed-off-by: Felipe Contreras 
>> >>> ---
>> >>>
>> >> [...]
>> >>>
>> >>> diff --git a/kernel/panic.c b/kernel/panic.c
>> >>> index b6c482c..d865263 100644
>> >>> --- a/kernel/panic.c
>> >>> +++ b/kernel/panic.c
>> >>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>> >>>
>> >>>  #endif
>> >>>
>> >>> -core_param(panic, panic_timeout, int, 0644);
>> >>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>> >>>
>> >>> +static int __init set_panic_timeout(char *val)
>> >>> +{
>> >>> + long timeout;
>> >>> + int ret;
>> >>> +
>> >>> + ret = kstrtol(val, 0, );
>> >>> + if (ret < 0)
>> >>> + return ret;
>> >>> +
>> >>> + panic_timeout = timeout;
>> >>> + return 0;
>> >>> +}
>> >>
>> >> I think the type of the 'timeout' local variable should match the type of
>> >> 'panic_timeout' (which is 'int', not 'long').
>> >
>> > So you would rather have this?
>> >
>> >   kstrtol(val, 0, (long *));
>> >
>> > Couldn't that potentially write the value beyond the memory allocated
>> > to 'timeout'?
>> >
>>
>> No, 'panic_timeout' is a variable of type 'int'.
>> Your 'long timeout;' line is wrong and should say 'int timeout;'
>
> Oh really? Something like this?
>
>   --- a/kernel/panic.c
>   +++ b/kernel/panic.c
>   @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
>static int __init set_panic_timeout(char *val)
>{
>   -   long timeout;
>   +   int timeout;
>   int ret;
>
>   ret = kstrtol(val, 0, );
>
> And then what happens?
>
>   kernel/panic.c: In function ‘set_panic_timeout’:
>   kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from 
> incompatible pointer type [enabled by default]
> ret = kstrtol(val, 0, );
> ^
>   In file included from include/linux/debug_locks.h:4:0,
>from kernel/panic.c:11:
>   include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is 
> of type ‘int *’
>static inline int __must_check kstrtol(const char *s, unsigned int base, 
> long *res)
>
> To fix that warning you need this:
>   
>^
>   --- a/kernel/panic.c
>   +++ b/kernel/panic.c
>   @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);
>
>static int __init set_panic_timeout(char *val)
>{
>   -   long timeout;
>   +   int timeout;
>   int ret;
>
>   -   ret = kstrtol(val, 0, );
>   +   ret = kstrtol(val, 0, (long *));
>   if (ret < 0)
>   return ret;
>
>
> Which is the only logical conclusion I arrived to. What else do you suggest to
> fix the problem that kstrtol() expects a long? And since this fix is not what
> we want because we would be writing to the wrong memory, we don't want 
> 'timeout'
> to be int.
>
> Therefore 'timeout' should be a long. How is that not clear?
>
> You can even see that it's already done this way for parameters:
>
>   STANDARD_PARAM_DEF(int, int, "%i", long, kstrtol);
>
>   #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)   
>   \
>   int param_set_##name(const char *val, const struct kernel_param 
> *kp) \
>   { \
>   tmptype l;\
>   int ret;  \
>   \
>   ret = strtolfn(val, 0, );   \
>   if (ret < 0 || ((type)l != l))  
>   \
>   return ret < 0 ? ret : -EINVAL; 
>   \
>   *((type *)kp->arg) = l; 
>   \
>   return 0; \
>   } \
>
> So yes, we obviously want the temporary variable 'timeout' to be a long, even
> though the final destination is an int.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> Otherwise we might not reboot when the user needs it the most (early
>> on).
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>
> [...]
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..d865263 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>
>>  #endif
>>
>> -core_param(panic, panic_timeout, int, 0644);
>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>> +static int __init set_panic_timeout(char *val)
>> +{
>> + long timeout;
>> + int ret;
>> +
>> + ret = kstrtol(val, 0, );
>> + if (ret < 0)
>> + return ret;
>> +
>> + panic_timeout = timeout;
>> + return 0;
>> +}
>
> I think the type of the 'timeout' local variable should match the type of
> 'panic_timeout' (which is 'int', not 'long').

So you would rather have this?

  kstrtol(val, 0, (long *));

Couldn't that potentially write the value beyond the memory allocated
to 'timeout'?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 Otherwise we might not reboot when the user needs it the most (early
 on).

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---

 [...]

 diff --git a/kernel/panic.c b/kernel/panic.c
 index b6c482c..d865263 100644
 --- a/kernel/panic.c
 +++ b/kernel/panic.c
 @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);

  #endif

 -core_param(panic, panic_timeout, int, 0644);
  core_param(pause_on_oops, pause_on_oops, int, 0644);

 +static int __init set_panic_timeout(char *val)
 +{
 + long timeout;
 + int ret;
 +
 + ret = kstrtol(val, 0, timeout);
 + if (ret  0)
 + return ret;
 +
 + panic_timeout = timeout;
 + return 0;
 +}

 I think the type of the 'timeout' local variable should match the type of
 'panic_timeout' (which is 'int', not 'long').

So you would rather have this?

  kstrtol(val, 0, (long *)timeout);

Couldn't that potentially write the value beyond the memory allocated
to 'timeout'?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Hi,

Sending again since the previous one was rejected by the server.

On Thu, Nov 14, 2013 at 3:25 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 Levente Kurusa wrote:
 2013-11-14 12:16 keltezéssel, Felipe Contreras írta:
  On Tue, Nov 12, 2013 at 6:03 PM, Ingo Molnar mi...@kernel.org wrote:
 
  * Felipe Contreras felipe.contre...@gmail.com wrote:
 
  Otherwise we might not reboot when the user needs it the most (early
  on).
 
  Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
  ---
 
  [...]
 
  diff --git a/kernel/panic.c b/kernel/panic.c
  index b6c482c..d865263 100644
  --- a/kernel/panic.c
  +++ b/kernel/panic.c
  @@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
   #endif
 
  -core_param(panic, panic_timeout, int, 0644);
   core_param(pause_on_oops, pause_on_oops, int, 0644);
 
  +static int __init set_panic_timeout(char *val)
  +{
  + long timeout;
  + int ret;
  +
  + ret = kstrtol(val, 0, timeout);
  + if (ret  0)
  + return ret;
  +
  + panic_timeout = timeout;
  + return 0;
  +}
 
  I think the type of the 'timeout' local variable should match the type of
  'panic_timeout' (which is 'int', not 'long').
 
  So you would rather have this?
 
kstrtol(val, 0, (long *)timeout);
 
  Couldn't that potentially write the value beyond the memory allocated
  to 'timeout'?
 

 No, 'panic_timeout' is a variable of type 'int'.
 Your 'long timeout;' line is wrong and should say 'int timeout;'

 Oh really? Something like this?

   --- a/kernel/panic.c
   +++ b/kernel/panic.c
   @@ -472,7 +472,7 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);

static int __init set_panic_timeout(char *val)
{
   -   long timeout;
   +   int timeout;
   int ret;

   ret = kstrtol(val, 0, timeout);

 And then what happens?

   kernel/panic.c: In function ‘set_panic_timeout’:
   kernel/panic.c:478:2: warning: passing argument 3 of ‘kstrtol’ from 
 incompatible pointer type [enabled by default]
 ret = kstrtol(val, 0, timeout);
 ^
   In file included from include/linux/debug_locks.h:4:0,
from kernel/panic.c:11:
   include/linux/kernel.h:268:95: note: expected ‘long int *’ but argument is 
 of type ‘int *’
static inline int __must_check kstrtol(const char *s, unsigned int base, 
 long *res)

 To fix that warning you need this:
   
^
   --- a/kernel/panic.c
   +++ b/kernel/panic.c
   @@ -472,10 +472,10 @@ core_param(pause_on_oops, pause_on_oops, int, 0644);

static int __init set_panic_timeout(char *val)
{
   -   long timeout;
   +   int timeout;
   int ret;

   -   ret = kstrtol(val, 0, timeout);
   +   ret = kstrtol(val, 0, (long *)timeout);
   if (ret  0)
   return ret;


 Which is the only logical conclusion I arrived to. What else do you suggest to
 fix the problem that kstrtol() expects a long? And since this fix is not what
 we want because we would be writing to the wrong memory, we don't want 
 'timeout'
 to be int.

 Therefore 'timeout' should be a long. How is that not clear?

 You can even see that it's already done this way for parameters:

   STANDARD_PARAM_DEF(int, int, %i, long, kstrtol);

   #define STANDARD_PARAM_DEF(name, type, format, tmptype, strtolfn)   
   \
   int param_set_##name(const char *val, const struct kernel_param 
 *kp) \
   { \
   tmptype l;\
   int ret;  \
   \
   ret = strtolfn(val, 0, l);   \
   if (ret  0 || ((type)l != l))  
   \
   return ret  0 ? ret : -EINVAL; 
   \
   *((type *)kp-arg) = l; 
   \
   return 0; \
   } \

 So yes, we obviously want the temporary variable 'timeout' to be a long, even
 though the final destination is an int.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: make oldnoconfig fix?

[Felipe Contreras]

Hi David,

On Mon, Nov 11, 2013 at 12:11 PM, David Cohen
 wrote:

> Is there a plan to fix 'make oldnoconfig' option?
> I currently have need to it :)

I actually rely on this, but didn't know there was this option, this
is what I've been doing since a long time:

  % echo '' | make oldconfig

But it's very hacky and I see tons of errors reported, but it does
what I expect.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o  wrote:
> On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote:
>> > That's exactly what I did. Addressing feedback constructively doesn't
>> > mean do exactly what you say without arguing.
>>
>> Your reply to my routine feedback was obtuse, argumentative and needlessly
>> confrontative - that's not 'constructive'.
>
> Felipe, remember when on the Git list Junio said he would stop trying
> to respond to any patches that had problems because you couldn't
> respond constructively to feedback, and you claimed that you had no
> problems working with other folks, including on the Linux Kernel
> mailing list?

Ingo Molnar != kernel folks, and I don't see any hints of kernel folks
suggesting to drop patch #1 because of non-technical issues.

If the patch is technically correct, conforms to standard practices,
and solves a problem; it gets applied. Isn't that how it works in
Linux?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras 
---

This time using kstrtol() instead of get_option().

Interdiff:

diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -470,12 +470,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
-static int __init set_panic_timeout(char *str)
+static int __init set_panic_timeout(char *val)
 {
-   int timeout;
+   long timeout;
+   int ret;
 
-   if (!get_option(, ))
-   return -EINVAL;
+   ret = kstrtol(val, 0, );
+   if (ret < 0)
+   return ret;
 
panic_timeout = timeout;
return 0;

 kernel/panic.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   long timeout;
+   int ret;
+
+   ret = kstrtol(val, 0, );
+   if (ret < 0)
+   return ret;
+
+   panic_timeout = timeout;
+   return 0;
+}
+
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 8:22 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar  wrote:
>> >
>> > * Felipe Contreras  wrote:
>> >
>> >> Otherwise we might not reboot when the user needs it the most (early
>> >> on).
>> >>
>> >> Signed-off-by: Felipe Contreras 
>> >> ---
>> >>  kernel/panic.c | 14 +-
>> >>  1 file changed, 13 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/kernel/panic.c b/kernel/panic.c
>> >> index b6c482c..46e37ff 100644
>> >> --- a/kernel/panic.c
>> >> +++ b/kernel/panic.c
>> >> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
>> >>
>> >>  #endif
>> >>
>> >> -core_param(panic, panic_timeout, int, 0644);
>> >>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>> >>
>> >> +static int __init set_panic_timeout(char *str)
>> >> +{
>> >> + int timeout;
>> >> +
>> >> + if (!get_option(, ))
>> >> + return -EINVAL;
>> >> +
>> >> + panic_timeout = timeout;
>> >> + return 0;
>> >> +}
>> >> +
>> >> +early_param("panic_timeout", set_panic_timeout);
>> >> +
>> >
>> > For simple integer parameters simple_strtol() is better and (a tiny bit)
>> > faster.
>>
>> simple_strtol() is deprecated in favor of kstrtol().
>
> Yes, true - my main point is that get_option() isn't the right choice
> here, you need to use a single-integer parsing function.

Then somebody made the wrong choice for 'loglevel'.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:54 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar  wrote:
>> >
>> > * Felipe Contreras  wrote:
>> >
>> >> Signed-off-by: Felipe Contreras 
>> >
>> > The changelog is missing and the title is not self-explanatory.
>>
>> Either the local IRQs should be enabled for both the restart and halt
>> blinks, or it shouldn't be enabled for either. Why enable them for
>> halt, but not restart?
>>
>> I think enabling them for restart too makes sense.
>
> Such arguments belong into the changelog, with a description of what was
> done before and what is done after - please use the customary (verbose)
> changelog style we use in the kernel.

I'm not going to re-roll with such description only so you can NAK it
again. If you agree that such an explanation is OK, say so.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> Otherwise we might not reboot when the user needs it the most (early
>> on).
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>  kernel/panic.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..46e37ff 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
>>
>>  #endif
>>
>> -core_param(panic, panic_timeout, int, 0644);
>>  core_param(pause_on_oops, pause_on_oops, int, 0644);
>>
>> +static int __init set_panic_timeout(char *str)
>> +{
>> + int timeout;
>> +
>> + if (!get_option(, ))
>> + return -EINVAL;
>> +
>> + panic_timeout = timeout;
>> + return 0;
>> +}
>> +
>> +early_param("panic_timeout", set_panic_timeout);
>> +
>
> For simple integer parameters simple_strtol() is better and (a tiny bit)
> faster.

simple_strtol() is deprecated in favor of kstrtol().

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH try2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras 
---

This is exactly the same as in the previous try, but now alone since it was
mudded by largely irrelevant patches in the series.

 kernel/panic.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..46e37ff 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *str)
+{
+   int timeout;
+
+   if (!get_option(, ))
+   return -EINVAL;
+
+   panic_timeout = timeout;
+   return 0;
+}
+
+early_param("panic_timeout", set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar  wrote:
>> >
>> > * Felipe Contreras  wrote:
>> >
>> >> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar  wrote:
>> >> >
>> >> > * Felipe Contreras  wrote:
>> >> >
>> >> >> We want to calculate the blinks per second, and instead of making it 5
>> >> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
>> >> >> per second.
>> >> >
>> >> > Please use the customary changelog style we use in the kernel:
>> >> >
>> >> >   " Current code does (A), this has a problem when (B).
>> >> > We can improve this doing (C), because (D)."
>> >>
>> >> A is explained, B is empty, C is explained, D is because it makes sense.
>
> So one problem with your changelog is that you describe the change but
> don't explain a couple of things - for example why you changed '3600' to
> '1000'.

Yes, I am aware of that, and it probably should, but that has nothing
to do with (A)(B)(C) or (D).

>> > NAKed-by: Ingo Molnar 
>>
>> Suit yourself, stay with your buggy code then.
>
> I NAK-ed your patch because your patch has several technical problems.

No, this is why you NAK-ed the patch:

> > A is explained, B is empty, C is explained, D is because it makes sense.
>
> NAKed-by: Ingo Molnar 

That is not a technical problem, that's an allegedly administrative
one. I said I would fix the technical issues.

> To lift the NAK you'll need to address my review feedback constructively.

That's exactly what I did. Addressing feedback constructively doesn't
mean do exactly what you say without arguing.

I will resend the patches separately since you are focusing on the
irrelevant patches and not paying attention to the one I made clear
was the important one, muddying it. I will address the technical and
administrative issues in the 2nd and 3rd patches in the way I think is
best.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> Signed-off-by: Felipe Contreras 
>
> The changelog is missing and the title is not self-explanatory.

Either the local IRQs should be enabled for both the restart and halt
blinks, or it shouldn't be enabled for either. Why enable them for
halt, but not restart?

I think enabling them for restart too makes sense.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar  wrote:
>> >
>> > * Felipe Contreras  wrote:
>> >
>> >> We want to calculate the blinks per second, and instead of making it 5
>> >> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
>> >> per second.
>> >
>> > Please use the customary changelog style we use in the kernel:
>> >
>> >   " Current code does (A), this has a problem when (B).
>> > We can improve this doing (C), because (D)."
>>
>> A is explained, B is empty, C is explained, D is because it makes sense.
>
> NAKed-by: Ingo Molnar 

Suit yourself, stay with your buggy code then.

Patch #1 is the important one anyway.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar  wrote:
>
> * Felipe Contreras  wrote:
>
>> We want to calculate the blinks per second, and instead of making it 5
>> (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
>> per second.
>
> Please use the customary changelog style we use in the kernel:
>
>   " Current code does (A), this has a problem when (B).
> We can improve this doing (C), because (D)."

A is explained, B is empty, C is explained, D is because it makes sense.

>> Signed-off-by: Felipe Contreras 
>> ---
>>  kernel/panic.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index 46e37ff..5a0bdaa 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -25,7 +25,7 @@
>>  #include 
>>
>>  #define PANIC_TIMER_STEP 100
>> -#define PANIC_BLINK_SPD 18
>> +#define PANIC_BLINK_SPD 4
>
> Please while at it also do another patch that renames it to a sane name,
> PANIC_BLINK_SPEED or so.

If I can do that, I would rather use PANIC_BLINK_PER_SECOND.

>>  int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
>>  static unsigned long tainted_mask;
>> @@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
>>   touch_nmi_watchdog();
>>   if (i >= i_next) {
>>   i += panic_blink(state ^= 1);
>> - i_next = i + 3600 / PANIC_BLINK_SPD;
>> + i_next = i + 1000 / PANIC_BLINK_SPD;
>
> This changes a magic value to another magic value.

A magic value that is used all over the kernel, including
kernel/time.c and include/linux/delay.h. I'll change it to
MSEC_PER_SEC if that makes you happy.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 We want to calculate the blinks per second, and instead of making it 5
 (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
 per second.

 Please use the customary changelog style we use in the kernel:

Current code does (A), this has a problem when (B).
 We can improve this doing (C), because (D).

A is explained, B is empty, C is explained, D is because it makes sense.

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
  kernel/panic.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/kernel/panic.c b/kernel/panic.c
 index 46e37ff..5a0bdaa 100644
 --- a/kernel/panic.c
 +++ b/kernel/panic.c
 @@ -25,7 +25,7 @@
  #include linux/nmi.h

  #define PANIC_TIMER_STEP 100
 -#define PANIC_BLINK_SPD 18
 +#define PANIC_BLINK_SPD 4

 Please while at it also do another patch that renames it to a sane name,
 PANIC_BLINK_SPEED or so.

If I can do that, I would rather use PANIC_BLINK_PER_SECOND.

  int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
  static unsigned long tainted_mask;
 @@ -147,7 +147,7 @@ void panic(const char *fmt, ...)
   touch_nmi_watchdog();
   if (i = i_next) {
   i += panic_blink(state ^= 1);
 - i_next = i + 3600 / PANIC_BLINK_SPD;
 + i_next = i + 1000 / PANIC_BLINK_SPD;

 This changes a magic value to another magic value.

A magic value that is used all over the kernel, including
kernel/time.c and include/linux/delay.h. I'll change it to
MSEC_PER_SEC if that makes you happy.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote:
 
  * Felipe Contreras felipe.contre...@gmail.com wrote:
 
  We want to calculate the blinks per second, and instead of making it 5
  (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
  per second.
 
  Please use the customary changelog style we use in the kernel:
 
 Current code does (A), this has a problem when (B).
  We can improve this doing (C), because (D).

 A is explained, B is empty, C is explained, D is because it makes sense.

 NAKed-by: Ingo Molnar mi...@kernel.org

Suit yourself, stay with your buggy code then.

Patch #1 is the important one anyway.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com

 The changelog is missing and the title is not self-explanatory.

Either the local IRQs should be enabled for both the restart and halt
blinks, or it shouldn't be enabled for either. Why enable them for
halt, but not restart?

I think enabling them for restart too makes sense.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:28 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 On Mon, Nov 11, 2013 at 6:49 AM, Ingo Molnar mi...@kernel.org wrote:
 
  * Felipe Contreras felipe.contre...@gmail.com wrote:
 
  On Mon, Nov 11, 2013 at 5:32 AM, Ingo Molnar mi...@kernel.org wrote:
  
   * Felipe Contreras felipe.contre...@gmail.com wrote:
  
   We want to calculate the blinks per second, and instead of making it 5
   (1000 / (3600 / 18)), let's make it 4, so the user can see two blinks
   per second.
  
   Please use the customary changelog style we use in the kernel:
  
  Current code does (A), this has a problem when (B).
   We can improve this doing (C), because (D).
 
  A is explained, B is empty, C is explained, D is because it makes sense.

 So one problem with your changelog is that you describe the change but
 don't explain a couple of things - for example why you changed '3600' to
 '1000'.

Yes, I am aware of that, and it probably should, but that has nothing
to do with (A)(B)(C) or (D).

  NAKed-by: Ingo Molnar mi...@kernel.org

 Suit yourself, stay with your buggy code then.

 I NAK-ed your patch because your patch has several technical problems.

No, this is why you NAK-ed the patch:

  A is explained, B is empty, C is explained, D is because it makes sense.

 NAKed-by: Ingo Molnar mi...@kernel.org

That is not a technical problem, that's an allegedly administrative
one. I said I would fix the technical issues.

 To lift the NAK you'll need to address my review feedback constructively.

That's exactly what I did. Addressing feedback constructively doesn't
mean do exactly what you say without arguing.

I will resend the patches separately since you are focusing on the
irrelevant patches and not paying attention to the one I made clear
was the important one, muddying it. I will address the technical and
administrative issues in the 2nd and 3rd patches in the way I think is
best.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH try2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---

This is exactly the same as in the previous try, but now alone since it was
mudded by largely irrelevant patches in the series.

 kernel/panic.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..46e37ff 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *str)
+{
+   int timeout;
+
+   if (!get_option(str, timeout))
+   return -EINVAL;
+
+   panic_timeout = timeout;
+   return 0;
+}
+
+early_param(panic_timeout, set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 Otherwise we might not reboot when the user needs it the most (early
 on).

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
  kernel/panic.c | 14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)

 diff --git a/kernel/panic.c b/kernel/panic.c
 index b6c482c..46e37ff 100644
 --- a/kernel/panic.c
 +++ b/kernel/panic.c
 @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);

  #endif

 -core_param(panic, panic_timeout, int, 0644);
  core_param(pause_on_oops, pause_on_oops, int, 0644);

 +static int __init set_panic_timeout(char *str)
 +{
 + int timeout;
 +
 + if (!get_option(str, timeout))
 + return -EINVAL;
 +
 + panic_timeout = timeout;
 + return 0;
 +}
 +
 +early_param(panic_timeout, set_panic_timeout);
 +

 For simple integer parameters simple_strtol() is better and (a tiny bit)
 faster.

simple_strtol() is deprecated in favor of kstrtol().

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] panic: enable local IRQs for restart timeout too

[Felipe Contreras]

On Mon, Nov 11, 2013 at 7:54 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 On Mon, Nov 11, 2013 at 7:19 AM, Ingo Molnar mi...@kernel.org wrote:
 
  * Felipe Contreras felipe.contre...@gmail.com wrote:
 
  Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 
  The changelog is missing and the title is not self-explanatory.

 Either the local IRQs should be enabled for both the restart and halt
 blinks, or it shouldn't be enabled for either. Why enable them for
 halt, but not restart?

 I think enabling them for restart too makes sense.

 Such arguments belong into the changelog, with a description of what was
 done before and what is done after - please use the customary (verbose)
 changelog style we use in the kernel.

I'm not going to re-roll with such description only so you can NAK it
again. If you agree that such an explanation is OK, say so.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] panic: setup panic_timeout early

[Felipe Contreras]

On Mon, Nov 11, 2013 at 8:22 AM, Ingo Molnar mi...@kernel.org wrote:

 * Felipe Contreras felipe.contre...@gmail.com wrote:

 On Mon, Nov 11, 2013 at 7:44 AM, Ingo Molnar mi...@kernel.org wrote:
 
  * Felipe Contreras felipe.contre...@gmail.com wrote:
 
  Otherwise we might not reboot when the user needs it the most (early
  on).
 
  Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
  ---
   kernel/panic.c | 14 +-
   1 file changed, 13 insertions(+), 1 deletion(-)
 
  diff --git a/kernel/panic.c b/kernel/panic.c
  index b6c482c..46e37ff 100644
  --- a/kernel/panic.c
  +++ b/kernel/panic.c
  @@ -468,9 +468,21 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
   #endif
 
  -core_param(panic, panic_timeout, int, 0644);
   core_param(pause_on_oops, pause_on_oops, int, 0644);
 
  +static int __init set_panic_timeout(char *str)
  +{
  + int timeout;
  +
  + if (!get_option(str, timeout))
  + return -EINVAL;
  +
  + panic_timeout = timeout;
  + return 0;
  +}
  +
  +early_param(panic_timeout, set_panic_timeout);
  +
 
  For simple integer parameters simple_strtol() is better and (a tiny bit)
  faster.

 simple_strtol() is deprecated in favor of kstrtol().

 Yes, true - my main point is that get_option() isn't the right choice
 here, you need to use a single-integer parsing function.

Then somebody made the wrong choice for 'loglevel'.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] panic: setup panic_timeout early

[Felipe Contreras]

Otherwise we might not reboot when the user needs it the most (early
on).

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---

This time using kstrtol() instead of get_option().

Interdiff:

diff --git a/kernel/panic.c b/kernel/panic.c
index 46e37ff..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -470,12 +470,14 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
-static int __init set_panic_timeout(char *str)
+static int __init set_panic_timeout(char *val)
 {
-   int timeout;
+   long timeout;
+   int ret;
 
-   if (!get_option(str, timeout))
-   return -EINVAL;
+   ret = kstrtol(val, 0, timeout);
+   if (ret  0)
+   return ret;
 
panic_timeout = timeout;
return 0;

 kernel/panic.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..d865263 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -468,9 +468,23 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 #endif
 
-core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
 
+static int __init set_panic_timeout(char *val)
+{
+   long timeout;
+   int ret;
+
+   ret = kstrtol(val, 0, timeout);
+   if (ret  0)
+   return ret;
+
+   panic_timeout = timeout;
+   return 0;
+}
+
+early_param(panic_timeout, set_panic_timeout);
+
 static int __init oops_setup(char *s)
 {
if (!s)
-- 
1.8.4.2+fc1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] panic: improve panic_timeout calculation

[Felipe Contreras]

On Mon, Nov 11, 2013 at 9:32 AM, Theodore Ts'o ty...@mit.edu wrote:
 On Mon, Nov 11, 2013 at 02:52:16PM +0100, Ingo Molnar wrote:
  That's exactly what I did. Addressing feedback constructively doesn't
  mean do exactly what you say without arguing.

 Your reply to my routine feedback was obtuse, argumentative and needlessly
 confrontative - that's not 'constructive'.

 Felipe, remember when on the Git list Junio said he would stop trying
 to respond to any patches that had problems because you couldn't
 respond constructively to feedback, and you claimed that you had no
 problems working with other folks, including on the Linux Kernel
 mailing list?

Ingo Molnar != kernel folks, and I don't see any hints of kernel folks
suggesting to drop patch #1 because of non-technical issues.

If the patch is technically correct, conforms to standard practices,
and solves a problem; it gets applied. Isn't that how it works in
Linux?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: make oldnoconfig fix?

[Felipe Contreras]

Hi David,

On Mon, Nov 11, 2013 at 12:11 PM, David Cohen
david.a.co...@linux.intel.com wrote:

 Is there a plan to fix 'make oldnoconfig' option?
 I currently have need to it :)

I actually rely on this, but didn't know there was this option, this
is what I've been doing since a long time:

  % echo '' | make oldconfig

But it's very hacky and I see tons of errors reported, but it does
what I expect.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 00/16] wl1251 patches from linux-n900 tree

[Felipe Contreras]

On Sat, Oct 26, 2013 at 3:33 PM, Pali Rohár  wrote:
> Hello, I'm sending wl1251 patches from linux-n900 tree [1] for comments. More
> patches come from David's monitor & packet injection work. Patches are tested
> with 3.12 rc5 kernel on Nokia N900.
>
> [1] - https://gitorious.org/linux-n900/linux-n900

How did you test these patches? I get a panic loop immediately after I
bring the interface loop in monitor mode (v3.12).

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 00/16] wl1251 patches from linux-n900 tree

[Felipe Contreras]

On Sat, Oct 26, 2013 at 3:33 PM, Pali Rohár pali.ro...@gmail.com wrote:
 Hello, I'm sending wl1251 patches from linux-n900 tree [1] for comments. More
 patches come from David's monitor  packet injection work. Patches are tested
 with 3.12 rc5 kernel on Nokia N900.

 [1] - https://gitorious.org/linux-n900/linux-n900

How did you test these patches? I get a panic loop immediately after I
bring the interface loop in monitor mode (v3.12).

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/