Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT

From: Linus Walleij <linus.walleij@linaro.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Mika Westerberg" <mika.westerberg@linux.intel.com>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	"open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"ACPI Devel Maling List" <linux-acpi@vger.kernel.org>
Subject: Re: [PATCH 2/2] drm/i915/vlv_dsi: Control panel and backlight enable GPIOs on BYT
Date: Wed, 11 Dec 2019 01:24:07 +0100	[thread overview]
Message-ID: <CACRpkdaJGZsJpYu3cgQCeWuJD1y9CQyzuk_VYfGfAT8WC=_1VA@mail.gmail.com> (raw)
In-Reply-To: <47c36b75-bc30-502b-7f8d-035cf2348fc4@redhat.com>

On Mon, Dec 2, 2019 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote:

> There is only one problem, currently is is not possible to
> unregister a mapping added with pinctrl_register_mappings
> and the i915 driver is typically a module which can be unloaded
> and I believe actually is unloaded as part of the i915 CI.
>
> pinctrl_register_mappings copies the passed in mapping, but
> it is a shallow copy, so it contains pointers to the modules
> const segment and we do not want to re-add another copy of
> the mapping when the module loads a second time.
>
> Fixing this is easy though, there already is a pinctrl_unregister_map()
> function, we just need to export it so that the i915 driver can
> remove the mapping when it is unbound.
>
> Linus would exporting this function be ok with you?

Yep!

> Linus, question what is the purpose of the "dupping" / shallow
> copying of the argument passed to pinctrl_register_map ?

The initial commit contained this comment later removed:

+       /*
+        * Make a copy of the map array - string pointers will end up in the
+        * kernel const section anyway so these do not need to be deep copied.
+        */

The use was to free up memory for platforms using boardfiles
with a gazillion variants and huge pin control tables, so these
could be marked  __initdata and discarded after boot.
As the strings would anyway stay around we didn't need to
deep copy.

See for example in arch/arm/mach-u300/core.c
static struct pinctrl_map __initdata u300_pinmux_map[]

> Since
> it is shallow the mem for any pointers contained within there need
> to be kept around by the caller, so why not let the caller keep
> the pinctrl_map struct itself around too?

So the strings will be kept around because the kernel can't get
rid of strings. (Yeah it is silly, should haven been fixed ages
ago, but not by me, haha :)

> If we are going to export pinctrl_unregister_map() we need to make it
> do the right thing for dupped maps too, we can just store the dup flag
> in struct pinctrl_maps. So this is easy, but I wonder if we cannot
> get rid of the dupping all together ?

Maybe ... I don't know. What do you think? I suppose you could
make u300 crash if you do that.

Yours,
Linus Walleij