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

[Andrew Jeffery]

On Thu, 4 Jul 2019, at 00:36, Eddie James wrote:
> 
> 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.

Sure, but the readability isn't great and the code suggests to the reader
(me?) that the interface is either being abused or wasn't thought through.
The lack of  `struct occ_request` also has an impact on readability at the
call-sites where we're manually stuffing the bytes into a buffer, some of
which are multi-byte where we need to deal with endianness.

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

Except it's worked so far even though we were always sending zero?

> I think ideally it would be like 
> you say, but it's also not ideal to change the interfaces at this stage.

What do you mean? It's an internal kernel API, not userspace API/ABI.
We can change it if we feel the need.

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

Yeah I missed that :/

Andrew