[Nouveau] [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Ilia Mirkin]

On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow at free.fr> wrote:
> Currently, every backlight interface created by Nouveau uses the same name,
> nv_backlight. This leads to a sysfs warning as it tries to create an already
> existing folder. This patch adds a incremented number to the name, but keeps
> the initial name as nv_backlight, to avoid possibly breaking userspace; the
> second interface will be named nv_backlight1, and so on.
>
> Fixes: fdo#86539
> Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
> ---
>  drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> index 89eb460..914e2cb 100644
> --- a/drm/nouveau/nouveau_backlight.c
> +++ b/drm/nouveau/nouveau_backlight.c
> @@ -36,6 +36,10 @@
>  #include "nouveau_reg.h"
>  #include "nouveau_encoder.h"
>
> +static atomic_t bl_interfaces_nb = { 0 };

static data is initialized to 0, this should be unnecessary.

I'd also call it "backlights" or something like that. No need for
multiple words...

> +
> +static char* nouveau_get_backlight_name(void);

Please organize the code to avoid forward declarations.

> +
>  static int
>  nv40_get_intensity(struct backlight_device *bd)
>  {
> @@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector)
>         struct nvif_object *device = &drm->device.object;
>         struct backlight_properties props;
>         struct backlight_device *bd;
> +       char* backlight_name = NULL;
>
>         if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
>                 return 0;
> @@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector)
>         memset(&props, 0, sizeof(struct backlight_properties));
>         props.type = BACKLIGHT_RAW;
>         props.max_brightness = 31;
> -       bd = backlight_device_register("nv_backlight", connector->kdev, drm,
> +       backlight_name = nouveau_get_backlight_name();
> +       bd = backlight_device_register(backlight_name , connector->kdev, drm,
>                                        &nv40_bl_ops, &props);
> +
> +       // backlight_device_register() makes a copy
> +       kfree(backlight_name);
> +       backlight_name = NULL;
> +
>         if (IS_ERR(bd))
>                 return PTR_ERR(bd);
>         drm->backlight = bd;
> @@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector)
>         struct backlight_properties props;
>         struct backlight_device *bd;
>         const struct backlight_ops *ops;
> +       char* backlight_name = NULL;
>
>         nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
>         if (!nv_encoder) {
> @@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector)
>         memset(&props, 0, sizeof(struct backlight_properties));
>         props.type = BACKLIGHT_RAW;
>         props.max_brightness = 100;
> -       bd = backlight_device_register("nv_backlight", connector->kdev,
> +       backlight_name = nouveau_get_backlight_name();
> +       bd = backlight_device_register(backlight_name, connector->kdev,
>                                        nv_encoder, ops, &props);
> +
> +       // backlight_device_register() makes a copy
> +       kfree(backlight_name);
> +       backlight_name = NULL;
> +
>         if (IS_ERR(bd))
>                 return PTR_ERR(bd);
>
> @@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev)
>                 drm->backlight = NULL;
>         }
>  }
> +
> +static char*
> +nouveau_get_backlight_name(void)
> +{
> +       // 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0'
> +       char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL);

Making this stack-allocated in the caller would be so much simpler...

> +       const int nb = atomic_inc_return(&bl_interfaces_nb) - 1;

This kinda sucks if you reload nouveau a bunch. How about using an
"ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that
one.

> +       if (nb > 0 && nb < 100)
> +               sprintf(backlight_name, "nv_backlight%d", nb);
> +       else
> +               sprintf(backlight_name, "nv_backlight");
> +       return backlight_name;
> +}
> --
> 2.8.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau