| 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: <537A735A.9070607@ti.com>
Date: Mon, 19 May 2014 17:10:50 -0400
From: Murali Karicheri <m-karicheri2@...com>
To: Arnd Bergmann <arnd@...db.de>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"Strashko, Grygorii" <grygorii.strashko@...com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
Jingoo Han <jg1.han@...sung.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Shilimkar, Santosh" <santosh.shilimkar@...com>,
Mohit Kumar <mohit.kumar@...com>,
Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v1 5/5] pci: keystone: add pcie driver based on designware
core driver
On 5/19/2014 8:06 AM, Arnd Bergmann wrote:
> On Friday 16 May 2014 16:26:51 Murali Karicheri wrote:
>> On 5/15/2014 2:20 PM, Arnd Bergmann wrote:
>>> On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote:
>>>>>> +#ifdef CONFIG_PCI_KEYSTONE
>>>>>> +/*
>>>>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes.
>>>>>> + */
>>>>>> +static void quirk_limit_readrequest(struct pci_dev *dev)
>>>>>> +{
>>>>>> + int readrq = pcie_get_readrq(dev);
>>>>>> +
>>>>>> + if (readrq > 256)
>>>>>> + pcie_set_readrq(dev, 256);
>>>>>> +}
>>>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);
>>>>>> +#endif /* CONFIG_PCI_KEYSTONE */
>>>>> This doesn't work: you can't just limit do this for all devices just based
>>>>> on PCI_KEYSTONE being enabled, you have to check if you are actually using
>>>>> this controller.
>>>>>
>>>>> Arnd
>>>> I assume, I need to check if PCI controller's vendor ID/ device ID
>>>> match with the keystone
>>>> PCI controller's ID and call pcie_set_readrq() for all of the slave
>>>> PCI devices and do this fixup.
>>>> Is this correct understanding? If you can point me to an example code
>>>> for this that will be
>>>> really helpful so that I can avoid re-inventing the wheel.
>>> I think it would be best to move the quirk into the keystone pci driver
>>> and compare compare the dev->driver pointer of the PCI controller device.
>>>
>>> Arnd
>> Arnd,
>>
>> I will move this quirk to keystone pci driver. So in that case, I guess
>> your original comment
>> is not applicable as this quirk gets enabled only with PCI keystone
>> driver enabled. Right?
> Of course you still have to fix the bug, not just move the code into
> the driver. Otherwise it's still broken for every machine after the keystone
> driver is enabled.
Agree. I have tried following to get this work so that the quirk gets
applied only for
keystone pci controller.
#define KS_K2HK_PCI_DEVICE_ID 0xb008
#define KS_K2E_PCI_DEVICE_ID 0xb009
#define KS_K2L_PCI_DEVICE_ID 0xb00a
static u16 root_pci_ids[] =
{KS_K2HK_PCI_DEVICE_ID, KS_K2E_PCI_DEVICE_ID, KS_K2L_PCI_DEVICE_ID,
0 };
/*
* The KeyStone PCIe controller has maximum read request size of 256 bytes.
*/
static void quirk_limit_readrequest(struct pci_dev *dev)
{
struct pci_dev *root_dev;
int i = 0;
/* Apply quirk only if this bridge device is keystone */
while (root_pci_ids[i]) {
root_dev = pci_get_device(PCI_VENDOR_ID_TI, root_pci_ids[i], NULL);
if ((root_dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
int readrq;
readrq = pcie_get_readrq(dev);
if (pcie_get_readrq(dev) > 256) {
pcie_set_readrq(dev, 256);
printk("Applied quirk\n");
}
break;
};
}
}
DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);
>
> I also agree with Jason that changing the PCI core to call
> pcie_bus_configure_settings() would be a better way of dealing with this
> if it solves the problem.
I tried following piece of code added to bios32.c
void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
{
struct pci_sys_data *sys;
LIST_HEAD(head);
pci_add_flags(PCI_REASSIGN_ALL_RSRC);
if (hw->preinit)
hw->preinit();
........
....
list_for_each_entry(sys, &head, node) {
struct pci_bus *bus = sys->bus;
if (!pci_has_flag(PCI_PROBE_ONLY)) {
/*
* Size the bridge windows.
*/
pci_bus_size_bridges(bus);
/*
* Assign resources.
*/
pci_bus_assign_resources(bus);
}
/*
* Tell drivers about devices found.
*/
pci_bus_add_devices(bus);
}
// New code starts here
list_for_each_entry(sys, &head, node) {
struct pci_bus *bus = sys->bus;
/* Configure PCI Express settings */
if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
struct pci_bus *child;
list_for_each_entry(child, &bus->children, node)
pcie_bus_configure_settings(child);
}
}
// New code ends.
This seems to do different things based on the pci bootargs. The key
requirement for Keystone PCI controller
is that the MRRS can't be more than 256 bytes. The only option that
works for keystone PCI controller
is with pci=pcie_bus_perf. I see following logs:-
[ 3.382747] pcie_bus_configure_settings, config 2
[ 3.382753] pcie_bus_configure_set
[ 3.382781] pcieport 0000:00:00.0: Max Payload Size set to 256/ 256
(was 128), Max Read Rq 256
[ 3.382788] pcie_bus_configure_set
[ 3.382846] pci 0000:01:00.0: Max Payload Size set to 256/ 256 (was
128), Max Read Rq 256
[ 3.382852] pcie_bus_configure_set
[ 3.382909] pci 0000:01:00.1: Max Payload Size set to 256/ 256 (was
128), Max Read Rq 256
On ARM, by default pci_bus_config seems to be set to 0
(PCIE_BUS_TUNE_OFF). So the code doesn't get
executed for this default. But for PCIE_BUS_SAFE, it doesn't change the
mrrs at the EP and is not
good for our platform w.r.t mrrs settings. For PCIE_BUS_PERFORMANCE, it
seems to increase the payload
size as well and Keystone Payload size is limited to 128 bytes. So it is
not safe to increase the payload
size to 256 based on the log.
On other platforms, Why the PCI core try to set the payload size equal
to mrrs? Is this explained in any
PCI spec? Looks like this is done for performance? Let me know if you
want me to send a patch for
review to add the pcie_bus_configure_settings() code to
arch/arm/kernel/bios32.c
For the Keystone PCI driver, I believe. it is safe to have the quirk so
that controller can handle the
read requests properly. Let me know if the quirk code above looks good
to go.
Murali
> Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists