Re: [PATCH 1/3] drm/xe/configfs: Don't touch survivability_mode on fini

[Riana Tauro <riana.tauro@intel.com>]

On 9/4/2025 1:17 AM, Michal Wajdeczko wrote:
> 
> 
> On 9/3/2025 7:50 PM, Lucas De Marchi wrote:
>> On Wed, Sep 03, 2025 at 07:49:39AM +0200, Michal Wajdeczko wrote:
>>>
>>>
>>> On 9/3/2025 7:39 AM, Riana Tauro wrote:
>>>>
>>>>
>>>> On 9/2/2025 11:34 PM, Lucas De Marchi wrote:
>>>>> On Tue, Sep 02, 2025 at 03:17:42PM +0200, Michal Wajdeczko wrote:
>>>>>> This is a user controlled configfs attribute, we should not
>>>>>> modify that outside the configfs attr.store() implementation.
>>>>>
>>>>> agreed... this was the first user of configfs and I don't think it was
>>>>> the right to do it. However for such a change we should propagate it to
> 
> should I add then:
> 
> Fixes: bc417e54e24b ("drm/xe: Enable configfs support for survivability mode")
> 
>>>>> stable too. Riana, any possible issue in userspace? With this change
>>>>> userspace should do a rmdir() before proceeding so it drops the
>>>>> configuration.
>>>>
>>>> Survivability mode is enabled in early probe and is used for firmware flashing by admin. If this is not cleared while exiting the mode the driver keeps entering survivability mode on every unbind/bind till admin clears the mode in configfs.
>>>
>>> hmm, but shouldn't driver enter survivability_mode *only* when there were some issues?
>>
>> entering survivability_mode via configfs was a user-initiated action. We
>> shouldn't clear it on unbind.
> 
> I get that as Acked-by, right?
> 
>>
>>>
>>>>
>>>> rmdir is general and can affect other entries. For survivability mode,
>>>> it's better to retain this clear.
>>>
>>> but it doesn't have to be rmdir
>>>
>>> mode could be cleared in similar way like it was enabled:
>>>
>>> # echo 0 > /sys/kernel/config/xe/0000:03:00.0/survivability_mode
>>>
>>> and since this user controlled config entry, I would let the user decide when to turn it off
>>
>> correct. It's even worse if there are other configs needed for that
>> platform, because then the user will play whac-a-mole if he has to
>> unbind to set additional stuff example:
>>
>> echo 1 > /sys/kernel/config/xe/0000:03:00.0/survivability_mode
>> bind 0000:03:00.0
>> # ... oops, forgot to set some additional things
>> unbind 0000:03:00.0
>> echo foobar > /sys/kernel/config/xe/0000:03:00.0/something_else
>> bind 0000:03:00.0
>> # ... oops, now it's not in survivability_mode anymore
>>
>> Doing it the other way is very simple:
>> 1) if all settings should be lost:  rmdir
>> 2) if just survivability_mode should be lost, echo 0


++

Hmm, then documentation needs a update in survivability mode along with 
the patch.

"It is the responsibility of the user to clear the
mode once firmware flash is complete."

Currently do not think fwupd is explicitly creating configfs. They
only check survivability mode on pcode failure. This is an option for 
admin incase pcode fails to detect or they need to manually enter the mode.

Thanks
Riana

>>
>> Lucas De Marchi
>>
>>>
>>>>
>>>> Thanks
>>>> Riana
>>>>>
>>>>> Lucas De Marchi
>>>>>
>>>>>>
>>>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>> Cc: Riana Tauro <riana.tauro@intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/xe/xe_survivability_mode.c | 1 -
>>>>>> 1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/ drm/xe/xe_survivability_mode.c
>>>>>> index 53c5af4b810c..79426ea46861 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>> @@ -180,7 +180,6 @@ static void xe_survivability_mode_fini(void *arg)
>>>>>>      struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>>>>>      struct device *dev = &pdev->dev;
>>>>>>
>>>>>> -    xe_configfs_clear_survivability_mode(pdev);
>>>>>>      sysfs_remove_file(&dev->kobj, &dev_attr_survivability_mode.attr);
>>>>>> }
>>>>>>
>>>>>> -- 
>>>>>> 2.47.1
>>>>>>
>>>>
>>>
>