diff mbox

[01/23] drm/sun4i: Implement endpoint parsing using kfifo

Message ID 4ecb323e787918208f6a5d9f0ebba12c62583c98.1508231063.git-series.maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard Oct. 17, 2017, 9:06 a.m. UTC
The commit da82b8785eeb ("drm/sun4i: add components in breadth first
traversal order") implemented a breadth first traversal of our device tree
nodes graph. However, it was relying on the kernel linked lists, and those
are not really safe for addition.

Indeed, in a single pipeline stage, your first stage (ie, the mixer or
fronted) will be queued, and it will be the final iteration of that list as
far as list_for_each_entry_safe is concerned. Then, during that final
iteration, we'll queue another element (the TCON or the backend) that
list_for_each_entry_safe will not account for, and we will leave the loop
without having iterated over all the elements. And since we won't have
built our components list properly, the DRM driver will be left
non-functional.

We can instead use a kfifo to queue and enqueue components in-order, as was
the original intention. This also has the benefit of removing any dynamic
allocation, making the error handling path simpler too. The only thing
we're losing is the ability to tell whether an element has already been
queued, but that was only needed to remove spurious logs, and therefore
purely cosmetic.

This means that this commit effectively reverses e8afb7b67fba ("drm/sun4i:
don't add components that are already in the queue").

Fixes: da82b8785eeb ("drm/sun4i: add components in breadth first traversal order")
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 71 +++++---------------------------
 1 file changed, 13 insertions(+), 58 deletions(-)

Comments

Chen-Yu Tsai Oct. 17, 2017, 9:19 a.m. UTC | #1
On Tue, Oct 17, 2017 at 5:06 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The commit da82b8785eeb ("drm/sun4i: add components in breadth first
> traversal order") implemented a breadth first traversal of our device tree
> nodes graph. However, it was relying on the kernel linked lists, and those
> are not really safe for addition.
>
> Indeed, in a single pipeline stage, your first stage (ie, the mixer or
> fronted) will be queued, and it will be the final iteration of that list as
> far as list_for_each_entry_safe is concerned. Then, during that final
> iteration, we'll queue another element (the TCON or the backend) that
> list_for_each_entry_safe will not account for, and we will leave the loop
> without having iterated over all the elements. And since we won't have
> built our components list properly, the DRM driver will be left
> non-functional.
>
> We can instead use a kfifo to queue and enqueue components in-order, as was
> the original intention. This also has the benefit of removing any dynamic
> allocation, making the error handling path simpler too. The only thing
> we're losing is the ability to tell whether an element has already been
> queued, but that was only needed to remove spurious logs, and therefore
> purely cosmetic.
>
> This means that this commit effectively reverses e8afb7b67fba ("drm/sun4i:
> don't add components that are already in the queue").
>
> Fixes: da82b8785eeb ("drm/sun4i: add components in breadth first traversal order")
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_drv.c | 71 +++++---------------------------
>  1 file changed, 13 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index b5879d4620d8..a27efad9bc76 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -11,6 +11,7 @@
>   */
>
>  #include <linux/component.h>
> +#include <linux/kfifo.h>
>  #include <linux/of_graph.h>
>  #include <linux/of_reserved_mem.h>
>
> @@ -222,29 +223,15 @@ static int compare_of(struct device *dev, void *data)
>   * matching system handles this for us.
>   */
>  struct endpoint_list {
> -       struct device_node *node;
> -       struct list_head list;
> +       DECLARE_KFIFO(fifo, struct device_node *, 16);
>  };

Is there any reason to keep using struct endpoint_list, other than
to avoid using kfifo in function parameter lists?

Otherwise the rest of the code looks sound.

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Maxime Ripard Oct. 17, 2017, 2:29 p.m. UTC | #2
On Tue, Oct 17, 2017 at 05:19:04PM +0800, Chen-Yu Tsai wrote:
> On Tue, Oct 17, 2017 at 5:06 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The commit da82b8785eeb ("drm/sun4i: add components in breadth first
> > traversal order") implemented a breadth first traversal of our device tree
> > nodes graph. However, it was relying on the kernel linked lists, and those
> > are not really safe for addition.
> >
> > Indeed, in a single pipeline stage, your first stage (ie, the mixer or
> > fronted) will be queued, and it will be the final iteration of that list as
> > far as list_for_each_entry_safe is concerned. Then, during that final
> > iteration, we'll queue another element (the TCON or the backend) that
> > list_for_each_entry_safe will not account for, and we will leave the loop
> > without having iterated over all the elements. And since we won't have
> > built our components list properly, the DRM driver will be left
> > non-functional.
> >
> > We can instead use a kfifo to queue and enqueue components in-order, as was
> > the original intention. This also has the benefit of removing any dynamic
> > allocation, making the error handling path simpler too. The only thing
> > we're losing is the ability to tell whether an element has already been
> > queued, but that was only needed to remove spurious logs, and therefore
> > purely cosmetic.
> >
> > This means that this commit effectively reverses e8afb7b67fba ("drm/sun4i:
> > don't add components that are already in the queue").
> >
> > Fixes: da82b8785eeb ("drm/sun4i: add components in breadth first traversal order")
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_drv.c | 71 +++++---------------------------
> >  1 file changed, 13 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > index b5879d4620d8..a27efad9bc76 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> > @@ -11,6 +11,7 @@
> >   */
> >
> >  #include <linux/component.h>
> > +#include <linux/kfifo.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/of_reserved_mem.h>
> >
> > @@ -222,29 +223,15 @@ static int compare_of(struct device *dev, void *data)
> >   * matching system handles this for us.
> >   */
> >  struct endpoint_list {
> > -       struct device_node *node;
> > -       struct list_head list;
> > +       DECLARE_KFIFO(fifo, struct device_node *, 16);
> >  };
> 
> Is there any reason to keep using struct endpoint_list, other than
> to avoid using kfifo in function parameter lists?

It appears that you can't just pass a kifo pointer as a function
argument, so the only two remaining solutions were to have a global
pointer or embed it in a structure. It just seems like the best
solution.

> 
> Otherwise the rest of the code looks sound.
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime
diff mbox

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index b5879d4620d8..a27efad9bc76 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -11,6 +11,7 @@ 
  */
 
 #include <linux/component.h>
+#include <linux/kfifo.h>
 #include <linux/of_graph.h>
 #include <linux/of_reserved_mem.h>
 
@@ -222,29 +223,15 @@  static int compare_of(struct device *dev, void *data)
  * matching system handles this for us.
  */
 struct endpoint_list {
-	struct device_node *node;
-	struct list_head list;
+	DECLARE_KFIFO(fifo, struct device_node *, 16);
 };
 
-static bool node_is_in_list(struct list_head *endpoints,
-			    struct device_node *node)
-{
-	struct endpoint_list *endpoint;
-
-	list_for_each_entry(endpoint, endpoints, list)
-		if (endpoint->node == node)
-			return true;
-
-	return false;
-}
-
 static int sun4i_drv_add_endpoints(struct device *dev,
-				   struct list_head *endpoints,
+				   struct endpoint_list *list,
 				   struct component_match **match,
 				   struct device_node *node)
 {
 	struct device_node *port, *ep, *remote;
-	struct endpoint_list *endpoint;
 	int count = 0;
 
 	/*
@@ -304,19 +291,7 @@  static int sun4i_drv_add_endpoints(struct device *dev,
 			}
 		}
 
-		/* skip downstream node if it is already in the queue */
-		if (node_is_in_list(endpoints, remote))
-			continue;
-
-		/* Add downstream nodes to the queue */
-		endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
-		if (!endpoint) {
-			of_node_put(remote);
-			return -ENOMEM;
-		}
-
-		endpoint->node = remote;
-		list_add_tail(&endpoint->list, endpoints);
+		kfifo_put(&list->fifo, remote);
 	}
 
 	return count;
@@ -325,10 +300,11 @@  static int sun4i_drv_add_endpoints(struct device *dev,
 static int sun4i_drv_probe(struct platform_device *pdev)
 {
 	struct component_match *match = NULL;
-	struct device_node *np = pdev->dev.of_node;
-	struct endpoint_list *endpoint, *endpoint_temp;
+	struct device_node *np = pdev->dev.of_node, *endpoint;
+	struct endpoint_list list;
 	int i, ret, count = 0;
-	LIST_HEAD(endpoints);
+
+	INIT_KFIFO(list.fifo);
 
 	for (i = 0;; i++) {
 		struct device_node *pipeline = of_parse_phandle(np,
@@ -337,31 +313,19 @@  static int sun4i_drv_probe(struct platform_device *pdev)
 		if (!pipeline)
 			break;
 
-		endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL);
-		if (!endpoint) {
-			ret = -ENOMEM;
-			goto err_free_endpoints;
-		}
-
-		endpoint->node = pipeline;
-		list_add_tail(&endpoint->list, &endpoints);
+		kfifo_put(&list.fifo, pipeline);
 	}
 
-	list_for_each_entry_safe(endpoint, endpoint_temp, &endpoints, list) {
+	while (kfifo_get(&list.fifo, &endpoint)) {
 		/* process this endpoint */
-		ret = sun4i_drv_add_endpoints(&pdev->dev, &endpoints, &match,
-					      endpoint->node);
+		ret = sun4i_drv_add_endpoints(&pdev->dev, &list, &match,
+					      endpoint);
 
 		/* sun4i_drv_add_endpoints can fail to allocate memory */
 		if (ret < 0)
-			goto err_free_endpoints;
+			return ret;
 
 		count += ret;
-
-		/* delete and cleanup the current entry */
-		list_del(&endpoint->list);
-		of_node_put(endpoint->node);
-		kfree(endpoint);
 	}
 
 	if (count)
@@ -370,15 +334,6 @@  static int sun4i_drv_probe(struct platform_device *pdev)
 						       match);
 	else
 		return 0;
-
-err_free_endpoints:
-	list_for_each_entry_safe(endpoint, endpoint_temp, &endpoints, list) {
-		list_del(&endpoint->list);
-		of_node_put(endpoint->node);
-		kfree(endpoint);
-	}
-
-	return ret;
 }
 
 static int sun4i_drv_remove(struct platform_device *pdev)