Nouveau - Mar 2020 - [PATCH RESEND v2 2/2] PCI: Use ioremap(), not phys_to_virt() for platform ROM

On Fri, Mar 13, 2020 at 06:22:58PM -0400, Mikel Rychliski
wrote:
>  /**
> + * pci_platform_rom - ioremap() the ROM image provided by the platform
>   * @pdev: pointer to pci device struct
>   * @size: pointer to receive size of pci window over ROM
> + *
> + * Return: kernel virtual pointer to image of ROM
> + *
> + * The caller is responsible for removing the mapping with iounmap()
>   */
>  void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t *size)
>  {
>  	if (pdev->rom && pdev->romlen) {
>  		*size = pdev->romlen;
> -		return phys_to_virt((phys_addr_t)pdev->rom);
> +		return ioremap(pdev->rom, pdev->romlen);
>  	}
What is the value of this helper over just open coding an ioremap
of pdev->rom in the callers?

Hi Christoph,

Thanks for your comments. I'm also replying here to your comments on the 
previous series.

On Tuesday, March 17, 2020 10:28:35 AM EDT Christoph Hellwig
wrote:
> Any reason drivers can't just use pci_map_rom instead? which already
> does the right thing?
Some machines don't expose the video BIOS in the PCI BAR and instead only
make
it available via EFI boot services calls. So drivers need to be able to use 
the ROM provided by EFI calls, but only if they can't find a valid one
anywhere
else.

At one point, the EFI provided ROM in pdev->rom *was* exposed via 
pci_map_rom(). However it had to be split out into a separate function so that 
drivers could have more control over which sources were preferred.

On Tuesday, March 17, 2020 10:29:13 AM EDT Christoph Hellwig
wrote:
> This and the next patch really need to be folded into the previous
> one to avoid regressions (assuming my other suggestion doesn't work
> for some reason).
Addressed in v2

On Tuesday, March 17, 2020 10:47:31 AM EDT Christoph Hellwig
wrote:
> What is the value of this helper over just open coding an ioremap
> of pdev->rom in the callers?
I think the direct access to pdev->rom you suggest makes sense, especially 
because users of the pci_platform_rom() are exposed to the fact that it just 
calls ioremap().

I decided against wrapping the iounmap() with a pci_unmap_platform_rom(), but 
I didn't apply the same consideration to the existing function.

How about something like this (with pci_platform_rom() removed)?

static bool radeon_read_platform_bios(struct radeon_device *rdev)
{
	phys_addr_t rom = rdev->pdev->rom;
	size_t romlen = rdev->pdev->romlen;
	void __iomem *bios;

	rdev->bios = NULL;

	if (!rom || romlen == 0)
		return false;

	rdev->bios = kzalloc(romlen, GFP_KERNEL);
	if (!rdev->bios)
		return false;

	bios = ioremap(rom, romlen);
	if (!bios)
		goto free_bios;

	memcpy_fromio(rdev->bios, bios, romlen);
	iounmap(bios);

	if (rdev->bios[0] != 0x55 || rdev->bios[1] != 0xaa)
		goto free_bios;

	return true;
free_bios:
	kfree(rdev->bios);
	return false;
}

If this is preferred, I'll send an updated series. I'm assuming that the
removal of pci_platform_rom() and updating of all the callers should be 
combined into this patch.

Thanks,
Mikel

On Tue, Mar 17, 2020 at 09:34:33PM -0400, Mikel Rychliski
wrote:
> I think the direct access to pdev->rom you suggest makes sense,
especially
> because users of the pci_platform_rom() are exposed to the fact that it
just
> calls ioremap().
> 
> I decided against wrapping the iounmap() with a pci_unmap_platform_rom(),
but
> I didn't apply the same consideration to the existing function.
> 
> How about something like this (with pci_platform_rom() removed)?
That looks sensible to me.