archive mirror
 help / color / mirror / Atom feed
From: Mikel Rychliski <>
To: Christoph Hellwig <>
	"Alex Deucher" <>,
	"Christian König" <>,
	"David (ChunMing) Zhou" <>,
	"Bjorn Helgaas" <>,
	"Matthew Garrett" <>,
	"Ben Skeggs" <>
Subject: Re: [PATCH RESEND v2 2/2] PCI: Use ioremap(), not phys_to_virt() for platform ROM
Date: Tue, 17 Mar 2020 21:34:33 -0400	[thread overview]
Message-ID: <49518530.5kixuQOrMm@glidewell> (raw)
In-Reply-To: <>

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 

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);

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

	return true;
	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.


  reply	other threads:[~2020-03-18  1:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-13 22:22 [PATCH RESEND v2 0/2] Fix loading of platform ROM from 32-bit EFI Mikel Rychliski
2020-03-13 22:22 ` [PATCH RESEND v2 1/2] drm/radeon: Stop directly referencing iomem Mikel Rychliski
2020-03-13 22:22 ` [PATCH RESEND v2 2/2] PCI: Use ioremap(), not phys_to_virt() for platform ROM Mikel Rychliski
2020-03-17 14:47   ` Christoph Hellwig
2020-03-18  1:34     ` Mikel Rychliski [this message]
2020-03-18  6:51       ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49518530.5kixuQOrMm@glidewell \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).