| Message ID | 20230724223057.1208122-3-quic_eberman@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Implement a PSCI SYSTEM_RESET2 reboot-mode driver | expand |
On 7/25/2023 4:00 AM, Elliot Berman wrote: > Allow the reboot mode type to be wired to magic. > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c > index a646aefa041b..4ea97ccbaf51 100644 > --- a/drivers/power/reset/reboot-mode.c > +++ b/drivers/power/reset/reboot-mode.c > @@ -22,12 +22,8 @@ struct mode_info { > static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot, > const char *cmd, unsigned int *magic) > { > - const char *normal = "normal"; > struct mode_info *info; > > - if (!cmd) > - cmd = normal; > - > list_for_each_entry(info, &reboot->head, list) { > if (!strcmp(info->mode, cmd)) { > *magic = info->magic; > @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block *this, > unsigned int magic; > > reboot = container_of(this, struct reboot_mode_driver, reboot_notifier); > + > + if (!cmd) { > + switch (mode) { IIUC, mode will be filled up with reboot_mode during restart notifier and not reboot notifiers ? > + case REBOOT_COLD: > + cmd = "cold"; > + break; > + case REBOOT_WARM: > + cmd = "warm"; > + break; > + case REBOOT_HARD: > + cmd = "hard"; > + break; > + case REBOOT_SOFT: > + cmd = "soft"; > + break; > + case REBOOT_GPIO: > + cmd = "gpio"; These strings are already there kernel/reboot.c Can it be reused ? #define REBOOT_COLD_STR "cold" #define REBOOT_WARM_STR "warm" #define REBOOT_HARD_STR "hard" #define REBOOT_SOFT_STR "soft" #define REBOOT_GPIO_STR "gpio" #define REBOOT_UNDEFINED_STR "undefined" > + break; > + } > + if (get_reboot_mode_magic(reboot, cmd, &magic)) { Is info->mode is going to filled up with mode-cold, mode-warm and so on from DT to compare against cmd? What if , cmd is not among the one above switch, NULL pointer during strcmp ? -Mukesh > + reboot->write(reboot, magic); > + return NOTIFY_DONE; > + } > + cmd = "normal"; > + } > + > if (get_reboot_mode_magic(reboot, cmd, &magic)) > reboot->write(reboot, magic); >
On 7/25/2023 3:03 AM, Mukesh Ojha wrote: > > > On 7/25/2023 4:00 AM, Elliot Berman wrote: >> Allow the reboot mode type to be wired to magic. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++---- >> 1 file changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/power/reset/reboot-mode.c >> b/drivers/power/reset/reboot-mode.c >> index a646aefa041b..4ea97ccbaf51 100644 >> --- a/drivers/power/reset/reboot-mode.c >> +++ b/drivers/power/reset/reboot-mode.c >> @@ -22,12 +22,8 @@ struct mode_info { >> static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot, >> const char *cmd, unsigned int *magic) >> { >> - const char *normal = "normal"; >> struct mode_info *info; >> - if (!cmd) >> - cmd = normal; >> - >> list_for_each_entry(info, &reboot->head, list) { >> if (!strcmp(info->mode, cmd)) { >> *magic = info->magic; >> @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block >> *this, >> unsigned int magic; >> reboot = container_of(this, struct reboot_mode_driver, >> reboot_notifier); >> + >> + if (!cmd) { >> + switch (mode) { > > IIUC, mode will be filled up with reboot_mode during restart > notifier and not reboot notifiers ? > Ah, you're correct. I should register a restart notifier and not use reboot mode framework. I'll follow similar bindings. >> + case REBOOT_COLD: >> + cmd = "cold"; >> + break; >> + case REBOOT_WARM: >> + cmd = "warm"; >> + break; >> + case REBOOT_HARD: >> + cmd = "hard"; >> + break; >> + case REBOOT_SOFT: >> + cmd = "soft"; >> + break; >> + case REBOOT_GPIO: >> + cmd = "gpio"; > > These strings are already there kernel/reboot.c > Can it be reused ? > > #define REBOOT_COLD_STR "cold" > #define REBOOT_WARM_STR "warm" > #define REBOOT_HARD_STR "hard" > #define REBOOT_SOFT_STR "soft" > #define REBOOT_GPIO_STR "gpio" > #define REBOOT_UNDEFINED_STR "undefined" > > One set of constants are "binding" for devicetree and the other is for sysfs. I think they should be kept separate. >> + break; >> + } >> + if (get_reboot_mode_magic(reboot, cmd, &magic)) { > > Is info->mode is going to filled up with mode-cold, mode-warm and so > on from DT to compare against cmd? > > What if , cmd is not among the one above switch, NULL pointer during > strcmp ? > > -Mukesh > >> + reboot->write(reboot, magic); >> + return NOTIFY_DONE; >> + } >> + cmd = "normal"; >> + } >> + >> if (get_reboot_mode_magic(reboot, cmd, &magic)) >> reboot->write(reboot, magic);
On Tue, Jul 25, 2023 at 02:04:28PM -0700, Elliot Berman wrote: > > > On 7/25/2023 3:03 AM, Mukesh Ojha wrote: > > > > > > On 7/25/2023 4:00 AM, Elliot Berman wrote: > > > Allow the reboot mode type to be wired to magic. > > > > > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > > --- > > > drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++---- > > > 1 file changed, 26 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/power/reset/reboot-mode.c > > > b/drivers/power/reset/reboot-mode.c > > > index a646aefa041b..4ea97ccbaf51 100644 > > > --- a/drivers/power/reset/reboot-mode.c > > > +++ b/drivers/power/reset/reboot-mode.c > > > @@ -22,12 +22,8 @@ struct mode_info { > > > static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot, > > > const char *cmd, unsigned int *magic) > > > { > > > - const char *normal = "normal"; > > > struct mode_info *info; > > > - if (!cmd) > > > - cmd = normal; > > > - > > > list_for_each_entry(info, &reboot->head, list) { > > > if (!strcmp(info->mode, cmd)) { > > > *magic = info->magic; > > > @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct > > > notifier_block *this, > > > unsigned int magic; > > > reboot = container_of(this, struct reboot_mode_driver, > > > reboot_notifier); > > > + > > > + if (!cmd) { > > > + switch (mode) { > > > > IIUC, mode will be filled up with reboot_mode during restart > > notifier and not reboot notifiers ? > > > I went through the patch in isolation and came to the same conclusion on why you are using mode directly here. Now that it is clarified, why not use reboot_mode directly instead of introducing restart notifiers here? Also you might want to clarify that we are using reboot_mode as fallback to wire the magic. Thanks, Pavan
diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c index a646aefa041b..4ea97ccbaf51 100644 --- a/drivers/power/reset/reboot-mode.c +++ b/drivers/power/reset/reboot-mode.c @@ -22,12 +22,8 @@ struct mode_info { static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd, unsigned int *magic) { - const char *normal = "normal"; struct mode_info *info; - if (!cmd) - cmd = normal; - list_for_each_entry(info, &reboot->head, list) { if (!strcmp(info->mode, cmd)) { *magic = info->magic; @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block *this, unsigned int magic; reboot = container_of(this, struct reboot_mode_driver, reboot_notifier); + + if (!cmd) { + switch (mode) { + case REBOOT_COLD: + cmd = "cold"; + break; + case REBOOT_WARM: + cmd = "warm"; + break; + case REBOOT_HARD: + cmd = "hard"; + break; + case REBOOT_SOFT: + cmd = "soft"; + break; + case REBOOT_GPIO: + cmd = "gpio"; + break; + } + if (get_reboot_mode_magic(reboot, cmd, &magic)) { + reboot->write(reboot, magic); + return NOTIFY_DONE; + } + cmd = "normal"; + } + if (get_reboot_mode_magic(reboot, cmd, &magic)) reboot->write(reboot, magic);
Allow the reboot mode type to be wired to magic. Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> --- drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)