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.
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
