Re: [PATCH v4 19/32] drm/xe: Report all probe errors using SIGID

On 8/13/2026 11:58 AM, Mallesh, Koujalagi wrote:
> 
> On 13-08-2026 02:42 pm, Michal Wajdeczko wrote:
>>
>> On 8/13/2026 8:50 AM, Mallesh, Koujalagi wrote:
>>> On 13-08-2026 12:44 am, Michal Wajdeczko wrote:
>>>> For completeness, we should catch and report all probe errors, not
>>>> just the ones that have explicit error message. Split xe_pci_probe()
>>>> function after a display check to avoid reporting -EPROBE_DEFER and
>>>> use xe_log_err_fatal() helper to report all returned errors.
>>>>
>>>> Signed-off-by: Michal Wajdeczko <[email protected]>
>>>> Cc: Rodrigo Vivi <[email protected]>
>>>> Cc: Aravind Iddamsetty <[email protected]>
>>>> Cc: Mallesh Koujalagi <[email protected]>
>>>> ---
>>>> Cc: Dnyaneshwar Bhadane <[email protected]>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_pci.c | 27 +++++++++++++++++++++------
>>>>    1 file changed, 21 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>>> index b19efef1c5cd..ab4da1d9a9f1 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>> @@ -1147,17 +1147,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>>>     * caller. Therefore there is no consequence on those specific callers when
>>>>     * function error injection skips the whole function.
>>>>     */
>>> nit: Align xe_pci_probe comment message with latest changes.
>> hmm, what exactly do you want to change here?
> 
> Since after refactoring the code, xe_device_probe move to __xe_pci_probe so
> 
> we need to change "functions called directly from __xe_pci_probe() or indirectly" right?

I guess it's a matter or the missing comma in the comment

"directly from xe_pci_probe() or indirectly, for example through xe_device_probe()"
                                           ^
                                           here

> 
>>
>> comment above is about "Probe the PCI device" sequence in general
>> not about the specific xe_pci_probe() function
> 
> Agreed! however the are using xe_device_probe function name etc, which is part of  __xe_pci_probe right.

which IMO still makes xe_device_probe part of the xe_pci_probe()

those are just technical code splits, to avoid large refactoring
or error prone use of goto's - either way not worth to be mentioned
in a general comment which is more about error injection anyway

> 
> Thanks,
> 
> -/Mallesh
> 
>>> Reviewed-by: Mallesh Koujalagi <[email protected]>
>> thanks!
>>
>>>> +static int __xe_pci_probe(struct pci_dev *pdev, const struct xe_device_desc *desc);
>>>>    static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>    {
>>>> -    struct xe_probed_info probed_info = {};
>>>>        const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>>> -    const struct xe_subplatform_desc *subplatform_desc;
>>>> -    struct xe_device *xe;
>>>> -    void *group;
>>>>        int err;
>>>>    -    subplatform_desc = find_subplatform(desc, pdev->device);
>>>> -
>>>>        xe_configfs_check_device(pdev);
>>>>          if (desc->require_force_probe && !id_forced(pdev->device)) {
>>>> @@ -1181,6 +1176,26 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>        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;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __xe_pci_probe(struct pci_dev *pdev, const struct xe_device_desc *desc)
>>>> +{
>>>> +    const struct xe_subplatform_desc *subplatform_desc;
>>>> +    struct xe_probed_info probed_info = {};
>>>> +    struct xe_device *xe;
>>>> +    void *group;
>>>> +    int err;
>>>> +
>>>> +    subplatform_desc = find_subplatform(desc, pdev->device);
>>>> +
>>>>        /* Group all devres so xe_pci_error_slot_reset() can release them as a unit. */
>>>>        group = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
>>>>        if (!group)