Re: [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI MMIO writes are not flushed before udelay() or hardware dependent operations, and wmb() is misused to flush PCI writes.
- [High] The miscinit0 field from the BIOS configuration table is ignored and incorrectly hardcoded to 0.
--

--- Patch [3]: [PATCH 3/5] fbdev: tdfxfb: Manually boot unbooted cards ---
commit 2a277f3197dc755d9c642a91cfd45c9bd1ee77f8
Author: Daniel Palmer <daniel@0x0f.com>

fbdev: tdfxfb: Manually boot unbooted cards
    
If the card is detected as being unbooted it isn't too difficult
to use the config table in its BIOS to fire it up so do it.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> index 1d8bffe1e9fdf..4c1d847c89c9d 100644
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
[ ... ]
> @@ -414,19 +415,62 @@ static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
>   * Try to work out if the card was booted or not, just checks
>   * if the register reported memory amount matches what the BIOS
>   * reports for now.
> + *
> + * If we have a BIOS config table attempt to manually boot the
> + * card if needed.
>   */
>  static int tdfxfb_hw_init(struct fb_info *info, struct pci_dev *pdev)
>  {
[ ... ]
> +	/* memory clock, and the graphics clock if the card wants one */
> +	tdfx_outl(par, PLLCTRL1, mempll);
> +	if (gfxpll)
> +		tdfx_outl(par, PLLCTRL2, gfxpll);
> +	/* PLL lock */
> +	udelay(100);

[Severity: High]
Does this delay reliably wait for the PLL lock?

tdfx_outl uses writel which performs a posted write on PCI buses, so the delay
might start before the PCI bridge actually delivers the write to the device.

Would adding a dummy read to a device register (e.g. tdfx_inl) before the
udelay ensure the posted writes are flushed?

> +
> +	tdfx_outl(par, MISCINIT1, miscinit1);
> +	tdfx_outl(par, DRAMINIT0, draminit0);
> +	tdfx_outl(par, DRAMINIT1, draminit1);
> +
> +	/* Make sure the DRAM config is applied before continuing */
> +	wmb();

[Severity: High]
Does wmb actually flush the PCI posted writes?

A CPU memory barrier typically only flushes CPU store buffers, not PCI
posted write buffers, so the DRAM configuration might still be pending
when the subsequent wake-up commands are sent.

> +
> +	/* SDRAM/SGRAM wake up: load the mode register */
> +	tdfx_outl(par, DRAMDATA, dram_mode);
> +	tdfx_outl(par, DRAMCOMMAND, 0x10d);
> +
> +	tdfx_outl(par, LFBMEMORYCONFIG, 0x00001fff);
> +	tdfx_outl(par, MISCINIT0, 0);

[Severity: High]
Is this hardcoded 0 intentional?

The rest of the fields (like miscinit1) are populated from the BIOS
configuration table. It appears le32_to_cpu(cfg.miscinit0) is completely
ignored, which could overwrite essential hardware configurations managed
by MISCINIT0 with 0.

> +
> +	/* Make sure the remaining config is applied */
> +	wmb();
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182640.2808572-1-daniel@0x0f.com?part=3