| 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: <8dade04c-609b-f69d-0809-4aa8fecd9b87@amd.com>
Date: Mon, 29 Jul 2024 11:19:03 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Rob Herring <robh@...nel.org>, <devicetree@...r.kernel.org>, "Saravana
Kannan" <saravanak@...gle.com>, Kowshik Jois B S <kowsjois@...ux.ibm.com>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kvm-ppc@...r.kernel.org>, Vaidyanathan Srinivasan <svaidy@...ux.ibm.com>,
Lukas Wunner <lukas@...ner.de>, Nicholas Piggin <npiggin@...il.com>, "Bjorn
Helgaas" <bhelgaas@...gle.com>, Vaibhav Jain <vaibhav@...ux.ibm.com>,
<linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v2] PCI: Fix crash during pci_dev hot-unplug on pseries
KVM guest
On 7/29/24 09:55, Amit Machhiwal wrote:
> Hi Lizhi,
>
> On 2024/07/29 09:47 AM, Lizhi Hou wrote:
>> Hi Amit
>>
>> On 7/29/24 04:13, Amit Machhiwal wrote:
>>> Hi Lizhi,
>>>
>>> On 2024/07/26 11:45 AM, Lizhi Hou wrote:
>>>> On 7/26/24 10:52, Rob Herring wrote:
>>>>> On Thu, Jul 25, 2024 at 6:06 PM Lizhi Hou <lizhi.hou@....com> wrote:
>>>>>> Hi Amit,
>>>>>>
>>>>>>
>>>>>> I try to follow the option which add a OF flag. If Rob is ok with this,
>>>>>> I would suggest to use it instead of V1 patch
>>>>>>
>>>>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>>>>> index dda6092e6d3a..a401ed0463d9 100644
>>>>>> --- a/drivers/of/dynamic.c
>>>>>> +++ b/drivers/of/dynamic.c
>>>>>> @@ -382,6 +382,11 @@ void of_node_release(struct kobject *kobj)
>>>>>> __func__, node);
>>>>>> }
>>>>>>
>>>>>> + if (of_node_check_flag(node, OF_CREATED_WITH_CSET)) {
>>>>>> + of_changeset_revert(node->data);
>>>>>> + of_changeset_destroy(node->data);
>>>>>> + }
>>>>> What happens if multiple nodes are created in the changeset?
>>>> Ok. multiple nodes will not work.
>>>>>> +
>>>>>> if (node->child)
>>>>>> pr_err("ERROR: %s() unexpected children for %pOF/%s\n",
>>>>>> __func__, node->parent, node->full_name);
>>>>>> @@ -507,6 +512,7 @@ struct device_node *of_changeset_create_node(struct
>>>>>> of_changeset *ocs,
>>>>>> np = __of_node_dup(NULL, full_name);
>>>>>> if (!np)
>>>>>> return NULL;
>>>>>> + of_node_set_flag(np, OF_CREATED_WITH_CSET);
>>>>> This should be set where the data ptr is set.
>>>> Ok. It sounds the fix could be simplified to 3 lines change.
>>> Thanks for the patch. The hot-plug and hot-unplug of PCI device seem to work
>>> fine as expected. I see this patch would attempt to remove only the nodes which
>>> were created in `of_pci_make_dev_node()` with the help of the newly introduced
>>> flag, which looks good to me.
>>>
>>> Also, since a call to `of_pci_make_dev_node()` from `pci_bus_add_device()`, that
>>> creates devices nodes only for bridge devices, is conditional on
>>> `pci_is_bridge()`, it only makes sense to retain the logical symmetry and call
>>> `of_pci_remove_node()` conditionally on `pci_is_bridge()` as well in
>>> `pci_stop_dev()`. Hence, I would like to propose the below change along with the
>>> above patch:
>>>
>>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>>> index 910387e5bdbf..c6394bf562cd 100644
>>> --- a/drivers/pci/remove.c
>>> +++ b/drivers/pci/remove.c
>>> @@ -23,7 +23,8 @@ static void pci_stop_dev(struct pci_dev *dev)
>>> device_release_driver(&dev->dev);
>>> pci_proc_detach_device(dev);
>>> pci_remove_sysfs_dev_files(dev);
>>> - of_pci_remove_node(dev);
>>> + if (pci_is_bridge(dev))
>>> + of_pci_remove_node(dev);
>>> pci_dev_assign_added(dev, false);
>>> }
>>>
>>> Please let me know of your thoughts on this and based on that I can spin the v3
>>> of this patch.
>> As I mentioned, there are endpoints in pci quirks (pci/quirks.c) will also
>> create nodes by of_pci_make_dev_node(). So please remove above two lines.
> Sorry if I'm misinterpreting something here but as I mentioned,
> `of_pci_make_dev_node()` is called only for bridge devices with check performed
> via `pci_is_bridge()`, could you please elaborate more on why the same check
> can't be put while removing the node via `of_pci_remove_node()`?
For devices added in quirks, of_pci_make_dev_node() will be called
through pci_fixup_device().
Lizhi
>
> Thanks,
> Amit
>
>> Thanks,
>>
>> Lizhi
>>
>>> In addition to this, can this patch be taken as part of 6.11 as a bug fix?
>>>
>>> Thanks,
>>> Amit
>>>
>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>> index 51e3dd0ea5ab..0b3ba1e1b18c 100644
>>>> --- a/drivers/pci/of.c
>>>> +++ b/drivers/pci/of.c
>>>> @@ -613,7 +613,7 @@ void of_pci_remove_node(struct pci_dev *pdev)
>>>> struct device_node *np;
>>>>
>>>> np = pci_device_to_OF_node(pdev);
>>>> - if (!np || !of_node_check_flag(np, OF_DYNAMIC))
>>>> + if (!np || !of_node_check_flag(np, OF_CREATED_WITH_CSET))
>>>> return;
>>>> pdev->dev.of_node = NULL;
>>>>
>>>> @@ -672,6 +672,7 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
>>>> if (ret)
>>>> goto out_free_node;
>>>>
>>>> + of_node_set_flag(np, OF_CREATED_WITH_CSET);
>>>> np->data = cset;
>>>> pdev->dev.of_node = np;
>>>> kfree(name);
>>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>>> index a0bedd038a05..a46317f6626e 100644
>>>> --- a/include/linux/of.h
>>>> +++ b/include/linux/of.h
>>>> @@ -153,6 +153,7 @@ extern struct device_node *of_stdout;
>>>> #define OF_POPULATED_BUS 4 /* platform bus created for children */
>>>> #define OF_OVERLAY 5 /* allocated for an overlay */
>>>> #define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */
>>>> +#define OF_CREATED_WITH_CSET 7 /* created by of_changeset_create_node */
>>>>
>>>> #define OF_BAD_ADDR ((u64)-1)
>>>>
>>>>
>>>> Lizhi
>>>>
>>>>> Rob
Powered by blists - more mailing lists