* [RFC PATCH] cxl: switch usp to parent cxl_port
@ 2021-09-24 21:28 Ben Widawsky
2021-09-27 21:49 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-24 21:28 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl, Ben Widawsky
Hi Dan.
As discussed in #cxl, I'm trying to do the switch enumeration and not entirely
sure of the best way to go about it (and no real way to test it). Do you have a
proposed way to do this better?
---
drivers/cxl/core/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 936b6b5665c3..372d30b4bafd 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -419,6 +419,71 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
}
EXPORT_SYMBOL_GPL(devm_cxl_add_port);
+static int match_add_switches(struct pci_dev *pdev, void *data)
+{
+ struct cxl_walk_context *ctx = data;
+ struct pci_bus *root_bus = ctx->root;
+ struct device *dev = &pdev->dev;
+ int type = pci_pcie_type(pdev);
+ struct cxl_port *parent_port;
+ struct cxl_register_map map;
+ int rc;
+
+ if (pdev->bus != root_bus)
+ return 0;
+
+ if (!pci_is_pcie(pdev))
+ return 0;
+ if (type != PCI_EXP_TYPE_UPSTREAM)
+ return 0;
+
+ /* Get component registers */
+ rc = cxl_find_register_block(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+ if (rc) {
+ ctx->error = rc;
+ }
+
+ /* to_pci_dev(dev->parent) == downstream port, or root port */
+ /* to_pci_dev(dev->parent->parent) == upstream port, or hostbridge */
+
+ if (dev_WARN_ONCE(dev, dev->parent->parent == NULL,
+ "No valid parent\n")) {
+ ctx->error = -ENODEV;
+ parent_port = NULL;
+ } else {
+ parent_port = to_cxl_port(dev->parent->parent);
+ }
+
+ devm_cxl_add_port(ctx->dev, dev,
+ pci_resource_start(pdev, map.barno) +
+ map.block_offset,
+ parent_port);
+
+ return 0;
+}
+
+int enumerate_ports_from_root_port(struct cxl_port *root_port)
+{
+ struct device *d = root_port->uport;
+ struct cxl_walk_context ctx;
+ struct pci_dev *pdev;
+
+ if (!dev_is_pci(d))
+ return -ENODEV;
+
+ pdev = to_pci_dev(d);
+ ctx = (struct cxl_walk_context){
+ .dev = d,
+ .root = pdev->bus,
+ .port = root_port,
+ };
+
+ pci_walk_bus(pdev->bus, match_add_switches, &ctx);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(enumerate_ports_from_root_port);
+
static struct cxl_dport *find_dport(struct cxl_port *port, int id)
{
struct cxl_dport *dport;
base-commit: 5c46722547fb375f460dbe293f40fad0ca32ab90
prerequisite-patch-id: f3e7ffbd5f60cd3f55ad8e174afd9d8c37d546f4
prerequisite-patch-id: a74a0ce5f4a1e4cec50e68181fcbb92624cb0687
prerequisite-patch-id: 3e2e86cbc2631b99c1b5c0179f35799d3df31f91
prerequisite-patch-id: 68cf009af310e296626b7842fc798fc778fcc98e
prerequisite-patch-id: 4063abbb095774d723ad727cb91bc8576d6a5532
prerequisite-patch-id: 3f11cc68d4dfc7f578f8c6a440663e9ef4e27466
prerequisite-patch-id: 5ef0b7ff2acf5896b955dd4f70a06581f42d7904
prerequisite-patch-id: 8cc6066d94bc859ff094c056d77eb63a967a3657
prerequisite-patch-id: b4fa2b34c36f3068743c965ebc01462ab016df4d
prerequisite-patch-id: a8798a409f1ff81979fb35a2c66a991ba6cc69f5
prerequisite-patch-id: d68e5ca2dbd408f206acb40c54f1ed9e6d981aa0
--
2.33.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-24 21:28 [RFC PATCH] cxl: switch usp to parent cxl_port Ben Widawsky
@ 2021-09-27 21:49 ` Dan Williams
2021-09-27 22:45 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2021-09-27 21:49 UTC (permalink / raw)
To: Ben Widawsky; +Cc: linux-cxl
On Fri, Sep 24, 2021 at 2:28 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Hi Dan.
>
> As discussed in #cxl, I'm trying to do the switch enumeration and not entirely
> sure of the best way to go about it (and no real way to test it). Do you have a
> proposed way to do this better?
>
>
> ---
> drivers/cxl/core/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index 936b6b5665c3..372d30b4bafd 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -419,6 +419,71 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> }
> EXPORT_SYMBOL_GPL(devm_cxl_add_port);
>
> +static int match_add_switches(struct pci_dev *pdev, void *data)
> +{
> + struct cxl_walk_context *ctx = data;
> + struct pci_bus *root_bus = ctx->root;
> + struct device *dev = &pdev->dev;
> + int type = pci_pcie_type(pdev);
> + struct cxl_port *parent_port;
> + struct cxl_register_map map;
> + int rc;
> +
> + if (pdev->bus != root_bus)
> + return 0;
This bus is the peer-bus of the other root ports in the system. You
would want to be scanning the buses of the switch's subordinate
downstream ports. Also I don't think you want to filter by bus, and
just let pci_walk_bus() iterate the full sub-hierarchy.
So if pdev is a CXL uport, then the next uport would be on one of the
subordinate bridges. With the expectation that each cxl_port includes
a listing of its dports I would expect something like:
list_for_each_entry(dport, &root_port->dports, list)
pci_walk_bus(dport->subordinate, match_uport, ctx);
> +
> + if (!pci_is_pcie(pdev))
> + return 0;
> + if (type != PCI_EXP_TYPE_UPSTREAM)
> + return 0;
Yes, assuming that that pci_walk_bus() scans bridges before descending
to the next level you can try to scan for CXL capability on anything
that claims to be an upstream port. You could also save another
pci_walk_bus() by doing the dport check here too... i.e.
if (type == PCI_EXP_TYPE_DOWNSTREAM) {
port = find_cxl_port(pdev->parent);
...determine id and component reg base...
cxl_add_dport(port, pdev, ...);
}
> +
> + /* Get component registers */
> + rc = cxl_find_register_block(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> + if (rc) {
> + ctx->error = rc;
> + }
> +
> + /* to_pci_dev(dev->parent) == downstream port, or root port */
> + /* to_pci_dev(dev->parent->parent) == upstream port, or hostbridge */
> +
> + if (dev_WARN_ONCE(dev, dev->parent->parent == NULL,
> + "No valid parent\n")) {
> + ctx->error = -ENODEV;
> + parent_port = NULL;
Not sure what this is for? I would think filtering pci_walk_bus() by
PCI_EXP_TYPE_{UP,DOWN}STREAM is sufficient and that this association
is assumed.
> + } else {
> + parent_port = to_cxl_port(dev->parent->parent);
No, you can't walk from a pci device to a cxl port, so it would need
something like find_cxl_port() like I handwaved above.
> + }
> +
> + devm_cxl_add_port(ctx->dev, dev,
> + pci_resource_start(pdev, map.barno) +
> + map.block_offset,
> + parent_port);
> +
> + return 0;
> +}
> +
> +int enumerate_ports_from_root_port(struct cxl_port *root_port)
> +{
> + struct device *d = root_port->uport;
> + struct cxl_walk_context ctx;
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(d))
> + return -ENODEV;
> +
> + pdev = to_pci_dev(d);
> + ctx = (struct cxl_walk_context){
> + .dev = d,
> + .root = pdev->bus,
> + .port = root_port,
> + };
> +
> + pci_walk_bus(pdev->bus, match_add_switches, &ctx);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(enumerate_ports_from_root_port);
> +
> static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> {
> struct cxl_dport *dport;
>
> base-commit: 5c46722547fb375f460dbe293f40fad0ca32ab90
> prerequisite-patch-id: f3e7ffbd5f60cd3f55ad8e174afd9d8c37d546f4
> prerequisite-patch-id: a74a0ce5f4a1e4cec50e68181fcbb92624cb0687
> prerequisite-patch-id: 3e2e86cbc2631b99c1b5c0179f35799d3df31f91
> prerequisite-patch-id: 68cf009af310e296626b7842fc798fc778fcc98e
> prerequisite-patch-id: 4063abbb095774d723ad727cb91bc8576d6a5532
> prerequisite-patch-id: 3f11cc68d4dfc7f578f8c6a440663e9ef4e27466
> prerequisite-patch-id: 5ef0b7ff2acf5896b955dd4f70a06581f42d7904
> prerequisite-patch-id: 8cc6066d94bc859ff094c056d77eb63a967a3657
> prerequisite-patch-id: b4fa2b34c36f3068743c965ebc01462ab016df4d
> prerequisite-patch-id: a8798a409f1ff81979fb35a2c66a991ba6cc69f5
> prerequisite-patch-id: d68e5ca2dbd408f206acb40c54f1ed9e6d981aa0
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-27 21:49 ` Dan Williams
@ 2021-09-27 22:45 ` Ben Widawsky
2021-09-27 23:54 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-27 22:45 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl
On 21-09-27 14:49:40, Dan Williams wrote:
> On Fri, Sep 24, 2021 at 2:28 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Hi Dan.
> >
> > As discussed in #cxl, I'm trying to do the switch enumeration and not entirely
> > sure of the best way to go about it (and no real way to test it). Do you have a
> > proposed way to do this better?
> >
> >
> > ---
> > drivers/cxl/core/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 65 insertions(+)
> >
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 936b6b5665c3..372d30b4bafd 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -419,6 +419,71 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> > }
> > EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> >
> > +static int match_add_switches(struct pci_dev *pdev, void *data)
> > +{
> > + struct cxl_walk_context *ctx = data;
> > + struct pci_bus *root_bus = ctx->root;
> > + struct device *dev = &pdev->dev;
> > + int type = pci_pcie_type(pdev);
> > + struct cxl_port *parent_port;
> > + struct cxl_register_map map;
> > + int rc;
> > +
> > + if (pdev->bus != root_bus)
> > + return 0;
>
> This bus is the peer-bus of the other root ports in the system.
I really dislike the word term "root port". You specifically mean other
hostbridges in the system, correct (or do you mean peer root ports in a
hostbridge)?
> You would want to be scanning the buses of the switch's subordinate downstream
> ports. Also I don't think you want to filter by bus, and just let
> pci_walk_bus() iterate the full sub-hierarchy.
Yes. That's fine. This was copy-paste from cxl_acpi, but I don't see a reason to
filter on the bus either.
>
> So if pdev is a CXL uport, then the next uport would be on one of the
> subordinate bridges. With the expectation that each cxl_port includes
> a listing of its dports I would expect something like:
>
> list_for_each_entry(dport, &root_port->dports, list)
> pci_walk_bus(dport->subordinate, match_uport, ctx);
>
Not sure why I need to iterate over the dports, isn't just walking down the from
the root_port sufficient? I suppose it does some pre-filtering which might be a
win.
> > +
> > + if (!pci_is_pcie(pdev))
> > + return 0;
> > + if (type != PCI_EXP_TYPE_UPSTREAM)
> > + return 0;
>
> Yes, assuming that that pci_walk_bus() scans bridges before descending
> to the next level you can try to scan for CXL capability on anything
> that claims to be an upstream port. You could also save another
> pci_walk_bus() by doing the dport check here too... i.e.
>
> if (type == PCI_EXP_TYPE_DOWNSTREAM) {
> port = find_cxl_port(pdev->parent);
> ...determine id and component reg base...
> cxl_add_dport(port, pdev, ...);
> }
>
find_cxl_port handwave is where I was asking for advice. I believe I know how to
implement find_cxl_port(struct pci_dev *upstream_port), but...
(see bottom of email for sample with everything)
> > +
> > + /* Get component registers */
> > + rc = cxl_find_register_block(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> > + if (rc) {
> > + ctx->error = rc;
> > + }
> > +
> > + /* to_pci_dev(dev->parent) == downstream port, or root port */
> > + /* to_pci_dev(dev->parent->parent) == upstream port, or hostbridge */
> > +
> > + if (dev_WARN_ONCE(dev, dev->parent->parent == NULL,
> > + "No valid parent\n")) {
> > + ctx->error = -ENODEV;
> > + parent_port = NULL;
>
> Not sure what this is for? I would think filtering pci_walk_bus() by
> PCI_EXP_TYPE_{UP,DOWN}STREAM is sufficient and that this association
> is assumed.
>
> > + } else {
> > + parent_port = to_cxl_port(dev->parent->parent);
>
> No, you can't walk from a pci device to a cxl port, so it would need
> something like find_cxl_port() like I handwaved above.
>
This is fundamentally where I'm "stuck". As I add switches, I need to get to the
parent port and I do not know how to do that. Is it safe to do
find_cxl_port(dev->parent->parent)?
> > + }
> > +
> > + devm_cxl_add_port(ctx->dev, dev,
> > + pci_resource_start(pdev, map.barno) +
> > + map.block_offset,
> > + parent_port);
> > +
> > + return 0;
> > +}
> > +
> > +int enumerate_ports_from_root_port(struct cxl_port *root_port)
> > +{
> > + struct device *d = root_port->uport;
> > + struct cxl_walk_context ctx;
> > + struct pci_dev *pdev;
> > +
> > + if (!dev_is_pci(d))
> > + return -ENODEV;
> > +
> > + pdev = to_pci_dev(d);
> > + ctx = (struct cxl_walk_context){
> > + .dev = d,
> > + .root = pdev->bus,
> > + .port = root_port,
> > + };
> > +
> > + pci_walk_bus(pdev->bus, match_add_switches, &ctx);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(enumerate_ports_from_root_port);
> > +
> > static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> > {
> > struct cxl_dport *dport;
> >
> > base-commit: 5c46722547fb375f460dbe293f40fad0ca32ab90
> > prerequisite-patch-id: f3e7ffbd5f60cd3f55ad8e174afd9d8c37d546f4
> > prerequisite-patch-id: a74a0ce5f4a1e4cec50e68181fcbb92624cb0687
> > prerequisite-patch-id: 3e2e86cbc2631b99c1b5c0179f35799d3df31f91
> > prerequisite-patch-id: 68cf009af310e296626b7842fc798fc778fcc98e
> > prerequisite-patch-id: 4063abbb095774d723ad727cb91bc8576d6a5532
> > prerequisite-patch-id: 3f11cc68d4dfc7f578f8c6a440663e9ef4e27466
> > prerequisite-patch-id: 5ef0b7ff2acf5896b955dd4f70a06581f42d7904
> > prerequisite-patch-id: 8cc6066d94bc859ff094c056d77eb63a967a3657
> > prerequisite-patch-id: b4fa2b34c36f3068743c965ebc01462ab016df4d
> > prerequisite-patch-id: a8798a409f1ff81979fb35a2c66a991ba6cc69f5
> > prerequisite-patch-id: d68e5ca2dbd408f206acb40c54f1ed9e6d981aa0
> > --
> > 2.33.0
> >
int match_port(struct device *dev, const void *data)
{
struct pci_dev *pdev = data;
if (!is_cxl_port(dev))
return 0;
return to_cxl_port(dev)->uport == &pdev->dev;
}
struct cxl_port *find_cxl_port(struct pci_dev *upstream_port)
{
struct device *port_dev;
port_dev = bus_find_device(&cxl_bus_type, NULL, upstream_port, match_port);
if (port_dev)
return to_cxl_port(port_dev);
return NULL;
}
static int match_add_switches(struct pci_dev *pdev, void *data)
{
...
dev = &pdev->dev;
if (type == PCI_EXP_TYPE_UPSTREAM) {
parent_port = find_cxl_port(dev->parent->parent);
if (!parent_port)
return 1;
port = devm_cxl_add_port(..., parent_port); //upstream port
return 0;
}
if (type == PCI_EXP_TYPE_DOWNSTREAM) {
uport = find_cxl_port(dev->parent);
// get component reg base of dport
cxl_add_dport(uport, dev, ...)
}
return 0;
}
{
... pci_walk_bus(pdev->bus, match_add_switches, &ctx);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-27 22:45 ` Ben Widawsky
@ 2021-09-27 23:54 ` Dan Williams
2021-09-28 0:06 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2021-09-27 23:54 UTC (permalink / raw)
To: Ben Widawsky; +Cc: linux-cxl
On Mon, Sep 27, 2021 at 3:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-09-27 14:49:40, Dan Williams wrote:
> > On Fri, Sep 24, 2021 at 2:28 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > Hi Dan.
> > >
> > > As discussed in #cxl, I'm trying to do the switch enumeration and not entirely
> > > sure of the best way to go about it (and no real way to test it). Do you have a
> > > proposed way to do this better?
> > >
> > >
> > > ---
> > > drivers/cxl/core/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 65 insertions(+)
> > >
> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > index 936b6b5665c3..372d30b4bafd 100644
> > > --- a/drivers/cxl/core/bus.c
> > > +++ b/drivers/cxl/core/bus.c
> > > @@ -419,6 +419,71 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> > > }
> > > EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> > >
> > > +static int match_add_switches(struct pci_dev *pdev, void *data)
> > > +{
> > > + struct cxl_walk_context *ctx = data;
> > > + struct pci_bus *root_bus = ctx->root;
> > > + struct device *dev = &pdev->dev;
> > > + int type = pci_pcie_type(pdev);
> > > + struct cxl_port *parent_port;
> > > + struct cxl_register_map map;
> > > + int rc;
> > > +
> > > + if (pdev->bus != root_bus)
> > > + return 0;
> >
> > This bus is the peer-bus of the other root ports in the system.
>
> I really dislike the word term "root port". You specifically mean other
> hostbridges in the system, correct (or do you mean peer root ports in a
> hostbridge)?
I mean both.
All of the PCIE root ports in the system may appear to be on the same
root bus regardless of which hostbridge they belong, but that's not
relevant for switch scanning beneath root ports.
>
> > You would want to be scanning the buses of the switch's subordinate downstream
> > ports. Also I don't think you want to filter by bus, and just let
> > pci_walk_bus() iterate the full sub-hierarchy.
>
> Yes. That's fine. This was copy-paste from cxl_acpi, but I don't see a reason to
> filter on the bus either.
cxl_acpi is not scanning for subordinates like this code wants to do.
>
> >
> > So if pdev is a CXL uport, then the next uport would be on one of the
> > subordinate bridges. With the expectation that each cxl_port includes
> > a listing of its dports I would expect something like:
> >
> > list_for_each_entry(dport, &root_port->dports, list)
> > pci_walk_bus(dport->subordinate, match_uport, ctx);
> >
>
> Not sure why I need to iterate over the dports, isn't just walking down the from
> the root_port sufficient? I suppose it does some pre-filtering which might be a
> win.
You know you can skip a level because that first level is handled by
cxl_acpi, and that first level has the ACPI hostbridge weirdness.
>
>
> > > +
> > > + if (!pci_is_pcie(pdev))
> > > + return 0;
> > > + if (type != PCI_EXP_TYPE_UPSTREAM)
> > > + return 0;
> >
> > Yes, assuming that that pci_walk_bus() scans bridges before descending
> > to the next level you can try to scan for CXL capability on anything
> > that claims to be an upstream port. You could also save another
> > pci_walk_bus() by doing the dport check here too... i.e.
> >
> > if (type == PCI_EXP_TYPE_DOWNSTREAM) {
> > port = find_cxl_port(pdev->parent);
> > ...determine id and component reg base...
> > cxl_add_dport(port, pdev, ...);
> > }
> >
>
> find_cxl_port handwave is where I was asking for advice. I believe I know how to
> implement find_cxl_port(struct pci_dev *upstream_port), but...
>
> (see bottom of email for sample with everything)
>
>
> > > +
> > > + /* Get component registers */
> > > + rc = cxl_find_register_block(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> > > + if (rc) {
> > > + ctx->error = rc;
> > > + }
> > > +
> > > + /* to_pci_dev(dev->parent) == downstream port, or root port */
> > > + /* to_pci_dev(dev->parent->parent) == upstream port, or hostbridge */
> > > +
> > > + if (dev_WARN_ONCE(dev, dev->parent->parent == NULL,
> > > + "No valid parent\n")) {
> > > + ctx->error = -ENODEV;
> > > + parent_port = NULL;
> >
> > Not sure what this is for? I would think filtering pci_walk_bus() by
> > PCI_EXP_TYPE_{UP,DOWN}STREAM is sufficient and that this association
> > is assumed.
> >
> > > + } else {
> > > + parent_port = to_cxl_port(dev->parent->parent);
> >
> > No, you can't walk from a pci device to a cxl port, so it would need
> > something like find_cxl_port() like I handwaved above.
> >
>
> This is fundamentally where I'm "stuck". As I add switches, I need to get to the
> parent port and I do not know how to do that. Is it safe to do
> find_cxl_port(dev->parent->parent)?
Yes, but only if you start the walk from dport->subordinate otherwise
this walks off the top of the hierarchy.
>
> > > + }
> > > +
> > > + devm_cxl_add_port(ctx->dev, dev,
> > > + pci_resource_start(pdev, map.barno) +
> > > + map.block_offset,
> > > + parent_port);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int enumerate_ports_from_root_port(struct cxl_port *root_port)
> > > +{
> > > + struct device *d = root_port->uport;
> > > + struct cxl_walk_context ctx;
> > > + struct pci_dev *pdev;
> > > +
> > > + if (!dev_is_pci(d))
> > > + return -ENODEV;
> > > +
> > > + pdev = to_pci_dev(d);
> > > + ctx = (struct cxl_walk_context){
> > > + .dev = d,
> > > + .root = pdev->bus,
> > > + .port = root_port,
> > > + };
> > > +
> > > + pci_walk_bus(pdev->bus, match_add_switches, &ctx);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(enumerate_ports_from_root_port);
> > > +
> > > static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> > > {
> > > struct cxl_dport *dport;
> > >
> > > base-commit: 5c46722547fb375f460dbe293f40fad0ca32ab90
> > > prerequisite-patch-id: f3e7ffbd5f60cd3f55ad8e174afd9d8c37d546f4
> > > prerequisite-patch-id: a74a0ce5f4a1e4cec50e68181fcbb92624cb0687
> > > prerequisite-patch-id: 3e2e86cbc2631b99c1b5c0179f35799d3df31f91
> > > prerequisite-patch-id: 68cf009af310e296626b7842fc798fc778fcc98e
> > > prerequisite-patch-id: 4063abbb095774d723ad727cb91bc8576d6a5532
> > > prerequisite-patch-id: 3f11cc68d4dfc7f578f8c6a440663e9ef4e27466
> > > prerequisite-patch-id: 5ef0b7ff2acf5896b955dd4f70a06581f42d7904
> > > prerequisite-patch-id: 8cc6066d94bc859ff094c056d77eb63a967a3657
> > > prerequisite-patch-id: b4fa2b34c36f3068743c965ebc01462ab016df4d
> > > prerequisite-patch-id: a8798a409f1ff81979fb35a2c66a991ba6cc69f5
> > > prerequisite-patch-id: d68e5ca2dbd408f206acb40c54f1ed9e6d981aa0
> > > --
> > > 2.33.0
> > >
>
> int match_port(struct device *dev, const void *data)
> {
> struct pci_dev *pdev = data;
>
> if (!is_cxl_port(dev))
> return 0;
>
> return to_cxl_port(dev)->uport == &pdev->dev;
> }
>
> struct cxl_port *find_cxl_port(struct pci_dev *upstream_port)
> {
> struct device *port_dev;
>
> port_dev = bus_find_device(&cxl_bus_type, NULL, upstream_port, match_port);
> if (port_dev)
> return to_cxl_port(port_dev);
>
> return NULL;
> }
>
> static int match_add_switches(struct pci_dev *pdev, void *data)
> {
> ...
> dev = &pdev->dev;
>
> if (type == PCI_EXP_TYPE_UPSTREAM) {
> parent_port = find_cxl_port(dev->parent->parent);
Looks good.
For the final code you'll probably want to do find_cxl_port(dev) first
just to make sure you're not adding a port for a switch that was
scanned before in a previous API call.
> if (!parent_port)
> return 1;
>
> port = devm_cxl_add_port(..., parent_port); //upstream port
Looks good.
For the final code it will want a "put_device(&parent_port->dev)" here
to undo the reference taken by bus_find_device().
> return 0;
> }
>
> if (type == PCI_EXP_TYPE_DOWNSTREAM) {
> uport = find_cxl_port(dev->parent);
> // get component reg base of dport
> cxl_add_dport(uport, dev, ...)
Looks good.
> }
>
> return 0;
> }
>
> {
> ... pci_walk_bus(pdev->bus, match_add_switches, &ctx);
Per-above you probably want to start the scan below each root port, or
otherwise filter out the top-level scan work that cxl_acpi owns.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-27 23:54 ` Dan Williams
@ 2021-09-28 0:06 ` Ben Widawsky
2021-09-28 0:18 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-28 0:06 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl
On 21-09-27 16:54:25, Dan Williams wrote:
> On Mon, Sep 27, 2021 at 3:45 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-09-27 14:49:40, Dan Williams wrote:
> > > On Fri, Sep 24, 2021 at 2:28 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> > > > Hi Dan.
> > > >
> > > > As discussed in #cxl, I'm trying to do the switch enumeration and not entirely
> > > > sure of the best way to go about it (and no real way to test it). Do you have a
> > > > proposed way to do this better?
> > > >
> > > >
> > > > ---
> > > > drivers/cxl/core/bus.c | 65 ++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 65 insertions(+)
> > > >
> > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > > index 936b6b5665c3..372d30b4bafd 100644
> > > > --- a/drivers/cxl/core/bus.c
> > > > +++ b/drivers/cxl/core/bus.c
> > > > @@ -419,6 +419,71 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> > > > }
> > > > EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> > > >
> > > > +static int match_add_switches(struct pci_dev *pdev, void *data)
> > > > +{
> > > > + struct cxl_walk_context *ctx = data;
> > > > + struct pci_bus *root_bus = ctx->root;
> > > > + struct device *dev = &pdev->dev;
> > > > + int type = pci_pcie_type(pdev);
> > > > + struct cxl_port *parent_port;
> > > > + struct cxl_register_map map;
> > > > + int rc;
> > > > +
> > > > + if (pdev->bus != root_bus)
> > > > + return 0;
> > >
> > > This bus is the peer-bus of the other root ports in the system.
> >
> > I really dislike the word term "root port". You specifically mean other
> > hostbridges in the system, correct (or do you mean peer root ports in a
> > hostbridge)?
>
> I mean both.
>
> All of the PCIE root ports in the system may appear to be on the same
> root bus regardless of which hostbridge they belong, but that's not
> relevant for switch scanning beneath root ports.
>
> >
> > > You would want to be scanning the buses of the switch's subordinate downstream
> > > ports. Also I don't think you want to filter by bus, and just let
> > > pci_walk_bus() iterate the full sub-hierarchy.
> >
> > Yes. That's fine. This was copy-paste from cxl_acpi, but I don't see a reason to
> > filter on the bus either.
>
> cxl_acpi is not scanning for subordinates like this code wants to do.
>
> >
> > >
> > > So if pdev is a CXL uport, then the next uport would be on one of the
> > > subordinate bridges. With the expectation that each cxl_port includes
> > > a listing of its dports I would expect something like:
> > >
> > > list_for_each_entry(dport, &root_port->dports, list)
> > > pci_walk_bus(dport->subordinate, match_uport, ctx);
> > >
> >
> > Not sure why I need to iterate over the dports, isn't just walking down the from
> > the root_port sufficient? I suppose it does some pre-filtering which might be a
> > win.
>
> You know you can skip a level because that first level is handled by
> cxl_acpi, and that first level has the ACPI hostbridge weirdness.
>
> >
> >
> > > > +
> > > > + if (!pci_is_pcie(pdev))
> > > > + return 0;
> > > > + if (type != PCI_EXP_TYPE_UPSTREAM)
> > > > + return 0;
> > >
> > > Yes, assuming that that pci_walk_bus() scans bridges before descending
> > > to the next level you can try to scan for CXL capability on anything
> > > that claims to be an upstream port. You could also save another
> > > pci_walk_bus() by doing the dport check here too... i.e.
> > >
> > > if (type == PCI_EXP_TYPE_DOWNSTREAM) {
> > > port = find_cxl_port(pdev->parent);
> > > ...determine id and component reg base...
> > > cxl_add_dport(port, pdev, ...);
> > > }
> > >
> >
> > find_cxl_port handwave is where I was asking for advice. I believe I know how to
> > implement find_cxl_port(struct pci_dev *upstream_port), but...
> >
> > (see bottom of email for sample with everything)
> >
> >
> > > > +
> > > > + /* Get component registers */
> > > > + rc = cxl_find_register_block(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> > > > + if (rc) {
> > > > + ctx->error = rc;
> > > > + }
> > > > +
> > > > + /* to_pci_dev(dev->parent) == downstream port, or root port */
> > > > + /* to_pci_dev(dev->parent->parent) == upstream port, or hostbridge */
> > > > +
> > > > + if (dev_WARN_ONCE(dev, dev->parent->parent == NULL,
> > > > + "No valid parent\n")) {
> > > > + ctx->error = -ENODEV;
> > > > + parent_port = NULL;
> > >
> > > Not sure what this is for? I would think filtering pci_walk_bus() by
> > > PCI_EXP_TYPE_{UP,DOWN}STREAM is sufficient and that this association
> > > is assumed.
> > >
> > > > + } else {
> > > > + parent_port = to_cxl_port(dev->parent->parent);
> > >
> > > No, you can't walk from a pci device to a cxl port, so it would need
> > > something like find_cxl_port() like I handwaved above.
> > >
> >
> > This is fundamentally where I'm "stuck". As I add switches, I need to get to the
> > parent port and I do not know how to do that. Is it safe to do
> > find_cxl_port(dev->parent->parent)?
>
> Yes, but only if you start the walk from dport->subordinate otherwise
> this walks off the top of the hierarchy.
>
> >
> > > > + }
> > > > +
> > > > + devm_cxl_add_port(ctx->dev, dev,
> > > > + pci_resource_start(pdev, map.barno) +
> > > > + map.block_offset,
> > > > + parent_port);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int enumerate_ports_from_root_port(struct cxl_port *root_port)
> > > > +{
> > > > + struct device *d = root_port->uport;
> > > > + struct cxl_walk_context ctx;
> > > > + struct pci_dev *pdev;
> > > > +
> > > > + if (!dev_is_pci(d))
> > > > + return -ENODEV;
> > > > +
> > > > + pdev = to_pci_dev(d);
> > > > + ctx = (struct cxl_walk_context){
> > > > + .dev = d,
> > > > + .root = pdev->bus,
> > > > + .port = root_port,
> > > > + };
> > > > +
> > > > + pci_walk_bus(pdev->bus, match_add_switches, &ctx);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(enumerate_ports_from_root_port);
> > > > +
> > > > static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> > > > {
> > > > struct cxl_dport *dport;
> > > >
> > > > base-commit: 5c46722547fb375f460dbe293f40fad0ca32ab90
> > > > prerequisite-patch-id: f3e7ffbd5f60cd3f55ad8e174afd9d8c37d546f4
> > > > prerequisite-patch-id: a74a0ce5f4a1e4cec50e68181fcbb92624cb0687
> > > > prerequisite-patch-id: 3e2e86cbc2631b99c1b5c0179f35799d3df31f91
> > > > prerequisite-patch-id: 68cf009af310e296626b7842fc798fc778fcc98e
> > > > prerequisite-patch-id: 4063abbb095774d723ad727cb91bc8576d6a5532
> > > > prerequisite-patch-id: 3f11cc68d4dfc7f578f8c6a440663e9ef4e27466
> > > > prerequisite-patch-id: 5ef0b7ff2acf5896b955dd4f70a06581f42d7904
> > > > prerequisite-patch-id: 8cc6066d94bc859ff094c056d77eb63a967a3657
> > > > prerequisite-patch-id: b4fa2b34c36f3068743c965ebc01462ab016df4d
> > > > prerequisite-patch-id: a8798a409f1ff81979fb35a2c66a991ba6cc69f5
> > > > prerequisite-patch-id: d68e5ca2dbd408f206acb40c54f1ed9e6d981aa0
> > > > --
> > > > 2.33.0
> > > >
> >
> > int match_port(struct device *dev, const void *data)
> > {
> > struct pci_dev *pdev = data;
> >
> > if (!is_cxl_port(dev))
> > return 0;
> >
> > return to_cxl_port(dev)->uport == &pdev->dev;
> > }
> >
> > struct cxl_port *find_cxl_port(struct pci_dev *upstream_port)
> > {
> > struct device *port_dev;
> >
> > port_dev = bus_find_device(&cxl_bus_type, NULL, upstream_port, match_port);
> > if (port_dev)
> > return to_cxl_port(port_dev);
> >
> > return NULL;
> > }
> >
> > static int match_add_switches(struct pci_dev *pdev, void *data)
> > {
> > ...
> > dev = &pdev->dev;
> >
> > if (type == PCI_EXP_TYPE_UPSTREAM) {
> > parent_port = find_cxl_port(dev->parent->parent);
>
> Looks good.
>
> For the final code you'll probably want to do find_cxl_port(dev) first
> just to make sure you're not adding a port for a switch that was
> scanned before in a previous API call.
>
> > if (!parent_port)
> > return 1;
> >
> > port = devm_cxl_add_port(..., parent_port); //upstream port
>
> Looks good.
>
> For the final code it will want a "put_device(&parent_port->dev)" here
> to undo the reference taken by bus_find_device().
>
> > return 0;
> > }
> >
> > if (type == PCI_EXP_TYPE_DOWNSTREAM) {
> > uport = find_cxl_port(dev->parent);
> > // get component reg base of dport
> > cxl_add_dport(uport, dev, ...)
>
> Looks good.
>
> > }
> >
> > return 0;
> > }
> >
> > {
> > ... pci_walk_bus(pdev->bus, match_add_switches, &ctx);
>
> Per-above you probably want to start the scan below each root port, or
> otherwise filter out the top-level scan work that cxl_acpi owns.
The scan will skip everything until it finds a USP or DSP which cxl_acpi
wouldn't be touching. I don't have any issue with:
> list_for_each_entry(dport, &root_port->dports, list)
> pci_walk_bus(dport->subordinate, match_uport, ctx);
but I don't see any issue with the current code either. Did I miss something?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 0:06 ` Ben Widawsky
@ 2021-09-28 0:18 ` Dan Williams
2021-09-28 0:21 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2021-09-28 0:18 UTC (permalink / raw)
To: Ben Widawsky; +Cc: linux-cxl
On Mon, Sep 27, 2021 at 5:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
[..]
> > Per-above you probably want to start the scan below each root port, or
> > otherwise filter out the top-level scan work that cxl_acpi owns.
>
> The scan will skip everything until it finds a USP or DSP which cxl_acpi
> wouldn't be touching. I don't have any issue with:
>
> > list_for_each_entry(dport, &root_port->dports, list)
> > pci_walk_bus(dport->subordinate, match_uport, ctx);
>
> but I don't see any issue with the current code either. Did I miss something?
if (pdev->bus != root_bus)
return 0;
Doesn't that always fail for the switches in question? Did I miss
where root_bus is updated?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 0:18 ` Dan Williams
@ 2021-09-28 0:21 ` Ben Widawsky
2021-09-28 0:32 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-28 0:21 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl
On 21-09-27 17:18:35, Dan Williams wrote:
> On Mon, Sep 27, 2021 at 5:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> [..]
> > > Per-above you probably want to start the scan below each root port, or
> > > otherwise filter out the top-level scan work that cxl_acpi owns.
> >
> > The scan will skip everything until it finds a USP or DSP which cxl_acpi
> > wouldn't be touching. I don't have any issue with:
> >
> > > list_for_each_entry(dport, &root_port->dports, list)
> > > pci_walk_bus(dport->subordinate, match_uport, ctx);
> >
> > but I don't see any issue with the current code either. Did I miss something?
>
> if (pdev->bus != root_bus)
> return 0;
>
> Doesn't that always fail for the switches in question? Did I miss
> where root_bus is updated?
In my previous comment I meant that I'd drop this check, but I see what you're
saying now. I need to pick some subordinate bus or I'll just walk over all
switches, even ones I don't care about...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 0:21 ` Ben Widawsky
@ 2021-09-28 0:32 ` Ben Widawsky
2021-09-28 1:09 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-28 0:32 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl
On 21-09-27 17:21:24, Ben Widawsky wrote:
> On 21-09-27 17:18:35, Dan Williams wrote:
> > On Mon, Sep 27, 2021 at 5:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > [..]
> > > > Per-above you probably want to start the scan below each root port, or
> > > > otherwise filter out the top-level scan work that cxl_acpi owns.
> > >
> > > The scan will skip everything until it finds a USP or DSP which cxl_acpi
> > > wouldn't be touching. I don't have any issue with:
> > >
> > > > list_for_each_entry(dport, &root_port->dports, list)
> > > > pci_walk_bus(dport->subordinate, match_uport, ctx);
> > >
> > > but I don't see any issue with the current code either. Did I miss something?
> >
> > if (pdev->bus != root_bus)
> > return 0;
> >
> > Doesn't that always fail for the switches in question? Did I miss
> > where root_bus is updated?
>
> In my previous comment I meant that I'd drop this check, but I see what you're
> saying now. I need to pick some subordinate bus or I'll just walk over all
> switches, even ones I don't care about...
Would this work?
-int enumerate_ports_from_root_port(struct cxl_port *root_port)
+int enumerate_ports_from_root_port(struct cxl_dport *root_port)
{
- struct device *d = root_port->uport;
+ struct device *d = root_port->dport;
struct cxl_walk_context ctx;
struct pci_dev *pdev;
if (!dev_is_pci(d))
return -ENODEV;
pdev = to_pci_dev(d);
ctx = (struct cxl_walk_context){
.dev = d,
.root = pdev->bus,
- .port = root_port,
+ .port = root_port->port,
};
pci_walk_bus(pdev->bus, match_add_switches, &ctx);
return 0;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 0:32 ` Ben Widawsky
@ 2021-09-28 1:09 ` Dan Williams
2021-09-28 3:23 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2021-09-28 1:09 UTC (permalink / raw)
To: Ben Widawsky; +Cc: linux-cxl
On Mon, Sep 27, 2021 at 5:32 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-09-27 17:21:24, Ben Widawsky wrote:
> > On 21-09-27 17:18:35, Dan Williams wrote:
> > > On Mon, Sep 27, 2021 at 5:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > [..]
> > > > > Per-above you probably want to start the scan below each root port, or
> > > > > otherwise filter out the top-level scan work that cxl_acpi owns.
> > > >
> > > > The scan will skip everything until it finds a USP or DSP which cxl_acpi
> > > > wouldn't be touching. I don't have any issue with:
> > > >
> > > > > list_for_each_entry(dport, &root_port->dports, list)
> > > > > pci_walk_bus(dport->subordinate, match_uport, ctx);
> > > >
> > > > but I don't see any issue with the current code either. Did I miss something?
> > >
> > > if (pdev->bus != root_bus)
> > > return 0;
> > >
> > > Doesn't that always fail for the switches in question? Did I miss
> > > where root_bus is updated?
> >
> > In my previous comment I meant that I'd drop this check, but I see what you're
> > saying now. I need to pick some subordinate bus or I'll just walk over all
> > switches, even ones I don't care about...
>
> Would this work?
Would need to see the whole patch in context, but I'm being thrown off
by the attempted reuse of 'struct cxl_walk_context'. That assumes that
the parent port is passed and only scans the top-level PCI bus.
Whereas for switches we're talking about walking all of them and using
something like find_cxl_port() to lookup parent ports as it goes.
>
> -int enumerate_ports_from_root_port(struct cxl_port *root_port)
> +int enumerate_ports_from_root_port(struct cxl_dport *root_port)
> {
> - struct device *d = root_port->uport;
> + struct device *d = root_port->dport;
> struct cxl_walk_context ctx;
> struct pci_dev *pdev;
>
> if (!dev_is_pci(d))
> return -ENODEV;
>
> pdev = to_pci_dev(d);
> ctx = (struct cxl_walk_context){
> .dev = d,
> .root = pdev->bus,
> - .port = root_port,
> + .port = root_port->port,
Per-above why would match_add_switches() care about this port once it
walks down a level in the tree?
> };
>
> pci_walk_bus(pdev->bus, match_add_switches, &ctx);
>
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 1:09 ` Dan Williams
@ 2021-09-28 3:23 ` Ben Widawsky
2021-09-28 4:29 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2021-09-28 3:23 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-cxl
On 21-09-27 18:09:46, Dan Williams wrote:
> On Mon, Sep 27, 2021 at 5:32 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-09-27 17:21:24, Ben Widawsky wrote:
> > > On 21-09-27 17:18:35, Dan Williams wrote:
> > > > On Mon, Sep 27, 2021 at 5:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > [..]
> > > > > > Per-above you probably want to start the scan below each root port, or
> > > > > > otherwise filter out the top-level scan work that cxl_acpi owns.
> > > > >
> > > > > The scan will skip everything until it finds a USP or DSP which cxl_acpi
> > > > > wouldn't be touching. I don't have any issue with:
> > > > >
> > > > > > list_for_each_entry(dport, &root_port->dports, list)
> > > > > > pci_walk_bus(dport->subordinate, match_uport, ctx);
> > > > >
> > > > > but I don't see any issue with the current code either. Did I miss something?
> > > >
> > > > if (pdev->bus != root_bus)
> > > > return 0;
> > > >
> > > > Doesn't that always fail for the switches in question? Did I miss
> > > > where root_bus is updated?
> > >
> > > In my previous comment I meant that I'd drop this check, but I see what you're
> > > saying now. I need to pick some subordinate bus or I'll just walk over all
> > > switches, even ones I don't care about...
> >
> > Would this work?
>
> Would need to see the whole patch in context, but I'm being thrown off
> by the attempted reuse of 'struct cxl_walk_context'. That assumes that
> the parent port is passed and only scans the top-level PCI bus.
> Whereas for switches we're talking about walking all of them and using
> something like find_cxl_port() to lookup parent ports as it goes.
cxl_walk_context is only used for convenience. It's not strictly required, but
it is a superset of what I was looking for.
>
> >
> > -int enumerate_ports_from_root_port(struct cxl_port *root_port)
> > +int enumerate_ports_from_root_port(struct cxl_dport *root_port)
> > {
> > - struct device *d = root_port->uport;
> > + struct device *d = root_port->dport;
> > struct cxl_walk_context ctx;
> > struct pci_dev *pdev;
> >
> > if (!dev_is_pci(d))
> > return -ENODEV;
> >
> > pdev = to_pci_dev(d);
> > ctx = (struct cxl_walk_context){
> > .dev = d,
> > .root = pdev->bus,
> > - .port = root_port,
> > + .port = root_port->port,
>
> Per-above why would match_add_switches() care about this port once it
> walks down a level in the tree?
I thought you wanted the host of every new port to be either the hostbridge or
the cfmws root port.
>
> > };
> >
> > pci_walk_bus(pdev->bus, match_add_switches, &ctx);
> >
> > return 0;
> > }
> >
> >
I'll get more context out shortly.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] cxl: switch usp to parent cxl_port
2021-09-28 3:23 ` Ben Widawsky
@ 2021-09-28 4:29 ` Dan Williams
0 siblings, 0 replies; 11+ messages in thread
From: Dan Williams @ 2021-09-28 4:29 UTC (permalink / raw)
To: Ben Widawsky; +Cc: linux-cxl
On Mon, Sep 27, 2021 at 8:23 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
[..]
> > > -int enumerate_ports_from_root_port(struct cxl_port *root_port)
> > > +int enumerate_ports_from_root_port(struct cxl_dport *root_port)
> > > {
> > > - struct device *d = root_port->uport;
> > > + struct device *d = root_port->dport;
> > > struct cxl_walk_context ctx;
> > > struct pci_dev *pdev;
> > >
> > > if (!dev_is_pci(d))
> > > return -ENODEV;
> > >
> > > pdev = to_pci_dev(d);
> > > ctx = (struct cxl_walk_context){
> > > .dev = d,
> > > .root = pdev->bus,
> > > - .port = root_port,
> > > + .port = root_port->port,
> >
> > Per-above why would match_add_switches() care about this port once it
> > walks down a level in the tree?
>
> I thought you wanted the host of every new port to be either the hostbridge or
> the cfmws root port.
The host is the device that owns the devm context. I was thinking the
natural device for hosting the whole CXL port hierarchy is the device
that registered the root cxl_port, the ACPI0017 platform device. I
expect It would just be passed in directly to this 'enumerate' helper.
Although, it's more 'rescan' than 'enumerate'.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-28 4:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-24 21:28 [RFC PATCH] cxl: switch usp to parent cxl_port Ben Widawsky
2021-09-27 21:49 ` Dan Williams
2021-09-27 22:45 ` Ben Widawsky
2021-09-27 23:54 ` Dan Williams
2021-09-28 0:06 ` Ben Widawsky
2021-09-28 0:18 ` Dan Williams
2021-09-28 0:21 ` Ben Widawsky
2021-09-28 0:32 ` Ben Widawsky
2021-09-28 1:09 ` Dan Williams
2021-09-28 3:23 ` Ben Widawsky
2021-09-28 4:29 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).