Re: [PATCH v2 4/6] drm/xe/cper: Support software CPER records

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/cper: sleeping in atomic context in cper_emit_sw_err()
- [High] drm/xe/cper: NULL pointer dereference in cper_emit_sw_err() when xe is NULL
--

commit c16a87d05ae708e1a17ac24623a75a82d558972b
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/cper: Support software CPER records

This commit adds support for handling software SIGIDs in xe_cper_emit() by
formatting the log message and emitting driver-notification CPER records via
cper_emit_sw_err().

> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> index e802f534eb45a..ec61605d0a314 100644
> --- a/drivers/gpu/drm/xe/xe_cper.c
> +++ b/drivers/gpu/drm/xe/xe_cper.c

[ ... ]

> @@ -239,6 +246,37 @@ static void cper_emit_hw_err(struct xe_device *xe, int cper_sev, enum xe_sigid s
>  	xe_cper_emit_counter(xe, cper_sev, sigid, counter, &response);
>  }
>  
> +#define XE_CPER_SW_MSG_MAX	256
> +
> +static size_t cper_format_sw_msg(struct va_format *vaf, char *buf, size_t size)
> +{
> +	return scnprintf(buf, size, "%pV", vaf) + 1;
> +}
> +
> +static void cper_emit_sw_err(struct xe_device *xe, int cper_sev, enum xe_sigid sigid,
> +			     u32 component, u32 location, const void *data,
> +			     struct va_format *vaf)
> +{
> +	struct xe_cper_intel_hdr ihdr = {};
> +	struct xe_cper_driver_error_class *driver = &ihdr.driver;
> +	u64 timestamp = ktime_get_boottime_ns();
> +	char msg[XE_CPER_SW_MSG_MAX];
> +	size_t msg_len;
> +
> +	might_sleep();
> +
> +	driver->error_type = cper_sev;
> +	driver->error_component = component;
> +	driver->tile = location;
> +	if (IS_ERR(data))
> +		driver->cause = -PTR_ERR(data);
> +
> +	msg_len = cper_format_sw_msg(vaf, msg, sizeof(msg));
> +
> +	cper_init_intel_hdr(xe, timestamp, sigid, 1, &ihdr);
> +	cper_record_emit(xe, cper_sev, &INTEL_CPER_NOTIFY_DRV_ERROR, &ihdr, msg, msg_len);
> +}

[Severity: High]
Can cper_emit_sw_err() sleep in atomic context when software SIGIDs are
logged?

In drivers/gpu/drm/xe/xe_log.c, __xe_log_emit() unconditionally calls
log_emit_cper() -> xe_cper_emit(). With this change, software SIGIDs are
routed to cper_emit_sw_err(), which calls might_sleep() and then calls
cper_record_emit() in drivers/gpu/drm/xe/xe_cper.c:

    total_len = sizeof(struct xe_cper_nonstd_record) + einfo_len;
    rec = kzalloc(total_len, GFP_KERNEL);

However, several paths in the driver emit software SIGID logs from hardirq
context, while holding spinlocks, or with preemption disabled:

1. In drivers/gpu/drm/xe/xe_guc_ct.c, xe_guc_ct_fast_path() runs from the
interrupt handler under xe->irq.lock and ct->fast_lock:

    spin_lock(&ct->fast_lock);
    do {
        len = g2h_read(ct, ct->fast_msg, true);
        if (len > 0)
            g2h_fast_path(ct, ct->fast_msg, len);
    } while (len > 0);
    spin_unlock(&ct->fast_lock);

Within this critical section (as well as in g2h_release_space(),
dequeue_one_g2h(), and __guc_ct_send_locked() while holding ct->fast_lock),
g2h_read() -> ct_corrupted() and g2h_fast_path() -> __g2h_release_space()
call xe_log_err() for the GuC component:

    xe_log_err(ct_to_gt(ct), GUC, err, "CT: %pV", &vaf);

2. In drivers/gpu/drm/xe/xe_pagefault.c, xe_pagefault_handler() (called from
hardirq context via xe_guc_ct_fast_path()) and xe_pagefault_queue_pop() hold
pf_queue->lock and call xe_pagefault_queue_add():

    guard(spinlock_irqsave)(&pf_queue->lock);

    empty = xe_pagefault_queue_empty(pf_queue);
    lpf = xe_pagefault_queue_add(pf_queue, pf);

When the queue is full, xe_pagefault_queue_add() calls:

    if (WARN_ON_ONCE(xe_pagefault_queue_full(pf_queue))) {
        xe_log_err(xe, PAGEFAULT, -ENOSPC, "Queue full!\n");
        return NULL;
    }

3. In drivers/gpu/drm/xe/xe_wedge.c, xe_wedge_declare() is documented as safe
for atomic callers and is invoked from hardirq context via
dg1_irq_handler() -> xe_mert_irq_handler() -> mert_handle_cat_error():

    xe_log_err_fatal(xe, WEDGED, -EIO, "Device declared wedged!\n");

4. In drivers/gpu/drm/xe/xe_pcode.c, xe_pcode_request() disables preemption
around pcode_try_request() -> pcode_mailbox_rw() -> __pcode_mailbox_rw() ->
pcode_mailbox_status(), which logs PCODE errors via xe_log_err():

    preempt_disable();
    ret = pcode_try_request(tile, mbox, request, reply_mask, reply, &status,
                            true, 50 * 1000, true);
    preempt_enable();

> +
>  /**
>   * xe_cper_emit() - Emit a CPER record for a SIGID log entry
>   * @xe: The &xe_device
> @@ -272,7 +310,8 @@ void xe_cper_emit(struct xe_device *xe, int cper_sev, enum xe_sigid sigid,
>  		return;
>  	}
>  
> -	/* TODO software CPER */
> +	if (is_sw_sigid(sigid))
> +		cper_emit_sw_err(xe, cper_sev, sigid, component, location, data, vaf);
>  }

[Severity: High]
Could this dereference a null xe pointer when xe_cper_emit() is called during
early PCI probe?

In drivers/gpu/drm/xe/xe_log.c, log_emit_cper() resolves the device pointer
via xe_any_to_xe():

    xe_cper_emit(xe_any_to_xe(pdev), cper_sev, sigid,
                 component, location, data, len, vaf);

When a struct pci_dev pointer is passed to the logging macros before
pci_set_drvdata() has been called in __xe_pci_probe(), pdev_to_xe_device(pdev)
returns NULL. This happens in drivers/gpu/drm/xe/xe_pci.c in xe_pci_probe()
when a device ID is blocked or if __xe_pci_probe() fails during early setup
(such as in devres_open_group(), pcim_enable_device(), or
xe_device_create()):

    if (id_blocked(pdev->device)) {
        xe_log_info(pdev, PROBE, "driver loading blocked for device '%04x'\n",
                    pdev->device);
        return -ENODEV;
    }

    if (xe_display_driver_probe_defer(pdev))
        return -EPROBE_DEFER;

    err = __xe_pci_probe(pdev, desc);
    if (err) {
        xe_log_err_fatal(pdev, PROBE, err, "driver loading failed for device '%04x'\n",
                         pdev->device);
        return err;
    }

Because those probe logs use a software SIGID, xe_cper_emit() now calls
cper_emit_sw_err() -> cper_init_intel_hdr() with xe set to NULL, which
dereferences xe->drm.dev in drivers/gpu/drm/xe/xe_cper.c:

    strscpy(ihdr->pci_bdf, pci_name(to_pci_dev(xe->drm.dev)), sizeof(ihdr->pci_bdf));

Should xe_cper_emit() check for a null xe pointer before proceeding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029132609.907017-8-mallesh.koujalagi@intel.com?part=4