* [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs
@ 2021-09-15 15:59 Ben Widawsky
2021-09-21 21:41 ` Dan Williams
0 siblings, 1 reply; 3+ messages in thread
From: Ben Widawsky @ 2021-09-15 15:59 UTC (permalink / raw)
To: linux-cxl
Cc: Ben Widawsky, Alison Schofield, Dan Williams, Ira Weiny,
Jonathan Cameron, Vishal Verma
Since the code to add decoders for switches and endpoints is on the
horizon, document the recently added interfaces that will be consumed by
them.
Part of the original version of this patch was subsumed by
f5786a5aedfc ("cxl/core: Split decoder setup into alloc + add")
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
v2:
- Dropped removal of host from cxl_decoder_add (Ben)
- Change nr_targets to unsigned int (Jonathan)
- Move description of 0 special case to param kdoc (Dan, Ben)
- Reword kdocs to be more accurate (Jonathan, Ben)
- Add back debug message to decoder_populate_targets (Ben)
---
drivers/cxl/core/bus.c | 34 ++++++++++++++++++++++++++++++++--
drivers/cxl/cxl.h | 2 ++
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 396252749477..d242afe08402 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -474,6 +474,7 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
rc = -ENXIO;
goto out_unlock;
}
+ dev_dbg(&cxld->dev, "%s: target: %d\n", dev_name(dport->dport), i);
cxld->target[i] = dport;
}
@@ -483,13 +484,28 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
return rc;
}
-struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
+/**
+ * cxl_decoder_alloc - Allocate a new CXL decoder
+ * @port: owning port of this decoder
+ * @nr_targets: downstream targets accessible by this decoder. All upstream
+ * ports and root ports must have at least 1 target. Endpoint
+ * devices will have 0 targets. Callers wishing to register an
+ * endpoint device should specify 0.
+ *
+ * A port should contain one or more decoders. Each of those decoders enable
+ * some address space for CXL.mem utilization. A decoder is expected to be
+ * configured by the caller before registering.
+ *
+ * Return: A new cxl decoder to be registered by cxl_decoder_add()
+ */
+struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
+ unsigned int nr_targets)
{
struct cxl_decoder *cxld;
struct device *dev;
int rc = 0;
- if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
+ if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets == 0)
return ERR_PTR(-EINVAL);
cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
@@ -521,6 +537,20 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
}
EXPORT_SYMBOL_GPL(cxl_decoder_alloc);
+/**
+ * cxl_decoder_add - Add a decoder with targets
+ * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
+ * @target_map: A list of downstream ports that this decoder can direct memory
+ * traffic to. These numbers should correspond with the port number
+ * in the PCIe Link Capabilities structure.
+ *
+ * Certain types of decoders may not have any targets. The main example of this
+ * is an endpoint device. A more awkward example is a hostbridge whose root
+ * ports get hot added (technically possible, though unlikely).
+ *
+ * Return: Negative error code if the decoder wasn't properly configured; else
+ * returns 0.
+ */
int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
{
struct cxl_port *port;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 7d6b011dd963..e632cc8da091 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -289,6 +289,8 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
struct cxl_decoder *to_cxl_decoder(struct device *dev);
bool is_root_decoder(struct device *dev);
struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets);
+struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
+ unsigned int nr_targets);
int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
--
2.33.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs
2021-09-15 15:59 [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs Ben Widawsky
@ 2021-09-21 21:41 ` Dan Williams
2021-09-21 21:58 ` Ben Widawsky
0 siblings, 1 reply; 3+ messages in thread
From: Dan Williams @ 2021-09-21 21:41 UTC (permalink / raw)
To: Ben Widawsky
Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma
On Wed, Sep 15, 2021 at 9:00 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Since the code to add decoders for switches and endpoints is on the
> horizon, document the recently added interfaces that will be consumed by
> them.
>
> Part of the original version of this patch was subsumed by
> f5786a5aedfc ("cxl/core: Split decoder setup into alloc + add")
It's dangerous to quote 'pending' commits. The stable commit id for this is now:
48667f676189 ("cxl/core: Split decoder setup into alloc + add")
...I would have just fixed this up on applying, but some notes below.
This overall feels like a patch that should wait to go in with first
introduction of endpoint decoders.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> v2:
> - Dropped removal of host from cxl_decoder_add (Ben)
> - Change nr_targets to unsigned int (Jonathan)
> - Move description of 0 special case to param kdoc (Dan, Ben)
> - Reword kdocs to be more accurate (Jonathan, Ben)
> - Add back debug message to decoder_populate_targets (Ben)
> ---
>
> drivers/cxl/core/bus.c | 34 ++++++++++++++++++++++++++++++++--
> drivers/cxl/cxl.h | 2 ++
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index 396252749477..d242afe08402 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -474,6 +474,7 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> rc = -ENXIO;
> goto out_unlock;
> }
> + dev_dbg(&cxld->dev, "%s: target: %d\n", dev_name(dport->dport), i);
Seems like this deserves its own patch since it's not related to $subject.
> cxld->target[i] = dport;
> }
>
> @@ -483,13 +484,28 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> return rc;
> }
>
> -struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> +/**
> + * cxl_decoder_alloc - Allocate a new CXL decoder
> + * @port: owning port of this decoder
> + * @nr_targets: downstream targets accessible by this decoder. All upstream
> + * ports and root ports must have at least 1 target. Endpoint
> + * devices will have 0 targets. Callers wishing to register an
> + * endpoint device should specify 0.
> + *
> + * A port should contain one or more decoders. Each of those decoders enable
> + * some address space for CXL.mem utilization. A decoder is expected to be
> + * configured by the caller before registering.
> + *
> + * Return: A new cxl decoder to be registered by cxl_decoder_add()
> + */
> +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> + unsigned int nr_targets)
> {
> struct cxl_decoder *cxld;
> struct device *dev;
> int rc = 0;
>
> - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
> + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets == 0)
This looks broken as the above text says that 0 is now an allowed
value. Shouldn't this be:
- if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
+ if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
...?
> return ERR_PTR(-EINVAL);
>
> cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> @@ -521,6 +537,20 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> }
> EXPORT_SYMBOL_GPL(cxl_decoder_alloc);
>
> +/**
> + * cxl_decoder_add - Add a decoder with targets
> + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> + * @target_map: A list of downstream ports that this decoder can direct memory
> + * traffic to. These numbers should correspond with the port number
> + * in the PCIe Link Capabilities structure.
> + *
> + * Certain types of decoders may not have any targets. The main example of this
> + * is an endpoint device. A more awkward example is a hostbridge whose root
> + * ports get hot added (technically possible, though unlikely).
I don't think this is possible, certainly not worth calling out unless
you want to prompt someone to go audit the PCI side for proper
operation in this case.
The endpoint note is enough.
> + *
> + * Return: Negative error code if the decoder wasn't properly configured; else
> + * returns 0.
> + */
> int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> {
> struct cxl_port *port;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 7d6b011dd963..e632cc8da091 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -289,6 +289,8 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> bool is_root_decoder(struct device *dev);
> struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets);
> +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> + unsigned int nr_targets);
Why are there now 2 different cxl_decoder_alloc() function signatures?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs
2021-09-21 21:41 ` Dan Williams
@ 2021-09-21 21:58 ` Ben Widawsky
0 siblings, 0 replies; 3+ messages in thread
From: Ben Widawsky @ 2021-09-21 21:58 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Alison Schofield, Ira Weiny, Jonathan Cameron, Vishal Verma
On 21-09-21 14:41:20, Dan Williams wrote:
> On Wed, Sep 15, 2021 at 9:00 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > Since the code to add decoders for switches and endpoints is on the
> > horizon, document the recently added interfaces that will be consumed by
> > them.
> >
> > Part of the original version of this patch was subsumed by
> > f5786a5aedfc ("cxl/core: Split decoder setup into alloc + add")
>
> It's dangerous to quote 'pending' commits. The stable commit id for this is now:
>
> 48667f676189 ("cxl/core: Split decoder setup into alloc + add")
Yes, sorry. At the time I thought pending was going to become next but that
didn't happen and thus the danger.
>
> ...I would have just fixed this up on applying, but some notes below.
>
> This overall feels like a patch that should wait to go in with first
> introduction of endpoint decoders.
It's fine with me to wait for intro of endpoint decoders. I probably should have
requested you just take the kdoc stuff with your last series, but not worth it
now.
>
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> > v2:
> > - Dropped removal of host from cxl_decoder_add (Ben)
> > - Change nr_targets to unsigned int (Jonathan)
> > - Move description of 0 special case to param kdoc (Dan, Ben)
> > - Reword kdocs to be more accurate (Jonathan, Ben)
> > - Add back debug message to decoder_populate_targets (Ben)
> > ---
> >
> > drivers/cxl/core/bus.c | 34 ++++++++++++++++++++++++++++++++--
> > drivers/cxl/cxl.h | 2 ++
> > 2 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 396252749477..d242afe08402 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -474,6 +474,7 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> > rc = -ENXIO;
> > goto out_unlock;
> > }
> > + dev_dbg(&cxld->dev, "%s: target: %d\n", dev_name(dport->dport), i);
>
> Seems like this deserves its own patch since it's not related to $subject.
I don't remember why I put this in this patch....
>
> > cxld->target[i] = dport;
> > }
> >
> > @@ -483,13 +484,28 @@ static int decoder_populate_targets(struct cxl_decoder *cxld,
> > return rc;
> > }
> >
> > -struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> > +/**
> > + * cxl_decoder_alloc - Allocate a new CXL decoder
> > + * @port: owning port of this decoder
> > + * @nr_targets: downstream targets accessible by this decoder. All upstream
> > + * ports and root ports must have at least 1 target. Endpoint
> > + * devices will have 0 targets. Callers wishing to register an
> > + * endpoint device should specify 0.
> > + *
> > + * A port should contain one or more decoders. Each of those decoders enable
> > + * some address space for CXL.mem utilization. A decoder is expected to be
> > + * configured by the caller before registering.
> > + *
> > + * Return: A new cxl decoder to be registered by cxl_decoder_add()
> > + */
> > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > + unsigned int nr_targets)
> > {
> > struct cxl_decoder *cxld;
> > struct device *dev;
> > int rc = 0;
> >
> > - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
> > + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets == 0)
>
> This looks broken as the above text says that 0 is now an allowed
> value. Shouldn't this be:
>
> - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1)
> + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
>
> ...?
>
Yes. This got messed up. I actually originally intended this change as part of
the endpoint decoder series. I'd rather drop this and just change to unsigned
int.
> > return ERR_PTR(-EINVAL);
> >
> > cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> > @@ -521,6 +537,20 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets)
> > }
> > EXPORT_SYMBOL_GPL(cxl_decoder_alloc);
> >
> > +/**
> > + * cxl_decoder_add - Add a decoder with targets
> > + * @cxld: The cxl decoder allocated by cxl_decoder_alloc()
> > + * @target_map: A list of downstream ports that this decoder can direct memory
> > + * traffic to. These numbers should correspond with the port number
> > + * in the PCIe Link Capabilities structure.
> > + *
> > + * Certain types of decoders may not have any targets. The main example of this
> > + * is an endpoint device. A more awkward example is a hostbridge whose root
> > + * ports get hot added (technically possible, though unlikely).
>
> I don't think this is possible, certainly not worth calling out unless
> you want to prompt someone to go audit the PCI side for proper
> operation in this case.
>
> The endpoint note is enough.
I ran it by Chet as to whether or not it's technically possible, which it is
according to him. I'm fine to drop it though.
>
> > + *
> > + * Return: Negative error code if the decoder wasn't properly configured; else
> > + * returns 0.
> > + */
> > int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
> > {
> > struct cxl_port *port;
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 7d6b011dd963..e632cc8da091 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -289,6 +289,8 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
> > struct cxl_decoder *to_cxl_decoder(struct device *dev);
> > bool is_root_decoder(struct device *dev);
> > struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets);
> > +struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > + unsigned int nr_targets);
>
> Why are there now 2 different cxl_decoder_alloc() function signatures?
This was unintentional.
My apologies for the sloppy patch.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-21 21:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 15:59 [PATCH v2] cxl/core/bus: Document and tighten up decoder APIs Ben Widawsky
2021-09-21 21:41 ` Dan Williams
2021-09-21 21:58 ` Ben Widawsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.