On 10/07/2011 02:41 AM, Benjamin Herrenschmidt wrote:
>
>>> I suppose your hammer is bigger than mine :) But I don't think it's good
>>> to have a flag to workaround a code issue that the user must pass to
>>> ensure functioning adapters.
>>>
>> It's a BIOS (or PPC BIOS-equiv) issue, and the kernel has a number of boot-time parameters
>> like the one proposed to resolve such nuances relative to known, working,
>> architected technology that works for the majority of cases it is employed.
>
> No. It's more than that.
>
> It's a given platform type on which SR-IOV will -never- work. Requiring
> users to always pass sriov=false for those platforms to be usable is
> beyond stupid.
>
Frankly this is only useful for developers and _a few_ enterprise users at this point. Maybe the value needs to be a config value, but that doesn't explain the increase in complexity of the code Nish is suggesting.
What we're saying is that the real fix isn't to plunk a bunch of code which will be dead in a few months when powerpc gets its act together on SRIOV. Going with a bunch of complex arch-specific code for little benefit seems overkill here.
[FWIW -- Bjorn's suggestion of pushing all of this into the sriov struct is the way to go]
P.
> Ben.
>
>>
>>>> -----8<------
>>>>
>>>> [PATCH] Add a kernel parameter that disables SRIOV
>>>>
>>>> Add sriov=[0|1] as a kernel parameter to disable or enable SRIOV. This is
>>>> useful for debugging SRIOV issues vs. bare-metal issues.
>>>>
>>>> Signed-off-by: Prarit Bhargava<prarit@xxxxxxxxxx>
>>>> ---
>>>> Documentation/kernel-parameters.txt | 4 ++++
>>>> drivers/pci/iov.c | 30 +++++++++++++++++++++++++++++-
>>>> 2 files changed, 33 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>> index 854ed5c..235b5c5 100644
>>>> --- a/Documentation/kernel-parameters.txt
>>>> +++ b/Documentation/kernel-parameters.txt
>>>> @@ -2385,6 +2385,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>> spia_pedr=
>>>> spia_peddr=
>>>>
>>>> + sriov= [HW]
>>>> + Format: [0|1]
>>>> + Enable or disable SRIOV support in kernel.
>>>> +
>>>> stacktrace [FTRACE]
>>>> Enabled the stack tracer on boot up.
>>>>
>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>>> index 42fae47..da96e3f 100644
>>>> --- a/drivers/pci/iov.c
>>>> +++ b/drivers/pci/iov.c
>>>> @@ -408,6 +408,19 @@ static void sriov_disable(struct pci_dev *dev)
>>>> iov->nr_virtfn = 0;
>>>> }
>>>>
>>>> +static unsigned int sriov_enabled = 1;
>>>> +static int __init do_sriov_enable(char *str)
>>>> +{
>>>> + get_option(&str,&sriov_enabled);
>>>> + if (!sriov_enabled)
>>>> + printk(KERN_INFO "SRIOV: Disabled\n");
>>>> + else
>>>> + printk(KERN_INFO "SRIOV: Enabled\n");
>>>> +
>>>> + return 1;
>>>> +}
>>>> +__setup("sriov=", do_sriov_enable);
>>>
>>> Hrm, we only will print the above is sriov= is passed on the kernel
>>> command-line, right? It seems like maybe we should print it always. And
>>> perhaps the prefix should be "SR-IOV"?
>>>
>>>> static int sriov_init(struct pci_dev *dev, int pos)
>>>> {
>>>> int i;
>>>> @@ -419,6 +432,12 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>>> struct resource *res;
>>>> struct pci_dev *pdev;
>>>>
>>>> + if (!sriov_enabled) {
>>>> + rc = -EPERM;
>>>> + /* in theory no flags should have been set ... */
>>>> + goto failed;
>>>> + }
>>>> +
>>>
>>> Hrm, is this needed if pci_enable_sriov() checks it already? Isn't that
>>> the only caller?
>>>
>>>> if (dev->pcie_type != PCI_EXP_TYPE_RC_END&&
>>>> dev->pcie_type != PCI_EXP_TYPE_ENDPOINT)
>>>> return -ENODEV;
>>>> @@ -614,7 +633,10 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>>>> struct resource tmp;
>>>> enum pci_bar_type type;
>>>> int reg = pci_iov_resource_bar(dev, resno,&type);
>>>> -
>>>> +
>>>> + if (!sriov_enabled)
>>>> + return 0;
>>>> +
>>>> if (!reg)
>>>> return 0;
>>>>
>>>> @@ -667,6 +689,9 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
>>>> {
>>>> might_sleep();
>>>>
>>>> + if (!sriov_enabled)
>>>> + return -EPERM;
>>>> +
>>>> if (!dev->is_physfn)
>>>> return -ENODEV;
>>>>
>>>> @@ -682,6 +707,9 @@ void pci_disable_sriov(struct pci_dev *dev)
>>>> {
>>>> might_sleep();
>>>>
>>>> + if (!sriov_enabled)
>>>> + return;
>>>> +
>>>> if (!dev->is_physfn)
>>>> return;
>>>
>>> Ah my patch needs these three checks as well, FWIW.
>>>
>>> Thanks,
>>> Nish
>>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
