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:
signature.asc
Description: PGP signature
