linux-kernel - RE: [PATCH v3 1/3] 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] [day] [month] [year] [list]
Message-ID: <SJ0PR19MB4528BF867F9CAB88675A6B5584969@SJ0PR19MB4528.namprd19.prod.outlook.com>
Date:   Fri, 5 Mar 2021 02:56:57 +0000
From:   "Yuan, Perry" <Perry.Yuan@...l.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Perry Yuan <perry979106@...il.com>,
        "oder_chiou@...ltek.com" <oder_chiou@...ltek.com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "hdegoede@...hat.com" <hdegoede@...hat.com>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>
CC:     "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "Limonciello, Mario" <Mario.Limonciello@...l.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "broonie@...nel.org" <broonie@...nel.org>
Subject: RE: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell
 hardware privacy

Hi Pierre

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: 2021年2月17日 22:24
> To: Perry Yuan; Yuan, Perry; oder_chiou@...ltek.com; perex@...ex.cz;
> tiwai@...e.com; hdegoede@...hat.com; mgross@...ux.intel.com
> Cc: alsa-devel@...a-project.org; Limonciello, Mario; linux-
> kernel@...r.kernel.org; lgirdwood@...il.com; platform-driver-
> x86@...r.kernel.org; broonie@...nel.org
> Subject: Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> 
> 
> On 2/17/21 6:47 AM, Perry Yuan wrote:
> > Hi Pierre:
> > On 2021/2/16 22:56, Pierre-Louis Bossart wrote:
> >>
> >>>>> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> >>>>> +    {"PNP0C09", 0},
> >>>>> +    { },
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> >>>>> +
> >>>>> +static struct platform_driver dell_privacy_platform_drv = {
> >>>>> +    .driver = {
> >>>>> +        .name = PRIVACY_PLATFORM_NAME,
> >>>>> +        .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> >>>>> +    },
> >>>>
> >>>> no .probe?
> >>> Originally i added the probe here, but it cause the driver  .probe
> >>> called twice. after i use platform_driver_probe to register the
> >>> driver loading process, the duplicated probe issue resolved.
> >>>
> >>> I
> >>>>
> >>>>> +    .remove = dell_privacy_acpi_remove, };
> >>>>> +
> >>>>> +int __init dell_privacy_acpi_init(void) {
> >>>>> +    int err;
> >>>>> +    struct platform_device *pdev;
> >>>>> +    int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> >>>>> +
> >>>>> +    if (!wmi_has_guid(DELL_PRIVACY_GUID))
> >>>>> +        return -ENODEV;
> >>>>> +
> >>>>> +    privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> >>>>> +    if (!privacy_acpi)
> >>>>> +        return -ENOMEM;
> >>>>> +
> >>>>> +    pdev = platform_device_register_simple(
> >>>>> +            PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL,
> 0);
> >>>>> +    if (IS_ERR(pdev)) {
> >>>>> +        err = PTR_ERR(pdev);
> >>>>> +        goto pdev_err;
> >>>>> +    }
> >>>>> +    err = platform_driver_probe(&dell_privacy_platform_drv,
> >>>>> +            dell_privacy_acpi_probe);
> >>>>> +    if (err)
> >>>>> +        goto pdrv_err;
> >>>>
> >>>> why is the probe done here? Put differently, what prevents you from
> >>>> using a 'normal' platform driver, and do the leds_setup in the
> >>>> .probe()?
> >>> At first ,I used the normal platform driver framework, however tt
> >>> cause the driver  .probe called twice. after i use
> >>> platform_driver_probe to register the driver loading process, the
> >>> duplicated probe issue resolved.
> >>
> >> This sounds very odd...
> >>
> >> this looks like a conflict with the ACPI subsystem finding a device
> >> and probing the driver that's associated with the PNP0C09 HID, and
> >> then this module __init  creating a device manually which leads to a
> >> second probe
> >>
> >> Neither the platform_device_register_simple() nor
> >> platform_driver_probe() seem necessary?Because this privacy acpi
> >> driver file has dependency on the ec handle,
> > so i want to determine if the driver can be loaded basing on the EC ID
> > PNP0C09 matching.
> >
> > So far,It works well for me to register the privacy driver with  the
> > register sequence.
> > Dose it hurt to keep current registering process with
> > platform_driver_probe used?
> 
> Sorry, I don't understand why you need to list PNP0C09 HID in a matching
> table if it's not used to probe anything.
> 
> The purpose of those matching tables is that when this HID is found, the core
> will invoke the probe callback with no need for any manual intervention.
> 
> If you want to do things manually with the module init, that's fine, it's the
> combination of the two that I find questionable.
> 
> It's like having both a manual and automatic transmission in a car, with the
> automatic transmission not coupled to the wheels.

I understand your point,I have removed the PNP0C09 ID from V4 patch.
Thanks for your suggestion very much !

Perry