Hi,
On 11-12-2019 01:24, Linus Walleij wrote:
On Mon, Dec 2, 2019 at 4:49 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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.
I've prepared a patch which makes pinctrl_register_mappings remember
if the mapping is dupped or not (store the dup value in struct pinctrl_maps);
and which modifies pinctrl_unregister_map() to do the right thing
depending on the stored dup value.
I still need to test the new series and then I will post it.
Regards,
Hans
