* [bug report] PCI: endpoint: Remove "core_init_notifier" flag
@ 2024-04-17 12:47 Dan Carpenter
2024-04-17 15:38 ` Niklas Cassel
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2024-04-17 12:47 UTC (permalink / raw)
To: manivannan.sadhasivam; +Cc: linux-pci
Hello Manivannan Sadhasivam,
Commit a01e7214bef9 ("PCI: endpoint: Remove "core_init_notifier"
flag") from Mar 27, 2024 (linux-next), leads to the following Smatch
static checker warning:
drivers/pci/endpoint/functions/pci-epf-test.c:784 pci_epf_test_core_init()
error: we previously assumed 'epc_features' could be null (see line 747)
drivers/pci/endpoint/functions/pci-epf-test.c
734 static int pci_epf_test_core_init(struct pci_epf *epf)
735 {
736 struct pci_epf_test *epf_test = epf_get_drvdata(epf);
737 struct pci_epf_header *header = epf->header;
738 const struct pci_epc_features *epc_features;
739 struct pci_epc *epc = epf->epc;
740 struct device *dev = &epf->dev;
741 bool linkup_notifier = false;
742 bool msix_capable = false;
743 bool msi_capable = true;
744 int ret;
745
746 epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
747 if (epc_features) {
^^^^^^^^^^^^
epc_features can be NULL
748 msix_capable = epc_features->msix_capable;
749 msi_capable = epc_features->msi_capable;
750 }
751
752 if (epf->vfunc_no <= 1) {
753 ret = pci_epc_write_header(epc, epf->func_no, epf->vfunc_no, header);
754 if (ret) {
755 dev_err(dev, "Configuration header write failed\n");
756 return ret;
757 }
758 }
759
760 ret = pci_epf_test_set_bar(epf);
761 if (ret)
762 return ret;
763
764 if (msi_capable) {
765 ret = pci_epc_set_msi(epc, epf->func_no, epf->vfunc_no,
766 epf->msi_interrupts);
767 if (ret) {
768 dev_err(dev, "MSI configuration failed\n");
769 return ret;
770 }
771 }
772
773 if (msix_capable) {
774 ret = pci_epc_set_msix(epc, epf->func_no, epf->vfunc_no,
775 epf->msix_interrupts,
776 epf_test->test_reg_bar,
777 epf_test->msix_table_offset);
778 if (ret) {
779 dev_err(dev, "MSI-X configuration failed\n");
780 return ret;
781 }
782 }
783
--> 784 linkup_notifier = epc_features->linkup_notifier;
^^^^^^^^^^^^^^
Unchecked dereference.
785 if (!linkup_notifier)
786 queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
787
788 return 0;
789 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] PCI: endpoint: Remove "core_init_notifier" flag
2024-04-17 12:47 [bug report] PCI: endpoint: Remove "core_init_notifier" flag Dan Carpenter
@ 2024-04-17 15:38 ` Niklas Cassel
2024-04-17 15:58 ` Manivannan Sadhasivam
0 siblings, 1 reply; 3+ messages in thread
From: Niklas Cassel @ 2024-04-17 15:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: manivannan.sadhasivam, linux-pci
On Wed, Apr 17, 2024 at 03:47:48PM +0300, Dan Carpenter wrote:
> Hello Manivannan Sadhasivam,
>
> Commit a01e7214bef9 ("PCI: endpoint: Remove "core_init_notifier"
> flag") from Mar 27, 2024 (linux-next), leads to the following Smatch
> static checker warning:
>
> drivers/pci/endpoint/functions/pci-epf-test.c:784 pci_epf_test_core_init()
> error: we previously assumed 'epc_features' could be null (see line 747)
>
> drivers/pci/endpoint/functions/pci-epf-test.c
> 734 static int pci_epf_test_core_init(struct pci_epf *epf)
> 735 {
> 736 struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> 737 struct pci_epf_header *header = epf->header;
> 738 const struct pci_epc_features *epc_features;
> 739 struct pci_epc *epc = epf->epc;
> 740 struct device *dev = &epf->dev;
> 741 bool linkup_notifier = false;
> 742 bool msix_capable = false;
> 743 bool msi_capable = true;
> 744 int ret;
We check pci_epc_get_features() in ->bind(), which comes before ->core_init()
so in practice, this shouldn't be able to be NULL here.
> 745
> 746 epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
> 747 if (epc_features) {
> ^^^^^^^^^^^^
> epc_features can be NULL
We should probably just chge this to:
"""
if (!epc_features)
return some_error;
msix_capable = epc_features->msix_capable;
msi_capable = epc_features->msi_capable;
"""
Such that we don't need another check further down for linkup_notifier.
Kind regards,
Niklas
>
> 748 msix_capable = epc_features->msix_capable;
> 749 msi_capable = epc_features->msi_capable;
> 750 }
> 751
> 752 if (epf->vfunc_no <= 1) {
> 753 ret = pci_epc_write_header(epc, epf->func_no, epf->vfunc_no, header);
> 754 if (ret) {
> 755 dev_err(dev, "Configuration header write failed\n");
> 756 return ret;
> 757 }
> 758 }
> 759
> 760 ret = pci_epf_test_set_bar(epf);
> 761 if (ret)
> 762 return ret;
> 763
> 764 if (msi_capable) {
> 765 ret = pci_epc_set_msi(epc, epf->func_no, epf->vfunc_no,
> 766 epf->msi_interrupts);
> 767 if (ret) {
> 768 dev_err(dev, "MSI configuration failed\n");
> 769 return ret;
> 770 }
> 771 }
> 772
> 773 if (msix_capable) {
> 774 ret = pci_epc_set_msix(epc, epf->func_no, epf->vfunc_no,
> 775 epf->msix_interrupts,
> 776 epf_test->test_reg_bar,
> 777 epf_test->msix_table_offset);
> 778 if (ret) {
> 779 dev_err(dev, "MSI-X configuration failed\n");
> 780 return ret;
> 781 }
> 782 }
> 783
> --> 784 linkup_notifier = epc_features->linkup_notifier;
> ^^^^^^^^^^^^^^
> Unchecked dereference.
>
> 785 if (!linkup_notifier)
> 786 queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
> 787
> 788 return 0;
> 789 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] PCI: endpoint: Remove "core_init_notifier" flag
2024-04-17 15:38 ` Niklas Cassel
@ 2024-04-17 15:58 ` Manivannan Sadhasivam
0 siblings, 0 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2024-04-17 15:58 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Dan Carpenter, linux-pci
On Wed, Apr 17, 2024 at 05:38:05PM +0200, Niklas Cassel wrote:
> On Wed, Apr 17, 2024 at 03:47:48PM +0300, Dan Carpenter wrote:
> > Hello Manivannan Sadhasivam,
> >
> > Commit a01e7214bef9 ("PCI: endpoint: Remove "core_init_notifier"
> > flag") from Mar 27, 2024 (linux-next), leads to the following Smatch
> > static checker warning:
> >
> > drivers/pci/endpoint/functions/pci-epf-test.c:784 pci_epf_test_core_init()
> > error: we previously assumed 'epc_features' could be null (see line 747)
> >
> > drivers/pci/endpoint/functions/pci-epf-test.c
> > 734 static int pci_epf_test_core_init(struct pci_epf *epf)
> > 735 {
> > 736 struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > 737 struct pci_epf_header *header = epf->header;
> > 738 const struct pci_epc_features *epc_features;
> > 739 struct pci_epc *epc = epf->epc;
> > 740 struct device *dev = &epf->dev;
> > 741 bool linkup_notifier = false;
> > 742 bool msix_capable = false;
> > 743 bool msi_capable = true;
> > 744 int ret;
>
> We check pci_epc_get_features() in ->bind(), which comes before ->core_init()
> so in practice, this shouldn't be able to be NULL here.
>
Right.
>
> > 745
> > 746 epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
> > 747 if (epc_features) {
> > ^^^^^^^^^^^^
> > epc_features can be NULL
>
> We should probably just chge this to:
>
> """
> if (!epc_features)
> return some_error;
>
> msix_capable = epc_features->msix_capable;
> msi_capable = epc_features->msi_capable;
> """
>
> Such that we don't need another check further down for linkup_notifier.
>
No. We should just remove this NULL check since the check is already present in
bind().
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-17 15:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 12:47 [bug report] PCI: endpoint: Remove "core_init_notifier" flag Dan Carpenter
2024-04-17 15:38 ` Niklas Cassel
2024-04-17 15:58 ` Manivannan Sadhasivam
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.