linux-kernel - Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c61b975a-a771-eb63-5232-2da184ca95fe@redhat.com>
Date:   Tue, 23 Mar 2021 20:40:17 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Perry Yuan <Perry.Yuan@...l.com>, pobrn@...tonmail.com,
        oder_chiou@...ltek.com, perex@...ex.cz, tiwai@...e.com,
        mgross@...ux.intel.com, Mario.Limonciello@...l.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        lgirdwood@...il.com, platform-driver-x86@...r.kernel.org,
        broonie@...nel.org
Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
 hardware privacy

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



> 
>> +{
>> +    struct platform_device *pdev = to_platform_device(privacy_acpi->dev);
>> +
>> +    platform_device_unregister(pdev);
>> +    platform_driver_unregister(&dell_privacy_platform_drv);
>> +    kfree(privacy_acpi);
>> +}
>> +
>> +MODULE_AUTHOR("Perry Yuan <perry_yuan@...l.com>");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
> 
>