Re: [PATCH] sparc/pci: Make pci_poke_lock a raw_spinlock_t.

[Waiman Long <llong@redhat.com>]

On 11/27/24 8:17 PM, Guenter Roeck wrote:
> On 11/27/24 16:31, Waiman Long wrote:
>>
>> On 11/27/24 7:08 PM, Guenter Roeck wrote:
>>> On 11/27/24 15:47, Waiman Long wrote:
>>>> On 11/27/24 12:44 PM, Guenter Roeck wrote:
>>>>> On 11/27/24 08:53, Sebastian Andrzej Siewior wrote:
>>>>>> On 2024-11-27 08:02:50 [-0800], Guenter Roeck wrote:
>>>>>>> On 11/27/24 07:39, Andreas Larsson wrote:
>>>>>>>> Even though this is for sparc64, there is work being done 
>>>>>>>> looking into
>>>>>>>> enabling RT for sparc32. If the amount of fixes needed to keep
>>>>>>>> PROVE_RAW_LOCK_NESTING enabled is quite small at the moment I'd 
>>>>>>>> rather
>>>>>>>> see it enabled for sparc rather than risking it becoming worse 
>>>>>>>> in the
>>>>>>>> future.
>>>>>>
>>>>>> Okay. So you seem to be in favour of fixing the sparc64 splats 
>>>>>> Guenter
>>>>>> reported?
>>>>>>
>>>>>>>> I don't know what the situation is for other architectures that 
>>>>>>>> does not
>>>>>>>> support RT.
>>>>>>>>
>>>>>>>
>>>>>>> For my part I still don't understand why PROVE_RAW_LOCK_NESTING 
>>>>>>> is no longer
>>>>>>> a configurable option, or in other words why it is mandated even 
>>>>>>> for architectures
>>>>>>> not supporting RT. To me this means that I'll either have to 
>>>>>>> disable PROVE_LOCKING
>>>>>>> for sparc or live with endless warning backtraces. The latter 
>>>>>>> obscures real
>>>>>>> problems, so it is a no-go.
>>>>>>
>>>>>> It is documented in Documentation/locking/locktypes.rst how the 
>>>>>> locks
>>>>>> should nest. It is just nobody enabled it on sparc64 and tested. The
>>>>>> option was meant temporary until the big read blocks are cleared.
>>>>>>
>>>>>
>>>>> That doesn't explain why PROVE_RAW_LOCK_NESTING is now mandatory if
>>>>> PROVE_LOCKING is enabled, even on architectures where is was not 
>>>>> tested.
>>>>> I am all for testing, but that doesn't include making it mandatory
>>>>> even where it is known to fail. Enabling it by default, sure, no 
>>>>> problem.
>>>>> Dropping the option entirely after it is proven to no longer needed,
>>>>> also no problem. But force-enabling it even where untested or, worse,
>>>>> known to fail, is two steps too far.
>>>>
>>>> The main reason for enforcing PROVE_RAW_LOCK_NESTING with 
>>>> PROVE_LOCKING is due to the fact that PREEMPT_RT kernel is much 
>>>> less tested than the non-RT kernel. I do agree that we shouldn't 
>>>> force this on arches that don't support PREEMPT_RT. However, once 
>>>> an arch decides to support PREEMPT_RT, they have to fix all these 
>>>> raw_spinlock nesting problems.
>>>>
>>>
>>> config PROVE_RAW_LOCK_NESTING
>>> -       bool
>>> +       bool "Enable raw_spinlock - spinlock nesting checks" if 
>>> ARCH_SUPPORTS_RT=n
>>>         depends on PROVE_LOCKING
>>> -       default y
>>> +       default y if ARCH_SUPPORTS_RT
>>>
>>> would have accomplished that while at the same time making it optional
>>> for non-RT architectures.
>>
>> I had actually thought about doing exactly that, but decide to keep 
>> the current mode for forcing  PROVE_RAW_LOCK_NESTING for arches that 
>> support PREEMPT_RT. I won't mind doing this alternative if others agree.
>>
>
> Forcing PROVE_RAW_LOCK_NESTING for arches that support PREEMPT_RT is 
> exactly
> what the above does.
>
>     bool "Enable raw_spinlock - spinlock nesting checks" if 
> ARCH_SUPPORTS_RT=n
>
> makes the flag visible (only) if ARCH_SUPPORTS_RT=n, and
>
>     default y if ARCH_SUPPORTS_RT
>
> (force-)enables it if ARCH_SUPPORTS_RT=y.

OK, I missed the "if" part after the string. Yes, that do force 
PREEMPT_RT supporting arches to set PROVE_RAW_LOCK_NESTING while 
enabling arches that do not support PREEMPT_RT to optionally set it. I 
will post a v2 patch with that change.

Thanks,
Longman