Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <0e2e0e8f-91dd-b458-e39e-b00baf98b4e0@wanadoo.fr>
Date:   Sun, 17 Sep 2023 19:58:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
Content-Language: fr, en-US
To:     stephan@gerhold.net, Jeff LaBundy <jeff@labundy.com>
Cc:     conor+dt@kernel.org, devicetree@vger.kernel.org,
        dmitry.torokhov@gmail.com, jonathan.albrieux@gmail.com,
        krzysztof.kozlowski+dt@linaro.org, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org, robh+dt@kernel.org,
        rydberg@bitmath.org
References: <20230913-hx852x-v1-0-9c1ebff536eb@gerhold.net>
 <20230913-hx852x-v1-2-9c1ebff536eb@gerhold.net>
 <abf36591-3b3c-dc47-b1aa-e574325499f4@wanadoo.fr> <ZQcrQIfXYCv5aMK7@nixie71>
From:   Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <ZQcrQIfXYCv5aMK7@nixie71>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 17/09/2023 à 18:37, Jeff LaBundy a écrit :

>>> +	error = input_register_device(hx->input_dev);
>>> +	if (error) {
>>
>> input_mt_destroy_slots() should be called here, or in an error handling path
>> below, or via a devm_add_action_or_reset().
> 
> This seems like a memory leak in every touchscreen driver; maybe it is more
> practical to have the input core handle this clean-up.
> 
> Other drivers can and do insert other return paths between input_mt_init_slots()
> and input_register_device(), so it seems that we cannot solve this by calling
> input_mt_destroy_slots() from the error path within input_register_device().
> 
> Maybe a better option is to update input_mt_init_slots() to use device-managed
> allocation instead?

I think that devm_ is the way to go:

    $ git grep input_mt_init_slots | wc -l
    82

    $ git grep input_mt_destroy_slots | wc -l
    6

I'll send a patch for it.

> 
>>
>> It should also be called in a .remove function (unless
>> devm_add_action_or_reset is prefered)
> 
> I think the remove path is OK, as input_dev_release() handles this for us. In
> case I have misunderstood, please let me know.

Agreed. I missed that.

CJ