* [bug report] spi: add support for microchip fpga spi controllers
@ 2022-06-15 8:09 Dan Carpenter
2022-06-15 8:33 ` Conor.Dooley
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-06-15 8:09 UTC (permalink / raw)
To: conor.dooley; +Cc: linux-spi
Hello Conor Dooley,
The patch 9ac8d17694b6: "spi: add support for microchip fpga spi
controllers" from Jun 7, 2022, leads to the following Smatch static
checker warning:
drivers/spi/spi-microchip-core.c:557 mchp_corespi_probe()
warn: passing zero to 'PTR_ERR'
drivers/spi/spi-microchip-core.c
506 static int mchp_corespi_probe(struct platform_device *pdev)
507 {
508 struct spi_master *master;
509 struct mchp_corespi *spi;
510 struct resource *res;
511 u32 num_cs;
512 int ret = 0;
513
514 master = spi_alloc_master(&pdev->dev, sizeof(*spi));
515 if (!master)
516 return dev_err_probe(&pdev->dev, -ENOMEM,
517 "unable to allocate master for SPI controller\n");
518
519 platform_set_drvdata(pdev, master);
520
521 if (of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs))
522 num_cs = MAX_CS;
523
524 master->num_chipselect = num_cs;
525 master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
526 master->setup = mchp_corespi_setup;
527 master->bits_per_word_mask = SPI_BPW_MASK(8);
528 master->transfer_one = mchp_corespi_transfer_one;
529 master->prepare_message = mchp_corespi_prepare_message;
530 master->set_cs = mchp_corespi_set_cs;
531 master->dev.of_node = pdev->dev.of_node;
532
533 spi = spi_master_get_devdata(master);
534
535 spi->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
536 if (IS_ERR(spi->regs)) {
537 ret = PTR_ERR(spi->regs);
538 goto error_release_master;
539 }
540
541 spi->irq = platform_get_irq(pdev, 0);
542 if (spi->irq <= 0) {
543 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
544 ret = spi->irq;
545 goto error_release_master;
546 }
547
548 ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
549 IRQF_SHARED, dev_name(&pdev->dev), master);
550 if (ret) {
551 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
552 goto error_release_master;
553 }
554
555 spi->clk = devm_clk_get(&pdev->dev, NULL);
556 if (!spi->clk || IS_ERR(spi->clk)) {
^^^^^^^^
NULL
--> 557 ret = PTR_ERR(spi->clk);
ret is 0/success.
Normally when functions like this return NULL, you're supposed to just
accept the NULL and add tests for it to avoid NULL related bugs. In
this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
to a divide by zero bug. So it's not clear which way to go on this?
Fix the error code or add more checks for NULL?
558 dev_err(&pdev->dev, "could not get clk: %d\n", ret);
559 goto error_release_master;
560 }
561
562 ret = clk_prepare_enable(spi->clk);
563 if (ret) {
564 dev_err(&pdev->dev, "failed to enable clock\n");
565 goto error_release_master;
566 }
567
568 mchp_corespi_init(master, spi);
569
570 ret = devm_spi_register_master(&pdev->dev, master);
571 if (ret) {
572 dev_err(&pdev->dev,
573 "unable to register master for SPI controller\n");
574 goto error_release_hardware;
575 }
576
577 dev_info(&pdev->dev, "Registered SPI controller %d\n", master->bus_num);
578
579 return 0;
580
581 error_release_hardware:
582 mchp_corespi_disable(spi);
583 clk_disable_unprepare(spi->clk);
584 error_release_master:
585 spi_master_put(master);
586
587 return ret;
588 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] spi: add support for microchip fpga spi controllers
2022-06-15 8:09 [bug report] spi: add support for microchip fpga spi controllers Dan Carpenter
@ 2022-06-15 8:33 ` Conor.Dooley
2022-06-15 8:40 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Conor.Dooley @ 2022-06-15 8:33 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-spi
On 15/06/2022 09:09, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hello Conor Dooley,
>
> The patch 9ac8d17694b6: "spi: add support for microchip fpga spi
> controllers" from Jun 7, 2022, leads to the following Smatch static
> checker warning:
>
> drivers/spi/spi-microchip-core.c:557 mchp_corespi_probe()
> warn: passing zero to 'PTR_ERR'
>
> drivers/spi/spi-microchip-core.c
> 506 static int mchp_corespi_probe(struct platform_device *pdev)
> 507 {
> 508 struct spi_master *master;
> 509 struct mchp_corespi *spi;
> 510 struct resource *res;
> 511 u32 num_cs;
> 512 int ret = 0;
> 513
> 514 master = spi_alloc_master(&pdev->dev, sizeof(*spi));
> 515 if (!master)
> 516 return dev_err_probe(&pdev->dev, -ENOMEM,
> 517 "unable to allocate master for SPI controller\n");
> 518
> 519 platform_set_drvdata(pdev, master);
> 520
> 521 if (of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs))
> 522 num_cs = MAX_CS;
> 523
> 524 master->num_chipselect = num_cs;
> 525 master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
> 526 master->setup = mchp_corespi_setup;
> 527 master->bits_per_word_mask = SPI_BPW_MASK(8);
> 528 master->transfer_one = mchp_corespi_transfer_one;
> 529 master->prepare_message = mchp_corespi_prepare_message;
> 530 master->set_cs = mchp_corespi_set_cs;
> 531 master->dev.of_node = pdev->dev.of_node;
> 532
> 533 spi = spi_master_get_devdata(master);
> 534
> 535 spi->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> 536 if (IS_ERR(spi->regs)) {
> 537 ret = PTR_ERR(spi->regs);
> 538 goto error_release_master;
> 539 }
> 540
> 541 spi->irq = platform_get_irq(pdev, 0);
> 542 if (spi->irq <= 0) {
> 543 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
> 544 ret = spi->irq;
> 545 goto error_release_master;
> 546 }
> 547
> 548 ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
> 549 IRQF_SHARED, dev_name(&pdev->dev), master);
> 550 if (ret) {
> 551 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
> 552 goto error_release_master;
> 553 }
> 554
> 555 spi->clk = devm_clk_get(&pdev->dev, NULL);
> 556 if (!spi->clk || IS_ERR(spi->clk)) {
> ^^^^^^^^
> NULL
>
> --> 557 ret = PTR_ERR(spi->clk);
>
> ret is 0/success.
>
> Normally when functions like this return NULL, you're supposed to just
> accept the NULL and add tests for it to avoid NULL related bugs. In
> this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
> to a divide by zero bug. So it's not clear which way to go on this?
> Fix the error code or add more checks for NULL?
Am I being dumb here, or should the null check just be removed like
every other driver? As in, devm_clk_get will only return a valid
clk or an IS_ERR() condition.
The correct solution seems to me to be remove the !spi->clk check?
Thanks for the report!
Conor.
>
> 558 dev_err(&pdev->dev, "could not get clk: %d\n", ret);
> 559 goto error_release_master;
> 560 }
> 561
> 562 ret = clk_prepare_enable(spi->clk);
> 563 if (ret) {
> 564 dev_err(&pdev->dev, "failed to enable clock\n");
> 565 goto error_release_master;
> 566 }
> 567
> 568 mchp_corespi_init(master, spi);
> 569
> 570 ret = devm_spi_register_master(&pdev->dev, master);
> 571 if (ret) {
> 572 dev_err(&pdev->dev,
> 573 "unable to register master for SPI controller\n");
> 574 goto error_release_hardware;
> 575 }
> 576
> 577 dev_info(&pdev->dev, "Registered SPI controller %d\n", master->bus_num);
> 578
> 579 return 0;
> 580
> 581 error_release_hardware:
> 582 mchp_corespi_disable(spi);
> 583 clk_disable_unprepare(spi->clk);
> 584 error_release_master:
> 585 spi_master_put(master);
> 586
> 587 return ret;
> 588 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] spi: add support for microchip fpga spi controllers
2022-06-15 8:33 ` Conor.Dooley
@ 2022-06-15 8:40 ` Dan Carpenter
2022-06-15 8:58 ` Conor.Dooley
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-06-15 8:40 UTC (permalink / raw)
To: Conor.Dooley; +Cc: linux-spi
On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@microchip.com wrote:
> > 541 spi->irq = platform_get_irq(pdev, 0);
> > 542 if (spi->irq <= 0) {
> > 543 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
> > 544 ret = spi->irq;
> > 545 goto error_release_master;
> > 546 }
> > 547
> > 548 ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
> > 549 IRQF_SHARED, dev_name(&pdev->dev), master);
> > 550 if (ret) {
> > 551 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
> > 552 goto error_release_master;
> > 553 }
> > 554
> > 555 spi->clk = devm_clk_get(&pdev->dev, NULL);
> > 556 if (!spi->clk || IS_ERR(spi->clk)) {
> > ^^^^^^^^
> > NULL
> >
> > --> 557 ret = PTR_ERR(spi->clk);
> >
> > ret is 0/success.
> >
> > Normally when functions like this return NULL, you're supposed to just
> > accept the NULL and add tests for it to avoid NULL related bugs. In
> > this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
> > to a divide by zero bug. So it's not clear which way to go on this?
> > Fix the error code or add more checks for NULL?
>
> Am I being dumb here, or should the null check just be removed like
> every other driver? As in, devm_clk_get will only return a valid
> clk or an IS_ERR() condition.
It can return NULL if CONFIG_HAVE_CLK is disabled. I don't know the
hardware or if that CONFIG_ is essential for booting.
>
> The correct solution seems to me to be remove the !spi->clk check?
That's the normal solution, yes. But if you do that, then please add a
check to prevent the divide by zero:
`grep -w clk drivers/spi/spi-microchip-core.c`
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] spi: add support for microchip fpga spi controllers
2022-06-15 8:40 ` Dan Carpenter
@ 2022-06-15 8:58 ` Conor.Dooley
2022-06-15 9:16 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Conor.Dooley @ 2022-06-15 8:58 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-spi
On 15/06/2022 09:40, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@microchip.com wrote:
>>> 541 spi->irq = platform_get_irq(pdev, 0);
>>> 542 if (spi->irq <= 0) {
>>> 543 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
>>> 544 ret = spi->irq;
>>> 545 goto error_release_master;
>>> 546 }
>>> 547
>>> 548 ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
>>> 549 IRQF_SHARED, dev_name(&pdev->dev), master);
>>> 550 if (ret) {
>>> 551 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
>>> 552 goto error_release_master;
>>> 553 }
>>> 554
>>> 555 spi->clk = devm_clk_get(&pdev->dev, NULL);
>>> 556 if (!spi->clk || IS_ERR(spi->clk)) {
>>> ^^^^^^^^
>>> NULL
>>>
>>> --> 557 ret = PTR_ERR(spi->clk);
>>>
>>> ret is 0/success.
>>>
>>> Normally when functions like this return NULL, you're supposed to just
>>> accept the NULL and add tests for it to avoid NULL related bugs. In
>>> this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
>>> to a divide by zero bug. So it's not clear which way to go on this?
>>> Fix the error code or add more checks for NULL?
>>
>> Am I being dumb here, or should the null check just be removed like
>> every other driver? As in, devm_clk_get will only return a valid
>> clk or an IS_ERR() condition.
>
> It can return NULL if CONFIG_HAVE_CLK is disabled. I don't know the
> hardware or if that CONFIG_ is essential for booting.
Ehh I guess it is /possible/ that CONFIG_HAVE_CLK could be off
if someone is accessing the FPGA from another device.
In that case, neither option really particularly appeals to me.
Just return -ENODEV I guess?
>
>>
>> The correct solution seems to me to be remove the !spi->clk check?
>
> That's the normal solution, yes. But if you do that, then please add a
> check to prevent the divide by zero:
> `grep -w clk drivers/spi/spi-microchip-core.c`
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] spi: add support for microchip fpga spi controllers
2022-06-15 8:58 ` Conor.Dooley
@ 2022-06-15 9:16 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2022-06-15 9:16 UTC (permalink / raw)
To: Conor.Dooley; +Cc: linux-spi
On Wed, Jun 15, 2022 at 08:58:53AM +0000, Conor.Dooley@microchip.com wrote:
> On 15/06/2022 09:40, Dan Carpenter wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@microchip.com wrote:
> >>> 541 spi->irq = platform_get_irq(pdev, 0);
> >>> 542 if (spi->irq <= 0) {
> >>> 543 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
> >>> 544 ret = spi->irq;
> >>> 545 goto error_release_master;
> >>> 546 }
> >>> 547
> >>> 548 ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
> >>> 549 IRQF_SHARED, dev_name(&pdev->dev), master);
> >>> 550 if (ret) {
> >>> 551 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
> >>> 552 goto error_release_master;
> >>> 553 }
> >>> 554
> >>> 555 spi->clk = devm_clk_get(&pdev->dev, NULL);
> >>> 556 if (!spi->clk || IS_ERR(spi->clk)) {
> >>> ^^^^^^^^
> >>> NULL
> >>>
> >>> --> 557 ret = PTR_ERR(spi->clk);
> >>>
> >>> ret is 0/success.
> >>>
> >>> Normally when functions like this return NULL, you're supposed to just
> >>> accept the NULL and add tests for it to avoid NULL related bugs. In
> >>> this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
> >>> to a divide by zero bug. So it's not clear which way to go on this?
> >>> Fix the error code or add more checks for NULL?
> >>
> >> Am I being dumb here, or should the null check just be removed like
> >> every other driver? As in, devm_clk_get will only return a valid
> >> clk or an IS_ERR() condition.
> >
> > It can return NULL if CONFIG_HAVE_CLK is disabled. I don't know the
> > hardware or if that CONFIG_ is essential for booting.
>
> Ehh I guess it is /possible/ that CONFIG_HAVE_CLK could be off
> if someone is accessing the FPGA from another device.
> In that case, neither option really particularly appeals to me.
> Just return -ENODEV I guess?
>
To be honest, I always prefer just accepting the NULL check and adding
the checks but also philosophical debates are kind of a waste of time.
Do whatever is easiest. :)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-15 9:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-15 8:09 [bug report] spi: add support for microchip fpga spi controllers Dan Carpenter
2022-06-15 8:33 ` Conor.Dooley
2022-06-15 8:40 ` Dan Carpenter
2022-06-15 8:58 ` Conor.Dooley
2022-06-15 9:16 ` Dan Carpenter
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).