Re: [PATCH 6/7] cxl/region: Fix 'distance' calculation with passthrough ports

[Dave Jiang <dave.jiang@intel.com>]

On 11/4/2022 11:59 AM, Dan Williams wrote:
> Verma, Vishal L wrote:
>> On Thu, 2022-11-03 at 17:30 -0700, Dan Williams wrote:
>>> When programming port decode targets, the algorithm wants to ensure that
>>> two devices are compatible to be programmed as peers beneath a given
>>> port. A compatible peer is a target that shares the same dport, and
>>> where that target's interleave position also routes it to the same
>>> dport. Compatibility is determined by the device's interleave position
>>> being >= to distance. For example, if a given dport can only map every
>>> Nth position then positions less than N away from the last target
>>> programmed are incompatible.
>>>
>>> The @distance for the host-bridge-cxl_port a simple dual-ported
>>                                     ^
>> Is this meant to be "the distance for the host-bridge to a cxl_port"?
> 
> No, but I will preface this explanation by admitting "distance" may not
> be the best term for what this value is describing. CXL decode routes to
> targets in a round robin fashion per-port. Take the diagram below:
> 
>   ┌───────────────────────────────────┬──┐
>   │WINDOW0                            │x2│
>   └─────────┬─────────────────┬───────┴──┘
>             │                 │
>   ┌─────────▼────┬──┐  ┌──────▼───────┬──┐
>   │HB0           │x2│  │HB1           │x2│
>   └──┬────────┬──┴──┘  └─┬─────────┬──┴──┘
>      │        │          │         │
>   ┌──▼─┬──┐ ┌─▼──┬──┐  ┌─▼──┬──┐ ┌─▼──┬──┐
>   │DEV0│x4│ │DEV1│x4│  │DEV2│x4│ │DEV3│x4│
>   └────┴──┘ └────┴──┘  └────┴──┘ └────┴──┘
>      0         2          1         3
> 
> ...where an x4 region is being established, and all the xN values are
> the interleave-ways settings for those ports/devices. The @distance
> value for that "HB0" port is 2. I.e. in order for 2 devices in that
> region to be mapped under HB0 they must be at position X and X+2 in the
> region.  The algorithm needs to be flexible to also allow this
> configuration:
> 
>   ┌───────────────────────────────────┬──┐
>   │WINDOW0                            │x2│
>   └─────────┬─────────────────┬───────┴──┘
>             │                 │
>   ┌─────────▼────┬──┐  ┌──────▼───────┬──┐
>   │HB0           │x2│  │HB1           │x2│
>   └──┬────────┬──┴──┘  └─┬─────────┬──┴──┘
>      │        │          │         │
>   ┌──▼─┬──┐ ┌─▼──┬──┐  ┌─▼──┬──┐ ┌─▼──┬──┐
>   │DEV3│x4│ │DEV2│x4│  │DEV1│x4│ │DEV0│x4│
>   └────┴──┘ └────┴──┘  └────┴──┘ └────┴──┘
>      3         1          2         0
> 
> ...and the algorithm can not know that a device is in the wrong position
> until trying to map the peer (like DEV0 and DEV1 are peers) into the
> decode. So "The @distance for the host-bridge-cxl_port" is referring to
> the value "2" for host-bridge-cxl_port:HB0 and host-bridge-cxl_port:HB1
> in the diagram.
> 
>> Also missing '/in/ a simple dual-ported..'?
> 
> Yes to this fixup though.
> 
>>
>>> host-bridge configuration with 2 direct-attached devices is 1. An x2
>>> region divded by 2 dports to reach 2 region targets.
>>
>> The second sentence seems slightly incomprehensible too.
> 
> Oh, I think I meant that to be s/. An/, i.e. an/:
> 
> "...host-bridge configuration with 2 direct-attached devices is 1, i.e.
> an x2 region divded by 2 dports to reach 2 region positions"

s/divded/divided/ ?

> 
> 
>>>
>>> An x4 region under an x2 host-bridge would need 2 intervening switches
>>> where the @distance at the host bridge level is 2 (x4 region divided by
>>> 2 switches to reach 4 devices).
>>>
>>> However, the distance between peers underneath a single ported
>>> host-bridge is always zero because there is no limit to the number of
>>> devices that can be mapped. In other words, there are no decoders to
>>> program in a passthrough, all descendants are mapped and distance only
>>> starts matters for the intervening descendant ports of the passthrough
>>
>> starts to matter?
> 
> s/starts matters/matters/
> 
>>
>>> port.
>>>
>>> Add tracking for the number of dports mapped to a port, and use that to
>>> detect the passthrough case for calculating @distance.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Reported-by: Bobo WL <lmw.bobo@gmail.com>
>>> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
>>> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> ---
>>>   drivers/cxl/core/port.c   |   11 +++++++++--
>>>   drivers/cxl/core/region.c |    9 ++++++++-
>>>   drivers/cxl/cxl.h         |    2 ++
>>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> Other than the above, looks good,
>>
>> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
>>
>>>
>>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>>> index bffde862de0b..e7556864ea80 100644
>>> --- a/drivers/cxl/core/port.c
>>> +++ b/drivers/cxl/core/port.c
>>> @@ -811,6 +811,7 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int id)
>>>   static int add_dport(struct cxl_port *port, struct cxl_dport *new)
>>>   {
>>>          struct cxl_dport *dup;
>>> +       int rc;
>>>   
>>>          device_lock_assert(&port->dev);
>>>          dup = find_dport(port, new->port_id);
>>> @@ -821,8 +822,14 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *new)
>>>                          dev_name(dup->dport));
>>>                  return -EBUSY;
>>>          }
>>> -       return xa_insert(&port->dports, (unsigned long)new->dport, new,
>>> -                        GFP_KERNEL);
>>> +
>>> +       rc = xa_insert(&port->dports, (unsigned long)new->dport, new,
>>> +                      GFP_KERNEL);
>>> +       if (rc)
>>> +               return rc;
>>> +
>>> +       port->nr_dports++;
>>> +       return 0;
>>>   }
>>>   
>>>   /*
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index c52465e09f26..c0253de74945 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -990,7 +990,14 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>>>          if (cxl_rr->nr_targets_set) {
>>>                  int i, distance;
>>>   
>>> -               distance = p->nr_targets / cxl_rr->nr_targets;
>>> +               /*
>>> +                * Passthrough ports impose no distance requirements between
>>> +                * peers
>>> +                */
>>> +               if (port->nr_dports == 1)
>>> +                       distance = 0;
>>> +               else
>>> +                       distance = p->nr_targets / cxl_rr->nr_targets;
>>>                  for (i = 0; i < cxl_rr->nr_targets_set; i++)
>>>                          if (ep->dport == cxlsd->target[i]) {
>>>                                  rc = check_last_peer(cxled, ep, cxl_rr,
>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>> index 1164ad49f3d3..ac75554b5d76 100644
>>> --- a/drivers/cxl/cxl.h
>>> +++ b/drivers/cxl/cxl.h
>>> @@ -457,6 +457,7 @@ struct cxl_pmem_region {
>>>    * @regions: cxl_region_ref instances, regions mapped by this port
>>>    * @parent_dport: dport that points to this port in the parent
>>>    * @decoder_ida: allocator for decoder ids
>>> + * @nr_dports: number of entries in @dports
>>>    * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>>>    * @commit_end: cursor to track highest committed decoder for commit ordering
>>>    * @component_reg_phys: component register capability base address (optional)
>>> @@ -475,6 +476,7 @@ struct cxl_port {
>>>          struct xarray regions;
>>>          struct cxl_dport *parent_dport;
>>>          struct ida decoder_ida;
>>> +       int nr_dports;
>>>          int hdm_end;
>>>          int commit_end;
>>>          resource_size_t component_reg_phys;
>>>
>>
> 
> 
>