RE: [PATCH v9 1/1] Add support for IPMB driver

[Asmaa Mnebhi]

Hi Wolfram,

Thank you for your response. Please see my inline response.

-----Original Message-----
From: Wolfram Sang <wsa@xxxxxxxxxxxxx> 
Sent: Monday, June 3, 2019 4:13 PM
To: Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx>
Cc: minyard@xxxxxxx; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; Michael Shych <michaelsh@xxxxxxxxxxxx>; rdunlap@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v9 1/1] Add support for IPMB driver

Hi Asmaa,

sorry for the long wait. I missed this mail was still sitting in my Drafts folder :(

> >> 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:

Well, not really. The spinlock is taken _before_ the atomic read. But the read is atomic, so there should be no need. I am asking if the code could look like this?

+	while (!atomic_read(&ipmb_dev->request_queue_len)) {
+		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);
+	if (list_empty(&ipmb_dev->request_queue)) {

> 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);

The unlock operation could come before the dev_err. We don't need to protect the printout and save time with the spinlock held.

Sounds good to me. I will post a new patch shortly.

> > +	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

Hmmm, does that really prove that rq_sa is the same address the request came from? Or does it only prove that the response packet is not mangled?
You are correct. This mainly proves that the response packet is not mangled.

> 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.

Can there be only one requester in the system?

There can be multiple requesters in the system but this driver was designed in a way that it creates a separate device file called "ipmb-<smbus number>" for each I2C master connected to this slave device.
So for example, if we have BMC#0 connected to this slave device via bus 1 and BMC#1 connected to this slave device via bus 12,
Then we would have 2 device files:
/dev/ipmb-1 for BMC#0
/dev/ipmb-12 for BMC#1
So we would have 2 device instances of ipmb_dev_int.
Of course, we assumed from the beginning that not many people want to have such poor design where they would have an i2c slave (responder) which has 2 masters (requesters) on the same bus. ð

Anyways, I will create a  global variable u8 reference_rq_sa[MAX_BUS_NUMBER]  which holds the address of the requester depending on the I2C bus number.

Thanks.
Asmae

Thanks,

   Wolfram