| Message ID | 20230710154932.68377-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: Header and core clean up and refactoring | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD e8605e8fdf42 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 28 this patch: 28 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 149 this patch: 149 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > Since the new spi_controller_xfer_timeout() helper appeared, > we may replace open coded variant in spi_transfer_wait(). > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > + * You didn't mention this bit in the changelog, and I'm not 100% convinced it was the best idea in the first place. It's going to result in some very big timeouts if it goes off, and we really should be doing validation much earlier in the process. > + u32 speed_hz = xfer->speed_hz ?: 100000; Not only the ternery operator, but the version without the second argument for extra clarity!
Il 10/07/23 17:49, Andy Shevchenko ha scritto: > Since the new spi_controller_xfer_timeout() helper appeared, > we may replace open coded variant in spi_transfer_wait(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/spi/spi.c | 25 ++----------------------- > include/linux/spi/spi.h | 6 +++++- > 2 files changed, 7 insertions(+), 24 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 125dea8fae00..c99ee4164f11 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1342,8 +1342,7 @@ static int spi_transfer_wait(struct spi_controller *ctlr, > { > struct spi_statistics __percpu *statm = ctlr->pcpu_statistics; > struct spi_statistics __percpu *stats = msg->spi->pcpu_statistics; > - u32 speed_hz = xfer->speed_hz; > - unsigned long long ms; > + unsigned long ms; > > if (spi_controller_is_slave(ctlr)) { > if (wait_for_completion_interruptible(&ctlr->xfer_completion)) { > @@ -1351,29 +1350,9 @@ static int spi_transfer_wait(struct spi_controller *ctlr, > return -EINTR; > } > } else { > - if (!speed_hz) > - speed_hz = 100000; > - > - /* > - * For each byte we wait for 8 cycles of the SPI clock. > - * Since speed is defined in Hz and we want milliseconds, > - * use respective multiplier, but before the division, > - * otherwise we may get 0 for short transfers. > - */ > - ms = 8LL * MSEC_PER_SEC * xfer->len; > - do_div(ms, speed_hz); > - > - /* > - * Increase it twice and add 200 ms tolerance, use > - * predefined maximum in case of overflow. > - */ > - ms += ms + 200; > - if (ms > UINT_MAX) > - ms = UINT_MAX; > - > + ms = spi_controller_xfer_timeout(ctlr, xfer); I agree on using helpers, but the logic is slightly changing here: yes it is unlikely (and also probably useless) to get ms == UINT_MAX, but the helper is limiting the maximum timeout value to 500mS, which may not work for some slow controllers/devices. This should get validated on more than a few platforms, and I'm not sure that this kind of validation would be "fast" to get... so, probably the best thing to do here is to add a warning in case the timeout exceeds 500mS, print the actual value, keep it like this for a kernel version or two and check reports: that would allow to understand what a safe maximum timeout value could be. Aside from that, I wouldn't drop those nice comments explaining how/why the timeout is calculated: I know how, but not everyone knows in advance. Regards, Angelo > ms = wait_for_completion_timeout(&ctlr->xfer_completion, > msecs_to_jiffies(ms)); > - > if (ms == 0) { > SPI_STATISTICS_INCREMENT_FIELD(statm, timedout); > SPI_STATISTICS_INCREMENT_FIELD(stats, timedout); > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 32c94eae8926..0ce1cb18a076 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -1270,12 +1270,16 @@ static inline bool spi_is_bpw_supported(struct spi_device *spi, u32 bpw) > * that it would take on a single data line and take twice this amount of time > * with a minimum of 500ms to avoid false positives on loaded systems. > * > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > + * > * Returns: Transfer timeout value in milliseconds. > */ > static inline unsigned int spi_controller_xfer_timeout(struct spi_controller *ctlr, > struct spi_transfer *xfer) > { > - return max(xfer->len * 8 * 2 / (xfer->speed_hz / 1000), 500U); > + u32 speed_hz = xfer->speed_hz ?: 100000; > + > + return max(xfer->len * 8 * 2 / (speed_hz / 1000), 500U); > } > > /*---------------------------------------------------------------------------*/
On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > Since the new spi_controller_xfer_timeout() helper appeared, > > we may replace open coded variant in spi_transfer_wait(). > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > + * > > You didn't mention this bit in the changelog, and I'm not 100% convinced > it was the best idea in the first place. It's going to result in some > very big timeouts if it goes off, and we really should be doing > validation much earlier in the process. Okay, let's drop this change. > > + u32 speed_hz = xfer->speed_hz ?: 100000; > > Not only the ternery operator, but the version without the second > argument for extra clarity! Elvis can be interpreted as "A _or_ B (if A is false/0)". Some pieces related to SPI use Elvis already IIRC.
On Tue, Jul 11, 2023 at 10:28:13AM +0200, AngeloGioacchino Del Regno wrote: > Il 10/07/23 17:49, Andy Shevchenko ha scritto: > > + ms = spi_controller_xfer_timeout(ctlr, xfer); > I agree on using helpers, but the logic is slightly changing here: yes it is > unlikely (and also probably useless) to get ms == UINT_MAX, but the helper is > limiting the maximum timeout value to 500mS, which may not work for some slow > controllers/devices. The helper is limiting the *minimum* timeout value to 500ms - it's using max() not min(). The idea is the other way around, that for a very fast transfer we don't want to end up with such a short timeout that it false triggers due to scheduling issues.
Il 11/07/23 15:05, Mark Brown ha scritto: > On Tue, Jul 11, 2023 at 10:28:13AM +0200, AngeloGioacchino Del Regno wrote: >> Il 10/07/23 17:49, Andy Shevchenko ha scritto: > >>> + ms = spi_controller_xfer_timeout(ctlr, xfer); > >> I agree on using helpers, but the logic is slightly changing here: yes it is >> unlikely (and also probably useless) to get ms == UINT_MAX, but the helper is >> limiting the maximum timeout value to 500mS, which may not work for some slow >> controllers/devices. > > The helper is limiting the *minimum* timeout value to 500ms - it's using > max() not min(). The idea is the other way around, that for a very fast > transfer we don't want to end up with such a short timeout that it false > triggers due to scheduling issues. After reading the code again, yeah, I've totally misread it the first time. Argh! Thanks! :-)
On Tue, Jul 11, 2023 at 02:01:13PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > You didn't mention this bit in the changelog, and I'm not 100% convinced > > it was the best idea in the first place. It's going to result in some > > very big timeouts if it goes off, and we really should be doing > > validation much earlier in the process. > Okay, let's drop this change. Like I say we *should* be fine with the refactoring without this, or at least if it's an issue we should improve the validation. > > > + u32 speed_hz = xfer->speed_hz ?: 100000; > > Not only the ternery operator, but the version without the second > > argument for extra clarity! > Elvis can be interpreted as "A _or_ B (if A is false/0)". > Some pieces related to SPI use Elvis already IIRC. I understand what it means, I just don't find it's adding clarity most of the times it's used (there's a few places where it is useful like pasting in strings in formats). There are some examples that I'd complain about in the code, most of them predating me working on SPI too much, but I'm not a fan.
On Tue, Jul 11, 2023 at 03:14:54PM +0100, Mark Brown wrote: > On Tue, Jul 11, 2023 at 02:01:13PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 10, 2023 at 06:30:32PM +0100, Mark Brown wrote: > > > On Mon, Jul 10, 2023 at 06:49:21PM +0300, Andy Shevchenko wrote: > > > > > + * Assume speed to be 100 kHz if it's not defined at the time of invocation. > > > > You didn't mention this bit in the changelog, and I'm not 100% convinced > > > it was the best idea in the first place. It's going to result in some > > > very big timeouts if it goes off, and we really should be doing > > > validation much earlier in the process. > > > Okay, let's drop this change. > > Like I say we *should* be fine with the refactoring without this, or at > least if it's an issue we should improve the validation. For the speeds < 1000 Hz, this change will lead to the div by 0 crash. It seems that the current code which this one removes is better than the spi_controller_xfer_timeout() provides. If anything, the spi_controller_xfer_timeout() should be improved first. So, for now I drop this for sure. Maybe in the future we can come back to it.
On Tue, Jul 11, 2023 at 06:30:06PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 03:14:54PM +0100, Mark Brown wrote: > > Like I say we *should* be fine with the refactoring without this, or at > > least if it's an issue we should improve the validation. > For the speeds < 1000 Hz, this change will lead to the div by 0 crash. > It seems that the current code which this one removes is better than > the spi_controller_xfer_timeout() provides. > If anything, the spi_controller_xfer_timeout() should be improved first. > So, for now I drop this for sure. Maybe in the future we can come back > to it. I don't think this is the only thing that might fall over without a speed, what we've generally been doing (and do try to do with speeds, we already need to default in the controller's speed and so on) is to sanitise input on the way into the subsystem rather than trying to ensure that all the users are handling everything.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 125dea8fae00..c99ee4164f11 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1342,8 +1342,7 @@ static int spi_transfer_wait(struct spi_controller *ctlr, { struct spi_statistics __percpu *statm = ctlr->pcpu_statistics; struct spi_statistics __percpu *stats = msg->spi->pcpu_statistics; - u32 speed_hz = xfer->speed_hz; - unsigned long long ms; + unsigned long ms; if (spi_controller_is_slave(ctlr)) { if (wait_for_completion_interruptible(&ctlr->xfer_completion)) { @@ -1351,29 +1350,9 @@ static int spi_transfer_wait(struct spi_controller *ctlr, return -EINTR; } } else { - if (!speed_hz) - speed_hz = 100000; - - /* - * For each byte we wait for 8 cycles of the SPI clock. - * Since speed is defined in Hz and we want milliseconds, - * use respective multiplier, but before the division, - * otherwise we may get 0 for short transfers. - */ - ms = 8LL * MSEC_PER_SEC * xfer->len; - do_div(ms, speed_hz); - - /* - * Increase it twice and add 200 ms tolerance, use - * predefined maximum in case of overflow. - */ - ms += ms + 200; - if (ms > UINT_MAX) - ms = UINT_MAX; - + ms = spi_controller_xfer_timeout(ctlr, xfer); ms = wait_for_completion_timeout(&ctlr->xfer_completion, msecs_to_jiffies(ms)); - if (ms == 0) { SPI_STATISTICS_INCREMENT_FIELD(statm, timedout); SPI_STATISTICS_INCREMENT_FIELD(stats, timedout); diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 32c94eae8926..0ce1cb18a076 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -1270,12 +1270,16 @@ static inline bool spi_is_bpw_supported(struct spi_device *spi, u32 bpw) * that it would take on a single data line and take twice this amount of time * with a minimum of 500ms to avoid false positives on loaded systems. * + * Assume speed to be 100 kHz if it's not defined at the time of invocation. + * * Returns: Transfer timeout value in milliseconds. */ static inline unsigned int spi_controller_xfer_timeout(struct spi_controller *ctlr, struct spi_transfer *xfer) { - return max(xfer->len * 8 * 2 / (xfer->speed_hz / 1000), 500U); + u32 speed_hz = xfer->speed_hz ?: 100000; + + return max(xfer->len * 8 * 2 / (speed_hz / 1000), 500U); } /*---------------------------------------------------------------------------*/
Since the new spi_controller_xfer_timeout() helper appeared, we may replace open coded variant in spi_transfer_wait(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/spi/spi.c | 25 ++----------------------- include/linux/spi/spi.h | 6 +++++- 2 files changed, 7 insertions(+), 24 deletions(-)