| Message ID | 20210716151850.28973-2-eajames@linux.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | OCC: fsi and hwmon: Set sequence number in submit interface | expand |
On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com> wrote: > > Set and increment the sequence number during the submit operation. > This prevents sequence number conflicts between different users of > the interface. A sequence number conflict may result in a user > getting an OCC response meant for a different command. Since the > sequence number is now modified, the checksum must be calculated and > set before submitting the command. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> Reviewed-by: Joel Stanley <joel@jms.id.au> > @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len, > return -EINVAL; > } > > + /* Checksum the request, ignoring first byte (sequence number). */ > + for (i = 1; i < req_len - 2; ++i) > + checksum += byte_request[i]; > + This could go below, after you've got the sequence number, so the checksumming all happens in the same spot? The driver has become a bit of a maze, I can't tell how you're deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If oyu have some ideas on how to simplify it then I would welcome those changes. > mutex_lock(&occ->occ_lock); > > - /* Extract the seq_no from the command (first byte) */ > - seq_no = *(const u8 *)request; > - rc = occ_putsram(occ, request, req_len); > + /* > + * Get a sequence number and update the counter. Avoid a sequence > + * number of 0 which would pass the response check below even if the > + * OCC response is uninitialized. Any sequence number the user is > + * trying to send is overwritten since this function is the only common > + * interface to the OCC and therefore the only place we can guarantee > + * unique sequence numbers. > + */ > + seq_no = occ->sequence_number++; > + if (!occ->sequence_number) > + occ->sequence_number = 1; > + checksum += seq_no; > + > + rc = occ_putsram(occ, request, req_len, seq_no, checksum); > if (rc) > goto done;
On Wed, 2021-07-21 at 02:37 +0000, Joel Stanley wrote: > On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com> > wrote: > > Set and increment the sequence number during the submit operation. > > This prevents sequence number conflicts between different users of > > the interface. A sequence number conflict may result in a user > > getting an OCC response meant for a different command. Since the > > sequence number is now modified, the checksum must be calculated > > and > > set before submitting the command. > > > > Signed-off-by: Eddie James <eajames@linux.ibm.com> > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > > @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const > > void *request, size_t req_len, > > return -EINVAL; > > } > > > > + /* Checksum the request, ignoring first byte (sequence > > number). */ > > + for (i = 1; i < req_len - 2; ++i) > > + checksum += byte_request[i]; > > + > > This could go below, after you've got the sequence number, so the > checksumming all happens in the same spot? It definitely could, I had the idea to do the checksumming outside the mutex in case it took a long time? Probably not worth it though. > > The driver has become a bit of a maze, I can't tell how you're > deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If > oyu have some ideas on how to simplify it then I would welcome those > changes. Well, it doesn't really matter in fsi_occ_submit vs occ_putsram, as the latter is only called in the former. occ_write wouldn't be used by the hwmon interface, which is why we're moving some of that to fsi_occ_submit, to have more in common. Agree it could probably be organized better but I don't immediately have a good idea how to do that. Thanks for the review! Eddie > > > > > mutex_lock(&occ->occ_lock); > > > > - /* Extract the seq_no from the command (first byte) */ > > - seq_no = *(const u8 *)request; > > - rc = occ_putsram(occ, request, req_len); > > + /* > > + * Get a sequence number and update the counter. Avoid a > > sequence > > + * number of 0 which would pass the response check below > > even if the > > + * OCC response is uninitialized. Any sequence number the > > user is > > + * trying to send is overwritten since this function is the > > only common > > + * interface to the OCC and therefore the only place we can > > guarantee > > + * unique sequence numbers. > > + */ > > + seq_no = occ->sequence_number++; > > + if (!occ->sequence_number) > > + occ->sequence_number = 1; > > + checksum += seq_no; > > + > > + rc = occ_putsram(occ, request, req_len, seq_no, checksum); > > if (rc) > > goto done;
diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c index b223f0ef337b..ecf738411fe2 100644 --- a/drivers/fsi/fsi-occ.c +++ b/drivers/fsi/fsi-occ.c @@ -50,6 +50,7 @@ struct occ { struct device *sbefifo; char name[32]; int idx; + u8 sequence_number; enum versions version; struct miscdevice mdev; struct mutex occ_lock; @@ -141,8 +142,7 @@ static ssize_t occ_write(struct file *file, const char __user *buf, { struct occ_client *client = file->private_data; size_t rlen, data_length; - u16 checksum = 0; - ssize_t rc, i; + ssize_t rc; u8 *cmd; if (!client) @@ -156,9 +156,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf, /* Construct the command */ cmd = client->buffer; - /* Sequence number (we could increment and compare with response) */ - cmd[0] = 1; - /* * Copy the user command (assume user data follows the occ command * format) @@ -178,14 +175,7 @@ static ssize_t occ_write(struct file *file, const char __user *buf, goto done; } - /* Calculate checksum */ - for (i = 0; i < data_length + 4; ++i) - checksum += cmd[i]; - - cmd[data_length + 4] = checksum >> 8; - cmd[data_length + 5] = checksum & 0xFF; - - /* Submit command */ + /* Submit command; 4 bytes before the data and 2 bytes after */ rlen = PAGE_SIZE; rc = fsi_occ_submit(client->occ->dev, cmd, data_length + 6, cmd, &rlen); @@ -314,11 +304,13 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len) return rc; } -static int occ_putsram(struct occ *occ, const void *data, ssize_t len) +static int occ_putsram(struct occ *occ, const void *data, ssize_t len, + u8 seq_no, u16 checksum) { size_t cmd_len, buf_len, resp_len, resp_data_len; u32 data_len = ((len + 7) / 8) * 8; /* must be multiples of 8 B */ __be32 *buf; + u8 *byte_buf; int idx = 0, rc; cmd_len = (occ->version == occ_p10) ? 6 : 5; @@ -358,6 +350,15 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len) buf[4 + idx] = cpu_to_be32(data_len); memcpy(&buf[5 + idx], data, len); + byte_buf = (u8 *)&buf[5 + idx]; + /* + * Overwrite the first byte with our sequence number and the last two + * bytes with the checksum. + */ + byte_buf[0] = seq_no; + byte_buf[len - 2] = checksum >> 8; + byte_buf[len - 1] = checksum & 0xff; + rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len); if (rc) goto free; @@ -467,9 +468,12 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len, struct occ *occ = dev_get_drvdata(dev); struct occ_response *resp = response; u8 seq_no; + u16 checksum = 0; u16 resp_data_length; + const u8 *byte_request = (const u8 *)request; unsigned long start; int rc; + size_t i; if (!occ) return -ENODEV; @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len, return -EINVAL; } + /* Checksum the request, ignoring first byte (sequence number). */ + for (i = 1; i < req_len - 2; ++i) + checksum += byte_request[i]; + mutex_lock(&occ->occ_lock); - /* Extract the seq_no from the command (first byte) */ - seq_no = *(const u8 *)request; - rc = occ_putsram(occ, request, req_len); + /* + * Get a sequence number and update the counter. Avoid a sequence + * number of 0 which would pass the response check below even if the + * OCC response is uninitialized. Any sequence number the user is + * trying to send is overwritten since this function is the only common + * interface to the OCC and therefore the only place we can guarantee + * unique sequence numbers. + */ + seq_no = occ->sequence_number++; + if (!occ->sequence_number) + occ->sequence_number = 1; + checksum += seq_no; + + rc = occ_putsram(occ, request, req_len, seq_no, checksum); if (rc) goto done; @@ -574,6 +593,7 @@ static int occ_probe(struct platform_device *pdev) occ->version = (uintptr_t)of_device_get_match_data(dev); occ->dev = dev; occ->sbefifo = dev->parent; + occ->sequence_number = 1; mutex_init(&occ->occ_lock); if (dev->of_node) {
Set and increment the sequence number during the submit operation. This prevents sequence number conflicts between different users of the interface. A sequence number conflict may result in a user getting an OCC response meant for a different command. Since the sequence number is now modified, the checksum must be calculated and set before submitting the command. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/fsi/fsi-occ.c | 54 +++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 17 deletions(-)