Re: [PATCH 5/5] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

[Damien Le Moal <dlemoal@kernel.org>]

On 8/13/25 8:41 PM, Niklas Cassel wrote:
> While the current code is perfectly fine (because we verify that the
> device is directly attached before using attached_phy to index the
> pm8001_ha->phy array), let's use the pm80xx_get_local_phy_id() helper
> anyway, to reduce the chance that someone will copy paste this pattern to
> other parts of the driver.
> 
> Note that in this specific case, we still need to keep the check that the
> device is not behind an expander, because we do not want to clear
> attached_phy of the expander if a device behind the expander disappears
> (as that would disable all the other devices behind the expander).
> 
> However, if it is the expander itself that disappears, attached_phy will
> be cleared, just like it would for any other directly attached device.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index b3bd61827ad6..96ecc5e63f53 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -782,8 +782,11 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
>  		 * The phy array only contains local phys. Thus, we cannot clear
>  		 * phy_attached for a device behind an expander.
>  		 */
> -		if (!(parent_dev && dev_is_expander(parent_dev->dev_type)))
> -			pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
> +		if (!(parent_dev && dev_is_expander(parent_dev->dev_type))) {

This pattern/check is repeated a lot. It may be good to have a
dev_parent_is_expander() helper:

static inline bool dev_parent_is_expander(struct domain_device *dev)
{
	if (!dev->parent)
		return false;

	return dev_is_expander(dev->parent->dev_type);
}

Which will allow simplifying the the test everywhere to:

	if (dev_parent_is_expander(dev))
or
	if (!dev_parent_is_expander(dev))

No ?

> +			u32 phy_id = pm80xx_get_local_phy_id(dev);
> +
> +			pm8001_ha->phy[phy_id].phy_attached = 0;
> +		}
>  		pm8001_free_dev(pm8001_dev);
>  	} else {
>  		pm8001_dbg(pm8001_ha, DISC, "Found dev has gone.\n");


-- 
Damien Le Moal
Western Digital Research