Re: [PATCH] ata: sata_mv: enable SoC SATA LED presence indication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ata: sata_mv: LED blink mode state desynchronization and register clobbering
- [Medium] ata: sata_mv: hardcoded LED multiplexing risks board-specific pin conflicts
--

commit 30e313ac1545b898b81aab6027f1bf3c7b86b831
Author: Carlos Junior <cjuniorfox@gmail.com>

ata: sata_mv: enable SoC SATA LED presence indication

This commit enables the SoC SATA LED controller in mv_soc_enable_leds().
It sets the enable, device presence multiplexing, and activity blink bits
in the host-controller register.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 365454390d7e0..90a4ca5b5c407 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
[ ... ]
> @@ -3382,12 +3383,21 @@ static void mv6_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
>  	writel(m2, port_mmio + PHY_MODE2);
>  }
>  
> -/* TODO: use the generic LED interface to configure the SATA Presence */
> -/* & Acitivy LEDs on the board */
> +/*
> + * Enable the SoC SATA LED controller and device presence
> + * indication on the board's SATA LEDs.
> + */
>  static void mv_soc_enable_leds(struct mv_host_priv *hpriv,
>  				      void __iomem *mmio)
>  {
> -	return;
> +	void __iomem *hc_mmio;
> +
> +	hc_mmio = mv_hc_base(mmio, 0);
> +
> +	writel(SOC_LED_CTRL_ENABLE |
> +		SOC_LED_CTRL_ACT_PRESENCE |
> +		SOC_LED_CTRL_BLINK,
> +		hc_mmio + SOC_LED_CTRL);

[Severity: High]
Does this blind writel clobber existing register configurations? Without a
read-modify-write pattern, any settings previously configured by the
bootloader will be overwritten.

Additionally, this enables blink mode in hardware without updating the
software state tracker. When mv_edma_cfg() later attempts to disable
blinking for non-NCQ devices, it relies on mv_soc_led_blink_disable():

    if (!(hpriv->hp_flags & MV_HP_QUIRK_LED_BLINK_EN))
        return;

Because hpriv->hp_flags was never updated to include
MV_HP_QUIRK_LED_BLINK_EN during initialization, this check will falsely
indicate that blinking is already disabled and return early. Will this
leave non-NCQ devices permanently stuck in blink mode?

[Severity: Medium]
Does forcing SOC_LED_CTRL_ACT_PRESENCE globally for all SoC devices risk
board-specific pin conflicts?

Unconditionally enabling LED multiplexing here overrides platform-specific
pinmux routing. On boards where these physical pins are utilized for other
GPIO functions like UART or I2C, forcing the controller to drive them
could result in hardware conflicts or erratic behavior. The previous
comment indicated this should be handled through the generic LED interface
to respect dynamic Device Tree routing.

>  }
>  
>  static void mv_soc_read_preamp(struct mv_host_priv *hpriv, int idx,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925193134.114566-1-cjuniorfox@gmail.com?part=1