* [PATCH 0/2] fsi: scom: Error handling fixes @ 2021-12-07 3:38 Joel Stanley 2021-12-07 3:38 ` [PATCH 1/2] fsi: scom: Fix error handling Joel Stanley 2021-12-07 3:38 ` [PATCH 2/2] fsi: scom: Remove retries in indirect scoms Joel Stanley 0 siblings, 2 replies; 5+ messages in thread From: Joel Stanley @ 2021-12-07 3:38 UTC (permalink / raw) To: Jeremy Kerr, Alistar Popple, Eddie James Cc: linux-fsi, linux-kernel, Dan Carpenter, Greg KH 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] fsi: scom: Fix error handling 2021-12-07 3:38 [PATCH 0/2] fsi: scom: Error handling fixes Joel Stanley @ 2021-12-07 3:38 ` Joel Stanley 2022-01-10 23:13 ` Eddie James 2021-12-07 3:38 ` [PATCH 2/2] fsi: scom: Remove retries in indirect scoms Joel Stanley 1 sibling, 1 reply; 5+ messages in thread From: Joel Stanley @ 2021-12-07 3:38 UTC (permalink / raw) To: Jeremy Kerr, Alistar Popple, Eddie James Cc: linux-fsi, linux-kernel, Dan Carpenter, Greg KH 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] fsi: scom: Fix error handling 2021-12-07 3:38 ` [PATCH 1/2] fsi: scom: Fix error handling Joel Stanley @ 2022-01-10 23:13 ` Eddie James 0 siblings, 0 replies; 5+ messages in thread From: Eddie James @ 2022-01-10 23:13 UTC (permalink / raw) To: Joel Stanley, Jeremy Kerr, Alistar Popple Cc: linux-fsi, linux-kernel, Dan Carpenter, Greg KH 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] fsi: scom: Remove retries in indirect scoms 2021-12-07 3:38 [PATCH 0/2] fsi: scom: Error handling fixes Joel Stanley 2021-12-07 3:38 ` [PATCH 1/2] fsi: scom: Fix error handling Joel Stanley @ 2021-12-07 3:38 ` Joel Stanley 2022-01-10 23:13 ` Eddie James 1 sibling, 1 reply; 5+ messages in thread From: Joel Stanley @ 2021-12-07 3:38 UTC (permalink / raw) To: Jeremy Kerr, Alistar Popple, Eddie James Cc: linux-fsi, linux-kernel, Dan Carpenter, Greg KH 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] fsi: scom: Remove retries in indirect scoms 2021-12-07 3:38 ` [PATCH 2/2] fsi: scom: Remove retries in indirect scoms Joel Stanley @ 2022-01-10 23:13 ` Eddie James 0 siblings, 0 replies; 5+ messages in thread From: Eddie James @ 2022-01-10 23:13 UTC (permalink / raw) To: Joel Stanley, Jeremy Kerr, Alistar Popple Cc: linux-fsi, linux-kernel, Dan Carpenter, Greg KH 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, ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-10 23:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-07 3:38 [PATCH 0/2] fsi: scom: Error handling fixes Joel Stanley 2021-12-07 3:38 ` [PATCH 1/2] fsi: scom: Fix error handling Joel Stanley 2022-01-10 23:13 ` Eddie James 2021-12-07 3:38 ` [PATCH 2/2] fsi: scom: Remove retries in indirect scoms Joel Stanley 2022-01-10 23:13 ` Eddie James
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.