Hi,
On Fri, Dec 19, 2025 at 10:39:49AM +0800, Ma Ke wrote:
[...]
From the discussion, I note two possible fix directions:
- Release the initial reference in etm_setup_aux() (current v2 patch)
- Modify the behavior of coresight_get_sink_by_id() itself so it
doesn't increase the reference count.
The option 2 is the right way to go.
To ensure the correctness of the v3 patch, I'd like to confirm which patch is preferred. If option 2 is the consensus, I'm happy to modify the implementation of coresight_get_sink_by_id() as suggested.
It is good to use a separate patch to fix coresight_find_device_by_fwnode() mentioned by James:
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode); - if (dev) - return dev;
/* * We have a configurable component - circle through the AMBA bus * looking for the device that matches the endpoint node. */ - return bus_find_device_by_fwnode(&amba_bustype, fwnode); + if (!dev) + dev = bus_find_device_by_fwnode(&amba_bustype, fwnode); + + put_device(dev); + return dev; }
/* @@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev,
of_node_put(rparent); of_node_put(rep); - put_device(rdev);
return ret; }
Thanks for working on this.
On Fri, Dec 19, 2025 at 11:48:35AM +0000, Suzuki K Poulose wrote:
[...]
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
I see, sorry. I saw some other uses of the device, but clearly I was wrong. May be we should simply re-structure the function to :
No worries and thanks for confirmation.
bool coresight_fwnode_device_present(fwnode) {
// find and drop the ref if required. return true/false; }
The name "find_device_by_fwnode" and returning a freed reference doesn't look good to me.
Renaming is good. Maybe use a separate patch to rename:
coresight_find_csdev_by_fwnode() -> coresight_fwnode_csdev_present()
Thanks, Leo
On 19/12/2025 09:41, Leo Yan wrote:
Hi,
On Fri, Dec 19, 2025 at 10:39:49AM +0800, Ma Ke wrote:
[...]
From the discussion, I note two possible fix directions:
- Release the initial reference in etm_setup_aux() (current v2 patch)
- Modify the behavior of coresight_get_sink_by_id() itself so it
doesn't increase the reference count.
The option 2 is the right way to go.
To ensure the correctness of the v3 patch, I'd like to confirm which patch is preferred. If option 2 is the consensus, I'm happy to modify the implementation of coresight_get_sink_by_id() as suggested.
It is good to use a separate patch to fix coresight_find_device_by_fwnode() mentioned by James:
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;/* * We have a configurable component - circle through the AMBA bus * looking for the device that matches the endpoint node. */
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; }
/* @@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
NAK for this.
Suzuki
return ret; }
Thanks for working on this.
On Fri, Dec 19, 2025 at 09:59:54AM +0000, Suzuki K Poulose wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;*/
- We have a configurable component - circle through the AMBA bus
- looking for the device that matches the endpoint node.
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; } /*
@@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
Thanks, Leo
On 19/12/2025 11:38, Leo Yan wrote:
On Fri, Dec 19, 2025 at 09:59:54AM +0000, Suzuki K Poulose wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;*/
- We have a configurable component - circle through the AMBA bus
- looking for the device that matches the endpoint node.
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; } /*
@@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
I see, sorry. I saw some other uses of the device, but clearly I was wrong. May be we should simply re-structure the function to :
bool coresight_fwnode_device_present(fwnode) {
// find and drop the ref if required. return true/false; }
The name "find_device_by_fwnode" and returning a freed reference doesn't look good to me.
Suzuki
Thanks, Leo
-
Leo Yan -
Suzuki K Poulose