| Message ID | 20210902195017.2516472-5-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enumerate midlevel and endpoint decoders | expand |
On Thu, 2 Sep 2021 12:50:08 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > Endpoints have decoders too. It is useful to share the same > infrastructure from cxl_core. Endpoints do not have dports (downstream > targets), only the underlying physical medium. As a result, some special > casing is needed. > > There is no functional change introduced yet as endpoints don't actually > enumerate decoders yet. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Comments inline... > --- > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index 8d5061b0794d..6202ce5a5ac2 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > NULL, > }; > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > + &cxl_decoder_base_attribute_group, > + &cxl_base_attribute_group, > + NULL, > +}; > + > static void cxl_decoder_release(struct device *dev) > { > struct cxl_decoder *cxld = to_cxl_decoder(dev); > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > kfree(cxld); > } > > +static const struct device_type cxl_decoder_endpoint_type = { > + .name = "cxl_decoder_endpoint", > + .release = cxl_decoder_release, > + .groups = cxl_decoder_endpoint_attribute_groups, > +}; > + > static const struct device_type cxl_decoder_switch_type = { > .name = "cxl_decoder_switch", > .release = cxl_decoder_release, > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > .groups = cxl_decoder_root_attribute_groups, > }; > > +static bool is_endpoint_decoder(struct device *dev) > +{ > + return dev->type == &cxl_decoder_endpoint_type; > +} > + > bool is_root_decoder(struct device *dev) > { > return dev->type == &cxl_decoder_root_type; > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > struct device *dev; > int rc = 0; > > - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1) Why do we let nr_targets go negative? Could make the parameter unsigned perhaps or check for that here (even though it makes no sense). > + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE) > return ERR_PTR(-EINVAL); > > cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL); > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > dev->parent = &port->dev; > dev->bus = &cxl_bus_type; > > + /* Endpoints don't have a target list */ > + if (nr_targets == 0) > + dev->type = &cxl_decoder_endpoint_type; Hmm. Whilst the check seems valid, it's a bit inelegant. The capability register unhelpfully simply has it defined as not applicable rather than 0, so I'd be nervous that might end up here (not checked yet). > /* root ports do not have a cxl_port_type parent */ > - if (port->dev.parent->type == &cxl_port_type) > + else if (port->dev.parent->type == &cxl_port_type) > dev->type = &cxl_decoder_switch_type; > else > dev->type = &cxl_decoder_root_type; > @@ -532,9 +552,11 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, > if (IS_ERR(cxld)) > return PTR_ERR(cxld); > > + dev = &cxld->dev; > + > port = to_cxl_port(cxld->dev.parent); > device_lock(&port->dev); > - if (list_empty(&port->dports)) { > + if (is_endpoint_decoder(dev) && list_empty(&port->dports)) { > rc = -EINVAL; > goto out_unlock; > } > @@ -551,7 +573,6 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, > } > device_unlock(&port->dev); > > - dev = &cxld->dev; > rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id); > if (rc) > return rc;
On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Endpoints have decoders too. It is useful to share the same > infrastructure from cxl_core. Endpoints do not have dports (downstream > targets), only the underlying physical medium. As a result, some special > casing is needed. > > There is no functional change introduced yet as endpoints don't actually > enumerate decoders yet. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index 8d5061b0794d..6202ce5a5ac2 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > NULL, > }; > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > + &cxl_decoder_base_attribute_group, > + &cxl_base_attribute_group, > + NULL, > +}; > + > static void cxl_decoder_release(struct device *dev) > { > struct cxl_decoder *cxld = to_cxl_decoder(dev); > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > kfree(cxld); > } > > +static const struct device_type cxl_decoder_endpoint_type = { > + .name = "cxl_decoder_endpoint", > + .release = cxl_decoder_release, > + .groups = cxl_decoder_endpoint_attribute_groups, > +}; > + > static const struct device_type cxl_decoder_switch_type = { > .name = "cxl_decoder_switch", > .release = cxl_decoder_release, > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > .groups = cxl_decoder_root_attribute_groups, > }; > > +static bool is_endpoint_decoder(struct device *dev) > +{ > + return dev->type == &cxl_decoder_endpoint_type; > +} > + > bool is_root_decoder(struct device *dev) > { > return dev->type == &cxl_decoder_root_type; > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > struct device *dev; > int rc = 0; > > - 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); > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > dev->parent = &port->dev; > dev->bus = &cxl_bus_type; > > + /* Endpoints don't have a target list */ > + if (nr_targets == 0) > + dev->type = &cxl_decoder_endpoint_type; Do you also plan to introduce the concept of endpoint ports, and if yes should that come before this patch? That would seem to be more robust than, for example, allowing a switch port to carry an endpoint decoder object as this allows.
On 21-09-10 12:19:24, Dan Williams wrote: > On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > Endpoints have decoders too. It is useful to share the same > > infrastructure from cxl_core. Endpoints do not have dports (downstream > > targets), only the underlying physical medium. As a result, some special > > casing is needed. > > > > There is no functional change introduced yet as endpoints don't actually > > enumerate decoders yet. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > index 8d5061b0794d..6202ce5a5ac2 100644 > > --- a/drivers/cxl/core/bus.c > > +++ b/drivers/cxl/core/bus.c > > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > > NULL, > > }; > > > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > > + &cxl_decoder_base_attribute_group, > > + &cxl_base_attribute_group, > > + NULL, > > +}; > > + > > static void cxl_decoder_release(struct device *dev) > > { > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > > kfree(cxld); > > } > > > > +static const struct device_type cxl_decoder_endpoint_type = { > > + .name = "cxl_decoder_endpoint", > > + .release = cxl_decoder_release, > > + .groups = cxl_decoder_endpoint_attribute_groups, > > +}; > > + > > static const struct device_type cxl_decoder_switch_type = { > > .name = "cxl_decoder_switch", > > .release = cxl_decoder_release, > > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > > .groups = cxl_decoder_root_attribute_groups, > > }; > > > > +static bool is_endpoint_decoder(struct device *dev) > > +{ > > + return dev->type == &cxl_decoder_endpoint_type; > > +} > > + > > bool is_root_decoder(struct device *dev) > > { > > return dev->type == &cxl_decoder_root_type; > > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > struct device *dev; > > int rc = 0; > > > > - 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); > > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > dev->parent = &port->dev; > > dev->bus = &cxl_bus_type; > > > > + /* Endpoints don't have a target list */ > > + if (nr_targets == 0) > > + dev->type = &cxl_decoder_endpoint_type; > > Do you also plan to introduce the concept of endpoint ports, and if > yes should that come before this patch? That would seem to be more > robust than, for example, allowing a switch port to carry an endpoint > decoder object as this allows. I didn't see a need as of yet to differentiate between endpoint ports and other ports. I don't entirely understand what you mean by "allowing a switch port to carry an endpoint decoder" means. Can you please elaborate?
On 21-09-03 15:35:42, Jonathan Cameron wrote: > On Thu, 2 Sep 2021 12:50:08 -0700 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > Endpoints have decoders too. It is useful to share the same > > infrastructure from cxl_core. Endpoints do not have dports (downstream > > targets), only the underlying physical medium. As a result, some special > > casing is needed. > > > > There is no functional change introduced yet as endpoints don't actually > > enumerate decoders yet. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > Comments inline... > > > --- > > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > index 8d5061b0794d..6202ce5a5ac2 100644 > > --- a/drivers/cxl/core/bus.c > > +++ b/drivers/cxl/core/bus.c > > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > > NULL, > > }; > > > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > > + &cxl_decoder_base_attribute_group, > > + &cxl_base_attribute_group, > > + NULL, > > +}; > > + > > static void cxl_decoder_release(struct device *dev) > > { > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > > kfree(cxld); > > } > > > > +static const struct device_type cxl_decoder_endpoint_type = { > > + .name = "cxl_decoder_endpoint", > > + .release = cxl_decoder_release, > > + .groups = cxl_decoder_endpoint_attribute_groups, > > +}; > > + > > static const struct device_type cxl_decoder_switch_type = { > > .name = "cxl_decoder_switch", > > .release = cxl_decoder_release, > > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > > .groups = cxl_decoder_root_attribute_groups, > > }; > > > > +static bool is_endpoint_decoder(struct device *dev) > > +{ > > + return dev->type == &cxl_decoder_endpoint_type; > > +} > > + > > bool is_root_decoder(struct device *dev) > > { > > return dev->type == &cxl_decoder_root_type; > > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > struct device *dev; > > int rc = 0; > > > > - if (nr_targets > CXL_DECODER_MAX_INTERLEAVE || nr_targets < 1) > > Why do we let nr_targets go negative? Could make the parameter unsigned perhaps or > check for that here (even though it makes no sense). Unsigned seems like a good idea to me as well. > > > + if (nr_targets > CXL_DECODER_MAX_INTERLEAVE) > > return ERR_PTR(-EINVAL); > > > > cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL); > > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > dev->parent = &port->dev; > > dev->bus = &cxl_bus_type; > > > > + /* Endpoints don't have a target list */ > > + if (nr_targets == 0) > > + dev->type = &cxl_decoder_endpoint_type; > > Hmm. Whilst the check seems valid, it's a bit inelegant. > The capability register unhelpfully simply has it defined > as not applicable rather than 0, so I'd be nervous that might > end up here (not checked yet). AFAICT, 0 isn't a valid value in the register at all. Additionally, from the API perspective I don't see much of an issue because nr_targets is the decoded value. So the only entity that should call this with a value of 0 would have to be something which is ignoring the field in the capability register, which would only be an endpoint. > > > /* root ports do not have a cxl_port_type parent */ > > - if (port->dev.parent->type == &cxl_port_type) > > + else if (port->dev.parent->type == &cxl_port_type) > > dev->type = &cxl_decoder_switch_type; > > else > > dev->type = &cxl_decoder_root_type; > > @@ -532,9 +552,11 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, > > if (IS_ERR(cxld)) > > return PTR_ERR(cxld); > > > > + dev = &cxld->dev; > > + > > port = to_cxl_port(cxld->dev.parent); > > device_lock(&port->dev); > > - if (list_empty(&port->dports)) { > > + if (is_endpoint_decoder(dev) && list_empty(&port->dports)) { > > rc = -EINVAL; > > goto out_unlock; > > } > > @@ -551,7 +573,6 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, > > } > > device_unlock(&port->dev); > > > > - dev = &cxld->dev; > > rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id); > > if (rc) > > return rc; >
On Mon, Sep 13, 2021 at 9:11 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-09-10 12:19:24, Dan Williams wrote: > > On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > Endpoints have decoders too. It is useful to share the same > > > infrastructure from cxl_core. Endpoints do not have dports (downstream > > > targets), only the underlying physical medium. As a result, some special > > > casing is needed. > > > > > > There is no functional change introduced yet as endpoints don't actually > > > enumerate decoders yet. > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > --- > > > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > > index 8d5061b0794d..6202ce5a5ac2 100644 > > > --- a/drivers/cxl/core/bus.c > > > +++ b/drivers/cxl/core/bus.c > > > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > > > NULL, > > > }; > > > > > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > > > + &cxl_decoder_base_attribute_group, > > > + &cxl_base_attribute_group, > > > + NULL, > > > +}; > > > + > > > static void cxl_decoder_release(struct device *dev) > > > { > > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > > > kfree(cxld); > > > } > > > > > > +static const struct device_type cxl_decoder_endpoint_type = { > > > + .name = "cxl_decoder_endpoint", > > > + .release = cxl_decoder_release, > > > + .groups = cxl_decoder_endpoint_attribute_groups, > > > +}; > > > + > > > static const struct device_type cxl_decoder_switch_type = { > > > .name = "cxl_decoder_switch", > > > .release = cxl_decoder_release, > > > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > > > .groups = cxl_decoder_root_attribute_groups, > > > }; > > > > > > +static bool is_endpoint_decoder(struct device *dev) > > > +{ > > > + return dev->type == &cxl_decoder_endpoint_type; > > > +} > > > + > > > bool is_root_decoder(struct device *dev) > > > { > > > return dev->type == &cxl_decoder_root_type; > > > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > struct device *dev; > > > int rc = 0; > > > > > > - 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); > > > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > dev->parent = &port->dev; > > > dev->bus = &cxl_bus_type; > > > > > > + /* Endpoints don't have a target list */ > > > + if (nr_targets == 0) > > > + dev->type = &cxl_decoder_endpoint_type; > > > > Do you also plan to introduce the concept of endpoint ports, and if > > yes should that come before this patch? That would seem to be more > > robust than, for example, allowing a switch port to carry an endpoint > > decoder object as this allows. > > I didn't see a need as of yet to differentiate between endpoint ports and other > ports. I don't entirely understand what you mean by "allowing a switch port to > carry an endpoint decoder" means. Can you please elaborate? If endpoint ports were an explicit type then this check could make sure that someone did not pass nr_targets set to 0 where the @port argument is referring to a switch where the target_list must be specified. Either that, or a comment in kernel-doc for this routine about the special meaning of nr_targets == 0 and expected usage.
On 21-09-13 15:07:44, Dan Williams wrote: > On Mon, Sep 13, 2021 at 9:11 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > On 21-09-10 12:19:24, Dan Williams wrote: > > > On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > Endpoints have decoders too. It is useful to share the same > > > > infrastructure from cxl_core. Endpoints do not have dports (downstream > > > > targets), only the underlying physical medium. As a result, some special > > > > casing is needed. > > > > > > > > There is no functional change introduced yet as endpoints don't actually > > > > enumerate decoders yet. > > > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > --- > > > > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > > > index 8d5061b0794d..6202ce5a5ac2 100644 > > > > --- a/drivers/cxl/core/bus.c > > > > +++ b/drivers/cxl/core/bus.c > > > > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > > > > NULL, > > > > }; > > > > > > > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > > > > + &cxl_decoder_base_attribute_group, > > > > + &cxl_base_attribute_group, > > > > + NULL, > > > > +}; > > > > + > > > > static void cxl_decoder_release(struct device *dev) > > > > { > > > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > > > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > > > > kfree(cxld); > > > > } > > > > > > > > +static const struct device_type cxl_decoder_endpoint_type = { > > > > + .name = "cxl_decoder_endpoint", > > > > + .release = cxl_decoder_release, > > > > + .groups = cxl_decoder_endpoint_attribute_groups, > > > > +}; > > > > + > > > > static const struct device_type cxl_decoder_switch_type = { > > > > .name = "cxl_decoder_switch", > > > > .release = cxl_decoder_release, > > > > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > > > > .groups = cxl_decoder_root_attribute_groups, > > > > }; > > > > > > > > +static bool is_endpoint_decoder(struct device *dev) > > > > +{ > > > > + return dev->type == &cxl_decoder_endpoint_type; > > > > +} > > > > + > > > > bool is_root_decoder(struct device *dev) > > > > { > > > > return dev->type == &cxl_decoder_root_type; > > > > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > > struct device *dev; > > > > int rc = 0; > > > > > > > > - 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); > > > > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > > dev->parent = &port->dev; > > > > dev->bus = &cxl_bus_type; > > > > > > > > + /* Endpoints don't have a target list */ > > > > + if (nr_targets == 0) > > > > + dev->type = &cxl_decoder_endpoint_type; > > > > > > Do you also plan to introduce the concept of endpoint ports, and if > > > yes should that come before this patch? That would seem to be more > > > robust than, for example, allowing a switch port to carry an endpoint > > > decoder object as this allows. > > > > I didn't see a need as of yet to differentiate between endpoint ports and other > > ports. I don't entirely understand what you mean by "allowing a switch port to > > carry an endpoint decoder" means. Can you please elaborate? > > If endpoint ports were an explicit type then this check could make > sure that someone did not pass nr_targets set to 0 where the @port > argument is referring to a switch where the target_list must be > specified. > > Either that, or a comment in kernel-doc for this routine about the > special meaning of nr_targets == 0 and expected usage. Well, since Jonathan also brought up a concern here perhaps I should entertain other ideas. I suppose future versions of the spec could break things, but as it stands today the only CXL component that implements decoders that can have a 0 value for this are endpoint devices (T2 or T3, or LD). I think it's fine to wait until we have a second reason to make an endpoint port type and update kdocs for now, but maybe it will also be a natural fit with a proper port driver.
On Mon, Sep 13, 2021 at 4:19 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-09-13 15:07:44, Dan Williams wrote: > > On Mon, Sep 13, 2021 at 9:11 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > On 21-09-10 12:19:24, Dan Williams wrote: > > > > On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > > > Endpoints have decoders too. It is useful to share the same > > > > > infrastructure from cxl_core. Endpoints do not have dports (downstream > > > > > targets), only the underlying physical medium. As a result, some special > > > > > casing is needed. > > > > > > > > > > There is no functional change introduced yet as endpoints don't actually > > > > > enumerate decoders yet. > > > > > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > > --- > > > > > drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- > > > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > > > > > index 8d5061b0794d..6202ce5a5ac2 100644 > > > > > --- a/drivers/cxl/core/bus.c > > > > > +++ b/drivers/cxl/core/bus.c > > > > > @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { > > > > > NULL, > > > > > }; > > > > > > > > > > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { > > > > > + &cxl_decoder_base_attribute_group, > > > > > + &cxl_base_attribute_group, > > > > > + NULL, > > > > > +}; > > > > > + > > > > > static void cxl_decoder_release(struct device *dev) > > > > > { > > > > > struct cxl_decoder *cxld = to_cxl_decoder(dev); > > > > > @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) > > > > > kfree(cxld); > > > > > } > > > > > > > > > > +static const struct device_type cxl_decoder_endpoint_type = { > > > > > + .name = "cxl_decoder_endpoint", > > > > > + .release = cxl_decoder_release, > > > > > + .groups = cxl_decoder_endpoint_attribute_groups, > > > > > +}; > > > > > + > > > > > static const struct device_type cxl_decoder_switch_type = { > > > > > .name = "cxl_decoder_switch", > > > > > .release = cxl_decoder_release, > > > > > @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { > > > > > .groups = cxl_decoder_root_attribute_groups, > > > > > }; > > > > > > > > > > +static bool is_endpoint_decoder(struct device *dev) > > > > > +{ > > > > > + return dev->type == &cxl_decoder_endpoint_type; > > > > > +} > > > > > + > > > > > bool is_root_decoder(struct device *dev) > > > > > { > > > > > return dev->type == &cxl_decoder_root_type; > > > > > @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > > > struct device *dev; > > > > > int rc = 0; > > > > > > > > > > - 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); > > > > > @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) > > > > > dev->parent = &port->dev; > > > > > dev->bus = &cxl_bus_type; > > > > > > > > > > + /* Endpoints don't have a target list */ > > > > > + if (nr_targets == 0) > > > > > + dev->type = &cxl_decoder_endpoint_type; > > > > > > > > Do you also plan to introduce the concept of endpoint ports, and if > > > > yes should that come before this patch? That would seem to be more > > > > robust than, for example, allowing a switch port to carry an endpoint > > > > decoder object as this allows. > > > > > > I didn't see a need as of yet to differentiate between endpoint ports and other > > > ports. I don't entirely understand what you mean by "allowing a switch port to > > > carry an endpoint decoder" means. Can you please elaborate? > > > > If endpoint ports were an explicit type then this check could make > > sure that someone did not pass nr_targets set to 0 where the @port > > argument is referring to a switch where the target_list must be > > specified. > > > > Either that, or a comment in kernel-doc for this routine about the > > special meaning of nr_targets == 0 and expected usage. > > Well, since Jonathan also brought up a concern here perhaps I should entertain > other ideas. I suppose future versions of the spec could break things, but as it > stands today the only CXL component that implements decoders that can have a 0 > value for this are endpoint devices (T2 or T3, or LD). I think it's fine to wait > until we have a second reason to make an endpoint port type and update kdocs for > now, but maybe it will also be a natural fit with a proper port driver. "Document only" sounds good to me.
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c index 8d5061b0794d..6202ce5a5ac2 100644 --- a/drivers/cxl/core/bus.c +++ b/drivers/cxl/core/bus.c @@ -175,6 +175,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = { NULL, }; +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = { + &cxl_decoder_base_attribute_group, + &cxl_base_attribute_group, + NULL, +}; + static void cxl_decoder_release(struct device *dev) { struct cxl_decoder *cxld = to_cxl_decoder(dev); @@ -184,6 +190,12 @@ static void cxl_decoder_release(struct device *dev) kfree(cxld); } +static const struct device_type cxl_decoder_endpoint_type = { + .name = "cxl_decoder_endpoint", + .release = cxl_decoder_release, + .groups = cxl_decoder_endpoint_attribute_groups, +}; + static const struct device_type cxl_decoder_switch_type = { .name = "cxl_decoder_switch", .release = cxl_decoder_release, @@ -196,6 +208,11 @@ static const struct device_type cxl_decoder_root_type = { .groups = cxl_decoder_root_attribute_groups, }; +static bool is_endpoint_decoder(struct device *dev) +{ + return dev->type == &cxl_decoder_endpoint_type; +} + bool is_root_decoder(struct device *dev) { return dev->type == &cxl_decoder_root_type; @@ -472,7 +489,7 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) struct device *dev; int rc = 0; - 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); @@ -491,8 +508,11 @@ struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port, int nr_targets) dev->parent = &port->dev; dev->bus = &cxl_bus_type; + /* Endpoints don't have a target list */ + if (nr_targets == 0) + dev->type = &cxl_decoder_endpoint_type; /* root ports do not have a cxl_port_type parent */ - if (port->dev.parent->type == &cxl_port_type) + else if (port->dev.parent->type == &cxl_port_type) dev->type = &cxl_decoder_switch_type; else dev->type = &cxl_decoder_root_type; @@ -532,9 +552,11 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, if (IS_ERR(cxld)) return PTR_ERR(cxld); + dev = &cxld->dev; + port = to_cxl_port(cxld->dev.parent); device_lock(&port->dev); - if (list_empty(&port->dports)) { + if (is_endpoint_decoder(dev) && list_empty(&port->dports)) { rc = -EINVAL; goto out_unlock; } @@ -551,7 +573,6 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, } device_unlock(&port->dev); - dev = &cxld->dev; rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id); if (rc) return rc;
Endpoints have decoders too. It is useful to share the same infrastructure from cxl_core. Endpoints do not have dports (downstream targets), only the underlying physical medium. As a result, some special casing is needed. There is no functional change introduced yet as endpoints don't actually enumerate decoders yet. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/core/bus.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)