Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 19 May 2025 09:33:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 5/9/25 2:20 AM, Alejandro Lucero Palau wrote:
> > 
> > On 5/7/25 01:43, Dave Jiang wrote:  
> >> Rename find_dport() to find_dport_by_num() to indicate that the function
> >> is trying to match a dport by its hardware number index.
> >>
> >> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>   drivers/cxl/core/port.c | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index e9c02e4d0d4c..1d7a4a2ef6ad 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
> >>   }
> >>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
> >>   -static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
> >> +static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
> >>   {
> >>       struct cxl_dport *dport;
> >>       unsigned long index;
> >> @@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
> >>       int rc;
> >>         device_lock_assert(&port->dev);
> >> -    dup = find_dport(port, dport->port_num);
> >> +    dup = find_dport_by_num(port, dport->port_num);
> >>       if (dup) {
> >>           dev_err(&port->dev,
> >>               "unable to add dport%d-%s non-unique port num (%s)\n",
> >> @@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
> >>    * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
> >>    * @port: the cxl_port that references this dport
> >>    * @dport_dev: firmware or PCI device representing the dport
> >> - * @port_id: identifier for this dport in a decoder's target list
> >> + * @port_num: identifier for this dport in a decoder's target list  
> > 
> > 
> > Not sure this change should be in this patch. It makes more sense to me in the previous one.
> > 
> > 
> > Also, maybe adding some reference for easily seeing where the identifier comes from:
> > 
> > + * @port_num: hardware identifier for this dport in a decoder's target list  
> 
> In the process of doing that, I realized that this entire comment line gets dropped in a later patch because we remove the port_num parameter. So probably no need to change since it goes away entirely.
>
Even though it's only a comment, if you are changing the name in
the function let's avoid the trivial issue of the kernel-doc
warning that will show up.  Make sure those are kept inline.

I'll assume you'll tidy this up one way or another - rest of patch is fine

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> DJ
> 
> > 
> >   
> >>    * @rcrb: mandatory location of a Root Complex Register Block
> >>    *
> >>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
> >>    */
> >>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> >> -                     struct device *dport_dev, int port_id,
> >> +                     struct device *dport_dev, int port_num,
> >>                        resource_size_t rcrb)
> >>   {
> >>       struct cxl_dport *dport;
> >> @@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> >>           return ERR_PTR(-EINVAL);
> >>       }
> >>   -    dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> >> +    dport = __devm_cxl_add_dport(port, dport_dev, port_num,
> >>                        CXL_RESOURCE_NONE, rcrb);
> >>       if (IS_ERR(dport)) {
> >>           dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> >> @@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
> >>         guard(rwsem_write)(&cxl_region_rwsem);
> >>       for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
> >> -        struct cxl_dport *dport = find_dport(port, target_map[i]);
> >> +        struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
> >>             if (!dport)
> >>               return -ENXIO;  
> 
>