| Message ID | 20240201010747.471141-3-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Intel On Demand: Add netlink interface for SPDM attestation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 1/31/24 5:07 PM, David E. Box wrote: > The current mailbox commands are either read-only or write-only and the > flow is different for each. New commands will need to send and receive > data. In preparation for these commands, create a common polling function > to handle sending data and receiving in the same transaction. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 35 deletions(-) > > diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c > index a70c071de6e2..05a35f2f85b6 100644 > --- a/drivers/platform/x86/intel/sdsi.c > +++ b/drivers/platform/x86/intel/sdsi.c > @@ -15,6 +15,7 @@ > #include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/overflow.h> > #include <linux/pci.h> > #include <linux/slab.h> > #include <linux/sysfs.h> > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status) > } > } > > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > - size_t *data_size) > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > + size_t *data_size) > { > struct device *dev = priv->dev; > u32 total, loop, eom, status, message_size; > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > > lockdep_assert_held(&priv->mb_lock); > > - /* Format and send the read command */ > - control = FIELD_PREP(CTRL_EOM, 1) | > - FIELD_PREP(CTRL_SOM, 1) | > - FIELD_PREP(CTRL_RUN_BUSY, 1) | > - FIELD_PREP(CTRL_PACKET_SIZE, info->size); > - writeq(control, priv->control_addr); > - > /* For reads, data sizes that are larger than the mailbox size are read in packets. */ > total = 0; > loop = 0; > do { > - void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop); > u32 packet_size; > > /* Poll on ready bit */ > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > if (ret) > break; > > + if (!packet_size) { > + sdsi_complete_transaction(priv); > + break; > + } > + It seems to be a generic check. Is this related to converting to a read/write function or a common fix you added together in this patch. > /* Only the last packet can be less than the mailbox size. */ > if (!eom && packet_size != SDSI_SIZE_MAILBOX) { > dev_err(dev, "Invalid packet size\n"); > @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > break; > } > > - sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD)); > + if (packet_size && info->buffer) { > + void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop); > > - total += packet_size; > + sdsi_memcpy64_fromio(buf, priv->mbox_addr, > + round_up(packet_size, SDSI_SIZE_CMD)); > + total += packet_size; > + } > > sdsi_complete_transaction(priv); > } while (!eom && ++loop < MBOX_MAX_PACKETS); > @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > dev_warn(dev, "Read count %u differs from expected count %u\n", > total, message_size); > > - *data_size = total; > + if (data_size) > + *data_size = total; > > return 0; > } > > -static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) > +static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > + size_t *data_size) > +{ > + u64 control; > + > + lockdep_assert_held(&priv->mb_lock); > + > + /* Format and send the read command */ > + control = FIELD_PREP(CTRL_EOM, 1) | > + FIELD_PREP(CTRL_SOM, 1) | > + FIELD_PREP(CTRL_RUN_BUSY, 1) | > + FIELD_PREP(CTRL_PACKET_SIZE, info->size); > + writeq(control, priv->control_addr); > + > + return sdsi_mbox_poll(priv, info, data_size); > +} > + > +static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > + size_t *data_size) > { > u64 control; > - u32 status; > - int ret; > > lockdep_assert_held(&priv->mb_lock); > > @@ -256,20 +275,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in > FIELD_PREP(CTRL_PACKET_SIZE, info->size); > writeq(control, priv->control_addr); > > - /* Poll on ready bit */ > - ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY, > - MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US); > - > - if (ret) > - goto release_mbox; > - > - status = FIELD_GET(CTRL_STATUS, control); > - ret = sdsi_status_to_errno(status); > - > -release_mbox: > - sdsi_complete_transaction(priv); > - > - return ret; > + return sdsi_mbox_poll(priv, info, data_size); > } > > static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info) > @@ -313,7 +319,8 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info > return ret; > } > > -static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) > +static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > + size_t *data_size) > { > int ret; > > @@ -323,7 +330,7 @@ static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) > if (ret) > return ret; > > - return sdsi_mbox_cmd_write(priv, info); > + return sdsi_mbox_cmd_write(priv, info, data_size); > } > > static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, size_t *data_size) > @@ -342,7 +349,7 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s > static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count, > enum sdsi_command command) > { > - struct sdsi_mbox_info info; > + struct sdsi_mbox_info info = {}; This change also looks like an independent fix. Is this related to common function usage you mentioned in the commit log. > int ret; > > if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD)) > @@ -364,7 +371,9 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count, > ret = mutex_lock_interruptible(&priv->mb_lock); > if (ret) > goto free_payload; > - ret = sdsi_mbox_write(priv, &info); > + > + ret = sdsi_mbox_write(priv, &info, NULL); > + > mutex_unlock(&priv->mb_lock); > > free_payload: > @@ -408,7 +417,7 @@ static ssize_t > certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off, > size_t count) > { > - struct sdsi_mbox_info info; > + struct sdsi_mbox_info info = {}; > size_t size; > int ret; >
Hi Sathya, On Thu, 2024-02-01 at 09:31 -0800, Kuppuswamy Sathyanarayanan wrote: > > On 1/31/24 5:07 PM, David E. Box wrote: > > The current mailbox commands are either read-only or write-only and the > > flow is different for each. New commands will need to send and receive > > data. In preparation for these commands, create a common polling function > > to handle sending data and receiving in the same transaction. > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++-------------- > > 1 file changed, 44 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/sdsi.c > > b/drivers/platform/x86/intel/sdsi.c > > index a70c071de6e2..05a35f2f85b6 100644 > > --- a/drivers/platform/x86/intel/sdsi.c > > +++ b/drivers/platform/x86/intel/sdsi.c > > @@ -15,6 +15,7 @@ > > #include <linux/iopoll.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > +#include <linux/overflow.h> > > #include <linux/pci.h> > > #include <linux/slab.h> > > #include <linux/sysfs.h> > > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status) > > } > > } > > > > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info > > *info, > > - size_t *data_size) > > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info > > *info, > > + size_t *data_size) > > { > > struct device *dev = priv->dev; > > u32 total, loop, eom, status, message_size; > > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, > > struct sdsi_mbox_info *inf > > > > lockdep_assert_held(&priv->mb_lock); > > > > - /* Format and send the read command */ > > - control = FIELD_PREP(CTRL_EOM, 1) | > > - FIELD_PREP(CTRL_SOM, 1) | > > - FIELD_PREP(CTRL_RUN_BUSY, 1) | > > - FIELD_PREP(CTRL_PACKET_SIZE, info->size); > > - writeq(control, priv->control_addr); > > - > > /* For reads, data sizes that are larger than the mailbox size are > > read in packets. */ > > total = 0; > > loop = 0; > > do { > > - void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop); > > u32 packet_size; > > > > /* Poll on ready bit */ > > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, > > struct sdsi_mbox_info *inf > > if (ret) > > break; > > > > + if (!packet_size) { > > + sdsi_complete_transaction(priv); > > + break; > > + } > > + > > It seems to be a generic check. Is this related to converting to a read/write > function or > a common fix you added together in this patch. Yes, it's related. The code that follows this only applies to reads. This check would be true only for writes which only need to poll once to confirm the write was successful. I'll add a comment to make it clear.
On Wed, 31 Jan 2024, David E. Box wrote: > The current mailbox commands are either read-only or write-only and the > flow is different for each. New commands will need to send and receive > data. In preparation for these commands, create a common polling function > to handle sending data and receiving in the same transaction. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 35 deletions(-) > > diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c > index a70c071de6e2..05a35f2f85b6 100644 > --- a/drivers/platform/x86/intel/sdsi.c > +++ b/drivers/platform/x86/intel/sdsi.c > @@ -15,6 +15,7 @@ > #include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/overflow.h> > #include <linux/pci.h> > #include <linux/slab.h> > #include <linux/sysfs.h> > @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status) > } > } > > -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > - size_t *data_size) > +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info, > + size_t *data_size) > { > struct device *dev = priv->dev; > u32 total, loop, eom, status, message_size; > @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > > lockdep_assert_held(&priv->mb_lock); > > - /* Format and send the read command */ > - control = FIELD_PREP(CTRL_EOM, 1) | > - FIELD_PREP(CTRL_SOM, 1) | > - FIELD_PREP(CTRL_RUN_BUSY, 1) | > - FIELD_PREP(CTRL_PACKET_SIZE, info->size); > - writeq(control, priv->control_addr); > - > /* For reads, data sizes that are larger than the mailbox size are read in packets. */ > total = 0; > loop = 0; > do { > - void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop); > u32 packet_size; > > /* Poll on ready bit */ > @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > if (ret) > break; > > + if (!packet_size) { > + sdsi_complete_transaction(priv); > + break; > + } > + > /* Only the last packet can be less than the mailbox size. */ > if (!eom && packet_size != SDSI_SIZE_MAILBOX) { > dev_err(dev, "Invalid packet size\n"); > @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > break; > } > > - sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD)); > + if (packet_size && info->buffer) { Why you check for packet_size here if you break earlier for !packet_size? > + void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop); > > - total += packet_size; > + sdsi_memcpy64_fromio(buf, priv->mbox_addr, > + round_up(packet_size, SDSI_SIZE_CMD)); > + total += packet_size; > + } > > sdsi_complete_transaction(priv); > } while (!eom && ++loop < MBOX_MAX_PACKETS); > @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf > dev_warn(dev, "Read count %u differs from expected count %u\n", > total, message_size); > > - *data_size = total; > + if (data_size) > + *data_size = total; > > return 0; > }
diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c index a70c071de6e2..05a35f2f85b6 100644 --- a/drivers/platform/x86/intel/sdsi.c +++ b/drivers/platform/x86/intel/sdsi.c @@ -15,6 +15,7 @@ #include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/pci.h> #include <linux/slab.h> #include <linux/sysfs.h> @@ -156,8 +157,8 @@ static int sdsi_status_to_errno(u32 status) } } -static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, - size_t *data_size) +static int sdsi_mbox_poll(struct sdsi_priv *priv, struct sdsi_mbox_info *info, + size_t *data_size) { struct device *dev = priv->dev; u32 total, loop, eom, status, message_size; @@ -166,18 +167,10 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf lockdep_assert_held(&priv->mb_lock); - /* Format and send the read command */ - control = FIELD_PREP(CTRL_EOM, 1) | - FIELD_PREP(CTRL_SOM, 1) | - FIELD_PREP(CTRL_RUN_BUSY, 1) | - FIELD_PREP(CTRL_PACKET_SIZE, info->size); - writeq(control, priv->control_addr); - /* For reads, data sizes that are larger than the mailbox size are read in packets. */ total = 0; loop = 0; do { - void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop); u32 packet_size; /* Poll on ready bit */ @@ -195,6 +188,11 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf if (ret) break; + if (!packet_size) { + sdsi_complete_transaction(priv); + break; + } + /* Only the last packet can be less than the mailbox size. */ if (!eom && packet_size != SDSI_SIZE_MAILBOX) { dev_err(dev, "Invalid packet size\n"); @@ -208,9 +206,13 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf break; } - sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD)); + if (packet_size && info->buffer) { + void *buf = info->buffer + array_size(SDSI_SIZE_MAILBOX, loop); - total += packet_size; + sdsi_memcpy64_fromio(buf, priv->mbox_addr, + round_up(packet_size, SDSI_SIZE_CMD)); + total += packet_size; + } sdsi_complete_transaction(priv); } while (!eom && ++loop < MBOX_MAX_PACKETS); @@ -230,16 +232,33 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf dev_warn(dev, "Read count %u differs from expected count %u\n", total, message_size); - *data_size = total; + if (data_size) + *data_size = total; return 0; } -static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) +static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, + size_t *data_size) +{ + u64 control; + + lockdep_assert_held(&priv->mb_lock); + + /* Format and send the read command */ + control = FIELD_PREP(CTRL_EOM, 1) | + FIELD_PREP(CTRL_SOM, 1) | + FIELD_PREP(CTRL_RUN_BUSY, 1) | + FIELD_PREP(CTRL_PACKET_SIZE, info->size); + writeq(control, priv->control_addr); + + return sdsi_mbox_poll(priv, info, data_size); +} + +static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info, + size_t *data_size) { u64 control; - u32 status; - int ret; lockdep_assert_held(&priv->mb_lock); @@ -256,20 +275,7 @@ static int sdsi_mbox_cmd_write(struct sdsi_priv *priv, struct sdsi_mbox_info *in FIELD_PREP(CTRL_PACKET_SIZE, info->size); writeq(control, priv->control_addr); - /* Poll on ready bit */ - ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY, - MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US); - - if (ret) - goto release_mbox; - - status = FIELD_GET(CTRL_STATUS, control); - ret = sdsi_status_to_errno(status); - -release_mbox: - sdsi_complete_transaction(priv); - - return ret; + return sdsi_mbox_poll(priv, info, data_size); } static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info) @@ -313,7 +319,8 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info return ret; } -static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) +static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info, + size_t *data_size) { int ret; @@ -323,7 +330,7 @@ static int sdsi_mbox_write(struct sdsi_priv *priv, struct sdsi_mbox_info *info) if (ret) return ret; - return sdsi_mbox_cmd_write(priv, info); + return sdsi_mbox_cmd_write(priv, info, data_size); } static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, size_t *data_size) @@ -342,7 +349,7 @@ static int sdsi_mbox_read(struct sdsi_priv *priv, struct sdsi_mbox_info *info, s static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count, enum sdsi_command command) { - struct sdsi_mbox_info info; + struct sdsi_mbox_info info = {}; int ret; if (count > (SDSI_SIZE_WRITE_MSG - SDSI_SIZE_CMD)) @@ -364,7 +371,9 @@ static ssize_t sdsi_provision(struct sdsi_priv *priv, char *buf, size_t count, ret = mutex_lock_interruptible(&priv->mb_lock); if (ret) goto free_payload; - ret = sdsi_mbox_write(priv, &info); + + ret = sdsi_mbox_write(priv, &info, NULL); + mutex_unlock(&priv->mb_lock); free_payload: @@ -408,7 +417,7 @@ static ssize_t certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off, size_t count) { - struct sdsi_mbox_info info; + struct sdsi_mbox_info info = {}; size_t size; int ret;
The current mailbox commands are either read-only or write-only and the flow is different for each. New commands will need to send and receive data. In preparation for these commands, create a common polling function to handle sending data and receiving in the same transaction. Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- drivers/platform/x86/intel/sdsi.c | 79 +++++++++++++++++-------------- 1 file changed, 44 insertions(+), 35 deletions(-)