[PATCH 0/2] fsi: scom: Error handling fixes

[PATCH 0/2] fsi: scom: Error handling fixes

[Joel Stanley]

Two error handling fixes, thank you to Dan for the bug report.

Joel Stanley (2):
  fsi: scom: Fix error handling
  fsi: scom: Remove retries in indirect scoms

 drivers/fsi/fsi-scom.c | 45 ++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

-- 
2.33.0

[PATCH 1/2] fsi: scom: Fix error handling

[Joel Stanley]

SCOM error handling is made complex by trying to pass around two bits of
information: the function return code, and a status parameter that
represents the CFAM error status register.

The commit f72ddbe1d7b7 ("fsi: scom: Remove retries") removed the
"hidden" retries in the SCOM driver, in preference of allowing the
calling code (userspace or driver) to decide how to handle a failed
SCOM. However it introduced a bug by attempting to be smart about the
return codes that were "errors" and which were ok to fall through to the
status register parsing.

We get the following errors:

 - EINVAL or ENXIO, for indirect scoms where the value is invalid
 - EINVAL, where the size or address is incorrect
 - EIO or ETIMEOUT, where FSI write failed (aspeed master)
 - EAGAIN, where the master detected a crc error (GPIO master only)
 - EBUSY, where the bus is disabled (GPIO master in external mode)

In all of these cases we should fail the SCOM read/write and return the
error.

Thanks to Dan Carpenter for the detailed bug report.

Fixes: f72ddbe1d7b7 ("fsi: scom: Remove retries")
Link: https://lists.ozlabs.org/pipermail/linux-fsi/2021-November/000235.html
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-scom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
index da1486bb6a14..3b427f7e9027 100644
--- a/drivers/fsi/fsi-scom.c
+++ b/drivers/fsi/fsi-scom.c
@@ -289,7 +289,7 @@ static int put_scom(struct scom_device *scom, uint64_t value,
 	int rc;
 
 	rc = raw_put_scom(scom, value, addr, &status);
-	if (rc == -ENODEV)
+	if (rc)
 		return rc;
 
 	rc = handle_fsi2pib_status(scom, status);
@@ -308,7 +308,7 @@ static int get_scom(struct scom_device *scom, uint64_t *value,
 	int rc;
 
 	rc = raw_get_scom(scom, value, addr, &status);
-	if (rc == -ENODEV)
+	if (rc)
 		return rc;
 
 	rc = handle_fsi2pib_status(scom, status);
-- 
2.33.0

[PATCH 2/2] fsi: scom: Remove retries in indirect scoms

[Joel Stanley]

In commit f72ddbe1d7b7 ("fsi: scom: Remove retries") the retries were
removed from get and put scoms. That patch missed the retires in get and
put indirect scom.

For the same reason, remove them from the scom driver to allow the
caller to decide to retry.

This removes the following special case which would have caused the
retry code to return early:

 -       if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
 -               return 0;

I believe this case is handled.

Fixes: f72ddbe1d7b7 ("fsi: scom: Remove retries")
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-scom.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
index 3b427f7e9027..bcb756dc9866 100644
--- a/drivers/fsi/fsi-scom.c
+++ b/drivers/fsi/fsi-scom.c
@@ -145,7 +145,7 @@ static int put_indirect_scom_form0(struct scom_device *scom, uint64_t value,
 				   uint64_t addr, uint32_t *status)
 {
 	uint64_t ind_data, ind_addr;
-	int rc, retries, err = 0;
+	int rc, err;
 
 	if (value & ~XSCOM_DATA_IND_DATA)
 		return -EINVAL;
@@ -156,19 +156,14 @@ static int put_indirect_scom_form0(struct scom_device *scom, uint64_t value,
 	if (rc || (*status & SCOM_STATUS_ANY_ERR))
 		return rc;
 
-	for (retries = 0; retries < SCOM_MAX_IND_RETRIES; retries++) {
-		rc = __get_scom(scom, &ind_data, addr, status);
-		if (rc || (*status & SCOM_STATUS_ANY_ERR))
-			return rc;
+	rc = __get_scom(scom, &ind_data, addr, status);
+	if (rc || (*status & SCOM_STATUS_ANY_ERR))
+		return rc;
 
-		err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
-		*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
-		if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
-			return 0;
+	err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
+	*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
 
-		msleep(1);
-	}
-	return rc;
+	return 0;
 }
 
 static int put_indirect_scom_form1(struct scom_device *scom, uint64_t value,
@@ -188,7 +183,7 @@ static int get_indirect_scom_form0(struct scom_device *scom, uint64_t *value,
 				   uint64_t addr, uint32_t *status)
 {
 	uint64_t ind_data, ind_addr;
-	int rc, retries, err = 0;
+	int rc, err;
 
 	ind_addr = addr & XSCOM_ADDR_DIRECT_PART;
 	ind_data = (addr & XSCOM_ADDR_INDIRECT_PART) | XSCOM_DATA_IND_READ;
@@ -196,21 +191,15 @@ static int get_indirect_scom_form0(struct scom_device *scom, uint64_t *value,
 	if (rc || (*status & SCOM_STATUS_ANY_ERR))
 		return rc;
 
-	for (retries = 0; retries < SCOM_MAX_IND_RETRIES; retries++) {
-		rc = __get_scom(scom, &ind_data, addr, status);
-		if (rc || (*status & SCOM_STATUS_ANY_ERR))
-			return rc;
-
-		err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
-		*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
-		*value = ind_data & XSCOM_DATA_IND_DATA;
+	rc = __get_scom(scom, &ind_data, addr, status);
+	if (rc || (*status & SCOM_STATUS_ANY_ERR))
+		return rc;
 
-		if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
-			return 0;
+	err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
+	*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
+	*value = ind_data & XSCOM_DATA_IND_DATA;
 
-		msleep(1);
-	}
-	return rc;
+	return 0;
 }
 
 static int raw_put_scom(struct scom_device *scom, uint64_t value,
-- 
2.33.0

Re: [PATCH 1/2] fsi: scom: Fix error handling

[Eddie James]

On 12/6/21 21:38, Joel Stanley wrote:
> SCOM error handling is made complex by trying to pass around two bits of
> information: the function return code, and a status parameter that
> represents the CFAM error status register.
>
> The commit f72ddbe1d7b7 ("fsi: scom: Remove retries") removed the
> "hidden" retries in the SCOM driver, in preference of allowing the
> calling code (userspace or driver) to decide how to handle a failed
> SCOM. However it introduced a bug by attempting to be smart about the
> return codes that were "errors" and which were ok to fall through to the
> status register parsing.
>
> We get the following errors:
>
>   - EINVAL or ENXIO, for indirect scoms where the value is invalid
>   - EINVAL, where the size or address is incorrect
>   - EIO or ETIMEOUT, where FSI write failed (aspeed master)
>   - EAGAIN, where the master detected a crc error (GPIO master only)
>   - EBUSY, where the bus is disabled (GPIO master in external mode)
>
> In all of these cases we should fail the SCOM read/write and return the
> error.
>
> Thanks to Dan Carpenter for the detailed bug report.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Fixes: f72ddbe1d7b7 ("fsi: scom: Remove retries")
> Link: https://lists.ozlabs.org/pipermail/linux-fsi/2021-November/000235.html
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>   drivers/fsi/fsi-scom.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
> index da1486bb6a14..3b427f7e9027 100644
> --- a/drivers/fsi/fsi-scom.c
> +++ b/drivers/fsi/fsi-scom.c
> @@ -289,7 +289,7 @@ static int put_scom(struct scom_device *scom, uint64_t value,
>   	int rc;
>   
>   	rc = raw_put_scom(scom, value, addr, &status);
> -	if (rc == -ENODEV)
> +	if (rc)
>   		return rc;
>   
>   	rc = handle_fsi2pib_status(scom, status);
> @@ -308,7 +308,7 @@ static int get_scom(struct scom_device *scom, uint64_t *value,
>   	int rc;
>   
>   	rc = raw_get_scom(scom, value, addr, &status);
> -	if (rc == -ENODEV)
> +	if (rc)
>   		return rc;
>   
>   	rc = handle_fsi2pib_status(scom, status);

Re: [PATCH 2/2] fsi: scom: Remove retries in indirect scoms

[Eddie James]

On 12/6/21 21:38, Joel Stanley wrote:
> In commit f72ddbe1d7b7 ("fsi: scom: Remove retries") the retries were
> removed from get and put scoms. That patch missed the retires in get and
> put indirect scom.
>
> For the same reason, remove them from the scom driver to allow the
> caller to decide to retry.
>
> This removes the following special case which would have caused the
> retry code to return early:
>
>   -       if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
>   -               return 0;
>
> I believe this case is handled.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Fixes: f72ddbe1d7b7 ("fsi: scom: Remove retries")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>   drivers/fsi/fsi-scom.c | 41 +++++++++++++++--------------------------
>   1 file changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
> index 3b427f7e9027..bcb756dc9866 100644
> --- a/drivers/fsi/fsi-scom.c
> +++ b/drivers/fsi/fsi-scom.c
> @@ -145,7 +145,7 @@ static int put_indirect_scom_form0(struct scom_device *scom, uint64_t value,
>   				   uint64_t addr, uint32_t *status)
>   {
>   	uint64_t ind_data, ind_addr;
> -	int rc, retries, err = 0;
> +	int rc, err;
>   
>   	if (value & ~XSCOM_DATA_IND_DATA)
>   		return -EINVAL;
> @@ -156,19 +156,14 @@ static int put_indirect_scom_form0(struct scom_device *scom, uint64_t value,
>   	if (rc || (*status & SCOM_STATUS_ANY_ERR))
>   		return rc;
>   
> -	for (retries = 0; retries < SCOM_MAX_IND_RETRIES; retries++) {
> -		rc = __get_scom(scom, &ind_data, addr, status);
> -		if (rc || (*status & SCOM_STATUS_ANY_ERR))
> -			return rc;
> +	rc = __get_scom(scom, &ind_data, addr, status);
> +	if (rc || (*status & SCOM_STATUS_ANY_ERR))
> +		return rc;
>   
> -		err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
> -		*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
> -		if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
> -			return 0;
> +	err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
> +	*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
>   
> -		msleep(1);
> -	}
> -	return rc;
> +	return 0;
>   }
>   
>   static int put_indirect_scom_form1(struct scom_device *scom, uint64_t value,
> @@ -188,7 +183,7 @@ static int get_indirect_scom_form0(struct scom_device *scom, uint64_t *value,
>   				   uint64_t addr, uint32_t *status)
>   {
>   	uint64_t ind_data, ind_addr;
> -	int rc, retries, err = 0;
> +	int rc, err;
>   
>   	ind_addr = addr & XSCOM_ADDR_DIRECT_PART;
>   	ind_data = (addr & XSCOM_ADDR_INDIRECT_PART) | XSCOM_DATA_IND_READ;
> @@ -196,21 +191,15 @@ static int get_indirect_scom_form0(struct scom_device *scom, uint64_t *value,
>   	if (rc || (*status & SCOM_STATUS_ANY_ERR))
>   		return rc;
>   
> -	for (retries = 0; retries < SCOM_MAX_IND_RETRIES; retries++) {
> -		rc = __get_scom(scom, &ind_data, addr, status);
> -		if (rc || (*status & SCOM_STATUS_ANY_ERR))
> -			return rc;
> -
> -		err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
> -		*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
> -		*value = ind_data & XSCOM_DATA_IND_DATA;
> +	rc = __get_scom(scom, &ind_data, addr, status);
> +	if (rc || (*status & SCOM_STATUS_ANY_ERR))
> +		return rc;
>   
> -		if ((ind_data & XSCOM_DATA_IND_COMPLETE) || (err != SCOM_PIB_BLOCKED))
> -			return 0;
> +	err = (ind_data & XSCOM_DATA_IND_ERR_MASK) >> XSCOM_DATA_IND_ERR_SHIFT;
> +	*status = err << SCOM_STATUS_PIB_RESP_SHIFT;
> +	*value = ind_data & XSCOM_DATA_IND_DATA;
>   
> -		msleep(1);
> -	}
> -	return rc;
> +	return 0;
>   }
>   
>   static int raw_put_scom(struct scom_device *scom, uint64_t value,