[PATCH 0/5] Miscellaneous SCMI fixes for v6.2

[PATCH 0/5] Miscellaneous SCMI fixes for v6.2

[Cristian Marussi]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] firmware: arm_scmi: Clear stale xfer->hdr.status

[Cristian Marussi]

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 <cristian.marussi@arm.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] firmware: arm_scmi: Harden shared memory access in fetch_response

[Cristian Marussi]

A misbheaving SCMI platform firmware could reply with out-of-spec messages,
shorter than the mimimum size comprising a header and a status field.

Harden shmem_fetch_response to properly truncate such a bad messages.

Fixes: 5c8a47a5a91d ("firmware: arm_scmi: Make scmi core independent of the transport type")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/shmem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c
index 1dfe534b8518..135f8718000f 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -81,10 +81,11 @@ u32 shmem_read_header(struct scmi_shared_mem __iomem *shmem)
 void shmem_fetch_response(struct scmi_shared_mem __iomem *shmem,
 			  struct scmi_xfer *xfer)
 {
+	size_t len = ioread32(&shmem->length);
+
 	xfer->hdr.status = ioread32(shmem->msg_payload);
 	/* Skip the length of header and status in shmem area i.e 8 bytes */
-	xfer->rx.len = min_t(size_t, xfer->rx.len,
-			     ioread32(&shmem->length) - 8);
+	xfer->rx.len = min_t(size_t, xfer->rx.len, len > 8 ? len - 8 : 0);
 
 	/* Take a copy to the rx buffer.. */
 	memcpy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer->rx.len);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] firmware: arm_scmi: Harden shared memory access in fetch_notification

[Cristian Marussi]

A misbheaving SCMI platform firmware could reply with out-of-spec
notifications, shorter than the mimimum size comprising a header.

Fixes: d5141f37c42e ("firmware: arm_scmi: Add notifications support in transport layer")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/shmem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c
index 135f8718000f..87b4f4d35f06 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -94,8 +94,10 @@ void shmem_fetch_response(struct scmi_shared_mem __iomem *shmem,
 void shmem_fetch_notification(struct scmi_shared_mem __iomem *shmem,
 			      size_t max_len, struct scmi_xfer *xfer)
 {
+	size_t len = ioread32(&shmem->length);
+
 	/* Skip only the length of header in shmem area i.e 4 bytes */
-	xfer->rx.len = min_t(size_t, max_len, ioread32(&shmem->length) - 4);
+	xfer->rx.len = min_t(size_t, max_len, len > 4 ? len - 4 : 0);
 
 	/* Take a copy to the rx buffer.. */
 	memcpy_fromio(xfer->rx.buf, shmem->msg_payload, xfer->rx.len);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] dt-bindings: firmware: arm,scmi: Add support for syspower protocol

[Cristian Marussi]

Add new SCMI Syspower protocol bindings definitions and example.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings: firmware: arm,scmi: Add support for syspower protocol

[Krzysztof Kozlowski]

On 22/12/2022 19:38, Cristian Marussi wrote:
> Add new SCMI Syspower protocol bindings definitions and example.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings: firmware: arm,scmi: Add support for syspower protocol

[Cristian Marussi]

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 <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings: firmware: arm,scmi: Add support for syspower protocol

[Krzysztof Kozlowski]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] dt-bindings: firmware: arm,scmi: Add support for syspower protocol

[Cristian Marussi]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] firmware: arm_scmi: Fix virtio channels cleanup on shutdown

[Cristian Marussi]

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 <cristian.marussi@arm.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel