| 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: <fbe53f25-2cce-4c1d-bace-e7976c4ba20c@redhat.com>
Date: Tue, 19 Nov 2024 19:28:16 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Shinichiro Kawasaki <shinichiro.kawasaki@....com>,
"Daniel Walker (danielwa)" <danielwa@...co.com>
Cc: 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
Hi,
On 19-Nov-24 3:20 AM, Shinichiro Kawasaki wrote:
> 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.
Thank you for looking into this.
Daniel can you give this a try? It should fix the regression you are seeing
without needing to rework your code (reworking your code to be cleaner
might still be a good idea though).
Shinichiro, can you turn this into a proper patch without all the debug
prints and maybe follow Andy's suggestion to just make this a separate
function.
Regards,
Hans
>
> 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