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

[Eddie James <eajames@linux.ibm.com>]

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,