Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 07:23:49PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote: > > The `is_thunderbolt` attribute originally had a well defined list of > > quirks that it existed for, but it has been overloaded with more > > meaning. > > > > Instead use the driver core removable attribute to indicate the > > detail a device is attached to a thunderbolt or USB4 chain. > ... > If these things are not specifically related to Thunderbolt, I'd > prefer to get rid of pci_is_thunderbolt_attached() and see if we can > help the GPU folks figure out what they really need. Ah. Guess I should read the whole series before commenting :) I see that you *did* remove pci_is_thunderbolt_attached() in the last patch. I'll look more at the rest tomorrow. Bjorn
Re: [Nouveau] [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Thu, Feb 24, 2022 at 03:51:12PM -0600, Mario Limonciello wrote:
> The `is_thunderbolt` attribute originally had a well defined list of
> quirks that it existed for, but it has been overloaded with more
> meaning.
>
> Instead use the driver core removable attribute to indicate the
> detail a device is attached to a thunderbolt or USB4 chain.
>
> Signed-off-by: Mario Limonciello
> ---
> drivers/pci/probe.c | 2 +-
> drivers/platform/x86/apple-gmux.c | 2 +-
> include/linux/pci.h | 5 ++---
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..1b752d425c47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1584,7 +1584,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> /* Is the device part of a Thunderbolt controller? */
> vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
> PCI_VSEC_ID_INTEL_TBT);
> if (vsec)
> - dev->is_thunderbolt = 1;
> + dev->external_facing = true;
I assume there's a spec for the PCI_VSEC_ID_INTEL_TBT Capability. Is
that public? Does the spec say that a device with that capability
must be external-facing?
Even if it's not public, I think a citation (name, revision, section)
would be useful.
> }
>
> static void set_pcie_untrusted(struct pci_dev *dev)
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 57553f9b4d1d..da0c39b0 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -596,7 +596,7 @@ static int gmux_resume(struct device *dev)
>
> static int is_thunderbolt(struct device *dev, void *data)
> {
> - return to_pci_dev(dev)->is_thunderbolt;
> + return to_pci_dev(dev)->external_facing;
This looks ... sort of weird. I don't know anything about
apple-gmux.c, so I guess I don't care, but assuming any
external-facing device must be a Thunderbolt device seems like a
stretch.
Ugh. This is used via "bus_for_each_dev(_bus_type)", which means
it's not hotplug-safe. I'm sure we "know" implicitly that hotplug
isn't an issue in apple-gmux, but it's better not to have examples
that get copied to places where it *is* an issue.
> }
>
> static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1e5b769e42fc..d9719eb14654 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -442,7 +442,6 @@ struct pci_dev {
> unsigned intis_virtfn:1;
> unsigned intis_hotplug_bridge:1;
> unsigned intshpc_managed:1; /* SHPC owned by shpchp */
> - unsigned intis_thunderbolt:1; /* Thunderbolt controller */
> unsigned intno_cmd_complete:1; /* Lies about command completed
> events */
>
> /*
> @@ -2447,11 +2446,11 @@ static inline bool pci_is_thunderbolt_attached(struct
> pci_dev *pdev)
> {
> struct pci_dev *parent = pdev;
>
> - if (pdev->is_thunderbolt)
> + if (dev_is_removable(>dev))
> return true;
>
> while ((parent = pci_upstream_bridge(parent)))
> - if (parent->is_thunderbolt)
> + if (dev_is_removable(>dev))
> return true;
I don't get this. Plain old PCI devices can be removable, too.
pci_is_thunderbolt_attached() is only used by GPU drivers. What
property of Thunderbolt do they care about?
nouveau_vga_init() and radeon_device_init() use it to decide to
register with vga_switcheroo. So maybe that's something to do with
removability? Of course, that's not specific to Thunderbolt, because
garden-variety PCIe devices are removable.
amdgpu_driver_load_kms() and radeon_driver_load_kms() apparently use
it for something related to power control. I don't know what the
Thunderbolt connection is.
nbio_v2_3_enable_aspm() looks like it uses it to change some ASPM
parameters. Seems like potentially a device erratum or quirk
material?
If these things are not specifically related to Thunderbolt, I'd
prefer to get rid of pci_is_thunderbolt_attached() and see if we can
help the GPU folks figure out what they really need.
> return false;
> --
> 2.34.1
>
Re: [Nouveau] [PATCH] drm/nouveau: Remove the unused header file nvif/list.h
Thanks for the ping!
Reviewed-by: Lyude Paul
I will push this to drm-misc-next in a bit
On Wed, 2022-02-23 at 10:18 +0800, Cai Huoqing wrote:
> On 09 2月 22 14:53:19, Cai Huoqing wrote:
> > The nouveau driver depends on include/linux/list.h instead of
> > nvif/list.h, so remove the obstacle-nvif/list.h.
> >
> > Signed-off-by: Cai Huoqing
> > ---
> Ping :)
> > drivers/gpu/drm/nouveau/include/nvif/list.h | 353
> > 1 file changed, 353 deletions(-)
> > delete mode 100644 drivers/gpu/drm/nouveau/include/nvif/list.h
> >
> > diff --git a/drivers/gpu/drm/nouveau/include/nvif/list.h
> > b/drivers/gpu/drm/nouveau/include/nvif/list.h
> > deleted file mode 100644
> > index 8af5d144ecb0..
> > --- a/drivers/gpu/drm/nouveau/include/nvif/list.h
> > +++ /dev/null
> > @@ -1,353 +0,0 @@
> > -/*
> > - * Copyright © 2010 Intel Corporation
> > - * Copyright © 2010 Francisco Jerez
> > - *
> > - * Permission is hereby granted, free of charge, to any person obtaining
> > a
> > - * copy of this software and associated documentation files (the
> > "Software"),
> > - * to deal in the Software without restriction, including without
> > limitation
> > - * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > - * and/or sell copies of the Software, and to permit persons to whom the
> > - * Software is furnished to do so, subject to the following conditions:
> > - *
> > - * The above copyright notice and this permission notice (including the
> > next
> > - * paragraph) shall be included in all copies or substantial portions of
> > the
> > - * Software.
> > - *
> > - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> > SHALL
> > - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > - * IN THE SOFTWARE.
> > - *
> > - */
> > -
> > -/* Modified by Ben Skeggs to match kernel list APIs
> > */
> > -
> > -#ifndef _XORG_LIST_H_
> > -#define _XORG_LIST_H_
> > -
> > -/**
> > - * @file Classic doubly-link circular list implementation.
> > - * For real usage examples of the linked list, see the file test/list.c
> > - *
> > - * Example:
> > - * We need to keep a list of struct foo in the parent struct bar, i.e.
> > what
> > - * we want is something like this.
> > - *
> > - * struct bar {
> > - * ...
> > - * struct foo *list_of_foos; -> struct foo {}, struct foo
> > {}, struct foo{}
> > - * ...
> > - * }
> > - *
> > - * We need one list head in bar and a list element in all list_of_foos
> > (both are of
> > - * data type 'struct list_head').
> > - *
> > - * struct bar {
> > - * ...
> > - * struct list_head list_of_foos;
> > - * ...
> > - * }
> > - *
> > - * struct foo {
> > - * ...
> > - * struct list_head entry;
> > - * ...
> > - * }
> > - *
> > - * Now we initialize the list head:
> > - *
> > - * struct bar bar;
> > - * ...
> > - * INIT_LIST_HEAD(_of_foos);
> > - *
> > - * Then we create the first element and add it to this list:
> > - *
> > - * struct foo *foo = malloc(...);
> > - *
> > - * list_add(>entry, _of_foos);
> > - *
> > - * Repeat the above for each element you want to add to the list.
> > Deleting
> > - * works with the element itself.
> > - * list_del(>entry);
> > - * free(foo);
> > - *
> > - * Note: calling list_del(_of_foos) will set bar.list_of_foos to
> > an empty
> > - * list again.
> > - *
> > - * Looping through the list requires a 'struct foo' as iterator and the
> > - * name of the field the subnodes use.
> > - *
> > - * struct foo *iterator;
> > - * list_for_each_entry(iterator, _of_foos, entry) {
> > - * if (iterator->something == ...)
> > - * ...
> > - * }
> > - *
> > - * Note: You must not call list_del() on the iterator if you continue the
> > - * loop. You need to run the safe for-each loop instead:
> > - *
> > - * struct foo *iterator, *next;
> > - * list_for_each_entry_safe(iterator, next, _of_foos, entry) {
> > - * if (...)
> > - * list_del(>entry);
> > - * }
> > - *
> > - */
> > -
> > -/**
> > - * The linkage struct for list nodes. This struct must be part of your
> > - * to-be-linked struct. struct list_head is required for both the head of
> > the
> > - * list and for each list node.
> > - *
> > - * Position and name of the struct list_head field is irrelevant.
> > - * There are no requirements that elements of a list are of the same
> > type.
> > - * There are no requirements for a list head, any struct list_head can be
> > a list
> > - * head.
> > - */
> > -struct list_head
