Re: [PATCH] hisi_sas: Fix unreasonable exit processing of hisi_sas_v3_probe

[John Garry <john.garry@xxxxxxxxxx>]

On 23/01/2020 06:12, Ye Bin wrote:

subject: scsi: hisi_sas: Fix...

/s/unreasonable/broken/

> In this function, the exception return missed some processing.

This is the same as hisi_sas_probe(), so, if this is wrong, then that is 
wrong also, i.e. needs to be fixed.

> Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
> ---
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index fa05e612d85a..394e20b8f622 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -3166,8 +3166,8 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	struct Scsi_Host *shost;
>   	struct hisi_hba *hisi_hba;
>   	struct device *dev = &pdev->dev;
> -	struct asd_sas_phy **arr_phy;
> -	struct asd_sas_port **arr_port;
> +	struct asd_sas_phy **arr_phy = NULL;
> +	struct asd_sas_port **arr_port = NULL;
>   	struct sas_ha_struct *sha;
>   	int rc, phy_nr, port_nr, i;
>   
> @@ -3213,7 +3213,7 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	arr_port = devm_kcalloc(dev, port_nr, sizeof(void *), GFP_KERNEL);
>   	if (!arr_phy || !arr_port) {
>   		rc = -ENOMEM;
> -		goto err_out_ha;
> +		goto err_out_iomap;
>   	}
>   
>   	sha->sas_phy = arr_phy;
> @@ -3254,7 +3254,7 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   
>   	rc = scsi_add_host(shost, dev);
>   	if (rc)
> -		goto err_out_ha;
> +		goto err_out_iomap;
>   
>   	rc = sas_register_ha(sha);
>   	if (rc)
> @@ -3262,14 +3262,20 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   
>   	rc = hisi_hba->hw->hw_init(hisi_hba);
>   	if (rc)
> -		goto err_out_register_ha;
> +		goto err_out_hw_init;
>   
>   	scsi_scan_host(shost);
>   
>   	return 0;
>   
> +err_out_hw_init:
> +	sas_unregister_ha(sha);
>   err_out_register_ha:
>   	scsi_remove_host(shost);
> +err_out_iomap:
> +	devm_kfree(dev, arr_phy);
> +	devm_kfree(dev, arr_port);

why do you need this? Surely these memories will be freed automatically 
for probe failure.

> +	pcim_iounmap(pdev, hisi_hba->regs);

This seems to be missing from hisi_sas_v3_remove() also.

>   err_out_ha:
>   	hisi_sas_debugfs_exit(hisi_hba);
>   	scsi_host_put(shost);


Thanks,
John