| 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
| ||
|
Message-ID: <p3lt3psoxenwlvxu6yjpjk4yskrplagj54vk4vxkg6biudghus@go6hpoakvfwh>
Date: Tue, 19 Nov 2024 02:20:48 +0000
From: Shinichiro Kawasaki <shinichiro.kawasaki@....com>
To: "Daniel Walker (danielwa)" <danielwa@...co.com>
CC: Hans de Goede <hdegoede@...hat.com>, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com>, Ilpo J�rvinen
<ilpo.jarvinen@...ux.intel.com>, Klara Modin <klarasmodin@...il.com>, Greg
Kroah-Hartman <gregkh@...uxfoundation.org>, Danil Rybakov
<danilrybakov249@...il.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "xe-linux-external(mailer list)"
<xe-linux-external@...co.com>
Subject: Re: platform/x86: p2sb: Allow p2sb_bar() calls during PCI device
probe
On Nov 18, 2024 / 17:15, Daniel Walker (danielwa) wrote:
> On Mon, Nov 18, 2024 at 05:00:52PM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 18-Nov-24 4:55 PM, Andy Shevchenko wrote:
[...]
> > > Hans, there will be no need to fix anything if they implement correct access
> > > to the GPIO, i.e. via driver and board code with GPIO lookup tables.
> >
> > Agreed, still I'm not sure how I feel about us hiding the previously unhidden P2SB.
> >
> > OTOH I guess it may have only been unhidden in the BIOS to make the hack they
> > are using possible in the first place.
>
> From a flexibility POV I would suggest if you can not hide it if it's not already
> hidden by the BIOS that would be better since some company may have a good
> reason to make a custom driver or to export the pci device to userspace thru
> UIO. The current situation is you can't make a custom driver if p2sb is enable
> with this additional patch even if you unhide the device inside the BIOS.
>
> In our case it seems like we could use the already existing solution with
> pinctrl, but others may not be able to do that or may not want to for different
> reasons.
I don't have strong opinion about the choice, but I wonder how the p2sb code
will be if we keep the unhidden P2SB. I created a trial patch below. If the
device is not hidden, it does not call pci_scan_single_device() and
pci_stop_and_remove_bus_device(). Instead, it calls pci_get_slot() and
pci_dev_put(). I don't have the environment which unhides P2SB. Daniel, if you
have time to afford, please try it out.
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 31f38309b389..dec3d43ce929 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -79,29 +79,49 @@ static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem)
mem->desc = bar0->desc;
}
-static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn)
+static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn,
+ bool hidden)
{
struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)];
- struct pci_dev *pdev;
+ struct pci_dev *pdev = NULL;
+
+ if (!hidden)
+ pdev = pci_get_slot(bus, devfn);
+
+ pr_info("%s: hidden=%d pci_get_slot=%px\n", __func__, hidden, pdev);
+
+ if (!pdev) {
+ hidden = true;
+ pdev = pci_scan_single_device(bus, devfn);
+ }
- pdev = pci_scan_single_device(bus, devfn);
if (!pdev)
return;
p2sb_read_bar0(pdev, &cache->res);
+
+ pr_info("%s: devfn=%x.%x\n", __func__,
+ PCI_SLOT(devfn), PCI_FUNC(devfn));
+ pr_info("%s: %llx-%llx: %lx\n", __func__,
+ cache->res.start, cache->res.end, cache->res.flags);
+
cache->bus_dev_id = bus->dev.id;
- pci_stop_and_remove_bus_device(pdev);
+ if (hidden)
+ pci_stop_and_remove_bus_device(pdev);
+ else
+ pci_dev_put(pdev);
}
-static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
+static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn,
+ bool hidden)
{
/* Scan the P2SB device and cache its BAR0 */
- p2sb_scan_and_cache_devfn(bus, devfn);
+ p2sb_scan_and_cache_devfn(bus, devfn, hidden);
/* On Goldmont p2sb_bar() also gets called for the SPI controller */
if (devfn == P2SB_DEVFN_GOLDMONT)
- p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
+ p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT, hidden);
if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
return -ENOENT;
@@ -127,9 +147,12 @@ static int p2sb_cache_resources(void)
unsigned int devfn_p2sb;
u32 value = P2SBC_HIDE;
struct pci_bus *bus;
+ bool hidden;
u16 class;
int ret;
+ pr_info("%s\n", __func__);
+
/* Get devfn for P2SB device itself */
p2sb_get_devfn(&devfn_p2sb);
@@ -157,13 +180,15 @@ static int p2sb_cache_resources(void)
* Unhide the P2SB device here, if needed.
*/
pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
- if (value & P2SBC_HIDE)
+ hidden = value & P2SBC_HIDE;
+ pr_info("%s: P2SBC_HIDE=%u\n", __func__, value & hidden);
+ if (hidden)
pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
- ret = p2sb_scan_and_cache(bus, devfn_p2sb);
+ ret = p2sb_scan_and_cache(bus, devfn_p2sb, hidden);
/* Hide the P2SB device, if it was hidden */
- if (value & P2SBC_HIDE)
+ if (hidden)
pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
pci_unlock_rescan_remove();
@@ -189,6 +214,8 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
{
struct p2sb_res_cache *cache;
+ pr_info("%s\n", __func__);
+
bus = p2sb_get_bus(bus);
if (!bus)
return -ENODEV;
@@ -204,6 +231,12 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
return -ENOENT;
memcpy(mem, &cache->res, sizeof(*mem));
+
+ pr_info("%s: devfn=%x.%x\n", __func__,
+ PCI_SLOT(devfn), PCI_FUNC(devfn));
+ pr_info("%s: %llx-%llx: %lx\n", __func__,
+ cache->res.start, cache->res.end, cache->res.flags);
+
return 0;
}
EXPORT_SYMBOL_GPL(p2sb_bar);
Powered by blists - more mailing lists