linux-kernel - Re: [PATCH] irqchip/qcom-pdc: Workaround hardware register bug on X1E80100

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] [day] [month] [year] [list]

Message-ID: <87h64xjuer.ffs@tglx>
Date: Fri, 14 Feb 2025 00:29:16 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Stephan Gerhold <stephan.gerhold@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio
 <konradybcio@...nel.org>, Rajendra Nayak <quic_rjendra@...cinc.com>,
 Maulik Shah <quic_mkshah@...cinc.com>, Srinivas Kandagatla
 <srinivas.kandagatla@...aro.org>, Abel Vesa <abel.vesa@...aro.org>, Johan
 Hovold <johan.hovold@...aro.org>, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] irqchip/qcom-pdc: Workaround hardware register bug on
 X1E80100

On Thu, Feb 13 2025 at 18:04, Stephan Gerhold wrote:
> +
> +static void _pdc_reg_write(void __iomem *base, int reg, u32 i, u32 val)

Please use two leading underscores to make this easy to
distinguish. But ideally you provide a proper function name which makes
it clear that this function operates on a given base address contrary to
pdc_reg_write() which uses pdc_base unconditionally.

> +{
> +	writel_relaxed(val, base + reg + i * sizeof(u32));
> +}
>  
>  static void pdc_reg_write(int reg, u32 i, u32 val)
>  {
> -	writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
> +	_pdc_reg_write(pdc_base, reg, i, val);
>  }
>  
>  static u32 pdc_reg_read(int reg, u32 i)
> @@ -60,6 +69,26 @@ static u32 pdc_reg_read(int reg, u32 i)
>  	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
>  }
>  
> +static void pdc_x1e_irq_enable_write(u32 bank, u32 enable)
> +{
> +	void __iomem *base = pdc_base; /* DRV2 */

Please do not use tail comments. Also what is DRV2? 

> +
> +	/*
> +	 * Workaround hardware bug in the register logic on X1E80100:
> +	 *  - For bank 0-1, writes need to be made to DRV1, bank 3 and 4.
> +	 *  - For bank 2-4, writes need to be made to DRV2, bank 0-2.
> +	 *  - Bank 5 works as expected.
> +	 */
> +	if (bank <= 1) {
> +		base = pdc_drv1;
> +		bank += 3;
> +	} else if (bank <= 4) {
> +		bank -= 2;
> +	}

This is confusing at best. You map two different base addresses:

  1) The existing pdc_base, which magically is associated to DRV2
     (whatever that means)

  2) A new magic pdc_drv1 mapping

Then you implement the workaround logic in a pretty uncomprehensible
way. I had to stare at it more than once to make sure that it matches
the comment. What about:

    /* Remap the bank access to work around the X1E hardware bug. */
    switch (bank) {
    case 0..1:
         /* Use special mapping and shift to bank 3-4 */
         base = pdc_base_x1e_quirk;
         bank += 3;
         break;
    case 2..4:
         /* Use regular mapping and shift to bank 0-2 */
         base = pdc_base;
         bank -= 2;
         break;
    case 5:
         /* No fixup required */
         base = pdc_base;
         break;
    }

which makes it obvious what this is about. Hmm?

> +	_pdc_reg_write(base, IRQ_ENABLE_BANK, bank, enable);

> @@ -324,10 +357,21 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  	if (res_size > resource_size(&res))
>  		pr_warn("%pOF: invalid reg size, please fix DT\n", node);
>  
> +	if (of_device_is_compatible(node, "qcom,x1e80100-pdc")) {
> +		pdc_drv1 = ioremap(res.start - PDC_DRV_OFFSET, IRQ_ENABLE_BANK_MAX);

What? This maps outside of the resource region. That's neither documented in
the change log nor explained here.

I assume this can't be properly fixed in the device tree for backwards
compability reasons, but leaving this undocumented is a recipe for head
scratching three month down the road.

PDC_DRV_OFFSET is not obvious either.

You really want to explain this properly at least in the change log,
i.e.:

    X1E80100 has a hardware bug related to the address decoding of write
    accesses to the IRQ_ENABLE_BANK register.

    Correct implementations access it linear from the base address:

      addr[bank] = base_addr + IRQ_ENABLE_BANK + bank * sizeof(u32);

    The X1E80100 bug requires the following address mangling:

      addr[bank0] = base_addr - 0x10000 + IRQ_ENABLE_BANK + 3 * sizeof(u32);
      addr[bank1] = base_addr - 0x10000 + IRQ_ENABLE_BANK + 4 * sizeof(u32);
      addr[bank2] = base_addr           + IRQ_ENABLE_BANK + 0 * sizeof(u32);
      addr[bank3] = base_addr           + IRQ_ENABLE_BANK + 1 * sizeof(u32);
      addr[bank4] = base_addr           + IRQ_ENABLE_BANK + 2 * sizeof(u32);
      addr[bank5] = base_addr           + IRQ_ENABLE_BANK + 5 * sizeof(u32);

    The offset (0x10000) is outside of the resource region and requires
    therefore a seperate mapping. This can't be expressed in the device
    tree for $sensible reasons.

    Mapping this region is safe because $sensible reason.

I might have oracled this out of the patch/change log incorrectly, but
you get the idea.

Thanks,

        tglx