Re: [PATCH 0/6] ipmi: Convert to platform remove callback returning void
On Sat, May 25, 2024 at 12:10:38PM +0200, Uwe Kleine-König wrote:
> Hello Corey,
>
> On Thu, Apr 11, 2024 at 10:11:21AM -0500, Corey Minyard wrote:
> > On Thu, Apr 11, 2024 at 09:15:03AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > On Tue, Mar 05, 2024 at 05:26:57PM +0100, Uwe Kleine-König wrote:
> > > > this series converts all drivers below drivers/char/ipmi to struct
> > > > platform_driver::remove_new(). See commit 5c5a7680e67b ("platform:
> > > > Provide a
> > > > remove callback that returns no value") for an extended explanation and
> > > > the
> > > > eventual goal.
> > > >
> > > > All conversations are trivial, because their .remove() callbacks
> > > > returned zero unconditionally.
> > > >
> > > > There are no interdependencies between these patches, so they could be
> > > > picked up individually. But I'd hope that they get picked up all
> > > > together by Corey.
> >
> > Yeah, I was kind of waiting for more reviews, but this is pretty
> > straightforward. I've pulled this into my tree.
>
> These changes are in next since a while but didn't land in Linus tree
> for v6.10-rc1. I intend to send a PR to Greg early next week changing
> platform_driver::remove to match remove_new. If these commits don't make
> it in in time, I'll be so bold and just include the commits from your
> for-next branch in my PR.
I sent them to Linus right after 6.9 dropped, let me resend...
-corey
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH 0/6] ipmi: Convert to platform remove callback returning void
On Thu, Apr 11, 2024 at 09:15:03AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Mar 05, 2024 at 05:26:57PM +0100, Uwe Kleine-König wrote:
> > this series converts all drivers below drivers/char/ipmi to struct
> > platform_driver::remove_new(). See commit 5c5a7680e67b ("platform: Provide a
> > remove callback that returns no value") for an extended explanation and the
> > eventual goal.
> >
> > All conversations are trivial, because their .remove() callbacks
> > returned zero unconditionally.
> >
> > There are no interdependencies between these patches, so they could be
> > picked up individually. But I'd hope that they get picked up all
> > together by Corey.
Yeah, I was kind of waiting for more reviews, but this is pretty
straightforward. I've pulled this into my tree.
-corey
>
> Apart from a (positive) review reply I didn't get any feedback to this
> series. My quest to change the prototype of struct
> platform_driver::remove depends on these patches, so it would be great
> if they made it in during the next merge window.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 12:41:22PM -0700, Allen wrote: > > > > I believe that work queues items are execute single-threaded for a work > > > > queue, so this should be good. I need to test this, though. It may be > > > > that an IPMI device can have its own work queue; it may not be important > > > > to run it in bh context. > > > > > > Fair point. Could you please let me know once you have had a chance to > > > test > > > these changes. Meanwhile, I will work on RFC wherein IPMI will have its > > > own > > > workqueue. > > > > > > Thanks for taking time out to review. > > > > After looking and thinking about it a bit, a BH context is still > > probably the best for this. > > > > I have tested this patch under load and various scenarios and it seems > > to work ok. So: > > > > Tested-by: Corey Minyard > > Acked-by: Corey Minyard > > > > Or I can take this into my tree. > > > > -corey > > Thank you very much. I think it should be okay for you to carry it into > your tree. Ok, it's in my for-next tree. I fixed the directory reference, and I changed all the comments where you changed "tasklet" to "work" to instead say "workqueue". -corey > > - Allen >
Re: [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 10:52:16AM -0700, Allen wrote:
> On Wed, Mar 27, 2024 at 11:05 AM Corey Minyard wrote:
> >
> > I believe that work queues items are execute single-threaded for a work
> > queue, so this should be good. I need to test this, though. It may be
> > that an IPMI device can have its own work queue; it may not be important
> > to run it in bh context.
>
> Fair point. Could you please let me know once you have had a chance to test
> these changes. Meanwhile, I will work on RFC wherein IPMI will have its own
> workqueue.
>
> Thanks for taking time out to review.
After looking and thinking about it a bit, a BH context is still
probably the best for this.
I have tested this patch under load and various scenarios and it seems
to work ok. So:
Tested-by: Corey Minyard
Acked-by: Corey Minyard
Or I can take this into my tree.
-corey
>
> - Allen
>
> >
> > -corey
> >
> > >
> > > Based on the work done by Tejun Heo
> > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-610
> > >
> > > Signed-off-by: Allen Pais
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 30 ++---
> > > 1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > > b/drivers/char/ipmi/ipmi_msghandler.c
> > > index b0eedc4595b3..fce2a2dbdc82 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -36,12 +36,13 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #define IPMI_DRIVER_VERSION "39.2"
> > >
> > > static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
> > > static int ipmi_init_msghandler(void);
> > > -static void smi_recv_tasklet(struct tasklet_struct *t);
> > > +static void smi_recv_work(struct work_struct *t);
> > > static void handle_new_recv_msgs(struct ipmi_smi *intf);
> > > static void need_waiter(struct ipmi_smi *intf);
> > > static int handle_one_recv_msg(struct ipmi_smi *intf,
> > > @@ -498,13 +499,13 @@ struct ipmi_smi {
> > > /*
> > >* Messages queued for delivery. If delivery fails (out of memory
> > >* for instance), They will stay in here to be processed later in a
> > > - * periodic timer interrupt. The tasklet is for handling received
> > > + * periodic timer interrupt. The work is for handling received
> > >* messages directly from the handler.
> > >*/
> > > spinlock_t waiting_rcv_msgs_lock;
> > > struct list_head waiting_rcv_msgs;
> > > atomic_t watchdog_pretimeouts_to_deliver;
> > > - struct tasklet_struct recv_tasklet;
> > > + struct work_struct recv_work;
> > >
> > > spinlock_t xmit_msgs_lock;
> > > struct list_head xmit_msgs;
> > > @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi
> > > *intf)
> > > struct cmd_rcvr *rcvr, *rcvr2;
> > > struct list_head list;
> > >
> > > - tasklet_kill(&intf->recv_tasklet);
> > > + cancel_work_sync(&intf->recv_work);
> > >
> > > free_smi_msg_list(&intf->waiting_rcv_msgs);
> > > free_recv_msg_list(&intf->waiting_events);
> > > @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref)
> > > {
> > > struct ipmi_user *user = container_of(ref, struct ipmi_user,
> > > refcount);
> > >
> > > - /* SRCU cleanup must happen in task context. */
> > > + /* SRCU cleanup must happen in work context. */
> > > queue_work(remove_work_wq, &user->remove_work);
> > > }
> > >
> > > @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner,
> > > intf->curr_seq = 0;
> > > spin_lock_init(&intf->waiting_rcv_msgs_lock);
> > > INIT_LIST_HEAD(&intf->waiting_rcv_msgs);
> > > - tasklet_setup(&intf->recv_tasklet,
> > > - smi_recv_tasklet);
> > > + INIT_WORK(&intf->recv_work, smi_recv_work);
> > > atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0);
> > > spin_lock_init(&intf->xmit_msgs_lock);
> > > INIT_LIST_HEAD(&intf->xmit_msgs);
> > > @@ -4779,7 +4779,7
Re: [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:11PM +, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/infiniband/* from tasklet to BH workqueue.
I think you mean drivers/char/ipmi/* here.
I believe that work queues items are execute single-threaded for a work
queue, so this should be good. I need to test this, though. It may be
that an IPMI device can have its own work queue; it may not be important
to run it in bh context.
-corey
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 30 ++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index b0eedc4595b3..fce2a2dbdc82 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -36,12 +36,13 @@
> #include
> #include
> #include
> +#include
>
> #define IPMI_DRIVER_VERSION "39.2"
>
> static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
> static int ipmi_init_msghandler(void);
> -static void smi_recv_tasklet(struct tasklet_struct *t);
> +static void smi_recv_work(struct work_struct *t);
> static void handle_new_recv_msgs(struct ipmi_smi *intf);
> static void need_waiter(struct ipmi_smi *intf);
> static int handle_one_recv_msg(struct ipmi_smi *intf,
> @@ -498,13 +499,13 @@ struct ipmi_smi {
> /*
>* Messages queued for delivery. If delivery fails (out of memory
>* for instance), They will stay in here to be processed later in a
> - * periodic timer interrupt. The tasklet is for handling received
> + * periodic timer interrupt. The work is for handling received
>* messages directly from the handler.
>*/
> spinlock_t waiting_rcv_msgs_lock;
> struct list_head waiting_rcv_msgs;
> atomic_t watchdog_pretimeouts_to_deliver;
> - struct tasklet_struct recv_tasklet;
> + struct work_struct recv_work;
>
> spinlock_t xmit_msgs_lock;
> struct list_head xmit_msgs;
> @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi *intf)
> struct cmd_rcvr *rcvr, *rcvr2;
> struct list_head list;
>
> - tasklet_kill(&intf->recv_tasklet);
> + cancel_work_sync(&intf->recv_work);
>
> free_smi_msg_list(&intf->waiting_rcv_msgs);
> free_recv_msg_list(&intf->waiting_events);
> @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref)
> {
> struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount);
>
> - /* SRCU cleanup must happen in task context. */
> + /* SRCU cleanup must happen in work context. */
> queue_work(remove_work_wq, &user->remove_work);
> }
>
> @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner,
> intf->curr_seq = 0;
> spin_lock_init(&intf->waiting_rcv_msgs_lock);
> INIT_LIST_HEAD(&intf->waiting_rcv_msgs);
> - tasklet_setup(&intf->recv_tasklet,
> - smi_recv_tasklet);
> + INIT_WORK(&intf->recv_work, smi_recv_work);
> atomic_set(&intf->watchdog_pretimeouts_to_deliver, 0);
> spin_lock_init(&intf->xmit_msgs_lock);
> INIT_LIST_HEAD(&intf->xmit_msgs);
> @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf)
>* To preserve message order, quit if we
>* can't handle a message. Add the message
>* back at the head, this is safe because this
> - * tasklet is the only thing that pulls the
> + * work is the only thing that pulls the
>* messages.
>*/
> list_add(&smi_msg->link, &intf->waiting_rcv_msgs);
> @@ -4812,10 +4812,10 @@ static void handle_new_recv_msgs(struct ipmi_smi
> *intf)
> }
> }
>
> -static void smi_recv_tasklet(struct tasklet_struct *t)
> +static void smi_recv_work(struct work_struct *t)
> {
> unsigned long flags = 0; /* keep us warning-free. */
> - struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet);
> + struct ipmi_smi *intf = from_work(intf, t, recv_work);
> int run_to_completion = intf->run_to_completion;
> struct ipmi_smi_msg *newmsg = NULL;
>
> @@ -4866,7 +4866,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf,
>
> /*
>* To preserve message order, we keep a queue and deliver from
> - * a tasklet.
> + * a work.
>*/
> if (!run_to_completion)
Re: [PATCH 21/30] panic: Introduce the panic pre-reboot notifier list
On Wed, Apr 27, 2022 at 07:49:15PM -0300, Guilherme G. Piccoli wrote: > This patch renames the panic_notifier_list to panic_pre_reboot_list; > the idea is that a subsequent patch will refactor the panic path > in order to better split the notifiers, running some of them very > early, some of them not so early [but still before kmsg_dump()] and > finally, the rest should execute late, after kdump. The latter ones > are now in the panic pre-reboot list - the name comes from the idea > that these notifiers execute before panic() attempts rebooting the > machine (if that option is set). > > We also took the opportunity to clean-up useless header inclusions, > improve some notifier block declarations (e.g. in ibmasm/heartbeat.c) > and more important, change some priorities - we hereby set 2 notifiers > to run late in the list [iss_panic_event() and the IPMI panic_event()] > due to the risks they offer (may not return, for example). > Proper documentation is going to be provided in a subsequent patch, > that effectively refactors the panic path. For the IPMI portion: Acked-by: Corey Minyard Note that the IPMI panic_event() should always return, but it may take some time, especially if the IPMI controller is no longer functional. So the risk of a long delay is there and it makes sense to move it very late. -corey > > Cc: Alex Elder > Cc: Alexander Gordeev > Cc: Anton Ivanov > Cc: Benjamin Herrenschmidt > Cc: Bjorn Andersson > Cc: Boris Ostrovsky > Cc: Chris Zankel > Cc: Christian Borntraeger > Cc: Corey Minyard > Cc: Dexuan Cui > Cc: "H. Peter Anvin" > Cc: Haiyang Zhang > Cc: Heiko Carstens > Cc: Helge Deller > Cc: Ivan Kokshaysky > Cc: "James E.J. Bottomley" > Cc: James Morse > Cc: Johannes Berg > Cc: Juergen Gross > Cc: "K. Y. Srinivasan" > Cc: Mathieu Poirier > Cc: Matt Turner > Cc: Mauro Carvalho Chehab > Cc: Max Filippov > Cc: Michael Ellerman > Cc: Paul Mackerras > Cc: Pavel Machek > Cc: Richard Henderson > Cc: Richard Weinberger > Cc: Robert Richter > Cc: Stefano Stabellini > Cc: Stephen Hemminger > Cc: Sven Schnelle > Cc: Tony Luck > Cc: Vasily Gorbik > Cc: Wei Liu > Signed-off-by: Guilherme G. Piccoli > --- > > Notice that, with this name change, out-of-tree code that relies in the global > exported "panic_notifier_list" will fail to build. We could easily keep the > retro-compatibility by making the old symbol to still exist and point to the > pre_reboot list (or even, keep the old naming). > > But our design choice was to allow the breakage, making users rethink their > notifiers, adding them in the list that fits best. If that wasn't a good > decision, we're open to change it, of course. > Thanks in advance for the review! > > arch/alpha/kernel/setup.c | 4 ++-- > arch/parisc/kernel/pdc_chassis.c | 3 +-- > arch/powerpc/kernel/setup-common.c| 2 +- > arch/s390/kernel/ipl.c| 4 ++-- > arch/um/drivers/mconsole_kern.c | 2 +- > arch/um/kernel/um_arch.c | 2 +- > arch/x86/xen/enlighten.c | 2 +- > arch/xtensa/platforms/iss/setup.c | 4 ++-- > drivers/char/ipmi/ipmi_msghandler.c | 12 +++- > drivers/edac/altera_edac.c| 3 +-- > drivers/hv/vmbus_drv.c| 4 ++-- > drivers/leds/trigger/ledtrig-panic.c | 3 +-- > drivers/misc/ibmasm/heartbeat.c | 16 +--- > drivers/net/ipa/ipa_smp2p.c | 5 ++--- > drivers/parisc/power.c| 4 ++-- > drivers/remoteproc/remoteproc_core.c | 6 -- > drivers/s390/char/con3215.c | 2 +- > drivers/s390/char/con3270.c | 2 +- > drivers/s390/char/sclp_con.c | 2 +- > drivers/s390/char/sclp_vt220.c| 2 +- > drivers/staging/olpc_dcon/olpc_dcon.c | 6 -- > drivers/video/fbdev/hyperv_fb.c | 4 ++-- > include/linux/panic_notifier.h| 2 +- > kernel/panic.c| 9 - > 24 files changed, 54 insertions(+), 51 deletions(-) > > diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c > index d88bdf852753..8ace0d7113b6 100644 > --- a/arch/alpha/kernel/setup.c > +++ b/arch/alpha/kernel/setup.c > @@ -472,8 +472,8 @@ setup_arch(char **cmdline_p) > } > > /* Register a call for panic conditions. */ > - atomic_notifier_chain_register(&panic_notifier_list, > - &alpha_panic_block); > + atomic_notifier_chain_register(&panic_pre_reboot_list, > + &alpha_panic_block); > > #ifndef alpha_using_srm > /* Assume that we've boo
Re: [PATCH v1 1/1] kernel.h: Split out panic and oops helpers
On Tue, Apr 06, 2021 at 04:31:58PM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt to start cleaning it up by splitting out panic and
> oops helpers.
>
> At the same time convert users in header and lib folder to use new header.
> Though for time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
For the IPMI portion:
Acked-by: Corey Minyard
>
> Signed-off-by: Andy Shevchenko
> ---
> arch/powerpc/kernel/setup-common.c | 1 +
> arch/x86/include/asm/desc.h | 1 +
> arch/x86/kernel/cpu/mshyperv.c | 1 +
> arch/x86/kernel/setup.c | 1 +
> drivers/char/ipmi/ipmi_msghandler.c | 1 +
> drivers/remoteproc/remoteproc_core.c | 1 +
> include/asm-generic/bug.h| 3 +-
> include/linux/kernel.h | 84 +---
> include/linux/panic.h| 98
> include/linux/panic_notifier.h | 12
> kernel/hung_task.c | 1 +
> kernel/kexec_core.c | 1 +
> kernel/panic.c | 1 +
> kernel/rcu/tree.c| 2 +
> kernel/sysctl.c | 1 +
> kernel/trace/trace.c | 1 +
> 16 files changed, 126 insertions(+), 84 deletions(-)
> create mode 100644 include/linux/panic.h
> create mode 100644 include/linux/panic_notifier.h
>
> diff --git a/arch/powerpc/kernel/setup-common.c
> b/arch/powerpc/kernel/setup-common.c
> index 74a98fff2c2f..046fe21b5c3b 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -9,6 +9,7 @@
> #undef DEBUG
>
> #include
> +#include
> #include
> #include
> #include
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index 476082a83d1c..ceb12683b6d1 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -9,6 +9,7 @@
> #include
> #include
>
> +#include
> #include
> #include
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 22f13343b5da..9e5c6f2b044d 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 59e5e0903b0c..570699eecf90 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 8a0e97b33cae..e96cb5c4f97a 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -16,6 +16,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 626a6b90fba2..76dd8e2b1e7e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 76a10e0dca9f..719410b93f99 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -17,7 +17,8 @@
> #endif
>
> #ifndef __ASSEMBLY__
> -#include
> +#include
> +#include
>
> #ifdef CONFIG_BUG
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 09035ac67d4b..6c5a05ac1ecb 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -70,7 +71,6 @@
> #define lower_32_bits(n) ((u32)((n) & 0x))
>
> struct completion;
> -struct pt_regs;
> struct user;
>
> #ifdef CONFIG_PREEMPT_VOLUNTARY
> @@ -175,14 +175,6 @@ void __might_fault(const char *file, int line);
> static inline void might_fault(void) { }
> #endif
>
> -extern struct atomic_notifier_head panic_notifier_list;
> -extern long (*panic_blink)(int state);
> -__printf(1, 2)
> -void panic(const char *fmt, ...) __noreturn __cold;
> -void nmi_panic(struct pt_regs *regs, const char *msg);
> -extern void oops_enter(void);
> -extern void oops_exit(void);
> -extern bool o
Re: [PATCH -next] ipmi/powernv: Fix error return code in ipmi_powernv_probe()
On 01/17/2018 10:04 PM, Alexey Kardashevskiy wrote:
On 17/01/18 22:25, Wei Yongjun wrote:
Fix to return a negative error code from the request_irq() error
handling case instead of 0, as done elsewhere in this function.
Signed-off-by: Wei Yongjun
Reviewed-by: Alexey Kardashevskiy
Queued for next release. Thanks!
-corey
---
drivers/char/ipmi/ipmi_powernv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c
index c687c8d..bcf493d 100644
--- a/drivers/char/ipmi/ipmi_powernv.c
+++ b/drivers/char/ipmi/ipmi_powernv.c
@@ -250,8 +250,9 @@ static int ipmi_powernv_probe(struct platform_device *pdev)
ipmi->irq = opal_event_request(prop);
}
- if (request_irq(ipmi->irq, ipmi_opal_event, IRQ_TYPE_LEVEL_HIGH,
- "opal-ipmi", ipmi)) {
+ rc = request_irq(ipmi->irq, ipmi_opal_event, IRQ_TYPE_LEVEL_HIGH,
+"opal-ipmi", ipmi);
+ if (rc) {
dev_warn(dev, "Unable to request irq\n");
goto err_dispose;
}
Re: [PATCH -next] ipmi/powernv: Fix error return code in ipmi_powernv_probe()
On 01/17/2018 05:25 AM, Wei Yongjun wrote:
Fix to return a negative error code from the request_irq() error
handling case instead of 0, as done elsewhere in this function.
I think you are right here. However, you had a bunch of people on the email
that probably didn't need to be there, and didn't have a few that should.
I've adjusted in this response.
This was introduced in change dce143c3381c355ef73be3dd97cf3ca1b15359b8,
you should add a "Fixes:" in the commit text.
I'll let the people that did this code comment, just to be sure, and
wait for
a v2 patch from you after that.
Thanks,
-corey
Signed-off-by: Wei Yongjun
---
drivers/char/ipmi/ipmi_powernv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c
index c687c8d..bcf493d 100644
--- a/drivers/char/ipmi/ipmi_powernv.c
+++ b/drivers/char/ipmi/ipmi_powernv.c
@@ -250,8 +250,9 @@ static int ipmi_powernv_probe(struct platform_device *pdev)
ipmi->irq = opal_event_request(prop);
}
- if (request_irq(ipmi->irq, ipmi_opal_event, IRQ_TYPE_LEVEL_HIGH,
- "opal-ipmi", ipmi)) {
+ rc = request_irq(ipmi->irq, ipmi_opal_event, IRQ_TYPE_LEVEL_HIGH,
+"opal-ipmi", ipmi);
+ if (rc) {
dev_warn(dev, "Unable to request irq\n");
goto err_dispose;
}
Re: [-next PATCH 0/4] sysfs and DEVICE_ATTR_
On 12/19/2017 12:15 PM, Joe Perches wrote: drivers/char/ipmi/ipmi_msghandler.c| 17 +++--- For ipmi: Acked-by: Corey Minyard
Re: [PATCH] char: ipmi: constify ipmi_smi_handlers structures
On 01/15/2017 01:15 PM, Bhumika Goyal wrote:
Declare ipmi_smi_handlers structures as const as they are only passed as
an argument to the function ipmi_register_smi. This argument is of type
const, so ipmi_smi_handlers structures having similar properties can be
declared const too.
Done using Coccinelle:
Thanks, queues for the next Linux patch mayhem.
-corey
@r1 disable optional_qualifier@
identifier i;
position p;
@@
static struct ipmi_smi_handlers i@p={...};
@ok1@
identifier r1.i;
position p;
@@
ipmi_register_smi(&i@p,...)
@bad@
position p!={r1.p,ok1.p};
identifier r1.i;
@@
i@p
@depends on !bad disable optional_qualifier@
identifier r1.i;
@@
+const
struct ipmi_smi_handlers i;
Size details after cross compiling the .o file for powerpc architecture
File size before:
textdata bss dec hex filename
2777 288 03065 bf9 drivers/char/ipmi/ipmi_powernv.o
File size after:
textdata bss dec hex filename
2873192 03065 bf9 drivers/char/ipmi/ipmi_powernv.o
Signed-off-by: Bhumika Goyal
---
drivers/char/ipmi/ipmi_powernv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c
index 6e658aa..b338a4b 100644
--- a/drivers/char/ipmi/ipmi_powernv.c
+++ b/drivers/char/ipmi/ipmi_powernv.c
@@ -196,7 +196,7 @@ static void ipmi_powernv_poll(void *send_info)
ipmi_powernv_recv(smi);
}
-static struct ipmi_smi_handlers ipmi_powernv_smi_handlers = {
+static const struct ipmi_smi_handlers ipmi_powernv_smi_handlers = {
.owner = THIS_MODULE,
.start_processing = ipmi_powernv_start_processing,
.sender = ipmi_powernv_send,
Re: [Openipmi-developer] [PATCH 3/4] ipmi: allow dynamic BMC version information
On 09/12/2016 03:55 AM, Jeremy Kerr wrote:
Currently, it's up to the IPMI SMIs to provide the product & version
details of BMCs behind registered IPMI SMI interfaces. This device ID is
provided on SMI regsitration, and kept around for all future queries.
However, this version information isn't always static. For example, a
BMC may be upgraded at runtime, making the old version information
stale.
This change allows querying the BMC device ID & version information
dynamically. If no static device_id argument is provided to
ipmi_register_smi, then the IPMI core code will perform a Get Device ID
IPMI command to query the version information when needed. We keep a
short-term cache of this information so we don't need to re-query
for every attribute access.
In all, this looks good. I have two minor nits inline below, and
two more major comments here:
I would prefer if it always queried the data, even if the device id
information is provided by the lower level driver. The other
two interfaces query the device id for blacklisting and interface
detection, so they already have it available. If the dev_id is
provided, you can just set the information valid and set the last
query time. Then the disabled state is no longer necessary.
Since the values can change while you are querying them, do
we need some sort of mutex on them? I know the chances are
pretty remote that you would ever hit an issue, but it's at least
theoretically possible.
Thanks,
-corey
Signed-off-by: Jeremy Kerr
---
drivers/char/ipmi/ipmi_msghandler.c | 158 ++--
1 file changed, 153 insertions(+), 5 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 41990bb..55feba0 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -90,6 +90,9 @@ static struct proc_dir_entry *proc_ipmi_root;
*/
#define IPMI_REQUEST_EV_TIME (1000 / (IPMI_TIMEOUT_TIME))
+/* How long should we cache dynamic device IDs? */
+#define IPMI_DYN_DEV_ID_EXPIRY (10 * HZ)
+
/*
* The main "user" data structure.
*/
@@ -192,10 +195,19 @@ struct ipmi_proc_entry {
};
#endif
+enum bmc_dyn_device_id_state {
+ BMC_DEVICE_DYN_ID_STATE_DISABLED,
+ BMC_DEVICE_DYN_ID_STATE_QUERYING,
+ BMC_DEVICE_DYN_ID_STATE_VALID,
+ BMC_DEVICE_DYN_ID_STATE_INVALID,
+};
+
struct bmc_device {
struct platform_device pdev;
struct ipmi_device_id id;
ipmi_smi_t intf;
+ intdyn_id;
+ unsigned long dyn_id_expiry;
unsigned char guid[16];
intguid_set;
char name[16];
@@ -1997,6 +2009,108 @@ int ipmi_request_supply_msgs(ipmi_user_t user,
}
EXPORT_SYMBOL(ipmi_request_supply_msgs);
+static void bmc_device_id_handler(ipmi_smi_t intf, struct ipmi_recv_msg *msg)
+{
+ int rc;
+
+ if ((msg->addr.addr_type != IPMI_SYSTEM_INTERFACE_ADDR_TYPE)
+ || (msg->msg.netfn != IPMI_NETFN_APP_RESPONSE)
+ || (msg->msg.cmd != IPMI_GET_DEVICE_ID_CMD))
+ return;
+
+ /* Do we have a success completion code? */
+ if (msg->msg.data[0] != 0) {
+ intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID;
+ goto out;
+ }
+
+ /* Do we have enough data to parse the device ID details? This doesn't
+* inclde the optional auxilliary version data. */
Minor nit: include is misspelled.
+ if (msg->msg.data_len < 12) {
+ intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID;
+ goto out;
+ }
+
+ rc = ipmi_demangle_device_id(msg->msg.netfn, msg->msg.cmd,
+ msg->msg.data, msg->msg.data_len, &intf->bmc->id);
+ if (rc) {
+ intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_INVALID;
+ goto out;
+ }
+
+ intf->ipmi_version_major = ipmi_version_major(&intf->bmc->id);
+ intf->ipmi_version_minor = ipmi_version_minor(&intf->bmc->id);
+
+ /* All good! mark the dynamic ID as valid, and set its expiration
+* time */
+ intf->bmc->dyn_id = BMC_DEVICE_DYN_ID_STATE_VALID;
+ intf->bmc->dyn_id_expiry = jiffies + IPMI_DYN_DEV_ID_EXPIRY;
+out:
+ wake_up(&intf->waitq);
+}
+
+
+static int bmc_update_device_id(struct bmc_device *bmc)
+{
+ struct ipmi_system_interface_addr si;
+ ipmi_smi_t intf = bmc->intf;
+ struct kernel_ipmi_msg msg;
+ int rc;
+
+ if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_DISABLED)
+ return 0;
+
+ if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_VALID &&
+ time_is_after_jiffies(bmc->dyn_id_expiry))
+ return 0;
+
+ /* do we have a request in progress? Just wait for that. */
+ if (bmc->dyn_id == BMC_DEVICE_DYN_ID_STATE_QUERYING)
+ return wait_event_t
Re: [PATCH] ipmi/powernv: Fix potential invalid pointer dereference
Ok, this looks fine. A couple of question...
Do I need to send this upstream right now? How well has this been tested?
Do you want this backported to 4.0 stable?
-corey
On 07/16/2015 06:16 AM, Neelesh Gupta wrote:
> If the OPAL call to receive the ipmi message fails, then we free up the
> smi message and return. But, the driver still holds the reference to
> old smi message in the 'cur_msg' which can potentially be accessed later
> and freed again leading to kernel oops. To fix it up,
>
> The kernel driver should reset the 'cur_msg' and send reply to the user
> in addition to freeing the message.
>
> Signed-off-by: Neelesh Gupta
> ---
> drivers/char/ipmi/ipmi_powernv.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_powernv.c
> b/drivers/char/ipmi/ipmi_powernv.c
> index 9b409c0..637486d 100644
> --- a/drivers/char/ipmi/ipmi_powernv.c
> +++ b/drivers/char/ipmi/ipmi_powernv.c
> @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct ipmi_smi_powernv
> *smi)
> pr_devel("%s: -> %d (size %lld)\n", __func__,
> rc, rc == 0 ? size : 0);
> if (rc) {
> - spin_unlock_irqrestore(&smi->msg_lock, flags);
> - ipmi_free_smi_msg(msg);
> - return 0;
> + /* If came via the poll, and response was not yet ready */
> + if (rc == OPAL_EMPTY) {
> + spin_unlock_irqrestore(&smi->msg_lock, flags);
> + return 0;
> + } else {
> + smi->cur_msg = NULL;
> + spin_unlock_irqrestore(&smi->msg_lock, flags);
> + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED);
> + return 0;
> + }
> }
>
> if (size < sizeof(*opal_msg)) {
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/8] ipmi/powernv: Convert to irq event interface
On 05/06/2015 10:16 PM, Alistair Popple wrote:
> Convert the opal ipmi driver to use the new irq interface for events.
>
> Signed-off-by: Alistair Popple
> Cc: Corey Minyard
> Cc: openipmi-develo...@lists.sourceforge.net
> ---
>
> Corey,
>
> If this looks ok can you please ack it? Michael Ellerman will then take
> the whole series via the powerpc tree. Thanks.
This looks fine; I don't really understand much of this, but I don't see
any issues.
The only thing I would suggest is passing the irq level
(IRQ_TYPE_LEVEL_HIGH) as
part of the openfirmware data instead of hard-coding it.
Acked-by: Corey Minyard
>
> drivers/char/ipmi/ipmi_powernv.c | 39 ++-
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_powernv.c
> b/drivers/char/ipmi/ipmi_powernv.c
> index 8753b0f..9b409c0 100644
> --- a/drivers/char/ipmi/ipmi_powernv.c
> +++ b/drivers/char/ipmi/ipmi_powernv.c
> @@ -15,6 +15,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
>
> @@ -23,8 +25,7 @@ struct ipmi_smi_powernv {
> u64 interface_id;
> struct ipmi_device_id ipmi_id;
> ipmi_smi_t intf;
> - u64 event;
> - struct notifier_block event_nb;
> + unsigned intirq;
>
> /**
>* We assume that there can only be one outstanding request, so
> @@ -197,15 +198,12 @@ static struct ipmi_smi_handlers
> ipmi_powernv_smi_handlers = {
> .poll = ipmi_powernv_poll,
> };
>
> -static int ipmi_opal_event(struct notifier_block *nb,
> - unsigned long events, void *change)
> +static irqreturn_t ipmi_opal_event(int irq, void *data)
> {
> - struct ipmi_smi_powernv *smi = container_of(nb,
> - struct ipmi_smi_powernv, event_nb);
> + struct ipmi_smi_powernv *smi = data;
>
> - if (events & smi->event)
> - ipmi_powernv_recv(smi);
> - return 0;
> + ipmi_powernv_recv(smi);
> + return IRQ_HANDLED;
> }
>
> static int ipmi_powernv_probe(struct platform_device *pdev)
> @@ -240,13 +238,16 @@ static int ipmi_powernv_probe(struct platform_device
> *pdev)
> goto err_free;
> }
>
> - ipmi->event = 1ull << prop;
> - ipmi->event_nb.notifier_call = ipmi_opal_event;
> + ipmi->irq = irq_of_parse_and_map(dev->of_node, 0);
> + if (!ipmi->irq) {
> + dev_info(dev, "Unable to map irq from device tree\n");
> + ipmi->irq = opal_event_request(prop);
> + }
>
> - rc = opal_notifier_register(&ipmi->event_nb);
> - if (rc) {
> - dev_warn(dev, "OPAL notifier registration failed (%d)\n", rc);
> - goto err_free;
> + if (request_irq(ipmi->irq, ipmi_opal_event, IRQ_TYPE_LEVEL_HIGH,
> + "opal-ipmi", ipmi)) {
> + dev_warn(dev, "Unable to request irq\n");
> + goto err_dispose;
> }
>
> ipmi->opal_msg = devm_kmalloc(dev,
> @@ -271,7 +272,9 @@ static int ipmi_powernv_probe(struct platform_device
> *pdev)
> err_free_msg:
> devm_kfree(dev, ipmi->opal_msg);
> err_unregister:
> - opal_notifier_unregister(&ipmi->event_nb);
> + free_irq(ipmi->irq, ipmi);
> +err_dispose:
> + irq_dispose_mapping(ipmi->irq);
> err_free:
> devm_kfree(dev, ipmi);
> return rc;
> @@ -282,7 +285,9 @@ static int ipmi_powernv_remove(struct platform_device
> *pdev)
> struct ipmi_smi_powernv *smi = dev_get_drvdata(&pdev->dev);
>
> ipmi_unregister_smi(smi->intf);
> - opal_notifier_unregister(&smi->event_nb);
> + free_irq(smi->irq, smi);
> + irq_dispose_mapping(smi->irq);
> +
> return 0;
> }
>
> --
> 1.8.3.2
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] drivers/char/ipmi: Add powernv IPMI driver
This looks good. Can this go into the IPMI tree now, or does it need
work done in the PowerPC tree first?
Thanks,
-corey
On 11/12/2014 01:41 AM, Jeremy Kerr wrote:
> This change adds an initial IPMI driver for powerpc OPAL firmware. The
> interface is exposed entirely through firmware: we have two functions to
> send and receive IPMI messages, and an interrupt notification from the
> firmware to signify that a message is available.
>
> Signed-off-by: Jeremy Kerr
>
> ---
> v2: Update for ipmi/for-next tree, add copyright header
>
> ---
> drivers/char/ipmi/Kconfig|6
> drivers/char/ipmi/Makefile |1
> drivers/char/ipmi/ipmi_powernv.c | 307 +++
> 3 files changed, 314 insertions(+)
>
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index c1fccf4..65fb008 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -72,6 +72,12 @@ config IPMI_SSIF
>have a driver that must be accessed over an I2C bus instead of a
>standard interface. This module requires I2C support.
>
> +config IPMI_POWERNV
> + depends on PPC_POWERNV
> + tristate 'POWERNV (OPAL firmware) IPMI interface'
> + help
> + Provides a driver for OPAL firmware-based IPMI interfaces.
> +
> config IPMI_WATCHDOG
> tristate 'IPMI Watchdog Timer'
> help
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index 115c08d..f3ffde1 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o
> obj-$(CONFIG_IPMI_DEVICE_INTERFACE) += ipmi_devintf.o
> obj-$(CONFIG_IPMI_SI) += ipmi_si.o
> obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
> +obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
> obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
> obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
> diff --git a/drivers/char/ipmi/ipmi_powernv.c
> b/drivers/char/ipmi/ipmi_powernv.c
> new file mode 100644
> index 000..50134ec
> --- /dev/null
> +++ b/drivers/char/ipmi/ipmi_powernv.c
> @@ -0,0 +1,307 @@
> +/*
> + * PowerNV OPAL IPMI driver
> + *
> + * Copyright 2014 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#define pr_fmt(fmt)"ipmi-powernv: " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +
> +struct ipmi_smi_powernv {
> + u64 interface_id;
> + struct ipmi_device_id ipmi_id;
> + ipmi_smi_t intf;
> + u64 event;
> + struct notifier_block event_nb;
> +
> + /**
> + * We assume that there can only be one outstanding request, so
> + * keep the pending message in cur_msg. We protect this from concurrent
> + * updates through send & recv calls, (and consequently opal_msg, which
> + * is in-use when cur_msg is set) with msg_lock
> + */
> + spinlock_t msg_lock;
> + struct ipmi_smi_msg *cur_msg;
> + struct opal_ipmi_msg*opal_msg;
> +};
> +
> +static int ipmi_powernv_start_processing(void *send_info, ipmi_smi_t intf)
> +{
> + struct ipmi_smi_powernv *smi = send_info;
> + smi->intf = intf;
> + return 0;
> +}
> +
> +static void send_error_reply(struct ipmi_smi_powernv *smi,
> + struct ipmi_smi_msg *msg, u8 completion_code)
> +{
> + msg->rsp[0] = msg->data[0] | 0x4;
> + msg->rsp[1] = msg->data[1];
> + msg->rsp[2] = completion_code;
> + msg->rsp_size = 3;
> + ipmi_smi_msg_received(smi->intf, msg);
> +}
> +
> +static void ipmi_powernv_send(void *send_info, struct ipmi_smi_msg *msg)
> +{
> + struct ipmi_smi_powernv *smi = send_info;
> + struct opal_ipmi_msg *opal_msg;
> + unsigned long flags;
> + int comp, rc;
> + size_t size;
> +
> + /* ensure data_len will fit in the opal_ipmi_msg buffer... */
> + if (msg->data_size > IPMI_MAX_MSG_LENGTH) {
> + comp = IPMI_REQ_LEN_EXCEEDED_ERR;
> + goto err;
> + }
> +
> + /* ... and that we at least have netfn and cmd bytes */
> + if (msg->data_size < 2) {
> + comp = IPMI_REQ_LEN_INVALID_ERR;
> + goto err;
> + }
> +
> + spin_lock_irqsave(&smi->msg_lock, flags);
> +
> + if (smi->cur_msg) {
> + comp = IPMI_NODE_BUSY_ERR;
> + goto err_unlock;
> + }
> +
> + /* format our data for the OPAL API */
> + opal_msg = smi->opal_msg;
> + opal_msg->version = OPAL_IPMI_MSG_FORMAT_VERSION_1;
> + opal_msg->netfn = msg->data[0];
> + opal_msg->cmd = msg->data[1];
> + if (msg->data_size > 2)
> + memcpy(opal_msg->data, msg->data + 2, msg->data_size - 2);
> +
> + /* data_size already includes the
Re: [PATCH 0/2] Add IPMI support for powernv powerpc machines
On 11/09/2014 09:26 PM, Jeremy Kerr wrote: > Hi Corey, > > Thanks for the review. > >>> IPMI folks: the IPMI driver could do with a little review, as it's >>> not a conventional BT/KCS/SMI SI, in that the low-level send/recv >>> interface will handle the entire message at once. >> Handling the entire message at once should be fine, as that's what >> this driver level is designed to do for the message handler. That >> part all looks correct. The code itself looks good, but I have a >> couple of high-level comments. >> >> The driver at this level can receive more than one message to handle >> at a time, so it needs some sort of queue. This is to allow multiple >> users and to allow the message handler to send its own commands while >> other commands are going on. You might argue that the queuing should >> be done in ipmi_msghandler, and you would probably be right. > Ah, that's what I'd been assuming was being done - I missed the > xmit_list in the si_intf code. It'd be great if this could be in the > generic msghandler code, otherwise I'd just be duplicating the si_intf > logic. > >> I'll look at doing that. If that is the case, then your NULL check >> for current message should probably be a BUG_ON(). > OK, I'll update this when the msghandler bit is implemented. I've finally finished that, you can get it at: git://git.code.sf.net/p/openipmi/linux-ipmi using the for-next branch. This is what goes into linux-next. >> Do you need to handle any BMC flags? Particularly incoming events? > Not at this stage - we may in future though. Ok. That will complicate things a bit when they are added, but not very much. Thanks, -corey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/2] Add IPMI support for powernv powerpc machines
On 11/05/2014 09:38 PM, Jeremy Kerr wrote: > This series adds IPMI driver and arch glue for OPAL-firmware-based > powernv machines. The first change exposes the firmware's IPMI API, and > the second adds an actual driver. > > IPMI folks: the IPMI driver could do with a little review, as it's not a > conventional BT/KCS/SMI SI, in that the low-level send/recv interface > will handle the entire message at once. Handling the entire message at once should be fine, as that's what this driver level is designed to do for the message handler. That part all looks correct. The code itself looks good, but I have a couple of high-level comments. The driver at this level can receive more than one message to handle at a time, so it needs some sort of queue. This is to allow multiple users and to allow the message handler to send its own commands while other commands are going on. You might argue that the queuing should be done in ipmi_msghandler, and you would probably be right. I'll look at doing that. If that is the case, then your NULL check for current message should probably be a BUG_ON(). Do you need to handle any BMC flags? Particularly incoming events? > Corey & Michael: if this is acceptable, it may be mergable as two > separate patches - one for the IPMI subsystem, one for the powernv > platform. However, we'd need to preserve their order: patch 2/2 depends > on 1/2, which provides the structure & function definitions. This'll > break the build if only 2/2 is in the tree, and CONFIG_IPMI_POWERNV is > set. > > Alternatively, they could be merged by one maintainer, pending an ack > from the other. I'm fine either way. Thanks, -corey > Comments / queries / etc welcome. > > Cheers, > > > Jeremy > > --- > Jeremy Kerr (2): > powerpc/powernv: Add OPAL IPMI interface > drivers/char/ipmi: Add powernv IPMI driver > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Add coherent_dma_mask to mv64x60 devices
From: Corey Minyard DMA ops requires that coherent_dma_mask be set properly for a device, but this was not being done for devices on the MV64x60 that use DMA. Both the serial and ethernet devices need this or they won't be able to allocate memory. Signed-off-by: Corey Minyard --- Mark Greer pointed me to the right place, I believe this is the correct way to handle the problem. Index: linux-2.6/arch/powerpc/sysdev/mv64x60_dev.c === --- linux-2.6.orig/arch/powerpc/sysdev/mv64x60_dev.c +++ linux-2.6/arch/powerpc/sysdev/mv64x60_dev.c @@ -16,6 +16,7 @@ #include #include #include +#include #include @@ -189,6 +190,7 @@ static int __init mv64x60_mpsc_device_se pdev = platform_device_alloc(MPSC_CTLR_NAME, port_number); if (!pdev) return -ENOMEM; + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); err = platform_device_add_resources(pdev, r, 5); if (err) @@ -302,6 +304,7 @@ static int __init mv64x60_eth_device_set if (!pdev) return -ENOMEM; + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); err = platform_device_add_resources(pdev, r, 1); if (err) goto error; ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Add coherent_dma_mask setting to platform devices
From: Corey Minyard DMA ops requires that coherent_dma_mask be set properly for a device, but this was not being done for platform devices on powerpc. The MPSC drivers, in particular, need this for both serial and ethernet or they won't be able to allocate memory. Signed-off-by: Corey Minyard --- How about this patch? It seems to work ok and I suppose this makes sense. I'll send the uart setting in another patch. Index: linux-2.6/arch/powerpc/kernel/setup-common.c === --- linux-2.6.orig/arch/powerpc/kernel/setup-common.c +++ linux-2.6/arch/powerpc/kernel/setup-common.c @@ -681,6 +681,7 @@ static int ppc_dflt_bus_notify(struct no return 0; set_dma_ops(dev, &dma_direct_ops); + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); return NOTIFY_DONE; } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Set the port device in the mpsc serial driver
From: Corey Minyard
The mpsc serial driver needx to set the port's device tree element
to register properly.
Signed-off-by: Corey Minyard
---
Index: linux-2.6/drivers/serial/mpsc.c
===
--- linux-2.6.orig/drivers/serial/mpsc.c
+++ linux-2.6/drivers/serial/mpsc.c
@@ -2070,6 +2070,7 @@ static int mpsc_drv_probe(struct platfor
if (!(rc = mpsc_drv_map_regs(pi, dev))) {
mpsc_drv_get_platform_data(pi, dev, dev->id);
+ pi->port.dev = &dev->dev;
if (!(rc = mpsc_make_ready(pi))) {
spin_lock_init(&pi->tx_lock);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: Add DMA mask to MPSC serial and network and UART device to serial
Benjamin Herrenschmidt wrote:
On Fri, 2010-01-29 at 18:04 -0600, Corey Minyard wrote:
From: Corey Minyard
The MPSC drivers that use DMA need to set coherent_dma_mask to allow
dma_alloc_xxx routines to work properly. Also, the mpsc serial driver
needed to set pi->port.dev to register properly. With these fixes,
the MPSC drivers seem to work again.
Signed-off-by: Corey Minyard
---
Is that enough ? Since 2.6.31 we also need the dma ops to be filled
properly and obviously that isn't going to happen for random platform
devices... Or are those initialized elsewhere by your platform code ? In
which case it might be a good place to also set the coherent mask...
That's done in ppc_dflt_bus_notify(), but that didn't seem an appropriate
place to do this. If it is, it's easy enough to add it there, but that
would
mean it would get set for all devices on any type of ppc system.
-corey
Maybe we should have a more generic way to set the "default" dma
information (including ops, masks etc...) for use by platform,
of_platform etc... devices.
Cheers,
Ben.
I'm not really sure about where to set the coherent_dma_mask, it seems
like a more general place would be better but I couldn't find it.
Without these, the console stops working after the switchover andantipode
the network driver fails to allocate buffers. With these, my board
boots up and works fine.
Index: linux-2.6.31/drivers/serial/mpsc.c
===
--- linux-2.6.31.orig/drivers/serial/mpsc.c
+++ linux-2.6.31/drivers/serial/mpsc.c
@@ -2071,6 +2071,9 @@ static int mpsc_drv_probe(struct platfor
if (!(rc = mpsc_drv_map_regs(pi, dev))) {
mpsc_drv_get_platform_data(pi, dev, dev->id);
+ dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ pi->port.dev = &dev->dev;
+
if (!(rc = mpsc_make_ready(pi))) {
spin_lock_init(&pi->tx_lock);
if (!(rc = uart_add_one_port(&mpsc_reg,
Index: linux-2.6.31/drivers/net/mv643xx_eth.c
===
--- linux-2.6.31.orig/drivers/net/mv643xx_eth.c
+++ linux-2.6.31/drivers/net/mv643xx_eth.c
@@ -2916,7 +2916,7 @@ static int mv643xx_eth_probe(struct plat
mp->shared = platform_get_drvdata(pd->shared);
mp->base = mp->shared->base + 0x0400 + (pd->port_number << 10);
mp->port_num = pd->port_number;
-
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
mp->dev = dev;
set_params(mp, pd);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Add DMA mask to MPSC serial and network and UART device to serial
From: Corey Minyard
The MPSC drivers that use DMA need to set coherent_dma_mask to allow
dma_alloc_xxx routines to work properly. Also, the mpsc serial driver
needed to set pi->port.dev to register properly. With these fixes,
the MPSC drivers seem to work again.
Signed-off-by: Corey Minyard
---
I'm not really sure about where to set the coherent_dma_mask, it seems
like a more general place would be better but I couldn't find it.
Without these, the console stops working after the switchover and
the network driver fails to allocate buffers. With these, my board
boots up and works fine.
Index: linux-2.6.31/drivers/serial/mpsc.c
===
--- linux-2.6.31.orig/drivers/serial/mpsc.c
+++ linux-2.6.31/drivers/serial/mpsc.c
@@ -2071,6 +2071,9 @@ static int mpsc_drv_probe(struct platfor
if (!(rc = mpsc_drv_map_regs(pi, dev))) {
mpsc_drv_get_platform_data(pi, dev, dev->id);
+ dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ pi->port.dev = &dev->dev;
+
if (!(rc = mpsc_make_ready(pi))) {
spin_lock_init(&pi->tx_lock);
if (!(rc = uart_add_one_port(&mpsc_reg,
Index: linux-2.6.31/drivers/net/mv643xx_eth.c
===
--- linux-2.6.31.orig/drivers/net/mv643xx_eth.c
+++ linux-2.6.31/drivers/net/mv643xx_eth.c
@@ -2916,7 +2916,7 @@ static int mv643xx_eth_probe(struct plat
mp->shared = platform_get_drvdata(pd->shared);
mp->base = mp->shared->base + 0x0400 + (pd->port_number << 10);
mp->port_num = pd->port_number;
-
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
mp->dev = dev;
set_params(mp, pd);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Add a new platform for maple so a different link address can be set
From: Corey Minyard The maple platform failed to load because it's firmware could not take a link address of 0x400. A new platform type with a link address of 0x40 had to be created for the maple. Signed-off-by: Corey Minyard --- Without this patch the firmware loader says it is unable to parse the ELF header and dies. This patch lets 2.6.31 boot without issue. At the current head of k.org git, at least with an NFS mount, I'm getting: Kernel panic - not syncing: No init found So I'm not sure what the deal is there, but this patch is still required to get the firmware to load the kernel. Index: linux-2.6.31/arch/powerpc/boot/wrapper === --- linux-2.6.31.orig/arch/powerpc/boot/wrapper +++ linux-2.6.31/arch/powerpc/boot/wrapper @@ -145,6 +145,10 @@ pseries) platformo=$object/of.o link_address='0x400' ;; +maple) +platformo=$object/of.o +link_address='0x40' +;; pmac|chrp) platformo=$object/of.o ;; @@ -313,7 +319,7 @@ fi # post-processing needed for some platforms case "$platform" in -pseries|chrp) +pseries|chrp|maple) $objbin/addnote "$ofile" ;; coff) Index: linux-2.6.31/arch/powerpc/boot/Makefile === --- linux-2.6.31.orig/arch/powerpc/boot/Makefile +++ linux-2.6.31/arch/powerpc/boot/Makefile @@ -167,7 +167,7 @@ quiet_cmd_wrap = WRAP$@ $(if $3, -s $3)$(if $4, -d $4)$(if $5, -i $5) vmlinux image-$(CONFIG_PPC_PSERIES)+= zImage.pseries -image-$(CONFIG_PPC_MAPLE) += zImage.pseries +image-$(CONFIG_PPC_MAPLE) += zImage.maple image-$(CONFIG_PPC_IBM_CELL_BLADE) += zImage.pseries image-$(CONFIG_PPC_PS3)+= dtbImage.ps3 image-$(CONFIG_PPC_CELLEB) += zImage.pseries @@ -346,7 +346,7 @@ install: $(CONFIGURE) $(addprefix $(obj) clean-files += $(image-) $(initrd-) cuImage.* dtbImage.* treeImage.* \ zImage zImage.initrd zImage.chrp zImage.coff zImage.holly \ zImage.iseries zImage.miboot zImage.pmac zImage.pseries \ - simpleImage.* otheros.bld *.dtb + zImage.maple simpleImage.* otheros.bld *.dtb # clean up files cached by wrapper clean-kernel := vmlinux.strip vmlinux.bin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
