| Message ID | 20230112042104.4107253-2-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > From: Prashant Malani <pmalani@chromium.org> > > 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 <pmalani@chromium.org> > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > Tested-by: Chen-Yu Tsai <wenst@chromium.org> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > > 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
Hi Pin-yen, On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > From: Prashant Malani <pmalani@chromium.org> > > 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 <pmalani@chromium.org> > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > Tested-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > 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);
HI Sakari, On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Pin-yen, > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > > From: Prashant Malani <pmalani@chromium.org> > > + /* > > + * 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
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote: > On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > > > From: Prashant Malani <pmalani@chromium.org> ... > > > + /* > > > + * 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.
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 > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Pin-yen, > > > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > > > From: Prashant Malani <pmalani@chromium.org> > > > + /* > > > + * 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
On Mon, Jan 16, 2023 at 5:07 AM Sakari Ailus <sakari.ailus@linux.intel.com> 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 > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Pin-yen, > > > > > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote: > > > > From: Prashant Malani <pmalani@chromium.org> > > > > + /* > > > > + * 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
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);