| 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
| ||
|
Message-ID: <8ae33696-f49c-41cf-97a9-ea9a405d05be@intel.com>
Date: Wed, 30 Jul 2025 17:17:25 +0530
From: "Nilawar, Badal" <badal.nilawar@...el.com>
To: Lucas De Marchi <lucas.demarchi@...el.com>, Rodrigo Vivi
<rodrigo.vivi@...el.com>
CC: <intel-xe@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>, <gregkh@...uxfoundation.org>,
<daniele.ceraolospurio@...el.com>, <anshuman.gupta@...el.com>,
<alexander.usyskin@...el.com>
Subject: Re: [PATCH 3/9] drm/xe/xe_late_bind_fw: Introducing xe_late_bind_fw
On 26-07-2025 03:08, Lucas De Marchi wrote:
> On Thu, Jul 10, 2025 at 11:08:34AM -0400, Rodrigo Vivi wrote:
>> From: Badal Nilawar <badal.nilawar@...el.com>
>>
>> Introducing xe_late_bind_fw to enable firmware loading for the devices,
>
> here and in the subject: no gerund, please.
Sure.
>
>> such as the fan controller, during the driver probe. Typically,
>> firmware for such devices are part of IFWI flash image but can be
>> replaced at probe after OEM tuning.
>> This patch binds mei late binding component to enable firmware loading.
>>
>> v2:
>> - Add devm_add_action_or_reset to remove the component (Daniele)
>> - Add INTEL_MEI_GSC check in xe_late_bind_init() (Daniele)
>> v3:
>> - Fail driver probe if late bind initialization fails,
>> add has_late_bind flag (Daniele)
>> v4:
>> - %S/I915_COMPONENT_LATE_BIND/INTEL_COMPONENT_LATE_BIND/
>> v6:
>> - rebased
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@...el.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@...el.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@...el.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_device.c | 5 ++
>> drivers/gpu/drm/xe/xe_device_types.h | 6 ++
>> drivers/gpu/drm/xe/xe_late_bind_fw.c | 84 ++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_late_bind_fw.h | 15 ++++
>> drivers/gpu/drm/xe/xe_late_bind_fw_types.h | 33 +++++++++
>> drivers/gpu/drm/xe/xe_pci.c | 2 +
>> drivers/gpu/drm/xe/xe_pci_types.h | 1 +
>> 8 files changed, 147 insertions(+)
>> create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw.c
>> create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw.h
>> create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 1d97e5b63f4e..97e2b1368a6e 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -76,6 +76,7 @@ xe-y += xe_bb.o \
>> xe_hw_fence.o \
>> xe_irq.o \
>> xe_lrc.o \
>> + xe_late_bind_fw.o \
>
> almost there, but still not sorted as it should be
It seems the rebase messed up the order—it was correctly placed in the
previous revision. I’ll fix it.
>
>> xe_migrate.o \
>> xe_mmio.o \
>> xe_mocs.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>> b/drivers/gpu/drm/xe/xe_device.c
>> index 0b73cb72bad1..cb595bae5f55 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -44,6 +44,7 @@
>> #include "xe_hw_engine_group.h"
>> #include "xe_hwmon.h"
>> #include "xe_irq.h"
>> +#include "xe_late_bind_fw.h"
>> #include "xe_mmio.h"
>> #include "xe_module.h"
>> #include "xe_nvm.h"
>> @@ -866,6 +867,10 @@ int xe_device_probe(struct xe_device *xe)
>> if (err)
>> return err;
>>
>> + err = xe_late_bind_init(&xe->late_bind);
>> + if (err && err != -ENODEV)
>
> let's not be creative in xe_device_probe(). Boring is good.
>
> All the other parts in this function handle ENODEV internally. It's up
> to them to decide if ENODEV -> return 0, not here.
sure, will update accordingly.
>
>
>> + return err;
>> +
>> err = xe_oa_init(xe);
>> if (err)
>> return err;
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>> b/drivers/gpu/drm/xe/xe_device_types.h
>> index 78c4acafd268..a8891833f980 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -16,6 +16,7 @@
>> #include "xe_devcoredump_types.h"
>> #include "xe_heci_gsc.h"
>> #include "xe_lmtt_types.h"
>> +#include "xe_late_bind_fw_types.h"
>
> should be sorted
Sure.
>
>> #include "xe_memirq_types.h"
>> #include "xe_oa_types.h"
>> #include "xe_platform_types.h"
>> @@ -325,6 +326,8 @@ struct xe_device {
>> u8 has_heci_cscfi:1;
>> /** @info.has_heci_gscfi: device has heci gscfi */
>> u8 has_heci_gscfi:1;
>> + /** @info.has_late_bind: Device has firmware late binding
>> support */
>> + u8 has_late_bind:1;
>> /** @info.has_llc: Device has a shared CPU+GPU last level
>> cache */
>> u8 has_llc:1;
>> /** @info.has_mbx_power_limits: Device has support to manage
>> power limits using
>> @@ -557,6 +560,9 @@ struct xe_device {
>> /** @nvm: discrete graphics non-volatile memory */
>> struct intel_dg_nvm_dev *nvm;
>>
>> + /** @late_bind: xe mei late bind interface */
>> + struct xe_late_bind late_bind;
>> +
>> /** @oa: oa observation subsystem */
>> struct xe_oa oa;
>>
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> new file mode 100644
>> index 000000000000..152f3e58de94
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> @@ -0,0 +1,84 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include <linux/component.h>
>> +#include <linux/delay.h>
>> +
>> +#include <drm/drm_managed.h>
>> +#include <drm/intel/i915_component.h>
>> +#include <drm/intel/intel_lb_mei_interface.h>
>> +#include <drm/drm_print.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_late_bind_fw.h"
>> +
>> +static struct xe_device *
>> +late_bind_to_xe(struct xe_late_bind *late_bind)
>> +{
>> + return container_of(late_bind, struct xe_device, late_bind);
>> +}
>> +
>> +static int xe_late_bind_component_bind(struct device *xe_kdev,
>> + struct device *mei_kdev, void *data)
>> +{
>> + struct xe_device *xe = kdev_to_xe_device(xe_kdev);
>> + struct xe_late_bind *late_bind = &xe->late_bind;
>> +
>> + late_bind->component.ops = data;
>> + late_bind->component.mei_dev = mei_kdev;
>> +
>> + return 0;
>> +}
>> +
>> +static void xe_late_bind_component_unbind(struct device *xe_kdev,
>> + struct device *mei_kdev, void *data)
>> +{
>> + struct xe_device *xe = kdev_to_xe_device(xe_kdev);
>> + struct xe_late_bind *late_bind = &xe->late_bind;
>> +
>> + late_bind->component.ops = NULL;
>> +}
>> +
>> +static const struct component_ops xe_late_bind_component_ops = {
>> + .bind = xe_late_bind_component_bind,
>> + .unbind = xe_late_bind_component_unbind,
>> +};
>> +
>> +static void xe_late_bind_remove(void *arg)
>> +{
>> + struct xe_late_bind *late_bind = arg;
>> + struct xe_device *xe = late_bind_to_xe(late_bind);
>> +
>> + component_del(xe->drm.dev, &xe_late_bind_component_ops);
>> +}
>> +
>> +/**
>> + * xe_late_bind_init() - add xe mei late binding component
>> + * @late_bind: pointer to late bind structure.
>> + *
>> + * Return: 0 if the initialization was successful, a negative errno
>> otherwise.
>> + */
>> +int xe_late_bind_init(struct xe_late_bind *late_bind)
>> +{
>> + struct xe_device *xe = late_bind_to_xe(late_bind);
>> + int err;
>> +
>> + if (!xe->info.has_late_bind)
>> + return 0;
>> +
>> + if (!IS_ENABLED(CONFIG_INTEL_MEI_LATE_BIND) ||
>> !IS_ENABLED(CONFIG_INTEL_MEI_GSC)) {
>> + drm_info(&xe->drm, "Can't init xe mei late bind missing mei
>> component\n");
>> + return -ENODEV;
>> + }
>> +
>> + err = component_add_typed(xe->drm.dev, &xe_late_bind_component_ops,
>> + INTEL_COMPONENT_LB);
>> + if (err < 0) {
>> + drm_info(&xe->drm, "Failed to add mei late bind component
>> (%pe)\n", ERR_PTR(err));
>> + return err;
>
> if you are going to return an error to stop the probe, then make this a
> drm_err()
Sure.
>
>> + }
>> +
>> + return devm_add_action_or_reset(xe->drm.dev,
>> xe_late_bind_remove, late_bind);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> new file mode 100644
>> index 000000000000..4c73571c3e62
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_LATE_BIND_FW_H_
>> +#define _XE_LATE_BIND_FW_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct xe_late_bind;
>> +
>> +int xe_late_bind_init(struct xe_late_bind *late_bind);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> new file mode 100644
>> index 000000000000..f79e5aefed94
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_LATE_BIND_TYPES_H_
>> +#define _XE_LATE_BIND_TYPES_H_
>> +
>> +#include <linux/iosys-map.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct xe_late_bind_component - Late Binding services component
>> + * @mei_dev: device that provide Late Binding service.
>> + * @ops: Ops implemented by Late Binding driver, used by Xe driver.
>> + *
>> + * Communication between Xe and MEI drivers for Late Binding services
>> + */
>> +struct xe_late_bind_component {
>> + struct device *mei_dev;
>> + const struct late_bind_component_ops *ops;
>> +};
>> +
>> +/**
>> + * struct xe_late_bind
>> + */
>> +struct xe_late_bind {
>> + /** @component: struct for communication with mei component */
>> + struct xe_late_bind_component component;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index ffd6ad569b7c..69b8ead9ca59 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -332,6 +332,7 @@ static const struct xe_device_desc bmg_desc = {
>> .has_gsc_nvm = 1,
>> .has_heci_cscfi = 1,
>> .max_gt_per_tile = 2,
>> + .has_late_bind = true,
>
> again, boring is good: all the has_xxxx flags should be sorted
> together.
Looks like the re-base affected this part as well. It was correctly
grouped in the previous revision — I’ll fix this.
Thanks,
Badal
>
> Lucas De Marchi
>
>> .needs_scratch = true,
>> };
>>
>> @@ -578,6 +579,7 @@ static int xe_info_init_early(struct xe_device *xe,
>> xe->info.has_gsc_nvm = desc->has_gsc_nvm;
>> xe->info.has_heci_gscfi = desc->has_heci_gscfi;
>> xe->info.has_heci_cscfi = desc->has_heci_cscfi;
>> + xe->info.has_late_bind = desc->has_late_bind;
>> xe->info.has_llc = desc->has_llc;
>> xe->info.has_pxp = desc->has_pxp;
>> xe->info.has_sriov = desc->has_sriov;
>> diff --git a/drivers/gpu/drm/xe/xe_pci_types.h
>> b/drivers/gpu/drm/xe/xe_pci_types.h
>> index 4de6f69ed975..51a607d323fb 100644
>> --- a/drivers/gpu/drm/xe/xe_pci_types.h
>> +++ b/drivers/gpu/drm/xe/xe_pci_types.h
>> @@ -39,6 +39,7 @@ struct xe_device_desc {
>> u8 has_gsc_nvm:1;
>> u8 has_heci_gscfi:1;
>> u8 has_heci_cscfi:1;
>> + u8 has_late_bind:1;
>> u8 has_llc:1;
>> u8 has_mbx_power_limits:1;
>> u8 has_pxp:1;
>> --
>> 2.49.0
>>
Powered by blists - more mailing lists