[PATCH] OCC: FSI and hwmon: Add sequence numbering

[Eddie James]

On 7/2/19 7:31 PM, Andrew Jeffery wrote:
> On Wed, 12 Jun 2019, at 06:31, Eddie James wrote:
>> Sequence numbering of the commands submitted to the OCC is required by
>> the OCC interface specification. Add sequence numbering and check for
>> the correct sequence number on the response.
>>
>> Signed-off-by: Eddie James <eajames at linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-occ.c      | 15 ++++++++++++---
>>   drivers/hwmon/occ/common.c |  4 ++--
>>   drivers/hwmon/occ/common.h |  1 +
>>   3 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
>> index a2301ce..7da9c81 100644
>> --- a/drivers/fsi/fsi-occ.c
>> +++ b/drivers/fsi/fsi-occ.c
>> @@ -412,6 +412,7 @@ int fsi_occ_submit(struct device *dev, const void
>> *request, size_t req_len,
>>   		msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>>   	struct occ *occ = dev_get_drvdata(dev);
>>   	struct occ_response *resp = response;
>> +	u8 seq_no;
>>   	u16 resp_data_length;
>>   	unsigned long start;
>>   	int rc;
>> @@ -426,6 +427,8 @@ int fsi_occ_submit(struct device *dev, const void
>> *request, size_t req_len,
>>   
>>   	mutex_lock(&occ->occ_lock);
>>   
>> +	/* Extract the seq_no from the command (first byte) */
>> +	seq_no = *(const u8 *)request;
> The fact that your doing this says to me that the fsi_occ_submit() interface
> is wrong.
>
> We already have `struct occ_response` in drivers/hwmon/occ/common.h.
> I think we should add an equivalent `struct occ_request` and pass a
> typed pointer through fsi_occ_submit(), that way we can access the
> sequence number by name rather than through dodgy casts.


I don't think it's too bad. The first byte is always simply the sequence 
number. The worst that can happen is a user doesn't write a request 
correctly and we have a "wrong" sequence number, but in that case the 
request most likely won't work anyway. I think ideally it would be like 
you say, but it's also not ideal to change the interfaces at this stage.


>
> Also why is this sent just to the OpenBMC list? Any reason it's not on
> upstream lists?


It was... it's been accepted.


Thanks,

Eddie



>
> Andrew
>
>>   	rc = occ_putsram(occ, OCC_SRAM_CMD_ADDR, request, req_len);
>>   	if (rc)
>>   		goto done;
>> @@ -441,11 +444,17 @@ int fsi_occ_submit(struct device *dev, const void
>> *request, size_t req_len,
>>   		if (rc)
>>   			goto done;
>>   
>> -		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
>> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG ||
>> +		    resp->seq_no != seq_no) {
>>   			rc = -ETIMEDOUT;
>>   
>> -			if (time_after(jiffies, start + timeout))
>> -				break;
>> +			if (time_after(jiffies, start + timeout)) {
>> +				dev_err(occ->dev, "resp timeout status=%02x "
>> +					"resp seq_no=%d our seq_no=%d\n",
>> +					resp->return_status, resp->seq_no,
>> +					seq_no);
>> +				goto done;
>> +			}
>>   
>>   			set_current_state(TASK_UNINTERRUPTIBLE);
>>   			schedule_timeout(wait_time);
>> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
>> index d7cc0d2..e9d7167 100644
>> --- a/drivers/hwmon/occ/common.c
>> +++ b/drivers/hwmon/occ/common.c
>> @@ -122,12 +122,12 @@ struct extended_sensor {
>>   static int occ_poll(struct occ *occ)
>>   {
>>   	int rc;
>> -	u16 checksum = occ->poll_cmd_data + 1;
>> +	u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
>>   	u8 cmd[8];
>>   	struct occ_poll_response_header *header;
>>   
>>   	/* big endian */
>> -	cmd[0] = 0;			/* sequence number */
>> +	cmd[0] = occ->seq_no++;		/* sequence number */
>>   	cmd[1] = 0;			/* cmd type */
>>   	cmd[2] = 0;			/* data length msb */
>>   	cmd[3] = 1;			/* data length lsb */
>> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
>> index fc13f3c..67e6968 100644
>> --- a/drivers/hwmon/occ/common.h
>> +++ b/drivers/hwmon/occ/common.h
>> @@ -95,6 +95,7 @@ struct occ {
>>   	struct occ_sensors sensors;
>>   
>>   	int powr_sample_time_us;	/* average power sample time */
>> +	u8 seq_no;
>>   	u8 poll_cmd_data;		/* to perform OCC poll command */
>>   	int (*send_cmd)(struct occ *occ, u8 *cmd);
>>   
>> -- 
>> 1.8.3.1
>>
>>