| Message ID | 1560285038-24233-1-git-send-email-eajames@linux.ibm.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | OCC: FSI and hwmon: Add sequence numbering | expand |
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@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. Also why is this sent just to the OpenBMC list? Any reason it's not on upstream lists? 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 > >
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@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 >> >>
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@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
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; 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);
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@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(-)