Messages in this thread | | | Subject | Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver | From | Corey Minyard <> | Date | Tue, 30 Jan 2018 19:52:41 -0600 |
| |
On 01/30/2018 07:37 PM, Wang, Haiyue wrote: > > > On 2018-01-31 09:25, Corey Minyard wrote: >> On 01/30/2018 07:02 PM, Wang, Haiyue wrote: >>> >>> >>> On 2018-01-31 08:52, Corey Minyard wrote: >>>> On 01/30/2018 06:02 PM, Wang, Haiyue wrote: >>>>> >>>>> >>>>> On 2018-01-30 21:49, Corey Minyard wrote: >>>>>> On 01/29/2018 07:57 AM, Wang, Haiyue wrote: >>>>>>> >>>>>>> >>>>>>> On 2018-01-26 22:48, Corey Minyard wrote: >>>>>>>> On 01/26/2018 12:08 AM, Wang, Haiyue wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2018-01-25 01:48, Corey Minyard wrote: >>>>>>>>>> On 01/24/2018 10:06 AM, Haiyue Wang wrote: >>>>>>>>>>> The KCS (Keyboard Controller Style) interface is used to >>>>>>>>>>> perform in-band >>>>>>>>>>> IPMI communication between a server host and its BMC >>>>>>>>>>> (BaseBoard Management >>>>>>>>>>> Controllers). >>>>>>>>>>> >>>>>>>>>>> This driver exposes the KCS interface on ASpeed SOCs >>>>>>>>>>> (AST2400 and AST2500) >>>>>>>>>>> as a character device. Such SOCs are commonly used as BMCs >>>>>>>>>>> and this driver >>>>>>>>>>> implements the BMC side of the KCS interface. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com> >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> v1->v2 >>>>>>>>>>> >>>>>>>>>>> - Divide the driver into two parts, one handles the BMC KCS >>>>>>>>>>> IPMI 2.0 state; >>>>>>>>>>> the other handles the BMC KCS controller such as AST2500 >>>>>>>>>>> IO accessing. >>>>>>>>>>> - Use the spin lock APIs to handle the device file >>>>>>>>>>> operations and BMC chip >>>>>>>>>>> IRQ inferface for accessing the same KCS BMC data structure. >>>>>>>>>>> - Enhanced the phases handling of the KCS BMC. >>>>>>>>>>> - Unified the IOCTL definition for IPMI BMC, it will be used >>>>>>>>>>> by KCS and BT. >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> + >>>>>>>>>>> +static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc) >>>>>>>>>>> +{ >>>>>>>>>>> + u8 data; >>>>>>>>>>> + >>>>>>>>>>> + switch (kcs_bmc->phase) { >>>>>>>>>>> + case KCS_PHASE_WRITE: >>>>>>>>>>> + set_state(kcs_bmc, WRITE_STATE); >>>>>>>>>>> + >>>>>>>>>>> + /* set OBF before reading data */ >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) >>>>>>>>>>> + kcs_bmc->data_in[kcs_bmc->data_in_idx++] = >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>> >>>>>>>> I missed this earlier, you need to issue a length error if the >>>>>>>> data is too large. >>>>>>>> >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_PHASE_WRITE_END: >>>>>>>>>>> + set_state(kcs_bmc, READ_STATE); >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) >>>>>>>>>>> + kcs_bmc->data_in[kcs_bmc->data_in_idx++] = >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>>>>> + >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_WAIT_READ; >>>>>>>>>>> + if (kcs_bmc->running) { >>>>>>>>>> >>>>>>>>>> Why do you only do this when running is set? It won't hurt >>>>>>>>>> anything if it's not >>>>>>>>>> set. As it is, you have a race if something opens the device >>>>>>>>>> while this code >>>>>>>>>> runs. >>>>>>>>>> >>>>>>>>>> Also, don't set the state to wait read until the "write" has >>>>>>>>>> finished (userland has >>>>>>>>>> read the data out of the buffer. More on that later. >>>>>>>>>> >>>>>>>>> Understood. >>>>>>>>>>> + kcs_bmc->data_in_avail = true; >>>>>>>>>>> + wake_up_interruptible(&kcs_bmc->queue); >>>>>>>>>>> + } >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_PHASE_READ: >>>>>>>>>>> + if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) >>>>>>>>>>> + set_state(kcs_bmc, IDLE_STATE); >>>>>>>>>>> + >>>>>>>>>>> + data = read_data(kcs_bmc); >>>>>>>>>>> + if (data != KCS_CMD_READ_BYTE) { >>>>>>>>>>> + set_state(kcs_bmc, ERROR_STATE); >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) { >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_IDLE; >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + write_data(kcs_bmc, >>>>>>>>>>> + kcs_bmc->data_out[kcs_bmc->data_out_idx++]); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_PHASE_ABORT_ERROR1: >>>>>>>>>>> + set_state(kcs_bmc, READ_STATE); >>>>>>>>>>> + >>>>>>>>>>> + /* Read the Dummy byte */ >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>>>>> + >>>>>>>>>>> + write_data(kcs_bmc, kcs_bmc->error); >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_ABORT_ERROR2; >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_PHASE_ABORT_ERROR2: >>>>>>>>>>> + set_state(kcs_bmc, IDLE_STATE); >>>>>>>>>>> + >>>>>>>>>>> + /* Read the Dummy byte */ >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>>>>> + >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_IDLE; >>>>>>>>>>> + >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + default: >>>>>>>>>>> + set_state(kcs_bmc, ERROR_STATE); >>>>>>>>>>> + >>>>>>>>>>> + /* Read the Dummy byte */ >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>>>>> + >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static void kcs_bmc_handle_command(struct kcs_bmc *kcs_bmc) >>>>>>>>>>> +{ >>>>>>>>>>> + u8 cmd; >>>>>>>>>>> + >>>>>>>>>>> + set_state(kcs_bmc, WRITE_STATE); >>>>>>>>>>> + >>>>>>>>>>> + /* Dummy data to generate OBF */ >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + >>>>>>>>>>> + cmd = read_data(kcs_bmc); >>>>>>>>>> >>>>>>>>>> Shouldn't you check the phase in all the cases below and do >>>>>>>>>> error >>>>>>>>>> handling if the phase isn't correct? >>>>>>>>>> >>>>>>>>>> Similar thing if the device here isn't open. You need to handle >>>>>>>>>> that gracefully. >>>>>>>>>> >>>>>>>>>> Also, you should remove data_in_avail and data_in_idx setting >>>>>>>>>> from >>>>>>>>>> here, for reasons I will explain later. >>>>>>>>>> >>>>>>>>> If host software sends the data twice such as a retry before >>>>>>>>> the BMC's IPMI service starts, >>>>>>>>> then the two IPMI requests will be merged into one, if not >>>>>>>>> clear data_in_idx after receving >>>>>>>>> KCS_CMD_WRITE_START. Most of the states are driven by host >>>>>>>>> software (SMS). :( >>>>>>>> >>>>>>>> True, but what if the host issues WRITE_START or a WRITE_END >>>>>>>> while this driver is in read >>>>>>>> state? The spec is unclear on this, but it really only makes >>>>>>>> sense for the host to issue >>>>>>>> WRITE_START in idle stat and WRITE_END in write state. IMHO it >>>>>>>> should go to error >>>>>>>> state. You might make the case that a WRITE_START anywhere >>>>>>>> restarts the transaction, >>>>>>>> but the feel of the error state machine kind of goes against >>>>>>>> that. WRITE_END is definitely >>>>>>>> wrong anywhere but write state. >>>>>>>> >>>>>>>> I just found the following in the spec (section 9.12): >>>>>>>> >>>>>>>> Thus, since the interface will allow a command transfer to be >>>>>>>> started or restarted >>>>>>>> at any time when the input buffer is empty, software could >>>>>>>> elect to >>>>>>>> simply retry >>>>>>>> the command upon detecting an error condition, or issue a >>>>>>>> ‘known good’ >>>>>>>> command in order to clear ERROR_STATE >>>>>>>> >>>>>>>> So a WRITE_START anywhere is ok. A WRITE_END in the wrong >>>>>>>> state should probably >>>>>>>> still go to error state. This means the user needs to be able >>>>>>>> to handle a write error at >>>>>>>> any time. It also means it's very important to make sure the >>>>>>>> user does a read before >>>>>>>> doing a write. If the host re-issues a WRITE_START and writes >>>>>>>> a new command >>>>>>>> between the time the use reads the data and writes the >>>>>>>> response, the response would >>>>>>>> be for the wrong command. >>>>>>>> >>>>>>>>>>> + switch (cmd) { >>>>>>>>>>> + case KCS_CMD_WRITE_START: >>>>>>>>>>> + kcs_bmc->data_in_avail = false; >>>>>>>>>>> + kcs_bmc->data_in_idx = 0; >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_WRITE; >>>>>>>>>>> + kcs_bmc->error = KCS_NO_ERROR; >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_CMD_WRITE_END: >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_WRITE_END; >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_CMD_ABORT: >>>>>>>>>>> + if (kcs_bmc->error == KCS_NO_ERROR) >>>>>>>>>>> + kcs_bmc->error = KCS_ABORTED_BY_COMMAND; >>>>>>>>>>> + >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_ABORT_ERROR1; >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + default: >>>>>>>>>>> + kcs_bmc->error = KCS_ILLEGAL_CONTROL_CODE; >>>>>>>>>>> + set_state(kcs_bmc, ERROR_STATE); >>>>>>>>>>> + write_data(kcs_bmc, kcs_bmc->error); >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_ERROR; >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc) >>>>>>>>>>> +{ >>>>>>>>>>> + unsigned long flags; >>>>>>>>>>> + int ret = 0; >>>>>>>>>>> + u8 status; >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irqsave(&kcs_bmc->lock, flags); >>>>>>>>>>> + >>>>>>>>>>> + status = read_status(kcs_bmc) & (KCS_STATUS_IBF | >>>>>>>>>>> KCS_STATUS_CMD_DAT); >>>>>>>>>>> + >>>>>>>>>>> + switch (status) { >>>>>>>>>>> + case KCS_STATUS_IBF | KCS_STATUS_CMD_DAT: >>>>>>>>>>> + kcs_bmc_handle_command(kcs_bmc); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case KCS_STATUS_IBF: >>>>>>>>>>> + kcs_bmc_handle_data(kcs_bmc); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + default: >>>>>>>>>>> + ret = -1; >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irqrestore(&kcs_bmc->lock, flags); >>>>>>>>>>> + >>>>>>>>>>> + return ret; >>>>>>>>>>> +} >>>>>>>>>>> +EXPORT_SYMBOL(kcs_bmc_handle_event); >>>>>>>>>>> + >>>>>>>>>>> +static inline struct kcs_bmc *file_kcs_bmc(struct file *filp) >>>>>>>>>>> +{ >>>>>>>>>>> + return container_of(filp->private_data, struct kcs_bmc, >>>>>>>>>>> miscdev); >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static int kcs_bmc_open(struct inode *inode, struct file >>>>>>>>>>> *filp) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + int ret = 0; >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + if (!kcs_bmc->running) { >>>>>>>>>>> + kcs_bmc->running = 1; >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_IDLE; >>>>>>>>>>> + kcs_bmc->data_in_avail = false; >>>>>>>>>> >>>>>>>>>> If you do everything right, setting the phase and >>>>>>>>>> data_in_avail should not >>>>>>>>>> be necessary here. >>>>>>>>>> >>>>>>>>>>> + } else { >>>>>>>>>>> + ret = -EBUSY; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return ret; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static unsigned int kcs_bmc_poll(struct file *filp, >>>>>>>>>>> poll_table *wait) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + unsigned int mask = 0; >>>>>>>>>>> + >>>>>>>>>>> + poll_wait(filp, &kcs_bmc->queue, wait); >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->data_in_avail) >>>>>>>>>>> + mask |= POLLIN; >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return mask; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static ssize_t kcs_bmc_read(struct file *filp, char *buf, >>>>>>>>>>> + size_t count, loff_t *offset) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + ssize_t ret = -EAGAIN; >>>>>>>>>>> + >>>>>>>>>> >>>>>>>>>> This function still has some issues. >>>>>>>>>> >>>>>>>>>> You can't call copy_to_user() with a spinlock held or >>>>>>>>>> interrupts disabled. >>>>>>>>>> To handle readers, you probably need a separate mutex. >>>>>>>>>> >>>>>>>>>> Also, this function can return -EAGAIN even if O_NONBLOCK is >>>>>>>>>> not set if >>>>>>>>>> kcs_bmc->data_in_avail changes between when you wait on the >>>>>>>>>> event >>>>>>>>>> and when you check it under the lock. >>>>>>>>>> >>>>>>>>>> You also clear data_in_avail even if the copy_to_user() >>>>>>>>>> fails, which is >>>>>>>>>> wrong. >>>>>>>>>> >>>>>>>>>> I believe the best way to handle this would be to have the >>>>>>>>>> spinlock >>>>>>>>>> protect the inner workings of the state machine and a mutex >>>>>>>>>> handle >>>>>>>>>> copying data out, setting/clearing the running flag (thus a >>>>>>>>>> mutex >>>>>>>>>> instead of spinlock in open and release) and the ioctl >>>>>>>>>> settings (except >>>>>>>>>> for abort where you will need to grab the spinlock). >>>>>>>>>> >>>>>>>>>> After the wait event below, grab the mutex. If data is not >>>>>>>>>> available >>>>>>>>>> and O_NONBLOCK is not set, drop the mutex and retry. Otherwise >>>>>>>>>> this is the only place (besides release) that sets >>>>>>>>>> data_in_avail to false. >>>>>>>>>> Do the copy_to_user(), grab the spinlock, clear data_in_avail >>>>>>>>>> and >>>>>>>>>> data_in_idx, then release the lock and mutex. If you are really >>>>>>>>>> adventurous you can do this without grabbing the lock using >>>>>>>>>> barriers, but it's probably not necessary here. >>>>>>>>>> >>>>>>>> >>>>>>>> With the state machine being able to be restarted at any time, >>>>>>>> you need >>>>>>>> something a little different here. You still need the mutex to >>>>>>>> handle >>>>>>>> multiple readers and the copy. I think the function should be >>>>>>>> something >>>>>>>> like: >>>>>>>> >>>>>>> Since KCS is not a multi-reader protocol from BMC's view, you >>>>>>> makes things complex. :-) >>>>>> >>>>>> No, I don't think you understand. The primary purpose of the >>>>>> complexity >>>>>> here is to protect the driver from the host system (on the other >>>>>> side of >>>>>> the KCS interface). Without this protection, it is possible for >>>>>> the host >>>>>> system to start a new write while the user on the BMC side is >>>>>> reading >>>>>> data out, resulting in corrupt data being read. >>>>>> >>>>>> I haven't thought too much about this. There may be a simpler way, >>>>>> but the protection needs to be there. >>>>>> >>>>>> And you may not think you need to protect the driver against a >>>>>> malicious BMC side user code, but you would be wrong. You can >>>>>> only have one opener, but with threads or a fork you can have >>>>>> multiple readers. And you don't know if a malicious piece of >>>>>> code has taken over userland. You always need to protect the >>>>>> kernel. >>>>>> >>>>> Sure, the read/write have protected the critical data area with >>>>> IRQ, and also, these >>>>> functions should be thread local safe I believe. >>>>> >>>>> spin_lock_irq(&kcs_bmc->lock); >>>>> ... >>>>> spin_unlock_irq(&kcs_bmc->lock); >>>>> >>>> >>>> But remember, you can't call copy_to_user() when IRQs are off or >>>> when you are holding >>>> a spinlock. That is an absolute no. It can crash the kernel. >>>> >>>> So you need a design that takes this into account, but will not >>>> result in the possibility >>>> of bad data being read. >>>> >>> Yes, sure, as I said before: access_ok(VERIFY_WRITE, to, n), then >>> memcpy in spin_lock. >> >> Where did you get the idea that this was ok? It's not. access_ok() >> is not actually very >> useful, since the permissions on memory can change at any time unless >> you are holding >> the mm lock, which is also not an ok thing to do. It is entirely >> possible for access_ok() >> to pass and copy_to_user() to fail. >> > I thought memcpy will not fail. :(
Oh, memcpy won't fail as long as the source and destination is kernel memory. I was a little confused by the access_ok() thing, it's common for people to assume that if they do access_ok(), that copy_to_user() won't fail.
>> I'm not exactly sure what you are saying, though. In any event, a >> well-designed read()/write() >> operation should leave the system unchanged if it gets an error. >> > I saw BT use a local buffer, If I change the '#define > KCS_MSG_BUFSIZ 1024' to ".. 512", should it be OK > as BT ? > > static ssize_t bt_bmc_read(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > struct bt_bmc *bt_bmc = file_bt_bmc(file); > u8 len; > int len_byte = 1; > u8 kbuffer[BT_BMC_BUFFER_SIZE]; --> #define BT_BMC_BUFFER_SIZE 256
It's good practice to keep larger things off the stack, which is why I dynamically allocated it. But if you have a mutex, you can put that buffer in struct bt_bmc since it would only be accessed when holding the mutex.
> >> -corey >> >>>>>>>> static ssize_t kcs_bmc_read(struct file *filp, char *buf, >>>>>>>> size_t count, loff_t *offset) >>>>>>>> { >>>>>>>> struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>> ssize_t ret; >>>>>>>> bool avail; >>>>>>>> size_t data_size; >>>>>>>> u8 *data; >>>>>>>> >>>>>>>> data = kmalloc(KCS_MSG_BUFSIZ, GFP_KERNEL); >>>>>>>> if (!data) >>>>>>>> return -ENOMEM; >>>>>>>> >>>>>>>> retry: >>>>>>>> ret = -EAGAIN; >>>>>>>> if (!(filp->f_flags & O_NONBLOCK)) >>>>>>>> wait_event_interruptible(kcs_bmc->queue, >>>>>>>> kcs_bmc->data_in_avail); >>>>>>>> >>>>>>>> mutex_lock(&kcs_bmc->read_mutex); >>>>>>>> >>>>>>>> spin_lock_irq(&kcs_bmc->lock); >>>>>>>> avail = kcs_bmc->data_in_avail; >>>>>>>> if (avail) { >>>>>>>> memcpy(data, kcs_bmc->data_in, kcs_bmc->data_in_idx); >>>>>>>> data_size = kcs_bmc->data_in_idx; >>>>>>>> } >>>>>>>> spin_unlock_irq(&kcs_bmc->lock); >>>>>>>> >>>>>>>> if (!avail) { >>>>>>>> if (filp->f_flags & O_NONBLOCK) >>>>>>>> goto out_mutex_unlock; >>>>>>>> mutex_unlock(&kcs_bmc->read_mutex); >>>>>>>> goto retry; >>>>>>>> } >>>>>>>> >>>>>>>> if (count < data_size) { >>>>>>>> ret = -EOVERFLOW; >>>>>>>> ? I'm not sure about the error, but userspace >>>>>>>> needs to know. >>>>>>>> goto out_mutex_unlock; >>>>>> >>>>>> Maybe a length error to the host side here? >>>> >>>> You didn't comment on this or the other length error. That needs >>>> to be >>>> handled. >>>> >>> Yes, will send a length error by following KCS spec. >>>>>> >>>>>>>> } >>>>>>>> >>>>>>>> if (!copy_to_user(buf, data, data_size)) { >>>>>>>> ret = -EFAULT; >>>>>>>> goto out_mutex_unlock; >>>>>>>> } >>>>>>>> >>>>>>>> ret = data_size; >>>>>>>> >>>>>>>> spin_lock_irq(&kcs_bmc->lock); >>>>>>>> >>>>>>>> if (kcs_bmc->phase != KCS_PHASE_WRITE_END_DONE) >>>>>>>> /* Something aborted or restarted the state >>>>>>>> machine. */ >>>>>>>> ? Maybe restart if O_NONBLOCK is not set and >>>>>>>> -EAGAIN if it is? >>>>>>>> ret = -EIO; >>>>>>>> } else { >>>>>>>> kcs_bmc->phase = KCS_PHASE_WAIT_READ; >>>>>>>> kcs_bmc->data_in_avail = false; >>>>>>>> kcs_bmc->data_in_idx = 0; >>>>>>>> } >>>>>>>> >>>>>>>> spin_unlock_irq(&kcs_bmc->lock); >>>>>>>> >>>>>>>> out_mutex_unlock: >>>>>>>> mutex_unlock(&kcs_bmc->read_mutex); >>>>>>>> >>>>>>>> kfree(data); >>>>>>>> >>>>>>>> return ret; >>>>>>>> } >>>>>>>> Note that I added a state, KCS_PHASE_WRITE_END_DONE, which >>>>>>>> would be >>>>>>>> set after the final byte from the host is received. You want >>>>>>>> the read here >>>>>>>> done before you can do the write below to avoid the race I >>>>>>>> talked about. >>>>>>>> >>>>>>>> There is a local copy made of the data. What you *never* want >>>>>>>> to happen >>>>>>>> here is for the state machine to start processing a new write >>>>>>>> command >>>>>>>> while the data is being copied. It could result in corrupt >>>>>>>> data being read >>>>>>>> and some random operation being done by the BMC. >>>>>>>> >>>>>>>> If you want to avoid the local copy, it could be done, but it's >>>>>>>> more complex. >>>>>>>> >>>>>>>>>>> + if (!(filp->f_flags & O_NONBLOCK)) >>>>>>>>>>> + wait_event_interruptible(kcs_bmc->queue, >>>>>>>>>>> + kcs_bmc->data_in_avail); >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->data_in_avail) { >>>>>>>>>>> + kcs_bmc->data_in_avail = false; >>>>>>>>>>> + >>>>>>>>>>> + if (count > kcs_bmc->data_in_idx) >>>>>>>>>>> + count = kcs_bmc->data_in_idx; >>>>>>>>>>> + >>>>>>>>>>> + if (!copy_to_user(buf, kcs_bmc->data_in, count)) >>>>>>>>>>> + ret = count; >>>>>>>>>>> + else >>>>>>>>>>> + ret = -EFAULT; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return ret; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static ssize_t kcs_bmc_write(struct file *filp, const char >>>>>>>>>>> *buf, >>>>>>>>>>> + size_t count, loff_t *offset) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + ssize_t ret = count; >>>>>>>>>>> + >>>>>>>>>>> + if (count < 1 || count > KCS_MSG_BUFSIZ) >>>>>>>>>>> + return -EINVAL; >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + if (kcs_bmc->phase == KCS_PHASE_WAIT_READ) { >>>>>>>>>>> + if (copy_from_user(kcs_bmc->data_out, buf, count)) { >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + return -EFAULT; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_READ; >>>>>>>>>>> + kcs_bmc->data_out_idx = 1; >>>>>>>>>>> + kcs_bmc->data_out_len = count; >>>>>>>>>>> + write_data(kcs_bmc, kcs_bmc->data_out[0]); >>>>>>>>>>> + } else if (kcs_bmc->phase == KCS_PHASE_READ) { >>>>>>>>>>> + ret = -EBUSY; >>>>>>>>>>> + } else { >>>>>>>>>>> + ret = -EINVAL; >>>>>>>>>> >>>>>>>>>> Is there a reason you return -EINVAL here? Why not just >>>>>>>>>> -EBUSY in all >>>>>>>>>> cases? Is there something that userland will need to do >>>>>>>>>> differently? >>>>>>>>>> >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return ret; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd, >>>>>>>>>>> + unsigned long arg) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + long ret = 0; >>>>>>>>>>> + >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + switch (cmd) { >>>>>>>>>>> + case IPMI_BMC_IOCTL_SET_SMS_ATN: >>>>>>>>>>> + update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN, >>>>>>>>>>> + KCS_STATUS_SMS_ATN); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case IPMI_BMC_IOCTL_CLEAR_SMS_ATN: >>>>>>>>>>> + update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN, >>>>>>>>>>> + 0); >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + case IPMI_BMC_IOCTL_FORCE_ABORT: >>>>>>>>>>> + set_state(kcs_bmc, ERROR_STATE); >>>>>>>>>>> + read_data(kcs_bmc); >>>>>>>>>>> + write_data(kcs_bmc, KCS_ZERO_DATA); >>>>>>>>>>> + >>>>>>>>>>> + kcs_bmc->phase = KCS_PHASE_ERROR; >>>>>>>>>>> + kcs_bmc->data_in_avail = false; >>>>>>>>>>> + break; >>>>>>>>>>> + >>>>>>>>>>> + default: >>>>>>>>>>> + ret = -EINVAL; >>>>>>>>>>> + break; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return ret; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +static int kcs_bmc_release(struct inode *inode, struct file >>>>>>>>>>> *filp) >>>>>>>>>>> +{ >>>>>>>>>>> + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); >>>>>>>>>>> + >>>>>>>>>> >>>>>>>>>> What happens if the device gets closed in the middle of a >>>>>>>>>> transaction? That's >>>>>>>>>> an important case to handle. If something is in process, you >>>>>>>>>> need to abort it. >>>>>>>>>> >>>>>>>>> The device just provides the read & write data, the >>>>>>>>> transaction is handled in the KCS >>>>>>>>> controller's IRQ handler. >>>>>>>> >>>>>>>> From the spec, section 9.14: >>>>>>>> >>>>>>>> The BMC must change the status to ERROR_STATE on any >>>>>>>> condition where it >>>>>>>> aborts a command transfer in progress. >>>>>>>> >>>>>>>> So you need to do something here. >>>>>>>> >>>>>>> In practice, we do this as spec said in ipmid, NOT in driver, >>>>>>> driver can't handle anything, let's >>>>>>> make it simple, thanks! >>>>>> >>>>>> If ipmid crashes or is killed, how does it accomplish this? >>>>>> >>>>> Every time ipmids (or kcsd) crashed or killed, it needs start to >>>>> call FORCE_ARBORT firstly, to sync with >>>>> host side software. >>>>>>> >>>>>>> Whenever the BMC is reset (from power-on or a hard reset), the >>>>>>> State Bits are initialized to “11 - Error State”. Doing so >>>>>>> allows SMS to detect that the BMC has been reset and that any >>>>>>> message in process has been terminated by the BMC. >>>>>> >>>>>> Right, that's fine, like it should be. But we are not talking >>>>>> about a reset. >>>>>> >>>>> I think the final error handling solution is that kcsd (user land) >>>>> runs, otherwise, the host software side still got stuck. We meet >>>>> this kind of issue, so in general, we just doesn't handle some >>>>> mirror errors in driver, then in kcsd, when it can provide the real >>>>> IPMI service, it will reset the channel firstly to sync with host >>>>> side software. >>>> >>>> "Userland will do the right thing" is not very convincing to a >>>> kernel developer. >>>> >>>> Plus if the above is true, I would think that you would just want >>>> to hold the device >>>> in an error state when it wasn't opened. >>>> >>> I understand your concern, of course, driver need handles things >>> well. But in fact, if a user app is truly a bad boy, it still can hang >>> the host side: set SMS_ATN, but no message returned when software >>> host side requests, then host open-ipmi driver will hang, we >>> meet this kind of error to hang the customer's host. :) In my >>> understanding, kcs-bmc should do the right thing about read and write, >>> the real transaction should be handled correctly by the kcsd. >>> >>> And if no kcsd starts, then this kind of BMC can't be sold out. :) >> >> True. I'm not as concerned about this sort of thing. It's nicer to >> the host side if >> it can detect problems quickly, but it will eventually time out. >> >> From what I can tell from the current design, if the BMC userland is >> not running, >> the driver will step through the state machine until it hits read >> state, then it >> will sit there until the host times out and aborts the operation. >> >> IMHO, it would be better for the host side if the driver just stayed >> in error state >> if nothing had it open. It would think the spec says that in the >> quote I referenced >> above, but that quote, like many things in the IPMI spec, is fairly >> vague and could >> be interpreted many ways. >> > Well, I will try to fix this errors as possible. >> -corey >> >> >>>> -corey >>>> >>>>>> -corey >>>>>> >>>>>>>>>>> + spin_lock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + kcs_bmc->running = 0; >>>>>>>>>>> + >>>>>>>>>>> + spin_unlock_irq(&kcs_bmc->lock); >>>>>>>>>>> + >>>>>>>>>>> + return 0; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
| |