Messages in this thread |  | | Subject | Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver | | From | "Wang, Haiyue" <> | | Date | Thu, 18 Jan 2018 08:57:43 +0800 |
| |
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.
|  |