diff mbox series

[net] net: wwan: iosm: Fix error handling path in ipc_pcie_probe()

Message ID 20230408065607.1633970-1-harshit.m.mogalapalli@oracle.com
State New
Headers
Series [net] net: wwan: iosm: Fix error handling path in ipc_pcie_probe() |

Commit Message

Harshit Mogalapalli April 8, 2023, 6:56 a.m. UTC 
  Smatch reports:
	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
	warn: missing unwind goto?

When dma_set_mask fails it directly returns without disabling pci
device and freeing ipc_pcie. Fix this my calling a correct goto label

As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
it finally returns -EIO.

Renamed the goto label as name of the label before this patch is not
relevant after this patch.

Fixes: 035e3befc191 ("net: wwan: iosm: fix driver not working with INTEL_IOMMU disabled")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
This is based on static analysis, only compile tested.
---
 drivers/net/wwan/iosm/iosm_ipc_pcie.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Simon Horman April 8, 2023, 3:32 p.m. UTC | #1 
On Fri, Apr 07, 2023 at 11:56:07PM -0700, Harshit Mogalapalli wrote:
> Smatch reports:
> 	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
> 	warn: missing unwind goto?
> 
> When dma_set_mask fails it directly returns without disabling pci
> device and freeing ipc_pcie. Fix this my calling a correct goto label
> 
> As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
> it finally returns -EIO.
> 
> Renamed the goto label as name of the label before this patch is not
> relevant after this patch.

nit: I agree that it's nice to name the labels after what they unwind,
rather than where they are called from. But now both schemes
are used in this function.
> 
> Fixes: 035e3befc191 ("net: wwan: iosm: fix driver not working with INTEL_IOMMU disabled")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> This is based on static analysis, only compile tested.

I agree with your analysis.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Harshit Mogalapalli April 8, 2023, 5:42 p.m. UTC | #2 
Hi Simon,

On 08/04/23 9:02 pm, Simon Horman wrote:
> On Fri, Apr 07, 2023 at 11:56:07PM -0700, Harshit Mogalapalli wrote:
>> Smatch reports:
>> 	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
>> 	warn: missing unwind goto?
>>
>> When dma_set_mask fails it directly returns without disabling pci
>> device and freeing ipc_pcie. Fix this my calling a correct goto label
>>
>> As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
>> it finally returns -EIO.
>>
>> Renamed the goto label as name of the label before this patch is not
>> relevant after this patch.
> 
> nit: I agree that it's nice to name the labels after what they unwind,
> rather than where they are called from. But now both schemes
> are used in this function.

Thanks a lot for the review.
I agree that the naming of the label is inconsistent, should we do 
something like below?

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c 
b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 5bf5a93937c9..04517bd3325a 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
@@ -295,7 +295,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
         ret = dma_set_mask(ipc_pcie->dev, DMA_BIT_MASK(64));
         if (ret) {
                 dev_err(ipc_pcie->dev, "Could not set PCI DMA mask: 
%d", ret);
-               return ret;
+               goto set_mask_fail;
         }

         ipc_pcie_config_aspm(ipc_pcie);
@@ -323,6 +323,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
  imem_init_fail:
         ipc_pcie_resources_release(ipc_pcie);
  resources_req_fail:
+set_mask_fail:
         pci_disable_device(pci);
  pci_enable_fail:
         kfree(ipc_pcie);



-- but resources_req_fail: has nothing in its block particularly.

Thanks,
Harshit

>>
>> Fixes: 035e3befc191 ("net: wwan: iosm: fix driver not working with INTEL_IOMMU disabled")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>> This is based on static analysis, only compile tested.
> 
> I agree with your analysis.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Simon Horman April 8, 2023, 7:30 p.m. UTC | #3 
On Sat, Apr 08, 2023 at 11:12:25PM +0530, Harshit Mogalapalli wrote:
> Hi Simon,
> 
> On 08/04/23 9:02 pm, Simon Horman wrote:
> > On Fri, Apr 07, 2023 at 11:56:07PM -0700, Harshit Mogalapalli wrote:
> > > Smatch reports:
> > > 	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
> > > 	warn: missing unwind goto?
> > > 
> > > When dma_set_mask fails it directly returns without disabling pci
> > > device and freeing ipc_pcie. Fix this my calling a correct goto label
> > > 
> > > As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
> > > it finally returns -EIO.
> > > 
> > > Renamed the goto label as name of the label before this patch is not
> > > relevant after this patch.
> > 
> > nit: I agree that it's nice to name the labels after what they unwind,
> > rather than where they are called from. But now both schemes
> > are used in this function.
> 
> Thanks a lot for the review.
> I agree that the naming of the label is inconsistent, should we do something
> like below?
> 
> diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> index 5bf5a93937c9..04517bd3325a 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> @@ -295,7 +295,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
>         ret = dma_set_mask(ipc_pcie->dev, DMA_BIT_MASK(64));
>         if (ret) {
>                 dev_err(ipc_pcie->dev, "Could not set PCI DMA mask: %d",
> ret);
> -               return ret;
> +               goto set_mask_fail;
>         }
> 
>         ipc_pcie_config_aspm(ipc_pcie);
> @@ -323,6 +323,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
>  imem_init_fail:
>         ipc_pcie_resources_release(ipc_pcie);
>  resources_req_fail:
> +set_mask_fail:
>         pci_disable_device(pci);
>  pci_enable_fail:
>         kfree(ipc_pcie);
> 
> 
> 
> -- but resources_req_fail: has nothing in its block particularly.

I think this situation is common when one names the labels
after where they come from. So I'd say this is ok.

An alternative would be to rename all three of labels after what they
unwind.
Harshit Mogalapalli April 8, 2023, 7:46 p.m. UTC | #4 
Hi Simon,

On 09/04/23 1:00 am, Simon Horman wrote:
> On Sat, Apr 08, 2023 at 11:12:25PM +0530, Harshit Mogalapalli wrote:
>> Hi Simon,
>>
>> On 08/04/23 9:02 pm, Simon Horman wrote:
>>> On Fri, Apr 07, 2023 at 11:56:07PM -0700, Harshit Mogalapalli wrote:
>>>> Smatch reports:
>>>> 	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
>>>> 	warn: missing unwind goto?
>>>>
>>>> When dma_set_mask fails it directly returns without disabling pci
>>>> device and freeing ipc_pcie. Fix this my calling a correct goto label
>>>>
>>>> As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
>>>> it finally returns -EIO.
>>>>
>>>> Renamed the goto label as name of the label before this patch is not
>>>> relevant after this patch.
>>>
>>> nit: I agree that it's nice to name the labels after what they unwind,
>>> rather than where they are called from. But now both schemes
>>> are used in this function.
>>
>> Thanks a lot for the review.
>> I agree that the naming of the label is inconsistent, should we do something
>> like below?
>>
>> diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
>> b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
>> index 5bf5a93937c9..04517bd3325a 100644
>> --- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
>> +++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
>> @@ -295,7 +295,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
>>          ret = dma_set_mask(ipc_pcie->dev, DMA_BIT_MASK(64));
>>          if (ret) {
>>                  dev_err(ipc_pcie->dev, "Could not set PCI DMA mask: %d",
>> ret);
>> -               return ret;
>> +               goto set_mask_fail;
>>          }
>>
>>          ipc_pcie_config_aspm(ipc_pcie);
>> @@ -323,6 +323,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
>>   imem_init_fail:
>>          ipc_pcie_resources_release(ipc_pcie);
>>   resources_req_fail:
>> +set_mask_fail:
>>          pci_disable_device(pci);
>>   pci_enable_fail:
>>          kfree(ipc_pcie);
>>
>>
>>
>> -- but resources_req_fail: has nothing in its block particularly.
> 
> I think this situation is common when one names the labels
> after where they come from. So I'd say this is ok.
> 
Thanks I have a sent a V2 with this.

> An alternative would be to rename all three of labels after what they
> unwind.

Other functions in this file are using similar labels. So may be we 
could with above diff(V2 patch).

Regards,
Harshit
Simon Horman April 10, 2023, 8 a.m. UTC | #5 
On Sun, Apr 09, 2023 at 01:16:49AM +0530, Harshit Mogalapalli wrote:
> Hi Simon,
> 
> On 09/04/23 1:00 am, Simon Horman wrote:
> > On Sat, Apr 08, 2023 at 11:12:25PM +0530, Harshit Mogalapalli wrote:
> > > Hi Simon,
> > > 
> > > On 08/04/23 9:02 pm, Simon Horman wrote:
> > > > On Fri, Apr 07, 2023 at 11:56:07PM -0700, Harshit Mogalapalli wrote:
> > > > > Smatch reports:
> > > > > 	drivers/net/wwan/iosm/iosm_ipc_pcie.c:298 ipc_pcie_probe()
> > > > > 	warn: missing unwind goto?
> > > > > 
> > > > > When dma_set_mask fails it directly returns without disabling pci
> > > > > device and freeing ipc_pcie. Fix this my calling a correct goto label
> > > > > 
> > > > > As dma_set_mask returns either 0 or -EIO, we can use a goto label, as
> > > > > it finally returns -EIO.
> > > > > 
> > > > > Renamed the goto label as name of the label before this patch is not
> > > > > relevant after this patch.
> > > > 
> > > > nit: I agree that it's nice to name the labels after what they unwind,
> > > > rather than where they are called from. But now both schemes
> > > > are used in this function.
> > > 
> > > Thanks a lot for the review.
> > > I agree that the naming of the label is inconsistent, should we do something
> > > like below?
> > > 
> > > diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> > > b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> > > index 5bf5a93937c9..04517bd3325a 100644
> > > --- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> > > +++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
> > > @@ -295,7 +295,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
> > >          ret = dma_set_mask(ipc_pcie->dev, DMA_BIT_MASK(64));
> > >          if (ret) {
> > >                  dev_err(ipc_pcie->dev, "Could not set PCI DMA mask: %d",
> > > ret);
> > > -               return ret;
> > > +               goto set_mask_fail;
> > >          }
> > > 
> > >          ipc_pcie_config_aspm(ipc_pcie);
> > > @@ -323,6 +323,7 @@ static int ipc_pcie_probe(struct pci_dev *pci,
> > >   imem_init_fail:
> > >          ipc_pcie_resources_release(ipc_pcie);
> > >   resources_req_fail:
> > > +set_mask_fail:
> > >          pci_disable_device(pci);
> > >   pci_enable_fail:
> > >          kfree(ipc_pcie);
> > > 
> > > 
> > > 
> > > -- but resources_req_fail: has nothing in its block particularly.
> > 
> > I think this situation is common when one names the labels
> > after where they come from. So I'd say this is ok.
> > 
> Thanks I have a sent a V2 with this.
> 
> > An alternative would be to rename all three of labels after what they
> > unwind.
> 
> Other functions in this file are using similar labels. So may be we could
> with above diff(V2 patch).

Yes, I agree that makes sense.
diff mbox series

Patch

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 5bf5a93937c9..a6a6a0df1f7d 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
@@ -295,7 +295,7 @@  static int ipc_pcie_probe(struct pci_dev *pci,
 	ret = dma_set_mask(ipc_pcie->dev, DMA_BIT_MASK(64));
 	if (ret) {
 		dev_err(ipc_pcie->dev, "Could not set PCI DMA mask: %d", ret);
-		return ret;
+		goto err_disable_pci;
 	}
 
 	ipc_pcie_config_aspm(ipc_pcie);
@@ -308,7 +308,7 @@  static int ipc_pcie_probe(struct pci_dev *pci,
 	ipc_pcie->suspend = 0;
 
 	if (ipc_pcie_resources_request(ipc_pcie))
-		goto resources_req_fail;
+		goto err_disable_pci;
 
 	/* Establish the link to the imem layer. */
 	ipc_pcie->imem = ipc_imem_init(ipc_pcie, pci->device,
@@ -322,7 +322,7 @@  static int ipc_pcie_probe(struct pci_dev *pci,
 
 imem_init_fail:
 	ipc_pcie_resources_release(ipc_pcie);
-resources_req_fail:
+err_disable_pci:
 	pci_disable_device(pci);
 pci_enable_fail:
 	kfree(ipc_pcie);