* [bug report] spi: core: Only check bits_per_word validity when explicitly provided
@ 2022-04-14 6:49 Dan Carpenter
2022-04-14 7:04 ` Paul Kocialkowski
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-04-14 6:49 UTC (permalink / raw)
To: paul.kocialkowski; +Cc: linux-spi
Hello Paul Kocialkowski,
The patch b3fe2e516741: "spi: core: Only check bits_per_word validity
when explicitly provided" from Apr 12, 2022, leads to the following
Smatch static checker warning:
drivers/spi/spi.c:3583 spi_setup()
error: uninitialized symbol 'status'.
drivers/spi/spi.c
3475 int spi_setup(struct spi_device *spi)
3476 {
3477 unsigned bad_bits, ugly_bits;
3478 int status;
3479
3480 /*
3481 * Check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO
3482 * are set at the same time.
3483 */
3484 if ((hweight_long(spi->mode &
3485 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) ||
3486 (hweight_long(spi->mode &
3487 (SPI_RX_DUAL | SPI_RX_QUAD | SPI_NO_RX)) > 1)) {
3488 dev_err(&spi->dev,
3489 "setup: can not select any two of dual, quad and no-rx/tx at the same time\n");
3490 return -EINVAL;
3491 }
3492 /* If it is SPI_3WIRE mode, DUAL and QUAD should be forbidden */
3493 if ((spi->mode & SPI_3WIRE) && (spi->mode &
3494 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
3495 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
3496 return -EINVAL;
3497 /*
3498 * Help drivers fail *cleanly* when they need options
3499 * that aren't supported with their current controller.
3500 * SPI_CS_WORD has a fallback software implementation,
3501 * so it is ignored here.
3502 */
3503 bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
3504 SPI_NO_TX | SPI_NO_RX);
3505 ugly_bits = bad_bits &
3506 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
3507 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);
3508 if (ugly_bits) {
3509 dev_warn(&spi->dev,
3510 "setup: ignoring unsupported mode bits %x\n",
3511 ugly_bits);
3512 spi->mode &= ~ugly_bits;
3513 bad_bits &= ~ugly_bits;
3514 }
3515 if (bad_bits) {
3516 dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
3517 bad_bits);
3518 return -EINVAL;
3519 }
3520
3521 if (!spi->bits_per_word) {
3522 spi->bits_per_word = 8;
"status" used to be set here, for sure. You would have to be pretty
unlucky to get through this function without setting status at all.
3523 } else {
3524 /*
3525 * Some controllers may not support the default 8 bits-per-word
3526 * so only perform the check when this is explicitly provided.
3527 */
3528 status = __spi_validate_bits_per_word(spi->controller,
3529 spi->bits_per_word);
3530 if (status)
3531 return status;
3532 }
3533
3534 if (spi->controller->max_speed_hz &&
3535 (!spi->max_speed_hz ||
3536 spi->max_speed_hz > spi->controller->max_speed_hz))
3537 spi->max_speed_hz = spi->controller->max_speed_hz;
3538
3539 mutex_lock(&spi->controller->io_mutex);
3540
3541 if (spi->controller->setup) {
3542 status = spi->controller->setup(spi);
3543 if (status) {
3544 mutex_unlock(&spi->controller->io_mutex);
3545 dev_err(&spi->controller->dev, "Failed to setup device: %d\n",
3546 status);
3547 return status;
3548 }
3549 }
3550
3551 if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
3552 status = pm_runtime_get_sync(spi->controller->dev.parent);
3553 if (status < 0) {
3554 mutex_unlock(&spi->controller->io_mutex);
3555 pm_runtime_put_noidle(spi->controller->dev.parent);
3556 dev_err(&spi->controller->dev, "Failed to power device: %d\n",
3557 status);
3558 return status;
3559 }
3560
3561 /*
3562 * We do not want to return positive value from pm_runtime_get,
3563 * there are many instances of devices calling spi_setup() and
3564 * checking for a non-zero return value instead of a negative
3565 * return value.
3566 */
3567 status = 0;
3568
3569 spi_set_cs(spi, false, true);
3570 pm_runtime_mark_last_busy(spi->controller->dev.parent);
3571 pm_runtime_put_autosuspend(spi->controller->dev.parent);
3572 } else {
3573 spi_set_cs(spi, false, true);
3574 }
3575
3576 mutex_unlock(&spi->controller->io_mutex);
3577
3578 if (spi->rt && !spi->controller->rt) {
3579 spi->controller->rt = true;
3580 spi_set_thread_rt(spi->controller);
3581 }
3582
3583 trace_spi_setup(spi, status);
3584
3585 dev_dbg(&spi->dev, "setup mode %lu, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
3586 spi->mode & SPI_MODE_X_MASK,
3587 (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
3588 (spi->mode & SPI_LSB_FIRST) ? "lsb, " : "",
3589 (spi->mode & SPI_3WIRE) ? "3wire, " : "",
3590 (spi->mode & SPI_LOOP) ? "loopback, " : "",
3591 spi->bits_per_word, spi->max_speed_hz,
3592 status);
3593
3594 return status;
3595 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] spi: core: Only check bits_per_word validity when explicitly provided
2022-04-14 6:49 [bug report] spi: core: Only check bits_per_word validity when explicitly provided Dan Carpenter
@ 2022-04-14 7:04 ` Paul Kocialkowski
0 siblings, 0 replies; 2+ messages in thread
From: Paul Kocialkowski @ 2022-04-14 7:04 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-spi
[-- Attachment #1: Type: text/plain, Size: 7752 bytes --]
Hi Dan,
On Thu 14 Apr 22, 09:49, Dan Carpenter wrote:
> Hello Paul Kocialkowski,
>
> The patch b3fe2e516741: "spi: core: Only check bits_per_word validity
> when explicitly provided" from Apr 12, 2022, leads to the following
> Smatch static checker warning:
>
> drivers/spi/spi.c:3583 spi_setup()
> error: uninitialized symbol 'status'.
>
> drivers/spi/spi.c
> 3475 int spi_setup(struct spi_device *spi)
> 3476 {
> 3477 unsigned bad_bits, ugly_bits;
> 3478 int status;
> 3479
> 3480 /*
> 3481 * Check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO
> 3482 * are set at the same time.
> 3483 */
> 3484 if ((hweight_long(spi->mode &
> 3485 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) ||
> 3486 (hweight_long(spi->mode &
> 3487 (SPI_RX_DUAL | SPI_RX_QUAD | SPI_NO_RX)) > 1)) {
> 3488 dev_err(&spi->dev,
> 3489 "setup: can not select any two of dual, quad and no-rx/tx at the same time\n");
> 3490 return -EINVAL;
> 3491 }
> 3492 /* If it is SPI_3WIRE mode, DUAL and QUAD should be forbidden */
> 3493 if ((spi->mode & SPI_3WIRE) && (spi->mode &
> 3494 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
> 3495 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
> 3496 return -EINVAL;
> 3497 /*
> 3498 * Help drivers fail *cleanly* when they need options
> 3499 * that aren't supported with their current controller.
> 3500 * SPI_CS_WORD has a fallback software implementation,
> 3501 * so it is ignored here.
> 3502 */
> 3503 bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
> 3504 SPI_NO_TX | SPI_NO_RX);
> 3505 ugly_bits = bad_bits &
> 3506 (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
> 3507 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);
> 3508 if (ugly_bits) {
> 3509 dev_warn(&spi->dev,
> 3510 "setup: ignoring unsupported mode bits %x\n",
> 3511 ugly_bits);
> 3512 spi->mode &= ~ugly_bits;
> 3513 bad_bits &= ~ugly_bits;
> 3514 }
> 3515 if (bad_bits) {
> 3516 dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
> 3517 bad_bits);
> 3518 return -EINVAL;
> 3519 }
> 3520
> 3521 if (!spi->bits_per_word) {
> 3522 spi->bits_per_word = 8;
>
> "status" used to be set here, for sure. You would have to be pretty
> unlucky to get through this function without setting status at all.
Oh I didn't realize that status was returned at the end. Yeah there are
definitely cases where that will happen.
Sending a fix to initialize status = 0 soon.
Thanks for the catch & report!
Paul
> 3523 } else {
> 3524 /*
> 3525 * Some controllers may not support the default 8 bits-per-word
> 3526 * so only perform the check when this is explicitly provided.
> 3527 */
> 3528 status = __spi_validate_bits_per_word(spi->controller,
> 3529 spi->bits_per_word);
> 3530 if (status)
> 3531 return status;
> 3532 }
> 3533
> 3534 if (spi->controller->max_speed_hz &&
> 3535 (!spi->max_speed_hz ||
> 3536 spi->max_speed_hz > spi->controller->max_speed_hz))
> 3537 spi->max_speed_hz = spi->controller->max_speed_hz;
> 3538
> 3539 mutex_lock(&spi->controller->io_mutex);
> 3540
> 3541 if (spi->controller->setup) {
> 3542 status = spi->controller->setup(spi);
> 3543 if (status) {
> 3544 mutex_unlock(&spi->controller->io_mutex);
> 3545 dev_err(&spi->controller->dev, "Failed to setup device: %d\n",
> 3546 status);
> 3547 return status;
> 3548 }
> 3549 }
> 3550
> 3551 if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
> 3552 status = pm_runtime_get_sync(spi->controller->dev.parent);
> 3553 if (status < 0) {
> 3554 mutex_unlock(&spi->controller->io_mutex);
> 3555 pm_runtime_put_noidle(spi->controller->dev.parent);
> 3556 dev_err(&spi->controller->dev, "Failed to power device: %d\n",
> 3557 status);
> 3558 return status;
> 3559 }
> 3560
> 3561 /*
> 3562 * We do not want to return positive value from pm_runtime_get,
> 3563 * there are many instances of devices calling spi_setup() and
> 3564 * checking for a non-zero return value instead of a negative
> 3565 * return value.
> 3566 */
> 3567 status = 0;
> 3568
> 3569 spi_set_cs(spi, false, true);
> 3570 pm_runtime_mark_last_busy(spi->controller->dev.parent);
> 3571 pm_runtime_put_autosuspend(spi->controller->dev.parent);
> 3572 } else {
> 3573 spi_set_cs(spi, false, true);
> 3574 }
> 3575
> 3576 mutex_unlock(&spi->controller->io_mutex);
> 3577
> 3578 if (spi->rt && !spi->controller->rt) {
> 3579 spi->controller->rt = true;
> 3580 spi_set_thread_rt(spi->controller);
> 3581 }
> 3582
> 3583 trace_spi_setup(spi, status);
> 3584
> 3585 dev_dbg(&spi->dev, "setup mode %lu, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> 3586 spi->mode & SPI_MODE_X_MASK,
> 3587 (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> 3588 (spi->mode & SPI_LSB_FIRST) ? "lsb, " : "",
> 3589 (spi->mode & SPI_3WIRE) ? "3wire, " : "",
> 3590 (spi->mode & SPI_LOOP) ? "loopback, " : "",
> 3591 spi->bits_per_word, spi->max_speed_hz,
> 3592 status);
> 3593
> 3594 return status;
> 3595 }
>
> regards,
> dan carpenter
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-04-14 7:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-14 6:49 [bug report] spi: core: Only check bits_per_word validity when explicitly provided Dan Carpenter
2022-04-14 7:04 ` Paul Kocialkowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).