Re: [PATCH v3 3/3] scsi: libsas: Handle errors in sas_ex_discover_expander()

[John Garry <john.g.garry@oracle.com>]

On 12/08/2026 20:48, Eshaan Deshmukh wrote:
> The function sas_ex_discover_expander() does not account for the potential
> failure of sas_port_alloc() for phy->port. It also calls BUG_ON in case
> sas_port_add fails for phy->port. Add a check for phy->port after
> sas_port_alloc() where
> 
> 
> The function sas_ex_discover_expander() does not account for the
> potential failure of sas_port_alloc() for phy->port. It also calls
> BUG_ON in case sas_port_add fails for phy->port. Add a check for
> phy->port after sas_port_alloc() where if it is NULL, it cleans up the
> child allocated device and returns NULL. Add another check for
> sas_port_add() where if it returns an error code it frees phy->port,
> sets it to NULL, cleans up the child allocated device, and returns NULL.
> 
> Signed-off-by: Eshaan Deshmukh <eshaan2031@icloud.com>
> ---
>    drivers/scsi/libsas/sas_expander.c | 41 ++++++++++++++++++++----------
>    1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index ab6afbad3..a83493f57 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -911,7 +911,6 @@ static struct domain_device *sas_ex_discover_expander(
>    	struct sas_rphy *rphy;
>    	struct sas_expander_device *edev;
>    	struct asd_sas_port *port;
> -	int res;
>    
>    	if (phy->routing_attr == DIRECT_ROUTING) {
>    		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
> @@ -925,9 +924,13 @@ static struct domain_device *sas_ex_discover_expander(
>    		return NULL;
>    
>    	phy->port = sas_port_alloc(&parent->rphy->dev, phy_id);
> -	/* FIXME: better error handling */
> -	BUG_ON(sas_port_add(phy->port) != 0);
> +	if (!phy->port) {

no need for {}, however please see comment further down.

> +		goto out_put_device;
> +	}
>    
> +	if (sas_port_add(phy->port)) {
> +		goto out_free_port;

ditto

> +	}
>    
>    	switch (phy->attached_dev_type) {
>    	case SAS_EDGE_EXPANDER_DEVICE:
> @@ -966,19 +969,29 @@ static struct domain_device *sas_ex_discover_expander(
>    	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
>    	spin_unlock_irq(&parent->port->dev_list_lock);
>    
> -	res = sas_discover_expander(child);
> -	if (res) {
> -		sas_rphy_delete(rphy);
> -		spin_lock_irq(&parent->port->dev_list_lock);
> -		list_del(&child->dev_list_node);
> -		spin_unlock_irq(&parent->port->dev_list_lock);
> -		sas_put_device(child);
> -		sas_port_delete(phy->port);

I missed that this was a sas_port_delete() call and not sas_port_free(), 
so not common to other error paths.

So it does not look like we can make much error paths common, so maybe 
it was better as previously.

> -		phy->port = NULL;
> -		return NULL;
> -	}
> +	if (sas_discover_expander(child))
> +		goto out_delete_rphy;
> +
>    	list_add_tail(&child->siblings, &parent->ex_dev.children);
>    	return child;
> +
> +out_free_port:
> +	sas_port_free(phy->port);
> +	phy->port = NULL;
> +
> +out_put_device:
> +	sas_put_device(child);
> +	return NULL;
> +
> +out_delete_rphy:
> +	sas_rphy_delete(rphy);
> +	spin_lock_irq(&parent->port->dev_list_lock);
> +	list_del(&child->dev_list_node);
> +	spin_unlock_irq(&parent->port->dev_list_lock);
> +	sas_put_device(child);
> +	sas_port_delete(phy->port);
> +	phy->port = NULL;
> +	return NULL;
>    }
>    
>    static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
> -- 
> 2.55.0
>