Re: [bug report] spi: gpio: Use devm_spi_register_master()

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Sep 8, 2020 at 12:24 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Andrey Smirnov,
>
> The patch 79567c1a321e: "spi: gpio: Use devm_spi_register_master()"
> from Apr 2, 2019, leads to the following static checker warning:
>
>         drivers/spi/spi-gpio.c:435 spi_gpio_probe()
>         warn: 'master->dev.kobj' not decremented on lines: 435.
>
> drivers/spi/spi-gpio.c
>    358  static int spi_gpio_probe(struct platform_device *pdev)
>    359  {
>    360          int                             status;
>    361          struct spi_master               *master;
>    362          struct spi_gpio                 *spi_gpio;
>    363          struct device                   *dev = &pdev->dev;
>    364          struct spi_bitbang              *bb;
>    365
>    366          master = spi_alloc_master(dev, sizeof(*spi_gpio));
>    367          if (!master)
>    368                  return -ENOMEM;
>    369
>    370          status = devm_add_action_or_reset(&pdev->dev, spi_gpio_put, master);
>    371          if (status) {
>    372                  spi_master_put(master);
>                         ^^^^^^^^^^^^^^^^^^^^^^
> The devm_add_action_or_reset() function calls spi_gpio_put() on error
> so this seems like a double free.
>

This does look like a double free. Can't comment on the logic behind
it, that's a change Navid made, so I'll let him speak for it.


>    373                  return status;
>    374          }
>    375
>    376          if (pdev->dev.of_node)
>    377                  status = spi_gpio_probe_dt(pdev, master);
>    378          else
>    379                  status = spi_gpio_probe_pdata(pdev, master);
>    380
>    381          if (status)
>    382                  return status;
>    383
>    384          spi_gpio = spi_master_get_devdata(master);
>    385
>    386          status = spi_gpio_request(dev, spi_gpio);
>    387          if (status)
>    388                  return status;
>    389
>    390          master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
>    391          master->mode_bits = SPI_3WIRE | SPI_3WIRE_HIZ | SPI_CPHA | SPI_CPOL |
>    392                              SPI_CS_HIGH;
>    393          if (!spi_gpio->mosi) {
>    394                  /* HW configuration without MOSI pin
>    395                   *
>    396                   * No setting SPI_MASTER_NO_RX here - if there is only
>    397                   * a MOSI pin connected the host can still do RX by
>    398                   * changing the direction of the line.
>    399                   */
>    400                  master->flags = SPI_MASTER_NO_TX;
>    401          }
>    402
>    403          master->bus_num = pdev->id;
>    404          master->setup = spi_gpio_setup;
>    405          master->cleanup = spi_gpio_cleanup;
>    406
>    407          bb = &spi_gpio->bitbang;
>    408          bb->master = master;
>    409          /*
>    410           * There is some additional business, apart from driving the CS GPIO
>    411           * line, that we need to do on selection. This makes the local
>    412           * callback for chipselect always get called.
>    413           */
>    414          master->flags |= SPI_MASTER_GPIO_SS;
>    415          bb->chipselect = spi_gpio_chipselect;
>    416          bb->set_line_direction = spi_gpio_set_direction;
>    417
>    418          if (master->flags & SPI_MASTER_NO_TX) {
>    419                  bb->txrx_word[SPI_MODE_0] = spi_gpio_spec_txrx_word_mode0;
>    420                  bb->txrx_word[SPI_MODE_1] = spi_gpio_spec_txrx_word_mode1;
>    421                  bb->txrx_word[SPI_MODE_2] = spi_gpio_spec_txrx_word_mode2;
>    422                  bb->txrx_word[SPI_MODE_3] = spi_gpio_spec_txrx_word_mode3;
>    423          } else {
>    424                  bb->txrx_word[SPI_MODE_0] = spi_gpio_txrx_word_mode0;
>    425                  bb->txrx_word[SPI_MODE_1] = spi_gpio_txrx_word_mode1;
>    426                  bb->txrx_word[SPI_MODE_2] = spi_gpio_txrx_word_mode2;
>    427                  bb->txrx_word[SPI_MODE_3] = spi_gpio_txrx_word_mode3;
>    428          }
>    429          bb->setup_transfer = spi_bitbang_setup_transfer;
>    430
>    431          status = spi_bitbang_init(&spi_gpio->bitbang);
>    432          if (status)
>    433                  return status;
>    434
>    435          return devm_spi_register_master(&pdev->dev, spi_master_get(master));
>                                                             ^^^^^^^^^^^^^^^^^^^^^^
> Why are we taking a second reference here?  Where will it be released?
>

This additional reference taking came from inlining of the code from
spi_bitbang_start():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi-bitbang.c?h=v5.9-rc4#n405

and the logic behind reference taking seems to have come from this
change: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers?h=v5.9-rc4&id=702a4879ec337463f858c8ab467482cce260bf18

Release should happen once devres action registered by

>    370          status = devm_add_action_or_reset(&pdev->dev, spi_gpio_put, master);

is executed upon devres "unwinding", see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers?h=v5.9-rc4&id=8b797490b4db09492acda4b4a4a4355d2311a614

At least I think that what my thinking was for making that change.