* [bug report] spi: gpio: Use devm_spi_register_master()
@ 2020-09-08 7:24 Dan Carpenter
2020-09-10 22:15 ` Andrey Smirnov
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-09-08 7:24 UTC (permalink / raw)
To: andrew.smirnov, Navid Emamdoost; +Cc: linux-spi
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.
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?
436 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] spi: gpio: Use devm_spi_register_master()
2020-09-08 7:24 [bug report] spi: gpio: Use devm_spi_register_master() Dan Carpenter
@ 2020-09-10 22:15 ` Andrey Smirnov
0 siblings, 0 replies; 2+ messages in thread
From: Andrey Smirnov @ 2020-09-10 22:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Navid Emamdoost, linux-spi
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-09-10 22:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 7:24 [bug report] spi: gpio: Use devm_spi_register_master() Dan Carpenter
2020-09-10 22:15 ` Andrey Smirnov
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).