[PATCH 0/3] platform/x86/intel/sdsi: Fixes for 5.18

[PATCH 0/3] platform/x86/intel/sdsi: Fixes for 5.18

[David E. Box]

The following patches provide fixes for the Intel SDSi driver based on
firmware updates and for one driver bug.

David E. Box (3):
  platform/x86/intel/sdsi: Handle leaky bucket
  platform/x86/intel/sdsi: Poll on ready bit for writes
  platform/x86/intel/sdsi: Fix bug in multi packet reads

 drivers/platform/x86/intel/sdsi.c | 44 +++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 14 deletions(-)


base-commit: b2d229d4ddb17db541098b83524d901257e93845
-- 
2.25.1

[PATCH 1/3] platform/x86/intel/sdsi: Handle leaky bucket

[David E. Box]

To prevent an agent from indefinitely holding the mailbox firmware has
implemented a leaky bucket algorithm. Repeated access to the mailbox may
now incur a delay of up to 2.1 seconds. Add a retry loop that tries for
up to 2.5 seconds to acquire the mailbox.

Fixes: 2546c6000430 ("platform/x86: Add Intel Software Defined Silicon driver")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 32 ++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index 11d14cc0ff0a..11f211402479 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -51,6 +51,8 @@
 #define MBOX_TIMEOUT_US			2000
 #define MBOX_TIMEOUT_ACQUIRE_US		1000
 #define MBOX_POLLING_PERIOD_US		100
+#define MBOX_ACQUIRE_NUM_RETRIES	5
+#define MBOX_ACQUIRE_RETRY_DELAY_MS	500
 #define MBOX_MAX_PACKETS		4
 
 #define MBOX_OWNER_NONE			0x00
@@ -263,7 +265,7 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info
 {
 	u64 control;
 	u32 owner;
-	int ret;
+	int ret, retries = 0;
 
 	lockdep_assert_held(&priv->mb_lock);
 
@@ -273,13 +275,29 @@ static int sdsi_mbox_acquire(struct sdsi_priv *priv, struct sdsi_mbox_info *info
 	if (owner != MBOX_OWNER_NONE)
 		return -EBUSY;
 
-	/* Write first qword of payload */
-	writeq(info->payload[0], priv->mbox_addr);
+	/*
+	 * If there has been no recent transaction and no one owns the mailbox,
+	 * we should acquire it in under 1ms. However, if we've accessed it
+	 * recently it may take up to 2.1 seconds to acquire it again.
+	 */
+	do {
+		/* Write first qword of payload */
+		writeq(info->payload[0], priv->mbox_addr);
+
+		/* Check for ownership */
+		ret = readq_poll_timeout(priv->control_addr, control,
+			FIELD_GET(CTRL_OWNER, control) == MBOX_OWNER_INBAND,
+			MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_ACQUIRE_US);
+
+		if (FIELD_GET(CTRL_OWNER, control) == MBOX_OWNER_NONE &&
+		    retries++ < MBOX_ACQUIRE_NUM_RETRIES) {
+			msleep(MBOX_ACQUIRE_RETRY_DELAY_MS);
+			continue;
+		}
 
-	/* Check for ownership */
-	ret = readq_poll_timeout(priv->control_addr, control,
-				 FIELD_GET(CTRL_OWNER, control) & MBOX_OWNER_INBAND,
-				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_ACQUIRE_US);
+		/* Either we got it or someone else did. */
+		break;
+	} while (true);
 
 	return ret;
 }
-- 
2.25.1

[PATCH 2/3] platform/x86/intel/sdsi: Poll on ready bit for writes

[David E. Box]

Due to change in firmware flow, update mailbox writes to poll on ready bit
instead of run_busy bit. This change makes the polling method consistent
for both writes and reads, which also uses the ready bit.

Fixes: 2546c6000430 ("platform/x86: Add Intel Software Defined Silicon driver")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index 11f211402479..89729fed030c 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -245,8 +245,8 @@ 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 run_busy bit */
-	ret = readq_poll_timeout(priv->control_addr, control, !(control & CTRL_RUN_BUSY),
+	/* Poll on ready bit */
+	ret = readq_poll_timeout(priv->control_addr, control, control & CTRL_READY,
 				 MBOX_POLLING_PERIOD_US, MBOX_TIMEOUT_US);
 
 	if (ret)
-- 
2.25.1

RE: [PATCH 2/3] platform/x86/intel/sdsi: Poll on ready bit for writes

[Limonciello, Mario]

[AMD Official Use Only]



> -----Original Message-----
> From: David E. Box <david.e.box@linux.intel.com>
> Sent: Wednesday, April 20, 2022 10:56
> To: hdegoede@redhat.com; david.e.box@linux.intel.com;
> markgross@kernel.org; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH 2/3] platform/x86/intel/sdsi: Poll on ready bit for writes
> 
> Due to change in firmware flow, update mailbox writes to poll on ready bit
> instead of run_busy bit. This change makes the polling method consistent
> for both writes and reads, which also uses the ready bit.

Does this need some sort of guard on the behavior based on the firmware
version you are running on or are these all pre-production still?

> 
> Fixes: 2546c6000430 ("platform/x86: Add Intel Software Defined Silicon driver")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel/sdsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index 11f211402479..89729fed030c 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -245,8 +245,8 @@ 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 run_busy bit */
> -	ret = readq_poll_timeout(priv->control_addr, control, !(control &
> CTRL_RUN_BUSY),
> +	/* Poll on ready bit */
> +	ret = readq_poll_timeout(priv->control_addr, control, control &
> CTRL_READY,
>  				 MBOX_POLLING_PERIOD_US,
> MBOX_TIMEOUT_US);
> 
>  	if (ret)
> --
> 2.25.1

Re: [PATCH 2/3] platform/x86/intel/sdsi: Poll on ready bit for writes

[David E. Box]

On Wed, 2022-04-20 at 16:34 +0000, Limonciello, Mario wrote:
> [AMD Official Use Only]
> 
> 
> 
> > -----Original Message-----
> > From: David E. Box <david.e.box@linux.intel.com>
> > Sent: Wednesday, April 20, 2022 10:56
> > To: hdegoede@redhat.com; david.e.box@linux.intel.com;
> > markgross@kernel.org; platform-driver-x86@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: [PATCH 2/3] platform/x86/intel/sdsi: Poll on ready bit for writes
> > 
> > Due to change in firmware flow, update mailbox writes to poll on ready bit
> > instead of run_busy bit. This change makes the polling method consistent
> > for both writes and reads, which also uses the ready bit.
> 
> Does this need some sort of guard on the behavior based on the firmware
> version you are running on or are these all pre-production still?

Firmware updates would include this change, but a guard isn't necessary. Polling
on run_busy would still work for the current ABI. Since currently mailbox writes
are single packet only, both ready and run_busy update at the same time. But it
would not work for future changes where there will be multi packet writes. In
that case, ready indicates completion of the packet while run_busy indicates
completion of the entire message.

David

> 
> > Fixes: 2546c6000430 ("platform/x86: Add Intel Software Defined Silicon
> > driver")
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/sdsi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/sdsi.c
> > b/drivers/platform/x86/intel/sdsi.c
> > index 11f211402479..89729fed030c 100644
> > --- a/drivers/platform/x86/intel/sdsi.c
> > +++ b/drivers/platform/x86/intel/sdsi.c
> > @@ -245,8 +245,8 @@ 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 run_busy bit */
> > -	ret = readq_poll_timeout(priv->control_addr, control, !(control &
> > CTRL_RUN_BUSY),
> > +	/* Poll on ready bit */
> > +	ret = readq_poll_timeout(priv->control_addr, control, control &
> > CTRL_READY,
> >  				 MBOX_POLLING_PERIOD_US,
> > MBOX_TIMEOUT_US);
> > 
> >  	if (ret)
> > --
> > 2.25.1

[PATCH 3/3] platform/x86/intel/sdsi: Fix bug in multi packet reads

[David E. Box]

Fix bug that added an offset to the mailbox addr during multi-packet
reads. Did not affect current ABI since it doesn't support multi-packet
transactions.

Fixes: 2546c6000430 ("platform/x86: Add Intel Software Defined Silicon driver")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
index 89729fed030c..c830e98dfa38 100644
--- a/drivers/platform/x86/intel/sdsi.c
+++ b/drivers/platform/x86/intel/sdsi.c
@@ -83,7 +83,7 @@ enum sdsi_command {
 
 struct sdsi_mbox_info {
 	u64	*payload;
-	u64	*buffer;
+	void	*buffer;
 	int	size;
 };
 
@@ -165,9 +165,7 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 	total = 0;
 	loop = 0;
 	do {
-		int offset = SDSI_SIZE_MAILBOX * loop;
-		void __iomem *addr = priv->mbox_addr + offset;
-		u64 *buf = info->buffer + offset / SDSI_SIZE_CMD;
+		void *buf = info->buffer + (SDSI_SIZE_MAILBOX * loop);
 		u32 packet_size;
 
 		/* Poll on ready bit */
@@ -198,7 +196,7 @@ static int sdsi_mbox_cmd_read(struct sdsi_priv *priv, struct sdsi_mbox_info *inf
 			break;
 		}
 
-		sdsi_memcpy64_fromio(buf, addr, round_up(packet_size, SDSI_SIZE_CMD));
+		sdsi_memcpy64_fromio(buf, priv->mbox_addr, round_up(packet_size, SDSI_SIZE_CMD));
 
 		total += packet_size;
 
-- 
2.25.1

Re: [PATCH 0/3] platform/x86/intel/sdsi: Fixes for 5.18

[Hans de Goede]

Hi,

On 4/20/22 17:56, David E. Box wrote:
> The following patches provide fixes for the Intel SDSi driver based on
> firmware updates and for one driver bug.

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans






> 
> David E. Box (3):
>   platform/x86/intel/sdsi: Handle leaky bucket
>   platform/x86/intel/sdsi: Poll on ready bit for writes
>   platform/x86/intel/sdsi: Fix bug in multi packet reads
> 
>  drivers/platform/x86/intel/sdsi.c | 44 +++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> 
> base-commit: b2d229d4ddb17db541098b83524d901257e93845