Re: [PATCH v4 2/3] scsi: libsas: Handle errors in sas_set_ex_phy()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Propagating errors from sas_set_ex_phy() aborts the entire expander discovery loop, leading to unintended and complete topology loss.
--

--- Patch [2]: [PATCH v4 2/3] scsi: libsas: Handle errors in sas_set_ex_phy() ---
commit 5c1f09068df832f5cb0e690bb6d4560d1c0cf52e
Author: Eshaan Deshmukh <[email protected]>

scsi: libsas: Handle errors in sas_set_ex_phy()

This commit modifies sas_set_ex_phy() to return an error instead of
using BUG_ON() or returning void. It updates the callers of
sas_set_ex_phy() to propagate the newly returned error codes.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f1a1417b09819..ab6afbad37feb 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c

[ ... ]

> @@ -220,14 +220,15 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
>  	struct sas_rphy *rphy = dev->rphy;
>  	bool new_phy = !phy->phy;
>  	char *type;
> +	int error;
>  
>  	if (new_phy) {
>  		if (WARN_ON_ONCE(test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state)))
> -			return;
> +			return -EBUSY;
>  		phy->phy = sas_phy_alloc(&rphy->dev, phy_id);
>  
> -		/* FIXME: error_handling */
> -		BUG_ON(!phy->phy);
> +		if (!phy->phy)
> +			return -ENOMEM;
>  	}
>  
>  	switch (disc_resp->result) {

[ ... ]

> @@ -397,8 +403,8 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
>  		pr_notice("Found loopback topology, just ignore it!\n");
>  		return 0;
>  	}
> -	sas_set_ex_phy(dev, single, disc_resp);
> -	return 0;
> +
> +	return sas_set_ex_phy(dev, single, disc_resp);
>  }

[Severity: High]
Does propagating this error up abort the entire expander discovery loop?

If sas_set_ex_phy() returns -EBUSY (e.g. when SATA error recovery is active)
or -ENOMEM, sas_ex_phy_discover_helper() will pass that error back to
sas_ex_phy_discover().

sas_ex_phy_discover() iterates over all PHYs in the expander:

drivers/scsi/libsas/sas_expander.c:sas_ex_phy_discover() {
        ...
        for (i = 0; i < ex->num_phys; i++) {
                res = sas_ex_phy_discover_helper(dev, disc_req,
                                                 disc_resp, i);
                if (res)
                        goto out_err;
        }
        ...
}

Could this cause the entire expander discovery to fail and tear down the
expander if a single PHY fails to initialize, instead of just skipping the
problematic PHY as it did previously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2