netdev - Re: [PATCH v4 1/2] cxl/region: Find free cxl decoder by device_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6dae308-ff34-4479-a638-8c12ff2e8d32@quicinc.com>
Date: Tue, 10 Sep 2024 11:17:22 +0800
From: quic_zijuhu <quic_zijuhu@...cinc.com>
To: Dan Williams <dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Zijun Hu
	<zijun_hu@...oud.com>
CC: Davidlohr Bueso <dave@...olabs.net>,
        Jonathan Cameron
	<jonathan.cameron@...wei.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Alison
 Schofield <alison.schofield@...el.com>,
        Vishal Verma
	<vishal.l.verma@...el.com>,
        Timur Tabi <timur@...nel.org>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, <linux-cxl@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] cxl/region: Find free cxl decoder by
 device_for_each_child()

On 9/10/2024 8:45 AM, Dan Williams wrote:
> Ira Weiny wrote:
> [..]
>>> This still feels more complex that I think it should be.  Why not just
>>> modify the needed device information after the device is found?  What
>>> exactly is being changed in the match_free_decoder that needs to keep
>>> "state"?  This feels odd.
>>
>> Agreed it is odd.
>>
>> How about adding?
> 
> I would prefer just dropping usage of device_find_ or device_for_each_
> with storing an array decoders in the port directly. The port already
> has arrays for dports , endpoints, and regions. Using the "device" APIs
> to iterate children was a bit lazy, and if the id is used as the array
> key then a direct lookup makes some cases simpler.

it seems Ira and Dan have corrected original logic to ensure
that all child decoders are sorted by ID in ascending order as shown
by below link.

https://lore.kernel.org/all/66df666ded3f7_3c80f229439@iweiny-mobl.notmuch/

based on above correction, as shown by my another exclusive fix
https://lore.kernel.org/all/20240905-fix_cxld-v2-1-51a520a709e4@quicinc.com/
there are a very simple change to solve the remaining original concern
that device_find_child() modifies caller's match data.

here is the simple change.

--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -797,23 +797,13 @@ static size_t show_targetN(struct cxl_region
*cxlr, char *buf, int pos)
 static int match_free_decoder(struct device *dev, void *data)
 {
        struct cxl_decoder *cxld;
-       int *id = data;

        if (!is_switch_decoder(dev))
                return 0;

        cxld = to_cxl_decoder(dev);

-       /* enforce ordered allocation */
-       if (cxld->id != *id)
-               return 0;
-
-       if (!cxld->region)
-               return 1;
-
-       (*id)++;
-
-       return 0;
+       return cxld->region ? 0 : 1;
 }

 static int match_auto_decoder(struct device *dev, void *data)
@@ -840,7 +830,6 @@ cxl_region_find_decoder(struct cxl_port *port,
                        struct cxl_region *cxlr)
 {
        struct device *dev;
-       int id = 0;

        if (port == cxled_to_port(cxled))
                return &cxled->cxld;
@@ -849,7 +838,7 @@ cxl_region_find_decoder(struct cxl_port *port,
                dev = device_find_child(&port->dev, &cxlr->params,
                                        match_auto_decoder);
        else
-               dev = device_find_child(&port->dev, &id,
match_free_decoder);
+               dev = device_find_child(&port->dev, NULL,
match_free_decoder);
        if (!dev)
                return NULL;