Re: [PATCH] ata: sata_mv: init and power phy per-port in port_start

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware hangs due to unpowered PHY register access.
--

--- Patch [1]: [PATCH] ata: sata_mv: init and power phy per-port in port_start ---
commit f57a0363f1ffdddb1a743417b19db2e65b2fcb67
Author: Rosen Penev <[email protected]>

ata: sata_mv: init and power phy per-port in port_start

This commit moves PHY initialization and power management from the probe
and remove paths to the per-port start and stop paths. It also reworks
error handling in probe to rely on devres cleanup.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index d260f04e6384b..e811202a68735 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -1711,11 +1713,23 @@ static int mv_port_start(struct ata_port *ap)
>  	mv_edma_cfg(ap, 0, 0);
>  	spin_unlock_irqrestore(ap->lock, flags);
>  
> +	if (phy) {
> +		rc = phy_init(phy);
> +		if (rc)
> +			goto out_port_free_dma_mem;
> +
> +		rc = phy_power_on(phy);

[Severity: High]
Will this cause hardware hangs or external aborts on access?

It looks like we are calling mv_save_cached_regs() and mv_edma_cfg()
just before this block, while the PHY is still unpowered. 

Since those functions read and write MAC interface registers, could this
access unpowered hardware?

[ ... ]

> @@ -4144,39 +4157,12 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	/* initialize adapter */
>  	rc = mv_init_host(host);
>  	if (rc)
> -		goto err;
> +		return rc;

[Severity: High]
Does moving the PHY power on step to port start break host initialization?

When mv_platform_probe() calls mv_init_host() here, the PHY is no longer
powered. However, mv_init_host() can execute PHY register accesses:

mv_platform_probe() -> mv_init_host():
        for (port = 0; port < host->n_ports; port++)
                if (hpriv->ops->read_preamp)
                        hpriv->ops->read_preamp(hpriv, port, mmio);

Since operations like read_preamp read and write PHY registers, will this
cause a synchronous external abort during boot?

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