diff mbox

drm/sun4i: Fix error handling

Message ID 20161030084926.15303-1-christophe.jaillet@wanadoo.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Christophe JAILLET Oct. 30, 2016, 8:49 a.m. UTC
'sun4i_layers_init()' returns an error pointer in case of error, not
NULL. So test it with IS_ERR.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christophe JAILLET Nov. 5, 2016, 6:15 a.m. UTC | #1
Le 02/11/2016 à 19:14, Maxime Ripard a écrit :
> Hi,
>
> On Sun, Oct 30, 2016 at 12:53:02PM +0100, Christophe JAILLET wrote:
>> BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially
>> the use of 'layer' in the for loop.
>> Just my 2 cents.
> What do you mean by it's spurious?
Hi Maxime,

what I mean is:

 > struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
 > {
 >     struct sun4i_drv *drv = drm->dev_private;
 >     struct sun4i_layer **layers;
 >     int i;
 >
 >     layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
 >                   sizeof(**layers), GFP_KERNEL);
Here, we allocate some memory for ARRAY_SIZE(sun4i_backend_planes) (i.e. 
2) 'struct sun4i_layer'.
We do not allocate some space for some pointers but for some structure.

Also, these structures are zeroed and seem to never be initialized by 
anything else.

 >     if (!layers)
 >         return ERR_PTR(-ENOMEM);
 >
 >     /*
 >      * The hardware is a bit unusual here.
 >      *
 >      * Even though it supports 4 layers, it does the composition
 >      * in two separate steps.
 >      *
 >      * The first one is assigning a layer to one of its two
 >      * pipes. If more that 1 layer is assigned to the same pipe,
 >      * and if pixels overlaps, the pipe will take the pixel from
 >      * the layer with the highest priority.
 >      *
 >      * The second step is the actual alpha blending, that takes
 >      * the two pipes as input, and uses the eventual alpha
 >      * component to do the transparency between the two.
 >      *
 >      * This two steps scenario makes us unable to guarantee a
 >      * robust alpha blending between the 4 layers in all
 >      * situations. So we just expose two layers, one per pipe. On
 >      * SoCs that support it, sprites could fill the need for more
 >      * layers.
 >      */
The comment make me think that this driver (and this function) only 
handles 2 layers ("So we just expose two layers"), which is consistent 
with ARRAY_SIZE(sun4i_backend_planes) (i.e. 2)
So I would expect that only 2 'struct sun4i_layer' to be allcoated

 >     for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
 >         const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
 >         struct sun4i_layer *layer = layers[i];
Here, we take the address of one of the 2 structure allocated above.
This is overridden, just the line after.

 >
 >         layer = sun4i_layer_init_one(drm, plane);
'sun4i_layer_init_one()' looks() like:

     struct sun4i_layer *layer;
     layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
     ...
     return layer;

So we once more allocate some 'struct sun4i_layer'

BUT, the corresponding address is stored into the 'layer' variable, and 
finally seems to get lost and no reference is kept of this. (i.e. 
'layers' (with an s) is left unchanged)

 >         if (IS_ERR(layer)) {
 >             dev_err(drm->dev, "Couldn't initialize %s plane\n",
 >                 i ? "overlay" : "primary");
 >             return ERR_CAST(layer);
 >         };
 >
 >         DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
 >                  i ? "overlay" : "primary", plane->pipe);
 >         regmap_update_bits(drv->backend->regs, 
SUN4I_BACKEND_ATTCTL_REG0(i),
 >                    SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK,
 > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe));
 >
 >         layer->id = i;
 >     };
 >
 >     return layers;
 > }


So, 4 'struct sun4i_layer' have been allocated. 2 in 
'sun4i_layers_init()' and 2 in 'sun4i_layer_init_one()' (once at a time, 
but called twice)

What looks spurious to me is either:
    - 'struct sun4i_layer *layer = layers[i];' which should just be 
'struct sun4i_layer *layer;'
or
    - 'layers' (with an s) should be an array of pointers and the 
addresses returned by 'sun4i_layer_init_one()' should be saved there.


I don't know at all this driver, so I'm maybe completely wrong.
What I would have expected would be something like: (un-tested, just to 
give an idea)


==============8<================================================

@@ -133,9 +133,9 @@ struct sun4i_layer **sun4i_layers_init(struct 
drm_device *drm)
      struct sun4i_layer **layers;
      int i;

      layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
-                  sizeof(**layers), GFP_KERNEL);
+                  sizeof(*layers), GFP_KERNEL);
      if (!layers)
          return ERR_PTR(-ENOMEM);

      /*
@@ -160,16 +160,17 @@ struct sun4i_layer **sun4i_layers_init(struct 
drm_device *drm)
       * layers.
       */
      for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
          const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
-        struct sun4i_layer *layer = layers[i];
+        struct sun4i_layer *layer;

          layer = sun4i_layer_init_one(drm, plane);
          if (IS_ERR(layer)) {
              dev_err(drm->dev, "Couldn't initialize %s plane\n",
                  i ? "overlay" : "primary");
              return ERR_CAST(layer);
          };
+        layers[i] = layer;

          DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
                   i ? "overlay" : "primary", plane->pipe);
          regmap_update_bits(drv->backend->regs, 
SUN4I_BACKEND_ATTCTL_REG0(i),


Best regards,
CJ
Maxime Ripard Nov. 8, 2016, 8:38 p.m. UTC | #2
Salut,

On Sat, Nov 05, 2016 at 07:15:45AM +0100, Christophe JAILLET wrote:
> Le 02/11/2016 à 19:14, Maxime Ripard a écrit :
> > Hi,
> > 
> > On Sun, Oct 30, 2016 at 12:53:02PM +0100, Christophe JAILLET wrote:
> > > BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially
> > > the use of 'layer' in the for loop.
> > > Just my 2 cents.
> > What do you mean by it's spurious?
> Hi Maxime,
> 
> what I mean is:
> 
> > struct sun4i_layer **sun4i_layers_init(struct drm_device *drm)
> > {
> >     struct sun4i_drv *drv = drm->dev_private;
> >     struct sun4i_layer **layers;
> >     int i;
> >
> >     layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
> >                   sizeof(**layers), GFP_KERNEL);
> Here, we allocate some memory for ARRAY_SIZE(sun4i_backend_planes) (i.e. 2)
> 'struct sun4i_layer'.
> We do not allocate some space for some pointers but for some structure.
> 
> Also, these structures are zeroed and seem to never be initialized by
> anything else.
> 
> >     if (!layers)
> >         return ERR_PTR(-ENOMEM);
> >
> >     /*
> >      * The hardware is a bit unusual here.
> >      *
> >      * Even though it supports 4 layers, it does the composition
> >      * in two separate steps.
> >      *
> >      * The first one is assigning a layer to one of its two
> >      * pipes. If more that 1 layer is assigned to the same pipe,
> >      * and if pixels overlaps, the pipe will take the pixel from
> >      * the layer with the highest priority.
> >      *
> >      * The second step is the actual alpha blending, that takes
> >      * the two pipes as input, and uses the eventual alpha
> >      * component to do the transparency between the two.
> >      *
> >      * This two steps scenario makes us unable to guarantee a
> >      * robust alpha blending between the 4 layers in all
> >      * situations. So we just expose two layers, one per pipe. On
> >      * SoCs that support it, sprites could fill the need for more
> >      * layers.
> >      */
> The comment make me think that this driver (and this function) only handles
> 2 layers ("So we just expose two layers"), which is consistent with
> ARRAY_SIZE(sun4i_backend_planes) (i.e. 2)
> So I would expect that only 2 'struct sun4i_layer' to be allcoated
> 
> >     for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
> >         const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
> >         struct sun4i_layer *layer = layers[i];
> Here, we take the address of one of the 2 structure allocated above.
> This is overridden, just the line after.
> 
> >
> >         layer = sun4i_layer_init_one(drm, plane);
> 'sun4i_layer_init_one()' looks() like:
> 
>     struct sun4i_layer *layer;
>     layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
>     ...
>     return layer;
> 
> So we once more allocate some 'struct sun4i_layer'
> 
> BUT, the corresponding address is stored into the 'layer' variable, and
> finally seems to get lost and no reference is kept of this. (i.e. 'layers'
> (with an s) is left unchanged)
> 
> >         if (IS_ERR(layer)) {
> >             dev_err(drm->dev, "Couldn't initialize %s plane\n",
> >                 i ? "overlay" : "primary");
> >             return ERR_CAST(layer);
> >         };
> >
> >         DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
> >                  i ? "overlay" : "primary", plane->pipe);
> >         regmap_update_bits(drv->backend->regs,
> SUN4I_BACKEND_ATTCTL_REG0(i),
> >                    SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK,
> > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe));
> >
> >         layer->id = i;
> >     };
> >
> >     return layers;
> > }
> 
> 
> So, 4 'struct sun4i_layer' have been allocated. 2 in 'sun4i_layers_init()'
> and 2 in 'sun4i_layer_init_one()' (once at a time, but called twice)
> 
> What looks spurious to me is either:
>    - 'struct sun4i_layer *layer = layers[i];' which should just be 'struct
> sun4i_layer *layer;'
> or
>    - 'layers' (with an s) should be an array of pointers and the addresses
> returned by 'sun4i_layer_init_one()' should be saved there.
> 
> 
> I don't know at all this driver, so I'm maybe completely wrong.
> What I would have expected would be something like: (un-tested, just to give
> an idea)
> 
> 
> ==============8<================================================
> 
> @@ -133,9 +133,9 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device
> *drm)
>      struct sun4i_layer **layers;
>      int i;
> 
>      layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes),
> -                  sizeof(**layers), GFP_KERNEL);
> +                  sizeof(*layers), GFP_KERNEL);
>      if (!layers)
>          return ERR_PTR(-ENOMEM);
> 
>      /*
> @@ -160,16 +160,17 @@ struct sun4i_layer **sun4i_layers_init(struct
> drm_device *drm)
>       * layers.
>       */
>      for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
>          const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
> -        struct sun4i_layer *layer = layers[i];
> +        struct sun4i_layer *layer;
> 
>          layer = sun4i_layer_init_one(drm, plane);
>          if (IS_ERR(layer)) {
>              dev_err(drm->dev, "Couldn't initialize %s plane\n",
>                  i ? "overlay" : "primary");
>              return ERR_CAST(layer);
>          };
> +        layers[i] = layer;
> 
>          DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
>                   i ? "overlay" : "primary", plane->pipe);
>          regmap_update_bits(drv->backend->regs,
> SUN4I_BACKEND_ATTCTL_REG0(i),

You're totally right. Can you send this as a formal patch?

Thanks!
Maxime
diff mbox

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 9776f0305834..628712e6edd6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -143,7 +143,7 @@  static int sun4i_drv_bind(struct device *dev)
 
 	/* Create our layers */
 	drv->layers = sun4i_layers_init(drm);
-	if (!drv->layers) {
+	if (IS_ERR(drv->layers)) {
 		dev_err(drm->dev, "Couldn't create the planes\n");
 		ret = -EINVAL;
 		goto free_drm;