Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
 hardware privacy
To:     "Yuan, Perry" <Perry.Yuan@dell.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        "pobrn@protonmail.com" <pobrn@protonmail.com>,
        "oder_chiou@realtek.com" <oder_chiou@realtek.com>,
        "perex@perex.cz" <perex@perex.cz>,
        "tiwai@suse.com" <tiwai@suse.com>,
        "mgross@linux.intel.com" <mgross@linux.intel.com>,
        "Limonciello, Mario" <Mario.Limonciello@dell.com>
Cc:     "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "broonie@kernel.org" <broonie@kernel.org>
References: <20210322093841.11840-1-Perry_Yuan@Dell.com>
 <a1fae1fc-2d4f-63e8-e0be-0f9c4c91640c@linux.intel.com>
 <c61b975a-a771-eb63-5232-2da184ca95fe@redhat.com>
 <SJ0PR19MB4528ED97AA4A5D0821A3E68584629@SJ0PR19MB4528.namprd19.prod.outlook.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <5ec52d3c-3587-6bf7-bf84-b6f8f31be0e7@redhat.com>
Date:   Thu, 25 Mar 2021 09:23:51 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <SJ0PR19MB4528ED97AA4A5D0821A3E68584629@SJ0PR19MB4528.namprd19.prod.outlook.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 3/25/21 8:52 AM, Yuan, Perry wrote:
> Hi Hans.
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Wednesday, March 24, 2021 3:40 AM
>> To: Pierre-Louis Bossart; Yuan, Perry; pobrn@protonmail.com;
>> oder_chiou@realtek.com; perex@perex.cz; tiwai@suse.com;
>> mgross@linux.intel.com; Limonciello, Mario
>> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org;
>> lgirdwood@gmail.com; platform-driver-x86@vger.kernel.org;
>> broonie@kernel.org
>> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
>> hardware privacy
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
>>> Minor comments below.
>>
>> <snip<
>>
>>>> +int __init dell_privacy_acpi_init(void)
>>>
>>> is the __init necessary? You call this routine from another which already has
>> this qualifier.
>>
>> Yes this is necessary, all functions which are only used during module_load /
>> from the module's init function must be marked as __init so that the kernel
>> can unload them after module loading is done.
>>
>> I do wonder if this one should not be static though (I've not looked at this
>> patch in detail yet).
>>
>>>
>>>> +{
>>>> +    int err;
>>>> +    struct platform_device *pdev;
>>>> +
>>>> +    if (!wmi_has_guid(DELL_PRIVACY_GUID))
>>>> +        return -ENODEV;
>>>> +
>>>> +    privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
>>>> +    if (!privacy_acpi)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    err = platform_driver_register(&dell_privacy_platform_drv);
>>>> +    if (err)
>>>> +        goto pdrv_err;
>>>> +
>>>> +    pdev = platform_device_register_simple(
>>>> +            PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
>>>> +    if (IS_ERR(pdev)) {
>>>> +        err = PTR_ERR(pdev);
>>>> +        goto pdev_err;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +pdev_err:
>>>> +    platform_device_unregister(pdev);
>>>> +pdrv_err:
>>>> +    kfree(privacy_acpi);
>>>> +    return err;
>>>> +}
>>>> +
>>>> +void __exit dell_privacy_acpi_exit(void)
>>>
>>> is the __exit required here?
>>
>> Idem. Also static ?
>>
>> Regards,
>>
>> Hans
>>
> 
> If adding static to the function,  I will be worried about that the init and exit cannot be called by another file .

That is right, which is why I added the "?".

But this is no longer relevant after my detailed review of the patch,
so lets discuss things further in the detailed review email-thread.

Regards,

Hans