| Message ID | 20220427224924.592546-16-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | The panic notifiers refactor | expand |
On 4/27/2022 3:49 PM, Guilherme G. Piccoli wrote: > This patch improves the panic/die notifiers in this driver by > making use of a passed "id" instead of comparing pointer > address; also, it removes an useless prototype declaration > and unnecessary header inclusion. > > This is part of a panic notifiers refactor - this notifier in > the future will be moved to a new list, that encompass the > information notifiers only. > > Fixes: 9eb60880d9a9 ("bus: brcmstb_gisb: add notifier handling") > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> Not sure if the Fixes tag is warranted however as this is a clean up, and not really fixing a bug.
On 02/05/2022 12:38, Florian Fainelli wrote: > [...] > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > Not sure if the Fixes tag is warranted however as this is a clean up, > and not really fixing a bug. Perfect, thanks Florian. I'll add your ACK and remove the fixes tag in V2. Cheers, Guilherme
On Wed 2022-04-27 19:49:09, Guilherme G. Piccoli wrote: > This patch improves the panic/die notifiers in this driver by > making use of a passed "id" instead of comparing pointer > address; also, it removes an useless prototype declaration > and unnecessary header inclusion. > > This is part of a panic notifiers refactor - this notifier in > the future will be moved to a new list, that encompass the > information notifiers only. > > --- a/drivers/bus/brcmstb_gisb.c > +++ b/drivers/bus/brcmstb_gisb.c > @@ -347,25 +346,14 @@ static irqreturn_t brcmstb_gisb_bp_handler(int irq, void *dev_id) > /* > * Dump out gisb errors on die or panic. > */ > -static int dump_gisb_error(struct notifier_block *self, unsigned long v, > - void *p); > - > -static struct notifier_block gisb_die_notifier = { > - .notifier_call = dump_gisb_error, > -}; > - > -static struct notifier_block gisb_panic_notifier = { > - .notifier_call = dump_gisb_error, > -}; > - > static int dump_gisb_error(struct notifier_block *self, unsigned long v, > void *p) > { > struct brcmstb_gisb_arb_device *gdev; > - const char *reason = "panic"; > + const char *reason = "die"; > > - if (self == &gisb_die_notifier) > - reason = "die"; > + if (v == PANIC_NOTIFIER) > + reason = "panic"; IMHO, the check of the @self parameter was the proper solution. "gisb_die_notifier" list uses @val from enum die_val. "gisb_panic_notifier" list uses @val from enum panic_notifier_val. These are unrelated types. It might easily break when someone defines the same constant also in enum die_val. Best Regards, Petr
On 10/05/2022 12:28, Petr Mladek wrote: > [...] > IMHO, the check of the @self parameter was the proper solution. > > "gisb_die_notifier" list uses @val from enum die_val. > "gisb_panic_notifier" list uses @val from enum panic_notifier_val. > > These are unrelated types. It might easily break when > someone defines the same constant also in enum die_val. > > Best Regards, > Petr OK Petr, I'll drop this idea for V2 - will just remove the useless header / prototype then. (CC Florian) Cheers, Guilherme
diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c index 183d5cc37d42..1ea7b015e225 100644 --- a/drivers/bus/brcmstb_gisb.c +++ b/drivers/bus/brcmstb_gisb.c @@ -19,7 +19,6 @@ #include <linux/pm.h> #include <linux/kernel.h> #include <linux/kdebug.h> -#include <linux/notifier.h> #ifdef CONFIG_MIPS #include <asm/traps.h> @@ -347,25 +346,14 @@ static irqreturn_t brcmstb_gisb_bp_handler(int irq, void *dev_id) /* * Dump out gisb errors on die or panic. */ -static int dump_gisb_error(struct notifier_block *self, unsigned long v, - void *p); - -static struct notifier_block gisb_die_notifier = { - .notifier_call = dump_gisb_error, -}; - -static struct notifier_block gisb_panic_notifier = { - .notifier_call = dump_gisb_error, -}; - static int dump_gisb_error(struct notifier_block *self, unsigned long v, void *p) { struct brcmstb_gisb_arb_device *gdev; - const char *reason = "panic"; + const char *reason = "die"; - if (self == &gisb_die_notifier) - reason = "die"; + if (v == PANIC_NOTIFIER) + reason = "panic"; /* iterate over each GISB arb registered handlers */ list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next) @@ -374,6 +362,14 @@ static int dump_gisb_error(struct notifier_block *self, unsigned long v, return NOTIFY_DONE; } +static struct notifier_block gisb_die_notifier = { + .notifier_call = dump_gisb_error, +}; + +static struct notifier_block gisb_panic_notifier = { + .notifier_call = dump_gisb_error, +}; + static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO, gisb_arb_get_timeout, gisb_arb_set_timeout);
This patch improves the panic/die notifiers in this driver by making use of a passed "id" instead of comparing pointer address; also, it removes an useless prototype declaration and unnecessary header inclusion. This is part of a panic notifiers refactor - this notifier in the future will be moved to a new list, that encompass the information notifiers only. Fixes: 9eb60880d9a9 ("bus: brcmstb_gisb: add notifier handling") Cc: Brian Norris <computersforpeace@gmail.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- drivers/bus/brcmstb_gisb.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)