linux-kernel - Re: [PATCH v3 1/1] regmap: Synchronize cache for the page selector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a193bcb5-1b35-48ba-801b-925ab2f92d6f@samsung.com>
Date: Fri, 17 Jan 2025 14:57:52 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Mark Brown
	<broonie@...nel.org>, linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
	<rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Dmitry Baryshkov
	<dmitry.baryshkov@...aro.org>, DRI mailing list
	<dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v3 1/1] regmap: Synchronize cache for the page selector

Dear All,

On 16.01.2025 13:42, Andy Shevchenko wrote:
> If the selector register is represented in each page, its value
> in accordance to the debugfs is stale because it gets synchronized
> only after the real page switch happens. Synchronize cache for
> the page selector.
>
> Before (offset followed by hexdump, the first byte is selector):
>
>      // Real registers
>      18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>      ...
>      // Virtual (per port)
>      40: 05 ff 00 00 e0 e0 00 00 00 00 00 1f
>      50: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>      60: 01 ff 00 00 ff ff 00 00 00 00 00 00
>      70: 02 ff 00 00 cf f3 00 00 00 00 00 0c
>      80: 03 ff 00 00 00 00 00 00 00 00 00 ff
>      90: 04 ff 00 00 ff 0f 00 00 f0 00 00 00
>
> After:
>
>      // Real registers
>      18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>      ...
>      // Virtual (per port)
>      40: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>      50: 01 ff 00 00 e0 e0 00 00 00 00 00 1f
>      60: 02 ff 00 00 ff ff 00 00 00 00 00 00
>      70: 03 ff 00 00 cf f3 00 00 00 00 00 0c
>      80: 04 ff 00 00 00 00 00 00 00 00 00 ff
>      90: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>
> Fixes: 6863ca622759 ("regmap: Add support for register indirect addressing.")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

This patch landed in linux-next some time ago as commit 1fd60ed1700c 
("regmap: Synchronize cache for the page selector"). Today I've noticed 
that it causes a regression for Lontium LT9611UXC HDMI bridge driver. 
With today's linux-next I got the following messages on QCom RB5 board:

# dmesg | grep  lt9611uxc
[   13.737346] lt9611uxc 5-002b: LT9611 revision: 0x00.00.00
[   13.804190] lt9611uxc 5-002b: LT9611 version: 0x00
[   13.870564] lt9611uxc 5-002b: FW version 0, enforcing firmware update
[   13.877437] lt9611uxc 5-002b: Direct firmware load for 
lt9611uxc_fw.bin failed with error -2
[   13.887517] lt9611uxc 5-002b: probe with driver lt9611uxc failed with 
error -2

after reverting the $subject patch, the driver probes fine on that board.

I'm not sure if this is really a bug caused by this change or simply the 
driver already was aligned to old regmap behavior. Dmitry, could you 
check the regamp usage and review the changes introduced by this patch? 
Let me know if there is anything to check on the real hardware to help 
resolving this issue.

> ---
> v3: rebased on the top of latest vanilla
>   drivers/base/regmap/regmap.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index f2843f814675..197c79b66828 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1557,24 +1557,40 @@ static int _regmap_select_page(struct regmap *map, unsigned int *reg,
>   			return -EINVAL;
>   	}
>   
> -	/* It is possible to have selector register inside data window.
> -	   In that case, selector register is located on every page and
> -	   it needs no page switching, when accessed alone. */
> +	/*
> +	 * It is possible to have selector register inside data window.
> +	 * In that case, selector register is located on every page and it
> +	 * needs no page switching, when accessed alone.
> +	 *
> +	 * Nevertheless we should synchronize the cache values for it.
> +	 */
>   	if (val_num > 1 ||
>   	    range->window_start + win_offset != range->selector_reg) {
> +		unsigned int page_off = win_page * range->window_len;
> +		unsigned int sel_offset = range->selector_reg - range->window_start;
> +		unsigned int sel_register = range->range_min + page_off + sel_offset;
> +		unsigned int val = win_page << range->selector_shift;
> +		unsigned int mask = range->selector_mask;
> +
>   		/* Use separate work_buf during page switching */
>   		orig_work_buf = map->work_buf;
>   		map->work_buf = map->selector_work_buf;
>   
> -		ret = _regmap_update_bits(map, range->selector_reg,
> -					  range->selector_mask,
> -					  win_page << range->selector_shift,
> +		ret = _regmap_update_bits(map, range->selector_reg, mask, val,
>   					  &page_chg, false);
>   
>   		map->work_buf = orig_work_buf;
>   
>   		if (ret != 0)
>   			return ret;
> +
> +		/*
> +		 * If selector register has been just updated, update the respective
> +		 * virtual copy as well.
> +		 */
> +		if (page_chg &&
> +		    in_range(range->selector_reg, range->window_start, range->window_len))
> +			_regmap_update_bits(map, sel_register, mask, val, NULL, false);
>   	}
>   
>   	*reg = range->window_start + win_offset;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland