Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
On Mon, Jan 16, 2023 at 5:07 AM Sakari Ailus
wrote:
>
> Hi Prashant,
>
> On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> > HI Sakari,
> >
> > On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> > wrote:
> > >
> > > Hi Pin-yen,
> > >
> > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > > From: Prashant Malani
> > > > + /*
> > > > + * Some drivers may register devices for endpoints. Check
> > > > + * the remote-endpoints for matches in addition to the
> > > > remote
> > > > + * port parent.
> > > > + */
> > > > + node = fwnode_graph_get_remote_endpoint(ep);
> > > > + if (fwnode_device_is_available(node)) {
> > > > + ret = match(node, con_id, data);
> > > > + if (ret) {
> > > > + if (matches)
> > > > + matches[count] = ret;
> > > > + count++;
> > > > + }
> > > > + }
> > >
> > > Aren't you missing fwnode_handle-put(node) here??
> >
> > It shouldn't be necessary. We aren't break-ing/continue-ing here,
> > and fwnode_handle_put(node) is called latter in the loop [1][2]
>
> It is, but node is overwritten just below this chunk --- before
> fwnode_handle_put() is called on it.
Ack. Thanks for pointing that out. My bad!
Pin-yen, please make this update when you send out a v11.
BR,
-Prashant
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
Hi Prashant,
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> HI Sakari,
>
> On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> wrote:
> >
> > Hi Pin-yen,
> >
> > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani
> > > + /*
> > > + * Some drivers may register devices for endpoints. Check
> > > + * the remote-endpoints for matches in addition to the
> > > remote
> > > + * port parent.
> > > + */
> > > + node = fwnode_graph_get_remote_endpoint(ep);
> > > + if (fwnode_device_is_available(node)) {
> > > + ret = match(node, con_id, data);
> > > + if (ret) {
> > > + if (matches)
> > > + matches[count] = ret;
> > > + count++;
> > > + }
> > > + }
> >
> > Aren't you missing fwnode_handle-put(node) here??
>
> It shouldn't be necessary. We aren't break-ing/continue-ing here,
> and fwnode_handle_put(node) is called latter in the loop [1][2]
It is, but node is overwritten just below this chunk --- before
fwnode_handle_put() is called on it.
>
> BR,
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
--
Regards,
Sakari Ailus
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> wrote:
> > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani
...
> > > + /*
> > > + * Some drivers may register devices for endpoints. Check
> > > + * the remote-endpoints for matches in addition to the
> > > remote
> > > + * port parent.
> > > + */
> > > + node = fwnode_graph_get_remote_endpoint(ep);
> > > + if (fwnode_device_is_available(node)) {
> > > + ret = match(node, con_id, data);
> > > + if (ret) {
> > > + if (matches)
> > > + matches[count] = ret;
> > > + count++;
> > > + }
> > > + }
> >
> > Aren't you missing fwnode_handle-put(node) here??
>
> It shouldn't be necessary. We aren't break-ing/continue-ing here,
> and fwnode_handle_put(node) is called latter in the loop [1][2]
>
> BR,
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
I'm really puzzled what do you mean by all this.
Sakari is right, btw.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
HI Sakari,
On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
wrote:
>
> Hi Pin-yen,
>
> On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > From: Prashant Malani
> > + /*
> > + * Some drivers may register devices for endpoints. Check
> > + * the remote-endpoints for matches in addition to the remote
> > + * port parent.
> > + */
> > + node = fwnode_graph_get_remote_endpoint(ep);
> > + if (fwnode_device_is_available(node)) {
> > + ret = match(node, con_id, data);
> > + if (ret) {
> > + if (matches)
> > + matches[count] = ret;
> > + count++;
> > + }
> > + }
>
> Aren't you missing fwnode_handle-put(node) here??
It shouldn't be necessary. We aren't break-ing/continue-ing here,
and fwnode_handle_put(node) is called latter in the loop [1][2]
BR,
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
Hi Pin-yen,
On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ static unsigned int fwnode_graph_devcon_matches(struct
> fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
> + if (fwnode_device_is_available(node)) {
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
Aren't you missing fwnode_handle-put(node) here??
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
--
Kind regards,
Sakari Ailus
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
Acked-by: Heikki Krogerus
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ static unsigned int fwnode_graph_devcon_matches(struct
> fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
> + if (fwnode_device_is_available(node)) {
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
> --
> 2.39.0.314.g84b9a713c41-goog
--
heikki
[PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
From: Prashant Malani
When searching the device graph for device matches, check the
remote-endpoint itself for a match.
Some drivers register devices for individual endpoints. This allows
the matcher code to evaluate those for a match too, instead
of only looking at the remote parent devices. This is required when a
device supports two mode switches in its endpoints, so we can't simply
register the mode switch with the parent node.
Signed-off-by: Prashant Malani
Signed-off-by: Pin-yen Lin
Reviewed-by: Chen-Yu Tsai
Tested-by: Chen-Yu Tsai
---
Changes in v10:
- Collected Reviewed-by and Tested-by tags
Changes in v6:
- New in v6
drivers/base/property.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2a5a37fcd998..48877af4e444 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1223,6 +1223,21 @@ static unsigned int fwnode_graph_devcon_matches(struct
fwnode_handle *fwnode,
break;
}
+ /*
+* Some drivers may register devices for endpoints. Check
+* the remote-endpoints for matches in addition to the remote
+* port parent.
+*/
+ node = fwnode_graph_get_remote_endpoint(ep);
+ if (fwnode_device_is_available(node)) {
+ ret = match(node, con_id, data);
+ if (ret) {
+ if (matches)
+ matches[count] = ret;
+ count++;
+ }
+ }
+
node = fwnode_graph_get_remote_port_parent(ep);
if (!fwnode_device_is_available(node)) {
fwnode_handle_put(node);
--
2.39.0.314.g84b9a713c41-goog
