[Skiboot] [PATCH 1/4] i2c: Add i2c_run_req() to crank the state machine for a request

[Stewart Smith]

Claudio Carvalho <cclaudio at linux.vnet.ibm.com> writes:
> On 11/23/2016 03:45 AM, Stewart Smith wrote:
>> Doing everything asynchronously is brilliant, it's exactly what we
>> want to do.
>> 
>> Except... the tpm driver wants to do things synchronously, which isn't
>> so cool.
>> 
>> For reasons that are not yet completely known, we spend an awful lot of
>> time in the main thread *not* running pollers (potentially seconds), which
>> doesn't bode well for I2C timeouts.
>> 
>> Since the TPM measure is done in a secondary thread, we do *not* run pollers
>> there either (as of 323c8aeb54bd4e0b9004091fcbb4a9daeda2f576 - which is
>> roughly as of skiboot 2.1.1).
>> 
>> But we still need to crank the i2c state machine, so we introduce a call
>> to do just that. It will return how long the poll interval should be, so
>> that we can time_wait() for a more appropriate time for whatever i2c
>> implementation is sitting behind things.
>> 
>> Without this, it was "easy" to get to a situation where the i2c state machine
>> wasn't cranked at all, and you'd hit the i2c timeout (for the issued operation)
>> before the poller to crank i2c was ever called.
>> 
>> Signed-off-by: Stewart Smith <stewart at linux.vnet.ibm.com>
>> ---
>>  hw/p8-i2c.c                        | 18 ++++++++++++++++++
>>  include/i2c.h                      |  8 ++++++++
>>  libstb/drivers/tpm_i2c_interface.c | 14 +++++++++-----
>>  3 files changed, 35 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
>> index 4e29432..12a4d1a 100644
>> --- a/hw/p8-i2c.c
>> +++ b/hw/p8-i2c.c
>> @@ -1064,6 +1064,23 @@ static void p8_i2c_set_request_timeout(struct i2c_request *req,
>>  	request->timeout = msecs_to_tb(duration);
>>  }
>>  
>> +static uint64_t p8_i2c_run_request(struct i2c_request *req)
>> +{
>> +	struct i2c_bus *bus = req->bus;
>> +	struct p8_i2c_master_port *port =
>> +		container_of(bus, struct p8_i2c_master_port, bus);
>> +	struct p8_i2c_master *master = port->master;
>> +	uint64_t poll_interval = 0;
>> +
>> +	lock(&master->lock);
>> +	p8_i2c_check_status(master);
>> +	p8_i2c_check_work(master);
>> +	poll_interval = master->poll_interval;
>> +	unlock(&master->lock);
>> +
>> +	return poll_interval;
>> +}
>> +
>>  static inline uint32_t p8_i2c_get_bit_rate_divisor(uint32_t lb_freq,
>>  						   uint32_t bus_speed)
>>  {
>> @@ -1399,6 +1416,7 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type)
>>  		port->bus.alloc_req = p8_i2c_alloc_request;
>>  		port->bus.free_req = p8_i2c_free_request;
>>  		port->bus.set_req_timeout = p8_i2c_set_request_timeout;
>> +		port->bus.run_req = p8_i2c_run_request;
>>  		i2c_add_bus(&port->bus);
>>  
>>  		/* Add OPAL properties to the bus node */
>> diff --git a/include/i2c.h b/include/i2c.h
>> index 83c6ec5..a10a179 100644
>> --- a/include/i2c.h
>> +++ b/include/i2c.h
>> @@ -28,6 +28,7 @@ struct i2c_bus {
>>  	void			(*free_req)(struct i2c_request *req);
>>  	void			(*set_req_timeout)(struct i2c_request *req,
>>  						   uint64_t duration);
>> +	uint64_t		(*run_req)(struct i2c_request *req);
>>  };
>>  
>>  /*
>> @@ -89,6 +90,13 @@ static inline void i2c_set_req_timeout(struct i2c_request *req,
>>  		req->bus->set_req_timeout(req, duration);
>>  }
>>  
>> +static inline uint64_t i2c_run_req(struct i2c_request *req)
>> +{
>> +	if (req->bus->run_req)
>> +		return req->bus->run_req(req);
>> +	return 0;
>> +}
>> +
>>  /* P8 implementation details */
>>  extern void p8_i2c_init(void);
>>  extern void p8_i2c_interrupt(uint32_t chip_id);
>> diff --git a/libstb/drivers/tpm_i2c_interface.c b/libstb/drivers/tpm_i2c_interface.c
>> index ba0c5a8..4d136fd 100644
>> --- a/libstb/drivers/tpm_i2c_interface.c
>> +++ b/libstb/drivers/tpm_i2c_interface.c
>> @@ -57,6 +57,7 @@ int tpm_i2c_request_send(int tpm_bus_id, int tpm_dev_addr, int read_write,
>>  	int rc, waited, retries, timeout;
>>  	struct i2c_request *req;
>>  	struct i2c_bus *bus;
>> +	uint64_t time_to_wait = 0;
>>  
>>  	bus = i2c_find_bus_by_id(tpm_bus_id);
>>  	if (!bus) {
>> @@ -106,9 +107,12 @@ int tpm_i2c_request_send(int tpm_bus_id, int tpm_dev_addr, int read_write,
>>  		i2c_queue_req(req);
>>  
>>  		do {
>> -			time_wait_ms(REQ_COMPLETE_POLLING);
>> -			waited += REQ_COMPLETE_POLLING;
>> -		} while (waited < timeout && rc == 1);
>> +			time_to_wait = i2c_run_req(req);
>> +			if (!time_to_wait)
>> +				time_to_wait = REQ_COMPLETE_POLLING;
>> +			time_wait(time_to_wait);
>> +			waited += time_to_wait;
>> +		} while (tb_to_msecs(waited) < timeout && rc == 1);
>
> With this we will be checking if the request is complete either on a
> master->poll_interval or REQ_COMPLETE_POLLING interval. Additionally,
> i2c_run_req() forces the request status to be updated on every polling
> interval.
>
> Do we need to force the request status to be updated on every polling
> interval?
>
> If we have something like below, we may end up checking the status
> REQ_COMPLETE_POLLING msec after it was updated. Is that what you are
> trying avoid here? rc seems to be updated even if we are running on a
> secondary thread. Maybe I don't fully understand the problem explained
> in the commit message.
>
> do {
> 	time_wait_ms(REQ_COMPLETE_POLLING);
> } while (rc == 1);

I think for the most part that if it is getting serviced, you're just
being lucky with timing - and this isn't always necessarily the case.


-- 
Stewart Smith
OPAL Architect, IBM.