diff mbox series

[v3,net-next,1/4] ionic: extract common bits from ionic_remove

Message ID 20230717170001.30539-2-shannon.nelson@amd.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ionic: add FLR support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 3 maintainers not CCed: pabeni@redhat.com jiri@resnulli.us edumazet@google.com
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1365 this patch: 1365
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 60 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Nelson, Shannon July 17, 2023, 4:59 p.m. UTC 
Pull out a chunk of code from ionic_remove() that will
be common in teardown paths.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 .../ethernet/pensando/ionic/ionic_bus_pci.c   | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Simon Horman July 19, 2023, 5:34 p.m. UTC | #1 
On Mon, Jul 17, 2023 at 09:59:58AM -0700, Shannon Nelson wrote:
> Pull out a chunk of code from ionic_remove() that will
> be common in teardown paths.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  .../ethernet/pensando/ionic/ionic_bus_pci.c   | 25 ++++++++++---------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index ab7d217b98b3..2bc3cab3967d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -213,6 +213,13 @@ static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
>  	return ret;
>  }
>  
> +static void ionic_clear_pci(struct ionic *ionic)
> +{
> +	ionic_unmap_bars(ionic);
> +	pci_release_regions(ionic->pdev);
> +	pci_disable_device(ionic->pdev);

Hi Shannon,

is it safe to call pci_release_regions() even if a successful call to
pci_request_regions() has not been made?

> +}
> +
>  static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -249,20 +256,20 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	err = pci_request_regions(pdev, IONIC_DRV_NAME);
>  	if (err) {
>  		dev_err(dev, "Cannot request PCI regions: %d, aborting\n", err);
> -		goto err_out_pci_disable_device;
> +		goto err_out_clear_pci;
>  	}
>  
>  	pcie_print_link_status(pdev);
>  
>  	err = ionic_map_bars(ionic);
>  	if (err)
> -		goto err_out_pci_release_regions;
> +		goto err_out_clear_pci;
>  
>  	/* Configure the device */
>  	err = ionic_setup(ionic);
>  	if (err) {
>  		dev_err(dev, "Cannot setup device: %d, aborting\n", err);
> -		goto err_out_unmap_bars;
> +		goto err_out_clear_pci;
>  	}
>  	pci_set_master(pdev);
>  
> @@ -353,12 +360,8 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	ionic_reset(ionic);
>  err_out_teardown:
>  	ionic_dev_teardown(ionic);
> -err_out_unmap_bars:
> -	ionic_unmap_bars(ionic);
> -err_out_pci_release_regions:
> -	pci_release_regions(pdev);
> -err_out_pci_disable_device:
> -	pci_disable_device(pdev);
> +err_out_clear_pci:
> +	ionic_clear_pci(ionic);
>  err_out_debugfs_del_dev:
>  	ionic_debugfs_del_dev(ionic);
>  err_out_clear_drvdata:

...
Nelson, Shannon July 20, 2023, 12:04 a.m. UTC | #2 
On 7/19/23 10:34 AM, Simon Horman wrote:
> 
> On Mon, Jul 17, 2023 at 09:59:58AM -0700, Shannon Nelson wrote:
>> Pull out a chunk of code from ionic_remove() that will
>> be common in teardown paths.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   .../ethernet/pensando/ionic/ionic_bus_pci.c   | 25 ++++++++++---------
>>   1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> index ab7d217b98b3..2bc3cab3967d 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> @@ -213,6 +213,13 @@ static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
>>        return ret;
>>   }
>>
>> +static void ionic_clear_pci(struct ionic *ionic)
>> +{
>> +     ionic_unmap_bars(ionic);
>> +     pci_release_regions(ionic->pdev);
>> +     pci_disable_device(ionic->pdev);
> 
> Hi Shannon,
> 
> is it safe to call pci_release_regions() even if a successful call to
> pci_request_regions() has not been made?

It complains a bit about freeing non-existent regions, but otherwise is 
safe.  Of course if we're on that path there are other more seriously 
broken things for this device.  I figured the cleaner code is worth an 
extra log complaint in a probably never seen path.

sln

> 
>> +}
>> +
>>   static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   {
>>        struct device *dev = &pdev->dev;
>> @@ -249,20 +256,20 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>        err = pci_request_regions(pdev, IONIC_DRV_NAME);
>>        if (err) {
>>                dev_err(dev, "Cannot request PCI regions: %d, aborting\n", err);
>> -             goto err_out_pci_disable_device;
>> +             goto err_out_clear_pci;
>>        }
>>
>>        pcie_print_link_status(pdev);
>>
>>        err = ionic_map_bars(ionic);
>>        if (err)
>> -             goto err_out_pci_release_regions;
>> +             goto err_out_clear_pci;
>>
>>        /* Configure the device */
>>        err = ionic_setup(ionic);
>>        if (err) {
>>                dev_err(dev, "Cannot setup device: %d, aborting\n", err);
>> -             goto err_out_unmap_bars;
>> +             goto err_out_clear_pci;
>>        }
>>        pci_set_master(pdev);
>>
>> @@ -353,12 +360,8 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>        ionic_reset(ionic);
>>   err_out_teardown:
>>        ionic_dev_teardown(ionic);
>> -err_out_unmap_bars:
>> -     ionic_unmap_bars(ionic);
>> -err_out_pci_release_regions:
>> -     pci_release_regions(pdev);
>> -err_out_pci_disable_device:
>> -     pci_disable_device(pdev);
>> +err_out_clear_pci:
>> +     ionic_clear_pci(ionic);
>>   err_out_debugfs_del_dev:
>>        ionic_debugfs_del_dev(ionic);
>>   err_out_clear_drvdata:
> 
> ...
diff mbox series

Patch

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index ab7d217b98b3..2bc3cab3967d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -213,6 +213,13 @@  static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
 	return ret;
 }
 
+static void ionic_clear_pci(struct ionic *ionic)
+{
+	ionic_unmap_bars(ionic);
+	pci_release_regions(ionic->pdev);
+	pci_disable_device(ionic->pdev);
+}
+
 static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct device *dev = &pdev->dev;
@@ -249,20 +256,20 @@  static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	err = pci_request_regions(pdev, IONIC_DRV_NAME);
 	if (err) {
 		dev_err(dev, "Cannot request PCI regions: %d, aborting\n", err);
-		goto err_out_pci_disable_device;
+		goto err_out_clear_pci;
 	}
 
 	pcie_print_link_status(pdev);
 
 	err = ionic_map_bars(ionic);
 	if (err)
-		goto err_out_pci_release_regions;
+		goto err_out_clear_pci;
 
 	/* Configure the device */
 	err = ionic_setup(ionic);
 	if (err) {
 		dev_err(dev, "Cannot setup device: %d, aborting\n", err);
-		goto err_out_unmap_bars;
+		goto err_out_clear_pci;
 	}
 	pci_set_master(pdev);
 
@@ -353,12 +360,8 @@  static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ionic_reset(ionic);
 err_out_teardown:
 	ionic_dev_teardown(ionic);
-err_out_unmap_bars:
-	ionic_unmap_bars(ionic);
-err_out_pci_release_regions:
-	pci_release_regions(pdev);
-err_out_pci_disable_device:
-	pci_disable_device(pdev);
+err_out_clear_pci:
+	ionic_clear_pci(ionic);
 err_out_debugfs_del_dev:
 	ionic_debugfs_del_dev(ionic);
 err_out_clear_drvdata:
@@ -386,9 +389,7 @@  static void ionic_remove(struct pci_dev *pdev)
 	ionic_port_reset(ionic);
 	ionic_reset(ionic);
 	ionic_dev_teardown(ionic);
-	ionic_unmap_bars(ionic);
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
+	ionic_clear_pci(ionic);
 	ionic_debugfs_del_dev(ionic);
 	mutex_destroy(&ionic->dev_cmd_lock);
 	ionic_devlink_free(ionic);