Re: [PATCH 03/13] platform/apple-gmux: Fix MMIO interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: missing unlock on timeout in gmux_mmio_read8/32()
- [High] platform/x86: apple-gmux: uninitialized return in gmux_mmio_read8/32()
--

commit b2c93280b42eef5a3e3e707b2033f83778f7828f
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Fix MMIO interface

This patch updates the apple-gmux MMIO helpers to sleep for 1ms per poll
iteration in gmux_mmio_wait(), check for wait timeouts before performing MMIO
reads and writes, and wait before writing in gmux_mmio_write32().

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 4e8786a5e58ec..076ed7ba6af90 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -250,12 +250,14 @@ static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
>  	u8 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread8(gmux_data->iomem_base);
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;

[Severity: High]
Does this early return in gmux_mmio_read8() in
drivers/platform/x86/apple-gmux.c leave gmux_data->index_lock locked when the
second gmux_mmio_wait(gmux_data) call times out and returns 0?

The same early return while holding gmux_data->index_lock also appears in
gmux_mmio_read32() below, which will cause subsequent GMIO accesses to
deadlock on mutex_lock(&gmux_data->index_lock).

> +		val = ioread8(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;
>  }

[ ... ]

> @@ -281,12 +286,16 @@ static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
>  	u32 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread32be(gmux_data->iomem_base);
> +
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +
> +		val = ioread32be(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;
>  }

[Severity: High]
Can gmux_mmio_read32() and gmux_mmio_read8() in
drivers/platform/x86/apple-gmux.c return an uninitialized val if the initial
gmux_mmio_wait(gmux_data) call times out and returns 0?

In both functions, val is declared without an initializer and is only
assigned inside the if (gmux_mmio_wait(gmux_data)) block before being
returned after mutex_unlock(&gmux_data->index_lock). Could val be initialized
to 0 (and returned after unlocking on both timeout paths)?

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