Hi,
a small series with a round of SCMI fixes, mostly trivial.
Notably, though:
- patch 4/5 adds a missing binding definition for SCMI SystemPower
protocol (merged a while ago and probably lost in translation txt->yaml)
- patch 5/5 addresses a possible deadlock, spotted by LOCKDEP, at SCMI
stack unload time, when using a virtio transport backend.
Applies on v6.1
Thanks,
Cristian
Cristian Marussi (5):
firmware: arm_scmi: Clear stale xfer->hdr.status
firmware: arm_scmi: Harden shared memory access in fetch_response
firmware: arm_scmi: Harden shared memory access in fetch_notification
dt-bindings: firmware: arm,scmi: Add support for syspower protocol
firmware: arm_scmi: Fix virtio channels cleanup on shutdown
.../devicetree/bindings/firmware/arm,scmi.yaml | 10 ++++++++++
drivers/firmware/arm_scmi/driver.c | 2 ++
drivers/firmware/arm_scmi/shmem.c | 9 ++++++---
drivers/firmware/arm_scmi/virtio.c | 7 ++++++-
4 files changed, 24 insertions(+), 4 deletions(-)
--
2.34.1
When unloading the SCMI core stack module, configured to use the virtio
SCMI transport, LOCKDEP reports the splat down below about unsafe locks
dependencies.
In order to avoid this possible unsafe locking scenario call upfront
virtio_break_device() before getting hold of vioch->lock.
=====================================================
WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
6.1.0-00067-g6b934395ba07-dirty #4 Not tainted
-----------------------------------------------------
rmmod/307 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
ffff000080c510e0 (&dev->vqs_list_lock){+.+.}-{3:3}, at: virtio_break_device+0x28/0x68
and this task is already holding:
ffff00008288ada0 (&channels[i].lock){-.-.}-{3:3}, at: virtio_chan_free+0x60/0x168 [scmi_module]
which would create a new lock dependency:
(&channels[i].lock){-.-.}-{3:3} -> (&dev->vqs_list_lock){+.+.}-{3:3}
but this new dependency connects a HARDIRQ-irq-safe lock:
(&channels[i].lock){-.-.}-{3:3}
... which became HARDIRQ-irq-safe at:
lock_acquire+0x128/0x398
_raw_spin_lock_irqsave+0x78/0x140
scmi_vio_complete_cb+0xb4/0x3b8 [scmi_module]
vring_interrupt+0x84/0x120
vm_interrupt+0x94/0xe8
__handle_irq_event_percpu+0xb4/0x3d8
handle_irq_event_percpu+0x20/0x68
handle_irq_event+0x50/0xb0
handle_fasteoi_irq+0xac/0x138
generic_handle_domain_irq+0x34/0x50
gic_handle_irq+0xa0/0xd8
call_on_irq_stack+0x2c/0x54
do_interrupt_handler+0x8c/0x90
el1_interrupt+0x40/0x78
el1h_64_irq_handler+0x18/0x28
el1h_64_irq+0x64/0x68
_raw_write_unlock_irq+0x48/0x80
ep_start_scan+0xf0/0x128
do_epoll_wait+0x390/0x858
do_compat_epoll_pwait.part.34+0x1c/0xb8
__arm64_sys_epoll_pwait+0x80/0xd0
invoke_syscall+0x4c/0x110
el0_svc_common.constprop.3+0x98/0x120
do_el0_svc+0x34/0xd0
el0_svc+0x40/0x98
el0t_64_sync_handler+0x98/0xc0
el0t_64_sync+0x170/0x174
to a HARDIRQ-irq-unsafe lock:
(&dev->vqs_list_lock){+.+.}-{3:3}
... which became HARDIRQ-irq-unsafe at:
...
lock_acquire+0x128/0x398
_raw_spin_lock+0x58/0x70
__vring_new_virtqueue+0x130/0x1c0
vring_create_virtqueue+0xc4/0x2b8
vm_find_vqs+0x20c/0x430
init_vq+0x308/0x390
virtblk_probe+0x114/0x9b0
virtio_dev_probe+0x1a4/0x248
really_probe+0xc8/0x3a8
__driver_probe_device+0x84/0x190
driver_probe_device+0x44/0x110
__driver_attach+0x104/0x1e8
bus_for_each_dev+0x7c/0xd0
driver_attach+0x2c/0x38
bus_add_driver+0x1e4/0x258
driver_register+0x6c/0x128
register_virtio_driver+0x2c/0x48
virtio_blk_init+0x70/0xac
do_one_initcall+0x84/0x420
kernel_init_freeable+0x2d0/0x340
kernel_init+0x2c/0x138
ret_from_fork+0x10/0x20
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&dev->vqs_list_lock);
local_irq_disable();
lock(&channels[i].lock);
lock(&dev->vqs_list_lock);
<Interrupt>
lock(&channels[i].lock);
*** DEADLOCK ***
================
Fixes: 42e90eb53bf3f ("firmware: arm_scmi: Add a virtio channel refcount")
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/virtio.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 33c9b81a55cd..1db975c08896 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -160,7 +160,6 @@ static void scmi_vio_channel_cleanup_sync(struct scmi_vio_channel *vioch)
}
vioch->shutdown_done = &vioch_shutdown_done;
- virtio_break_device(vioch->vqueue->vdev);
if (!vioch->is_rx && vioch->deferred_tx_wq)
/* Cannot be kicked anymore after this...*/
vioch->deferred_tx_wq = NULL;
@@ -482,6 +481,12 @@ static int virtio_chan_free(int id, void *p, void *data)
struct scmi_chan_info *cinfo = p;
struct scmi_vio_channel *vioch = cinfo->transport_info;
+ /*
+ * Break device to inhibit further traffic flowing while shutting down
+ * the channels: doing it later holding vioch->lock creates unsafe
+ * locking dependency chains as reported by LOCKDEP.
+ */
+ virtio_break_device(vioch->vqueue->vdev);
scmi_vio_channel_cleanup_sync(vioch);
scmi_free_channel(cinfo, data, id);
--
2.34.1
Stale error status reported from a previous message transaction must be
cleared before starting a new transaction to avoid being confusingly
reported in the following SCMI message dump traces.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/driver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f818d00bb2c6..ffdad59ec81f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -910,6 +910,8 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
xfer->hdr.protocol_id, xfer->hdr.seq,
xfer->hdr.poll_completion);
+ /* Clear any stale status */
+ xfer->hdr.status = SCMI_SUCCESS;
xfer->state = SCMI_XFER_SENT_OK;
/*
* Even though spinlocking is not needed here since no race is possible
--
2.34.1
Add new SCMI Syspower protocol bindings definitions and example.
Cc: Rob Herring <[email protected]>
Cc: [email protected]
Signed-off-by: Cristian Marussi <[email protected]>
---
Got lost in translation probably...from txt to yaml
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 1c0388da6721..f3dd77a470dd 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -111,6 +111,12 @@ properties:
required:
- '#power-domain-cells'
+ protocol@12:
+ type: object
+ properties:
+ reg:
+ const: 0x12
+
protocol@13:
type: object
properties:
@@ -285,6 +291,10 @@ examples:
#power-domain-cells = <1>;
};
+ scmi_syspower: protocol@12 {
+ reg = <0x12>;
+ };
+
scmi_dvfs: protocol@13 {
reg = <0x13>;
#clock-cells = <1>;
--
2.34.1
On 22/12/2022 19:38, Cristian Marussi wrote:
> Add new SCMI Syspower protocol bindings definitions and example.
>
> Cc: Rob Herring <[email protected]>
> Cc: [email protected]
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> Got lost in translation probably...from txt to yaml
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 1c0388da6721..f3dd77a470dd 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -111,6 +111,12 @@ properties:
> required:
> - '#power-domain-cells'
>
> + protocol@12:
> + type: object
> + properties:
> + reg:
> + const: 0x12
> +
Why? It did not got lost, it's already covered by pattern. If you refer
to particular warning, please paste it in commit msg. Otherwise it looks
incorrect.
Best regards,
Krzysztof
On Fri, Dec 23, 2022 at 11:11:27AM +0100, Krzysztof Kozlowski wrote:
> On 22/12/2022 19:38, Cristian Marussi wrote:
> > Add new SCMI Syspower protocol bindings definitions and example.
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > Got lost in translation probably...from txt to yaml
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
Hi Krzysztof,
thanks for the feedback and sorry I posted with an incomplete Cc list.
> > ---
> > .../devicetree/bindings/firmware/arm,scmi.yaml | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 1c0388da6721..f3dd77a470dd 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -111,6 +111,12 @@ properties:
> > required:
> > - '#power-domain-cells'
> >
> > + protocol@12:
> > + type: object
> > + properties:
> > + reg:
> > + const: 0x12
> > +
>
> Why? It did not got lost, it's already covered by pattern. If you refer
> to particular warning, please paste it in commit msg. Otherwise it looks
> incorrect.
>
Yes indeed, but as a matter of fact it seemed to me that we used to add an
entry and an example for all the currently published standard SCMI protocols,
even though already covered by the patternProp (which covers also any
custom-vendor protocol in the wild) and not sporting any additional
custom properties (see protocol@18), but maybe this is just a unneeded wrong
habit adding only cruft to the bindings.
If you think it does not add any value I can happily drop this, or
limiting the addition just to the example (and/or drop equally the unneeded
protocol@18 node too in this case).
Thanks,
Cristian
On 23/12/2022 11:37, Cristian Marussi wrote:
>>>
>>> + protocol@12:
>>> + type: object
>>> + properties:
>>> + reg:
>>> + const: 0x12
>>> +
>>
>> Why? It did not got lost, it's already covered by pattern. If you refer
>> to particular warning, please paste it in commit msg. Otherwise it looks
>> incorrect.
>>
>
> Yes indeed, but as a matter of fact it seemed to me that we used to add an
> entry and an example for all the currently published standard SCMI protocols,
> even though already covered by the patternProp (which covers also any
> custom-vendor protocol in the wild) and not sporting any additional
> custom properties (see protocol@18), but maybe this is just a unneeded wrong
> habit adding only cruft to the bindings.
>
> If you think it does not add any value I can happily drop this, or
> limiting the addition just to the example (and/or drop equally the unneeded
> protocol@18 node too in this case).
Duplicating the node (once in properties, second in patternProperties)
is not needed. I am also not sure what would be the point to add to the
example - example does not have to be complete DTS for all cases, but
illustrate the binding and allow is to test it.
Best regards,
Krzysztof
On Fri, Dec 23, 2022 at 12:22:34PM +0100, Krzysztof Kozlowski wrote:
> On 23/12/2022 11:37, Cristian Marussi wrote:
>
> >>>
> >>> + protocol@12:
> >>> + type: object
> >>> + properties:
> >>> + reg:
> >>> + const: 0x12
> >>> +
> >>
> >> Why? It did not got lost, it's already covered by pattern. If you refer
> >> to particular warning, please paste it in commit msg. Otherwise it looks
> >> incorrect.
> >>
> >
> > Yes indeed, but as a matter of fact it seemed to me that we used to add an
> > entry and an example for all the currently published standard SCMI protocols,
> > even though already covered by the patternProp (which covers also any
> > custom-vendor protocol in the wild) and not sporting any additional
> > custom properties (see protocol@18), but maybe this is just a unneeded wrong
> > habit adding only cruft to the bindings.
> >
> > If you think it does not add any value I can happily drop this, or
> > limiting the addition just to the example (and/or drop equally the unneeded
> > protocol@18 node too in this case).
>
> Duplicating the node (once in properties, second in patternProperties)
> is not needed. I am also not sure what would be the point to add to the
> example - example does not have to be complete DTS for all cases, but
> illustrate the binding and allow is to test it.
>
Thanks, I'll drop this patch.
Cristian