From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-spi@vger.kernel.org
Subject: Re: [bug report] spi: core: Only check bits_per_word validity when explicitly provided
Date: Thu, 14 Apr 2022 09:04:41 +0200 [thread overview]
Message-ID: <YlfHidrhxMvUYHmH@aptenodytes> (raw)
In-Reply-To: <20220414064948.GA9873@kili>
[-- 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 --]
prev parent reply other threads:[~2022-04-14 7:04 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YlfHidrhxMvUYHmH@aptenodytes \
--to=paul.kocialkowski@bootlin.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-spi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.