Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver
From: Wang, Haiyue
Date: Wed Jan 17 2018 - 19:58:00 EST
On 2018-01-18 00:31, Corey Minyard wrote:
On 01/17/2018 08:31 AM, Wang, Haiyue wrote:
Snip...
+
+struct kcs_bmc {
+ÂÂÂ struct regmap *map;
+ÂÂÂ spinlock_tÂÂÂÂ lock;
This lock is only used in threads, as far as I can tell. Couldn't
it just be a normal mutex?
But more on this later.
I missed this lock using in KCS ISR function, for AST2500 is single
core CPU. The critical data such as
data_in_avail is shared between ISR and user thread, spinlock_t
related API should be the right one ?
especially for SMP ?
Sort of. In the case below, you need to use spin_lock_irqsave(), you
don't necessarily get
here with interrupts disabled.
In the ones called from user context, you should really use
spin_lock_irq(). Interrupts
should always be on at that point, so it's better.
Understood, will change it with the right API call.
static irqreturn_t kcs_bmc_irq(int irq, void *arg)
{
ÂÂÂ ....
ÂÂÂ spin_lock(&kcs_bmc->lock);Â // <-- MISSED
ÂÂÂ switch (sts) {
ÂÂÂ case KCS_STR_IBF | KCS_STR_CMD_DAT:
ÂÂÂ ÂÂÂ kcs_rx_cmd(kcs_bmc);
ÂÂÂ ÂÂÂ break;
ÂÂÂ case KCS_STR_IBF:
ÂÂÂ ÂÂÂ kcs_rx_data(kcs_bmc);
ÂÂÂ ÂÂÂ break;
ÂÂÂ default:
ÂÂÂ ÂÂÂ ret = IRQ_NONE;
ÂÂÂ ÂÂÂ break;
ÂÂÂ }
ÂÂÂ spin_unlock(&kcs_bmc->lock); // <-- MISSED
ÂÂÂ return ret;
}
+ spin_lock_irqsave(&kcs_bmc->lock, flags);
+ÂÂÂ if (kcs_bmc->kcs_phase == KCS_PHASE_READ) {
If you don't modify kcs_phase here, you have a race condition. You
probably
need a KCS_WAIT_READ condition. Also, the nomenclature of "read"
and "write"
here is a little confusing, since your phases are from the host's
point of view,
not this driver's point of view. You might want to document that
explicitly.
The race condition means that the user MAY write the duplicated
response ?
Not exactly. Two threads can call this, and if it hasn't transitions
from the read phase,
the data out will be overwritten.
OK, will add new state KCS_WAIT_READ handling.