[PATCH] firmware: arm_scmi: fix voltage description in failure cases

[PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Etienne Carriere]

Reset the reception buffer max size when a voltage domain description
request fails, for example when the voltage domain returns an access
permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
word is read back for the remaining voltage description requests
responses leading to invalid information. The side effect of this
issue is that the voltage regulators registered from those remaining
SCMI voltage domain were assigned a wrong regulator name.

Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
 drivers/firmware/arm_scmi/voltage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 2175ffd6cef5..f1a7c04ae820 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -229,8 +229,10 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 		/* Retrieve domain attributes at first ... */
 		put_unaligned_le32(dom, td->tx.buf);
 		/* Skip domain on comms error */
-		if (ph->xops->do_xfer(ph, td))
+		if (ph->xops->do_xfer(ph, td)) {
+			ph->xops->reset_rx_to_maxsz(ph, td);
 			continue;
+		}
 
 		v = vinfo->domains + dom;
 		v->id = dom;
-- 
2.25.1

Re: [PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Cristian Marussi]

On Thu, Jul 25, 2024 at 08:53:17AM +0200, Etienne Carriere wrote:
> Reset the reception buffer max size when a voltage domain description

Hi Etienne,

Good catch, thanks for this.

Maybe a Fixes: too ?

Other than that, LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

> request fails, for example when the voltage domain returns an access
> permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
> word is read back for the remaining voltage description requests
> responses leading to invalid information. The side effect of this
> issue is that the voltage regulators registered from those remaining
> SCMI voltage domain were assigned a wrong regulator name.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> ---
>  drivers/firmware/arm_scmi/voltage.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> index 2175ffd6cef5..f1a7c04ae820 100644
> --- a/drivers/firmware/arm_scmi/voltage.c
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -229,8 +229,10 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
>  		/* Retrieve domain attributes at first ... */
>  		put_unaligned_le32(dom, td->tx.buf);
>  		/* Skip domain on comms error */
> -		if (ph->xops->do_xfer(ph, td))
> +		if (ph->xops->do_xfer(ph, td)) {
> +			ph->xops->reset_rx_to_maxsz(ph, td);
>  			continue;
> +		}
>  
>  		v = vinfo->domains + dom;
>  		v->id = dom;
> -- 
> 2.25.1
> 

Re: [PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Sudeep Holla]

On Thu, Jul 25, 2024 at 08:53:17AM +0200, Etienne Carriere wrote:
> Reset the reception buffer max size when a voltage domain description
> request fails, for example when the voltage domain returns an access
> permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
> word is read back for the remaining voltage description requests
> responses leading to invalid information. The side effect of this
> issue is that the voltage regulators registered from those remaining
> SCMI voltage domain were assigned a wrong regulator name.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> ---
>  drivers/firmware/arm_scmi/voltage.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> index 2175ffd6cef5..f1a7c04ae820 100644
> --- a/drivers/firmware/arm_scmi/voltage.c
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -229,8 +229,10 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
>  		/* Retrieve domain attributes at first ... */
>  		put_unaligned_le32(dom, td->tx.buf);
>  		/* Skip domain on comms error */
> -		if (ph->xops->do_xfer(ph, td))
> +		if (ph->xops->do_xfer(ph, td)) {
> +			ph->xops->reset_rx_to_maxsz(ph, td);

I am fine with this to keep it simple, but thought I will check my thoughts.
We usually use reset_rx_to_maxsz in iterators as we don't know the expected
size of the response, whereas here it must be max sizeof(*resp_dom).

That said, we don't have any helpers and changing xfer->rx.len directly
doesn't looks good ? Or may be it is OK ? Thoughts ?

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Cristian Marussi]

On Fri, Jul 26, 2024 at 04:16:03PM +0100, Sudeep Holla wrote:
> On Thu, Jul 25, 2024 at 08:53:17AM +0200, Etienne Carriere wrote:
> > Reset the reception buffer max size when a voltage domain description
> > request fails, for example when the voltage domain returns an access
> > permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
> > word is read back for the remaining voltage description requests
> > responses leading to invalid information. The side effect of this
> > issue is that the voltage regulators registered from those remaining
> > SCMI voltage domain were assigned a wrong regulator name.
> > 
> > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > ---
> >  drivers/firmware/arm_scmi/voltage.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> > index 2175ffd6cef5..f1a7c04ae820 100644
> > --- a/drivers/firmware/arm_scmi/voltage.c
> > +++ b/drivers/firmware/arm_scmi/voltage.c
> > @@ -229,8 +229,10 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
> >  		/* Retrieve domain attributes at first ... */
> >  		put_unaligned_le32(dom, td->tx.buf);
> >  		/* Skip domain on comms error */
> > -		if (ph->xops->do_xfer(ph, td))
> > +		if (ph->xops->do_xfer(ph, td)) {
> > +			ph->xops->reset_rx_to_maxsz(ph, td);
> 
> I am fine with this to keep it simple, but thought I will check my thoughts.
> We usually use reset_rx_to_maxsz in iterators as we don't know the expected
> size of the response, whereas here it must be max sizeof(*resp_dom).
> 
> That said, we don't have any helpers and changing xfer->rx.len directly
> doesn't looks good ? Or may be it is OK ? Thoughts ?

We do not access those xfer internal field explicitly from the protocol layer
(beside once in Base)...and surely not on write....in the past I was even
tempted to try to make these internal stuff untouchable by the protocol layer...
...that's the reason of course for the existence of reset_rx_to_maxsz() helpers
....not sure if it is worth adding another helper for this, given that the
using the maxsz should have any adverse effect (unless I am missing
something of course :P).

This kind of 'issues' are really common whenever in the SCMI stack we
reuse the same allocated xfer across multiple do_xfers in a loop
(reusing seq_nums is another thing...) since we wanted to avoid the
penalty of resetting some of these automatically on each do_xfer()...

....we could think of some mechanism to transparently reset/fill such xfer
fields automatically if the core detects a 'reuse'....got to check first,
though, if this does not break some of the current usage patterns...and
I would not say it is a high prio thing to explore as of now...

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Sudeep Holla]

On Thu, Aug 01, 2024 at 11:58:19AM +0100, Cristian Marussi wrote:
> On Fri, Jul 26, 2024 at 04:16:03PM +0100, Sudeep Holla wrote:
> > On Thu, Jul 25, 2024 at 08:53:17AM +0200, Etienne Carriere wrote:
> > > Reset the reception buffer max size when a voltage domain description
> > > request fails, for example when the voltage domain returns an access
> > > permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
> > > word is read back for the remaining voltage description requests
> > > responses leading to invalid information. The side effect of this
> > > issue is that the voltage regulators registered from those remaining
> > > SCMI voltage domain were assigned a wrong regulator name.
> > > 
> > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > ---
> > >  drivers/firmware/arm_scmi/voltage.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
> > > index 2175ffd6cef5..f1a7c04ae820 100644
> > > --- a/drivers/firmware/arm_scmi/voltage.c
> > > +++ b/drivers/firmware/arm_scmi/voltage.c
> > > @@ -229,8 +229,10 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
> > >  		/* Retrieve domain attributes at first ... */
> > >  		put_unaligned_le32(dom, td->tx.buf);
> > >  		/* Skip domain on comms error */
> > > -		if (ph->xops->do_xfer(ph, td))
> > > +		if (ph->xops->do_xfer(ph, td)) {
> > > +			ph->xops->reset_rx_to_maxsz(ph, td);
> > 
> > I am fine with this to keep it simple, but thought I will check my thoughts.
> > We usually use reset_rx_to_maxsz in iterators as we don't know the expected
> > size of the response, whereas here it must be max sizeof(*resp_dom).
> > 
> > That said, we don't have any helpers and changing xfer->rx.len directly
> > doesn't looks good ? Or may be it is OK ? Thoughts ?
> 
> We do not access those xfer internal field explicitly from the protocol layer
> (beside once in Base)...and surely not on write....in the past I was even
> tempted to try to make these internal stuff untouchable by the protocol layer...
> ...that's the reason of course for the existence of reset_rx_to_maxsz() helpers
> ....not sure if it is worth adding another helper for this, given that the
> using the maxsz should have any adverse effect (unless I am missing
> something of course :P).
> 
> This kind of 'issues' are really common whenever in the SCMI stack we
> reuse the same allocated xfer across multiple do_xfers in a loop
> (reusing seq_nums is another thing...) since we wanted to avoid the
> penalty of resetting some of these automatically on each do_xfer()...
> 
> ....we could think of some mechanism to transparently reset/fill such xfer
> fields automatically if the core detects a 'reuse'....got to check first,
> though, if this does not break some of the current usage patterns...and
> I would not say it is a high prio thing to explore as of now...
> 

Fair enough, I will merge this as is. I think it should be fine. My suggestion
might simply complicates things unnecessarily. Lets not do it unless this
becomes repeating pattern.

-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: fix voltage description in failure cases

[Sudeep Holla]

On Thu, 25 Jul 2024 08:53:17 +0200, Etienne Carriere wrote:
> Reset the reception buffer max size when a voltage domain description
> request fails, for example when the voltage domain returns an access
> permission error (SCMI_ERR_ACCESS) unless what only a single 32bit
> word is read back for the remaining voltage description requests
> responses leading to invalid information. The side effect of this
> issue is that the voltage regulators registered from those remaining
> SCMI voltage domain were assigned a wrong regulator name.
>
> [...]

Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!

[1/1] firmware: arm_scmi: fix voltage description in failure cases
      https://git.kernel.org/sudeep.holla/c/eedc060cff72
--
Regards,
Sudeep