| Message ID | 20241114213439.6022-1-dave@stgolabs.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Fix potential bogus return value upon successful probing | expand |
On Thu, Nov 14, 2024 at 01:34:39PM -0800, Davidlohr Bueso wrote: > If cxl_pci_ras_unmask() returns non-zero, cxl_pci_probe() will end up > returning that value, instead of zero. Found by code inspeaction. /inspeaction/inspection/ Other than that, Reviewed-by: Fan Ni <fan.ni@samsung.com> Fan > > Fixes: 248529edc86 (cxl: add RAS status unmasking for CXL) > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 188412d45e0d..01092e5b3b46 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -948,7 +948,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > pci_save_state(pdev); > > - return rc; > + return 0; > } > > static const struct pci_device_id cxl_mem_pci_tbl[] = {
On Thu, Nov 14, 2024 at 01:34:39PM -0800, Davidlohr Bueso wrote: > If cxl_pci_ras_unmask() returns non-zero, cxl_pci_probe() will end up > returning that value, instead of zero. Found by code inspeaction. > I agree that the return value should be 0 if we get this far, because all rc's that need to be returned, are returned immediately. But...the set and check of 'rc' for the calls cannot lead to a 'return rc' seems like a bad pattern. Most of those are dev_dbg() messages. This last one that bit us here, is repeating the same pattern of those before it: rc = cxl_pci_ras_unmask(pdev); if (rc) dev_dbg(&pdev->dev, "No RAS reporting unmasked\n"); This can be: if (cxl_pci_ras_unmask(pdev)) dev_dbg(&pdev->dev, "No RAS reporting unmasked\n"); There are a bunch that follow this pattern in that function, mostly in the pmu_count loop. How about tidying up the rc pattern throughout the function? --Alison > Fixes: 248529edc86 (cxl: add RAS status unmasking for CXL) > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 188412d45e0d..01092e5b3b46 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -948,7 +948,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > pci_save_state(pdev); > > - return rc; > + return 0; > } > > static const struct pci_device_id cxl_mem_pci_tbl[] = { > -- > 2.47.0 > >
On Thu, 14 Nov 2024, Alison Schofield wrote:\n >This can be: > > if (cxl_pci_ras_unmask(pdev)) > dev_dbg(&pdev->dev, "No RAS reporting unmasked\n"); Yeah, it later occured to me that this was better. Will send a v2. > >There are a bunch that follow this pattern in that function, mostly in >the pmu_count loop. > >How about tidying up the rc pattern throughout the function? I think since this is an actual fix, any tidying up can come later. Thanks, Davidlohr
Davidlohr Bueso wrote: > On Thu, 14 Nov 2024, Alison Schofield wrote:\n > >This can be: > > > > if (cxl_pci_ras_unmask(pdev)) > > dev_dbg(&pdev->dev, "No RAS reporting unmasked\n"); > > Yeah, it later occured to me that this was better. Will send a v2. > > > > >There are a bunch that follow this pattern in that function, mostly in > >the pmu_count loop. > > > >How about tidying up the rc pattern throughout the function? > > I think since this is an actual fix, any tidying up can come later. Yea that can come later. Sorry if I implied differently. Ira
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 188412d45e0d..01092e5b3b46 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -948,7 +948,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) pci_save_state(pdev); - return rc; + return 0; } static const struct pci_device_id cxl_mem_pci_tbl[] = {
If cxl_pci_ras_unmask() returns non-zero, cxl_pci_probe() will end up returning that value, instead of zero. Found by code inspeaction. Fixes: 248529edc86 (cxl: add RAS status unmasking for CXL) Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)