Re: [PATCH 0/2] Add IPMI support for powernv powerpc machines

[Corey Minyard <minyard@acm.org>]

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
>