Messages in this thread | | | From | Asmaa Mnebhi <> | Subject | RE: [PATCH v9 1/1] Add support for IPMB driver | Date | Mon, 20 May 2019 14:50:41 +0000 |
| |
-----Original Message----- From: Wolfram Sang <wsa@the-dreams.de> Sent: Sunday, May 19, 2019 10:03 AM To: Asmaa Mnebhi <Asmaa@mellanox.com> Cc: minyard@acm.org; Vadim Pasternak <vadimp@mellanox.com>; Michael Shych <michaelsh@mellanox.com>; rdunlap@infradead.org; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org Subject: Re: [PATCH v9 1/1] Add support for IPMB driver
> +static int receive_ipmb_request(struct ipmb_dev *ipmb_dev, > + bool non_blocking, > + struct ipmb_msg *ipmb_request) > +{ > + struct ipmb_request_elem *queue_elem; > + unsigned long flags; > + int res; > + > + spin_lock_irqsave(&ipmb_dev->lock, flags); > + > + while (!atomic_read(&ipmb_dev->request_queue_len)) {
>> Am I overlooking something? Why are you protecting an atomic_read with a spinlock?
A thread would lock the ipmb_dev->lock spinlock (above) for all the code below ONLY IF the atomic_read for the request_queue_len reports a value different from 0:
if (list_empty(&ipmb_dev->request_queue)) { 260 + dev_err(&ipmb_dev->client->dev, "request_queue is empty\n"); 261 + spin_unlock_irqrestore(&ipmb_dev->lock, flags); 262 + return -EIO; 263 + } 264 + 265 + queue_elem = list_first_entry(&ipmb_dev->request_queue, 266 + struct ipmb_request_elem, list); 267 + memcpy(ipmb_request, &queue_elem->request, sizeof(*ipmb_request)); 268 + list_del(&queue_elem->list); 269 + kfree(queue_elem); 270 + atomic_dec(&ipmb_dev->request_queue_len); 271 + 272 + spin_unlock_irqrestore(&ipmb_dev->lock, flags);
This is important because we do not want another thread to change/use the wrong value of request_queue_len, which is decremented eventually.
If the atomic read for the request_queue_len is 0, then we release the clock and call wait_event_interruptible until we receive something in the queue (i.e. request_queue_len has a value different from 0). The function ipmb_handle_request takes care of incrementing the value of request_queue_len and waking up the wait_queue.
> + spin_unlock_irqrestore(&ipmb_dev->lock, flags); > + > + if (non_blocking) > + return -EAGAIN; > + > + res = wait_event_interruptible(ipmb_dev->wait_queue, > + atomic_read(&ipmb_dev->request_queue_len)); > + if (res) > + return res; > + > + spin_lock_irqsave(&ipmb_dev->lock, flags); > + }
...
> + rq_sa = msg[RQ_SA_8BIT_IDX] >> 1; > + netf_rq_lun = msg[NETFN_LUN_IDX]; > + /* > + * subtract rq_sa and netf_rq_lun from the length of the msg passed to > + * i2c_smbus_write_block_data_local > + */ > + msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH; > + > + strcpy(rq_client.name, "ipmb_requester"); > + rq_client.adapter = ipmb_dev->client->adapter; > + rq_client.flags = ipmb_dev->client->flags; > + rq_client.addr = rq_sa;
>> Is it possible to determine in a race-free way if rq_sa (which came from userspace AFAIU) is really the address from which the request came in (again if I understood all this correctly)? Yes there is. I see 2 options:
1) This is less explicit than option 2 but uses existing code and is simpler. we can use the ipmb_verify_checksum1 function since the IPMB response format is as follows: Byte 1: rq_sa Byte 2: netfunction/rqLUN Byte 3: checksum1
So if checksum1 is verified, it means rq_sa is correct.
2) I am not sure we want this but have a global variable which stores the address of the requester once the first request is received. We would compare that address with the one received from userspace in the code above.
| |