Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6c05cc9e-815d-7a94-8b2d-f17fd5d47354@redhat.com>
Date:   Tue, 25 Apr 2023 12:34:05 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v10 03/14] HP BIOSCFG driver - bioscfg
Content-Language: en-US, nl
To:     Armin Wolf <W_Armin@gmx.de>, Jorge Lopez <jorgealtxwork@gmail.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        thomas@t-8ch.de
References: <20230419151321.6167-1-jorge.lopez2@hp.com>
 <20230419151321.6167-4-jorge.lopez2@hp.com>
 <38929a45-79de-964b-5d6f-cfa44099b35e@gmx.de>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <38929a45-79de-964b-5d6f-cfa44099b35e@gmx.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi All,

On 4/19/23 20:04, Armin Wolf wrote:
> Am 19.04.23 um 17:13 schrieb Jorge Lopez:

<snip>

>> +static int __init bioscfg_init(void)
>> +{
>> +    int ret = 0;
>> +    int bios_capable = wmi_has_guid(HP_WMI_BIOS_GUID);
>> +
>> +    if (!bios_capable) {
>> +        pr_err("Unable to run on non-HP system\n");
>> +        return -ENODEV;
>> +    }
>> +
> 
> Currently, this driver will no get automatically loaded on supported hardware,
> something which would be quite beneficial for users to have.
> Since the HP_WMI_BIOS_GUID is already handled by the hp-wmi driver, maybe this
> driver (which also already implements a function similar to hp_wmi_perform_query())
> could register a platform device which is then used by this driver? This together
> with MODULE_DEVICE_TABLE() would allow for automatically loading the module on supported hardware.

Both drivers can already co-exist since the old hp-wmi driver uses the old
wmi kernel functions and is not a "wmi_driver" so there is no need for
a platform_device for this driver to bind to since the wmi_device is
still free for it to bind to.

This does indeed need a MODULE_DEVICE_TABLE() statement for
the bios_attr_pass_interface_id_table[] id-table. Note only for that
table, because the HP_WMI_BIOS_GUID is present on models which do
not support this and we don't want the module to auto-load there.

Regards,

Hans


> 
> Armin Wolf
> 
>> +    ret = init_bios_attr_set_interface();
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = init_bios_attr_pass_interface();
>> +    if (ret)
>> +        goto err_exit_bios_attr_set_interface;
>> +
>> +    if (!bioscfg_drv.bios_attr_wdev || !bioscfg_drv.password_attr_wdev) {
>> +        pr_debug("Failed to find set or pass interface\n");
>> +        ret = -ENODEV;
>> +        goto err_exit_bios_attr_pass_interface;
>> +    }
>> +
>> +    ret = fw_attributes_class_get(&fw_attr_class);
>> +    if (ret)
>> +        goto err_exit_bios_attr_pass_interface;
>> +
>> +    bioscfg_drv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0),
>> +                          NULL, "%s", DRIVER_NAME);
>> +    if (IS_ERR(bioscfg_drv.class_dev)) {
>> +        ret = PTR_ERR(bioscfg_drv.class_dev);
>> +        goto err_unregister_class;
>> +    }
>> +
>> +    bioscfg_drv.main_dir_kset = kset_create_and_add("attributes", NULL,
>> +                            &bioscfg_drv.class_dev->kobj);
>> +    if (!bioscfg_drv.main_dir_kset) {
>> +        ret = -ENOMEM;
>> +        pr_debug("Failed to create and add attributes\n");
>> +        goto err_destroy_classdev;
>> +    }
>> +
>> +    bioscfg_drv.authentication_dir_kset = kset_create_and_add("authentication", NULL,
>> +                                  &bioscfg_drv.class_dev->kobj);
>> +    if (!bioscfg_drv.authentication_dir_kset) {
>> +        ret = -ENOMEM;
>> +        pr_debug("Failed to create and add authentication\n");
>> +        goto err_release_attributes_data;
>> +    }
>> +
>> +    /*
>> +     * sysfs level attributes.
>> +     * - pending_reboot
>> +     */
>> +    ret = create_attributes_level_sysfs_files();
>> +    if (ret)
>> +        pr_debug("Failed to create sysfs level attributes\n");
>> +
>> +    ret = hp_init_bios_attributes(HPWMI_STRING_TYPE, HP_WMI_BIOS_STRING_GUID);
>> +    if (ret)
>> +        pr_debug("Failed to populate string type attributes\n");
>> +
>> +    ret = hp_init_bios_attributes(HPWMI_INTEGER_TYPE, HP_WMI_BIOS_INTEGER_GUID);
>> +    if (ret)
>> +        pr_debug("Failed to populate integer type attributes\n");
>> +
>> +    ret = hp_init_bios_attributes(HPWMI_ENUMERATION_TYPE, HP_WMI_BIOS_ENUMERATION_GUID);
>> +    if (ret)
>> +        pr_debug("Failed to populate enumeration type attributes\n");
>> +
>> +    ret = hp_init_bios_attributes(HPWMI_ORDERED_LIST_TYPE, HP_WMI_BIOS_ORDERED_LIST_GUID);
>> +    if (ret)
>> +        pr_debug("Failed to populate ordered list object type attributes\n");
>> +
>> +    ret = hp_init_bios_attributes(HPWMI_PASSWORD_TYPE, HP_WMI_BIOS_PASSWORD_GUID);
>> +    if (ret)
>> +        pr_debug("Failed to populate password object type attributes\n");
>> +
>> +    bioscfg_drv.spm_data.attr_name_kobj = NULL;
>> +    ret = hp_add_other_attributes(HPWMI_SECURE_PLATFORM_TYPE);
>> +    if (ret)
>> +        pr_debug("Failed to populate secure platform object type attribute\n");
>> +
>> +    bioscfg_drv.sure_start_attr_kobj = NULL;
>> +    ret = hp_add_other_attributes(HPWMI_SURE_START_TYPE);
>> +    if (ret)
>> +        pr_debug("Failed to populate sure start object type attribute\n");
>> +
>> +    return 0;
>> +
>> +err_release_attributes_data:
>> +    release_attributes_data();
>> +
>> +err_destroy_classdev:
>> +    device_destroy(fw_attr_class, MKDEV(0, 0));
>> +
>> +err_unregister_class:
>> +    fw_attributes_class_put();
>> +
>> +err_exit_bios_attr_pass_interface:
>> +    exit_bios_attr_pass_interface();
>> +
>> +err_exit_bios_attr_set_interface:
>> +    exit_bios_attr_set_interface();
>> +
>> +    return ret;
>> +}
>> +
>> +static void __exit bioscfg_exit(void)
>> +{
>> +    release_attributes_data();
>> +    device_destroy(fw_attr_class, MKDEV(0, 0));
>> +
>> +    fw_attributes_class_put();
>> +    exit_bios_attr_set_interface();
>> +    exit_bios_attr_pass_interface();
>> +}
>> +
>> +module_init(bioscfg_init);
>> +module_exit(bioscfg_exit);
>