Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri 2020-11-13 22:28:18, Matteo Croce wrote: > On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek wrote: > > > > On Fri 2020-11-13 03:58:49, Matteo Croce wrote: > > > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton > > > wrote: > > > > > > > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce > > > > wrote: > > > > > > > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and > > > > > BOOT_CF9_SAFE is not user selectable, should I simply leave only > > > > > 'pci'? > > > > > > > > > > This way, we'll have the same set of options for both sysfs and > > > > > kernel cmdline. > > > > > > > > Well, you're the reboot expert ;) > > > > > > > > > > So honored! :) > > > > > > > But my $0.02 is yes, let's keep the command-line and sysfs interfaces > > > > in sync and cover it all in documentation. It would of course be > > > > problematic to change the existing reboot= interface. > > > > > > > > I assume that means doing this? > > > > > > > > - #define BOOT_CF9_FORCE_STR "cf9_force" > > > > + #define BOOT_CF9_FORCE_STR "pci" > > > > - #define BOOT_CF9_SAFE_STR "cf9_safe" > > > > > > Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference. > > > > > > The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please > > > both. > > > > The question is whether we should modify/allow to set these values at > > all. > > > > Anyway, we must prevent them on non-x86 architectures because > > the reboot behavior would be undefined there. They could probably > > make a mess even on many x86-architectures. > > > > That's right, but the same can be obtained by passing 'reboot=pci' on > non x86 machines: the cmdline parsing is generic and will set > reboot_type on all arches. Fair enough. Ah, I mixed reboot_type and reboot_mode and looked into wrong part of reboot_setup(). > I looked at the reboot_type usage, there isn't any reference outside > arch/x86. In fact, the parameter is just ignored: > > # uname -m > aarch64 > # cat /proc/cmdline > console=ttyS0,115200n8 reboot=pci > # reboot -ff > Rebooting. > [ 43.893833] reboot: Restarting system Good to know. Thanks for checking. > The same applies for reboot_force, the only flags available on > different architectures are reboot_mode and reboot_cpu. > We could hide some handlers for some architectures. We save some > space, and avoid letting the user set flags which do nothing. I am fine with the current patchset after all. We could always make it more safe when people hit it in the real life. All these worries were because I thought that this interface allowed to set values that were not possible before. > > Anyway, we should get input from some x86-experts about the BOOT_CF9 > > values. > > Sure, x...@kernel.org ? Yes but I do not resist on it any longer. Just if you were going to send another version just by chance then it would be nice to CC x86. Best Regards, Petr
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, Nov 13, 2020 at 9:06 PM Petr Mladek wrote: > > On Fri 2020-11-13 03:58:49, Matteo Croce wrote: > > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton > > wrote: > > > > > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce > > > wrote: > > > > > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and > > > > BOOT_CF9_SAFE is not user selectable, should I simply leave only > > > > 'pci'? > > > > > > > > This way, we'll have the same set of options for both sysfs and kernel > > > > cmdline. > > > > > > Well, you're the reboot expert ;) > > > > > > > So honored! :) > > > > > But my $0.02 is yes, let's keep the command-line and sysfs interfaces > > > in sync and cover it all in documentation. It would of course be > > > problematic to change the existing reboot= interface. > > > > > > I assume that means doing this? > > > > > > - #define BOOT_CF9_FORCE_STR "cf9_force" > > > + #define BOOT_CF9_FORCE_STR "pci" > > > - #define BOOT_CF9_SAFE_STR "cf9_safe" > > > > Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference. > > > > The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both. > > The question is whether we should modify/allow to set these values at > all. > > Anyway, we must prevent them on non-x86 architectures because > the reboot behavior would be undefined there. They could probably > make a mess even on many x86-architectures. > That's right, but the same can be obtained by passing 'reboot=pci' on non x86 machines: the cmdline parsing is generic and will set reboot_type on all arches. > I have to admit it has become much more complicated than I thought. > It brings back Andrew's original question whether this interface is > really needed. Are you going to use in the real life? > Yes, there are some cases. Not to mention complex use cases like let persist some memory regions, or change the page size, if a network driver fails to rmmod with the infamous "unregistered_netdevice: waiting for wlan0 to become free", enabling force on the fly allows to reboot the machine. > The interface might do more harm then good when it allows to set > reboot_type that is not normally accessible or disable it when > it is strictly needed. > I looked at the reboot_type usage, there isn't any reference outside arch/x86. In fact, the parameter is just ignored: # uname -m aarch64 # cat /proc/cmdline console=ttyS0,115200n8 reboot=pci # reboot -ff Rebooting. [ 43.893833] reboot: Restarting system The same applies for reboot_force, the only flags available on different architectures are reboot_mode and reboot_cpu. We could hide some handlers for some architectures. We save some space, and avoid letting the user set flags which do nothing. > Anyway, we should get input from some x86-experts about the BOOT_CF9 > values. > Sure, x...@kernel.org ? Regards, -- per aspera ad upstream
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri 2020-11-13 03:58:49, Matteo Croce wrote: > On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton > wrote: > > > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce > > wrote: > > > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and > > > BOOT_CF9_SAFE is not user selectable, should I simply leave only > > > 'pci'? > > > > > > This way, we'll have the same set of options for both sysfs and kernel > > > cmdline. > > > > Well, you're the reboot expert ;) > > > > So honored! :) > > > But my $0.02 is yes, let's keep the command-line and sysfs interfaces > > in sync and cover it all in documentation. It would of course be > > problematic to change the existing reboot= interface. > > > > I assume that means doing this? > > > > - #define BOOT_CF9_FORCE_STR "cf9_force" > > + #define BOOT_CF9_FORCE_STR "pci" > > - #define BOOT_CF9_SAFE_STR "cf9_safe" > > Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference. > > The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both. The question is whether we should modify/allow to set these values at all. Anyway, we must prevent them on non-x86 architectures because the reboot behavior would be undefined there. They could probably make a mess even on many x86-architectures. I have to admit it has become much more complicated than I thought. It brings back Andrew's original question whether this interface is really needed. Are you going to use in the real life? The interface might do more harm then good when it allows to set reboot_type that is not normally accessible or disable it when it is strictly needed. Anyway, we should get input from some x86-experts about the BOOT_CF9 values. Best Regards, Petr
Re: [PATCH] reboot: Fix variable assignments in type_store
On Wed 2020-11-11 20:50:23, Nathan Chancellor wrote:
> Clang warns:
>
> kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_TRIPLE;
> ~ ^~~
> kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_KBD;
> ~ ^~~~
> kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_BIOS;
> ~ ^
> kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_ACPI;
> ~ ^
> kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_EFI;
> ~ ^~~~
> kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_CF9_FORCE;
> ~ ^~
> kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_CF9_SAFE;
> ~ ^
> 7 warnings generated.
>
> It seems that these assignment should be to reboot_type, not
> reboot_mode. Fix it so there are no more warnings and the code works
> properly.
>
> Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> Signed-off-by: Nathan Chancellor
> ---
> kernel/reboot.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index deba133a071b..8599d0d44aec 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> return -EPERM;
>
> if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> - reboot_mode = BOOT_TRIPLE;
> + reboot_type = BOOT_TRIPLE;
> else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> - reboot_mode = BOOT_KBD;
> + reboot_type = BOOT_KBD;
> else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> - reboot_mode = BOOT_BIOS;
> + reboot_type = BOOT_BIOS;
> else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> - reboot_mode = BOOT_ACPI;
> + reboot_type = BOOT_ACPI;
> else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> - reboot_mode = BOOT_EFI;
> + reboot_type = BOOT_EFI;
> else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR)))
> - reboot_mode = BOOT_CF9_FORCE;
> + reboot_type = BOOT_CF9_FORCE;
> else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> - reboot_mode = BOOT_CF9_SAFE;
> + reboot_type = BOOT_CF9_SAFE;
> else
> return -EINVAL;
Great catch! I guess that it has been a cut mistake when writing
the code.
I feel shame that I have missed it. I think that I have even tested it
but I probably tried only mode_store() and mode_read().
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, Nov 13, 2020 at 3:46 AM Andrew Morton wrote: > > On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce > wrote: > > > At this point, since 'pci' enables BOOT_CF9_FORCE type and > > BOOT_CF9_SAFE is not user selectable, should I simply leave only > > 'pci'? > > This way, we'll have the same set of options for both sysfs and kernel > > cmdline. > > Well, you're the reboot expert ;) > So honored! :) > But my $0.02 is yes, let's keep the command-line and sysfs interfaces > in sync and cover it all in documentation. It would of course be > problematic to change the existing reboot= interface. > > I assume that means doing this? > > - #define BOOT_CF9_FORCE_STR "cf9_force" > + #define BOOT_CF9_FORCE_STR "pci" > - #define BOOT_CF9_SAFE_STR "cf9_safe" Either BOOT_PCI_STR or BOOT_CF9_FORCE_STR, I have no strong preference. The syntax is 'pci' while the enum BOOT_CF9_FORCE, so we can't please both. Regards, -- per aspera ad upstream
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, 13 Nov 2020 02:38:18 +0100 Matteo Croce wrote: > At this point, since 'pci' enables BOOT_CF9_FORCE type and > BOOT_CF9_SAFE is not user selectable, should I simply leave only > 'pci'? > This way, we'll have the same set of options for both sysfs and kernel > cmdline. Well, you're the reboot expert ;) But my $0.02 is yes, let's keep the command-line and sysfs interfaces in sync and cover it all in documentation. It would of course be problematic to change the existing reboot= interface. I assume that means doing this? - #define BOOT_CF9_FORCE_STR "cf9_force" + #define BOOT_CF9_FORCE_STR "pci" - #define BOOT_CF9_SAFE_STR "cf9_safe"
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, Nov 13, 2020 at 2:18 AM Andrew Morton wrote:
>
> On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce
> wrote:
>
> > While writing the script I found that in the documentation I left for
> > 'type' the values from
> > Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for
> > cf9_force reboot.
> > While at it, should we update the doc with the values 'cf9_force' and
> > 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the
> > kernel cmdline?
>
> I looked at Documentation/admin-guide/kernel-parameters.txt's "reboot="
> section and decided that I don't understand your above words :( Can you
> please expand on all of this? Simple akpm-compatible words ;)
>
Heh sorry :)
I misread the code, I thought that BOOT_CF9_SAFE was user selectable
because of the enum value:
enum reboot_type {
...
BOOT_CF9_FORCE = 'p',
BOOT_CF9_SAFE = 'q',
};
But when parsing the cmdline, 'q' is simply ignored, so it's just an
internal flag.
It's used only by arch/x86/kernel/reboot.c in the loop which tries to
reboot in different modes until it succeeds.
The doc is right, never mind.
At this point, since 'pci' enables BOOT_CF9_FORCE type and
BOOT_CF9_SAFE is not user selectable, should I simply leave only
'pci'?
This way, we'll have the same set of options for both sysfs and kernel cmdline.
--
per aspera ad upstream
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, 13 Nov 2020 01:20:29 +0100 Matteo Croce wrote: > While writing the script I found that in the documentation I left for > 'type' the values from > Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for > cf9_force reboot. > While at it, should we update the doc with the values 'cf9_force' and > 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the > kernel cmdline? I looked at Documentation/admin-guide/kernel-parameters.txt's "reboot=" section and decided that I don't understand your above words :( Can you please expand on all of this? Simple akpm-compatible words ;) > In any case, kernel-parameters.txt doesn't mention that reboot=q does > the 'cf9_safe' reboot type, so it must be fixed anyway. Thanks for noticing.
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, Nov 13, 2020 at 1:20 AM Matteo Croce wrote: > > On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton > wrote: > > > > On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor > > wrote: > > > > > Clang warns: > > > > > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode' > > > [-Wenum-conversion] > > > reboot_mode = BOOT_TRIPLE; > > > ~ ^~~ > > > > > > ... > > > > > > --- a/kernel/reboot.c > > > +++ b/kernel/reboot.c > > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, > > > struct kobj_attribute *attr, > > > return -EPERM; > > > > > > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR))) > > > - reboot_mode = BOOT_TRIPLE; > > > + reboot_type = BOOT_TRIPLE; > > > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR))) > > > - reboot_mode = BOOT_KBD; > > > + reboot_type = BOOT_KBD; > > > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR))) > > > - reboot_mode = BOOT_BIOS; > > > + reboot_type = BOOT_BIOS; > > > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR))) > > > - reboot_mode = BOOT_ACPI; > > > + reboot_type = BOOT_ACPI; > > > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR))) > > > - reboot_mode = BOOT_EFI; > > > + reboot_type = BOOT_EFI; > > > else if (!strncmp(buf, BOOT_CF9_FORCE_STR, > > > strlen(BOOT_CF9_FORCE_STR))) > > > - reboot_mode = BOOT_CF9_FORCE; > > > + reboot_type = BOOT_CF9_FORCE; > > > else if (!strncmp(buf, BOOT_CF9_SAFE_STR, > > > strlen(BOOT_CF9_SAFE_STR))) > > > - reboot_mode = BOOT_CF9_SAFE; > > > + reboot_type = BOOT_CF9_SAFE; > > > else > > > return -EINVAL; > > > > This is a fairly dramatic change to the original patch, but it eyeballs > > OK. > > > > Matteo, could you please comment? And preferably retest? > > > > Hi, > > I reviewed the patch and it looks good to me. > I tested it with this script which passes now with Nathan's fix: > > for i in cold warm hard soft gpio; do > echo $i > mode > read j [ $i = $j ] || echo "mode $i = $j" > done > > for i in bios acpi kbd triple efi cf9_force cf9_safe; do > echo $i > type > read j [ $i = $j ] || echo "type $i = $j" > done > > for i in $(seq 0 $(nproc --ignore=1)); do > echo $i > cpu > read j [ $i = $j ] || echo "cpu $i = $j" > done > > for i in 0 1; do > echo $i >force > read j [ $i = $j ] || echo "force $i = $j" > done > > While writing the script I found that in the documentation I left for > 'type' the values from > Documentation/admin-guide/kernel-parameters.txt, which is 'pci' for > cf9_force reboot. > While at it, should we update the doc with the values 'cf9_force' and > 'cf9_safe', or rename to 'pci' and 'pci_safe' to be coherent with the > kernel cmdline? > > In any case, kernel-parameters.txt doesn't mention that reboot=q does > the 'cf9_safe' reboot type, so it must be fixed anyway. > > Regards, > -- > per aspera ad upstream Reviewed-and-tested-by: Matteo Croce -- per aspera ad upstream
Re: [PATCH] reboot: Fix variable assignments in type_store
On Fri, Nov 13, 2020 at 12:13 AM Andrew Morton wrote: > > On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor > wrote: > > > Clang warns: > > > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode' > > [-Wenum-conversion] > > reboot_mode = BOOT_TRIPLE; > > ~ ^~~ > > > > ... > > > > --- a/kernel/reboot.c > > +++ b/kernel/reboot.c > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, > > struct kobj_attribute *attr, > > return -EPERM; > > > > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR))) > > - reboot_mode = BOOT_TRIPLE; > > + reboot_type = BOOT_TRIPLE; > > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR))) > > - reboot_mode = BOOT_KBD; > > + reboot_type = BOOT_KBD; > > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR))) > > - reboot_mode = BOOT_BIOS; > > + reboot_type = BOOT_BIOS; > > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR))) > > - reboot_mode = BOOT_ACPI; > > + reboot_type = BOOT_ACPI; > > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR))) > > - reboot_mode = BOOT_EFI; > > + reboot_type = BOOT_EFI; > > else if (!strncmp(buf, BOOT_CF9_FORCE_STR, > > strlen(BOOT_CF9_FORCE_STR))) > > - reboot_mode = BOOT_CF9_FORCE; > > + reboot_type = BOOT_CF9_FORCE; > > else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR))) > > - reboot_mode = BOOT_CF9_SAFE; > > + reboot_type = BOOT_CF9_SAFE; > > else > > return -EINVAL; > > This is a fairly dramatic change to the original patch, but it eyeballs > OK. > > Matteo, could you please comment? And preferably retest? > Hi, I reviewed the patch and it looks good to me. I tested it with this script which passes now with Nathan's fix: for i in cold warm hard soft gpio; do echo $i > mode read j type read j cpu read j force read j
Re: [PATCH] reboot: Fix variable assignments in type_store
On Wed, 11 Nov 2020 20:50:23 -0700 Nathan Chancellor wrote: > Clang warns: > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration > type 'enum reboot_type' to different enumeration type 'enum reboot_mode' > [-Wenum-conversion] > reboot_mode = BOOT_TRIPLE; > ~ ^~~ > > ... > > --- a/kernel/reboot.c > +++ b/kernel/reboot.c > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct > kobj_attribute *attr, > return -EPERM; > > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR))) > - reboot_mode = BOOT_TRIPLE; > + reboot_type = BOOT_TRIPLE; > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR))) > - reboot_mode = BOOT_KBD; > + reboot_type = BOOT_KBD; > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR))) > - reboot_mode = BOOT_BIOS; > + reboot_type = BOOT_BIOS; > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR))) > - reboot_mode = BOOT_ACPI; > + reboot_type = BOOT_ACPI; > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR))) > - reboot_mode = BOOT_EFI; > + reboot_type = BOOT_EFI; > else if (!strncmp(buf, BOOT_CF9_FORCE_STR, strlen(BOOT_CF9_FORCE_STR))) > - reboot_mode = BOOT_CF9_FORCE; > + reboot_type = BOOT_CF9_FORCE; > else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR))) > - reboot_mode = BOOT_CF9_SAFE; > + reboot_type = BOOT_CF9_SAFE; > else > return -EINVAL; This is a fairly dramatic change to the original patch, but it eyeballs OK. Matteo, could you please comment? And preferably retest?
Re: [PATCH] reboot: Fix variable assignments in type_store
On Thu, Nov 12, 2020 at 06:59:36PM +0100, Matteo Croce wrote:
> On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
> wrote:
> >
> > Hi Matteo,
> >
> > On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> > > On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> > > wrote:
> > > >
> > > > Clang warns:
> > > >
> > > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_TRIPLE;
> > > > ~ ^~~
> > > > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_KBD;
> > > > ~ ^~~~
> > > > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_BIOS;
> > > > ~ ^
> > > > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_ACPI;
> > > > ~ ^
> > > > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_EFI;
> > > > ~ ^~~~
> > > > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_CF9_FORCE;
> > > > ~ ^~
> > > > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > > [-Wenum-conversion]
> > > > reboot_mode = BOOT_CF9_SAFE;
> > > > ~ ^
> > > > 7 warnings generated.
> > > >
> > > > It seems that these assignment should be to reboot_type, not
> > > > reboot_mode. Fix it so there are no more warnings and the code works
> > > > properly.
> > > >
> > > > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > > > Signed-off-by: Nathan Chancellor
> > > > ---
> > > > kernel/reboot.c | 14 +++---
> > > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > > index deba133a071b..8599d0d44aec 100644
> > > > --- a/kernel/reboot.c
> > > > +++ b/kernel/reboot.c
> > > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj,
> > > > struct kobj_attribute *attr,
> > > > return -EPERM;
> > > >
> > > > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > > > - reboot_mode = BOOT_TRIPLE;
> > > > + reboot_type = BOOT_TRIPLE;
> > > > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > > > - reboot_mode = BOOT_KBD;
> > > > + reboot_type = BOOT_KBD;
> > > > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > > > - reboot_mode = BOOT_BIOS;
> > > > + reboot_type = BOOT_BIOS;
> > > > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > > > - reboot_mode = BOOT_ACPI;
> > > > + reboot_type = BOOT_ACPI;
> > > > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > > > - reboot_mode = BOOT_EFI;
> > > > + reboot_type = BOOT_EFI;
> > > > else if (!strncmp(buf, BOOT_CF9_FORCE_STR,
> > > > strlen(BOOT_CF9_FORCE_STR)))
> > > > - reboot_mode = BOOT_CF9_FORCE;
> > > > + reboot_type = BOOT_CF9_FORCE;
> > > > else if (!strncmp(buf, BOOT_CF9_SAFE_STR,
> > > > strlen(BOOT_CF9_SAFE_STR)))
> > > > - reboot_mode = BOOT_CF9_SAFE;
> > > > + reboot_type = BOOT_CF9_SAFE;
> > > > else
> > > > return -EINVAL;
> > > >
> > > >
> > > > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > > > --
> > > > 2.29.2
> > > >
> > >
> > > Hmm, this was introduced in v3 I think.
> > >
> > > I wonder why my compiler doesn't warn about it, the two variables are
> > > defined as different enum type.
> > > I get the same warnings with GCC and -Wenum-conversion.
> >
> > What version of GCC do you have? -Wenum-conversion is a fairly new
> > warning in GCC I think. Although if you get it now, maybe
Re: [PATCH] reboot: Fix variable assignments in type_store
On Thu, Nov 12, 2020 at 6:49 PM Nathan Chancellor
wrote:
>
> Hi Matteo,
>
> On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> > On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> > wrote:
> > >
> > > Clang warns:
> > >
> > > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_TRIPLE;
> > > ~ ^~~
> > > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_KBD;
> > > ~ ^~~~
> > > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_BIOS;
> > > ~ ^
> > > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_ACPI;
> > > ~ ^
> > > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_EFI;
> > > ~ ^~~~
> > > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_CF9_FORCE;
> > > ~ ^~
> > > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > > [-Wenum-conversion]
> > > reboot_mode = BOOT_CF9_SAFE;
> > > ~ ^
> > > 7 warnings generated.
> > >
> > > It seems that these assignment should be to reboot_type, not
> > > reboot_mode. Fix it so there are no more warnings and the code works
> > > properly.
> > >
> > > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > > Signed-off-by: Nathan Chancellor
> > > ---
> > > kernel/reboot.c | 14 +++---
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > index deba133a071b..8599d0d44aec 100644
> > > --- a/kernel/reboot.c
> > > +++ b/kernel/reboot.c
> > > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj,
> > > struct kobj_attribute *attr,
> > > return -EPERM;
> > >
> > > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > > - reboot_mode = BOOT_TRIPLE;
> > > + reboot_type = BOOT_TRIPLE;
> > > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > > - reboot_mode = BOOT_KBD;
> > > + reboot_type = BOOT_KBD;
> > > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > > - reboot_mode = BOOT_BIOS;
> > > + reboot_type = BOOT_BIOS;
> > > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > > - reboot_mode = BOOT_ACPI;
> > > + reboot_type = BOOT_ACPI;
> > > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > > - reboot_mode = BOOT_EFI;
> > > + reboot_type = BOOT_EFI;
> > > else if (!strncmp(buf, BOOT_CF9_FORCE_STR,
> > > strlen(BOOT_CF9_FORCE_STR)))
> > > - reboot_mode = BOOT_CF9_FORCE;
> > > + reboot_type = BOOT_CF9_FORCE;
> > > else if (!strncmp(buf, BOOT_CF9_SAFE_STR,
> > > strlen(BOOT_CF9_SAFE_STR)))
> > > - reboot_mode = BOOT_CF9_SAFE;
> > > + reboot_type = BOOT_CF9_SAFE;
> > > else
> > > return -EINVAL;
> > >
> > >
> > > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > > --
> > > 2.29.2
> > >
> >
> > Hmm, this was introduced in v3 I think.
> >
> > I wonder why my compiler doesn't warn about it, the two variables are
> > defined as different enum type.
> > I get the same warnings with GCC and -Wenum-conversion.
>
> What version of GCC do you have? -Wenum-conversion is a fairly new
> warning in GCC I think. Although if you get it now, maybe it was some
> configuration error?
>
Hi,
the one shipped in Fedora 33:
gcc version 10.2.1 20201016 (Red Hat 10.2.1-6) (GCC)
I enabled -Wenum-compare -Wenum-conversion globally in the root
Makefile and I had only 15 warnings for an 'allyesconfig' x86_64
build.
Maybe it's
Re: [PATCH] reboot: Fix variable assignments in type_store
Hi Matteo,
On Thu, Nov 12, 2020 at 12:26:45PM +0100, Matteo Croce wrote:
> On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
> wrote:
> >
> > Clang warns:
> >
> > kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_TRIPLE;
> > ~ ^~~
> > kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_KBD;
> > ~ ^~~~
> > kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_BIOS;
> > ~ ^
> > kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_ACPI;
> > ~ ^
> > kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_EFI;
> > ~ ^~~~
> > kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_CF9_FORCE;
> > ~ ^~
> > kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> > type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> > [-Wenum-conversion]
> > reboot_mode = BOOT_CF9_SAFE;
> > ~ ^
> > 7 warnings generated.
> >
> > It seems that these assignment should be to reboot_type, not
> > reboot_mode. Fix it so there are no more warnings and the code works
> > properly.
> >
> > Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> > Signed-off-by: Nathan Chancellor
> > ---
> > kernel/reboot.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index deba133a071b..8599d0d44aec 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj,
> > struct kobj_attribute *attr,
> > return -EPERM;
> >
> > if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> > - reboot_mode = BOOT_TRIPLE;
> > + reboot_type = BOOT_TRIPLE;
> > else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> > - reboot_mode = BOOT_KBD;
> > + reboot_type = BOOT_KBD;
> > else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> > - reboot_mode = BOOT_BIOS;
> > + reboot_type = BOOT_BIOS;
> > else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> > - reboot_mode = BOOT_ACPI;
> > + reboot_type = BOOT_ACPI;
> > else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> > - reboot_mode = BOOT_EFI;
> > + reboot_type = BOOT_EFI;
> > else if (!strncmp(buf, BOOT_CF9_FORCE_STR,
> > strlen(BOOT_CF9_FORCE_STR)))
> > - reboot_mode = BOOT_CF9_FORCE;
> > + reboot_type = BOOT_CF9_FORCE;
> > else if (!strncmp(buf, BOOT_CF9_SAFE_STR,
> > strlen(BOOT_CF9_SAFE_STR)))
> > - reboot_mode = BOOT_CF9_SAFE;
> > + reboot_type = BOOT_CF9_SAFE;
> > else
> > return -EINVAL;
> >
> >
> > base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> > --
> > 2.29.2
> >
>
> Hmm, this was introduced in v3 I think.
>
> I wonder why my compiler doesn't warn about it, the two variables are
> defined as different enum type.
> I get the same warnings with GCC and -Wenum-conversion.
What version of GCC do you have? -Wenum-conversion is a fairly new
warning in GCC I think. Although if you get it now, maybe it was some
configuration error?
Regardless, thank you for taking a look at the patch!
Cheers,
Nathan
Re: [PATCH] reboot: Fix variable assignments in type_store
On Thu, Nov 12, 2020 at 4:50 AM Nathan Chancellor
wrote:
>
> Clang warns:
>
> kernel/reboot.c:707:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_TRIPLE;
> ~ ^~~
> kernel/reboot.c:709:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_KBD;
> ~ ^~~~
> kernel/reboot.c:711:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_BIOS;
> ~ ^
> kernel/reboot.c:713:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_ACPI;
> ~ ^
> kernel/reboot.c:715:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_EFI;
> ~ ^~~~
> kernel/reboot.c:717:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_CF9_FORCE;
> ~ ^~
> kernel/reboot.c:719:17: warning: implicit conversion from enumeration
> type 'enum reboot_type' to different enumeration type 'enum reboot_mode'
> [-Wenum-conversion]
> reboot_mode = BOOT_CF9_SAFE;
> ~ ^
> 7 warnings generated.
>
> It seems that these assignment should be to reboot_type, not
> reboot_mode. Fix it so there are no more warnings and the code works
> properly.
>
> Fixes: eab8da48579d ("reboot: allow to specify reboot mode via sysfs")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1197
> Signed-off-by: Nathan Chancellor
> ---
> kernel/reboot.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index deba133a071b..8599d0d44aec 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -704,19 +704,19 @@ static ssize_t type_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> return -EPERM;
>
> if (!strncmp(buf, BOOT_TRIPLE_STR, strlen(BOOT_TRIPLE_STR)))
> - reboot_mode = BOOT_TRIPLE;
> + reboot_type = BOOT_TRIPLE;
> else if (!strncmp(buf, BOOT_KBD_STR, strlen(BOOT_KBD_STR)))
> - reboot_mode = BOOT_KBD;
> + reboot_type = BOOT_KBD;
> else if (!strncmp(buf, BOOT_BIOS_STR, strlen(BOOT_BIOS_STR)))
> - reboot_mode = BOOT_BIOS;
> + reboot_type = BOOT_BIOS;
> else if (!strncmp(buf, BOOT_ACPI_STR, strlen(BOOT_ACPI_STR)))
> - reboot_mode = BOOT_ACPI;
> + reboot_type = BOOT_ACPI;
> else if (!strncmp(buf, BOOT_EFI_STR, strlen(BOOT_EFI_STR)))
> - reboot_mode = BOOT_EFI;
> + reboot_type = BOOT_EFI;
> else if (!strncmp(buf, BOOT_CF9_FORCE_STR,
> strlen(BOOT_CF9_FORCE_STR)))
> - reboot_mode = BOOT_CF9_FORCE;
> + reboot_type = BOOT_CF9_FORCE;
> else if (!strncmp(buf, BOOT_CF9_SAFE_STR, strlen(BOOT_CF9_SAFE_STR)))
> - reboot_mode = BOOT_CF9_SAFE;
> + reboot_type = BOOT_CF9_SAFE;
> else
> return -EINVAL;
>
>
> base-commit: 3e14f70c05cda4794901ed8f976de3a88deebcc0
> --
> 2.29.2
>
Hmm, this was introduced in v3 I think.
I wonder why my compiler doesn't warn about it, the two variables are
defined as different enum type.
I get the same warnings with GCC and -Wenum-conversion.
Thanks,
--
per aspera ad upstream
