Buggy firmware can reply with duplicated PERF opps descriptors.

Ensure that the bad duplicates reported by the platform firmware doesn't
get added to the opp-tables.

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
A new version to include in this series that should address the limit case
described by Sibi...not tested, of course :P
---
 drivers/firmware/arm_scmi/perf.c | 40 ++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 2d77b5f40ca7..32f9a9acd3e9 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -373,7 +373,7 @@ static int iter_perf_levels_update_state(struct scmi_iterator_state *st,
 	return 0;
 }
 
-static inline void
+static inline int
 process_response_opp(struct device *dev, struct perf_dom_info *dom,
 		     struct scmi_opp *opp, unsigned int loop_idx,
 		     const struct scmi_msg_resp_perf_describe_levels *r)
@@ -386,12 +386,16 @@ process_response_opp(struct device *dev, struct perf_dom_info *dom,
 		le16_to_cpu(r->opp[loop_idx].transition_latency_us);
 
 	ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n",
 			 opp->perf, dom->info.name, ret);
+		return ret;
+	}
+
+	return 0;
 }
 
-static inline void
+static inline int
 process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 			struct scmi_opp *opp, unsigned int loop_idx,
 			const struct scmi_msg_resp_perf_describe_levels_v4 *r)
@@ -404,9 +408,11 @@ process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 		le16_to_cpu(r->opp[loop_idx].transition_latency_us);
 
 	ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n",
 			 opp->perf, dom->info.name, ret);
+		return ret;
+	}
 
 	/* Note that PERF v4 reports always five 32-bit words */
 	opp->indicative_freq = le32_to_cpu(r->opp[loop_idx].indicative_freq);
@@ -415,13 +421,21 @@ process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 
 		ret = xa_insert(&dom->opps_by_idx, opp->level_index, opp,
 				GFP_KERNEL);
-		if (ret)
+		if (ret) {
 			dev_warn(dev,
 				 "Failed to add opps_by_idx at %d for %s - ret:%d\n",
 				 opp->level_index, dom->info.name, ret);
 
+			/* Cleanup by_lvl too */
+			xa_erase(&dom->opps_by_lvl, opp->perf);
+
+			return ret;
+		}
+
 		hash_add(dom->opps_by_freq, &opp->hash, opp->indicative_freq);
 	}
+
+	return 0;
 }
 
 static int
@@ -429,16 +443,22 @@ iter_perf_levels_process_response(const struct scmi_protocol_handle *ph,
 				  const void *response,
 				  struct scmi_iterator_state *st, void *priv)
 {
+	int ret;
 	struct scmi_opp *opp;
 	struct scmi_perf_ipriv *p = priv;
 
-	opp = &p->perf_dom->opp[st->desc_index + st->loop_idx];
+	opp = &p->perf_dom->opp[p->perf_dom->opp_count];
 	if (PROTOCOL_REV_MAJOR(p->version) <= 0x3)
-		process_response_opp(ph->dev, p->perf_dom, opp, st->loop_idx,
-				     response);
+		ret = process_response_opp(ph->dev, p->perf_dom, opp,
+					   st->loop_idx, response);
 	else
-		process_response_opp_v4(ph->dev, p->perf_dom, opp, st->loop_idx,
-					response);
+		ret = process_response_opp_v4(ph->dev, p->perf_dom, opp,
+					      st->loop_idx, response);
+
+	/* Skip BAD duplicates received from firmware */
+	if (ret)
+		return ret == -EBUSY ? 0 : ret;
+
 	p->perf_dom->opp_count++;
 
 	dev_dbg(ph->dev, "Level %d Power %d Latency %dus Ifreq %d Index %d\n",
-- 
2.46.1

On Wed, Oct 09, 2024 at 03:39:05PM +0100, Cristian Marussi wrote:
> Buggy firmware can reply with duplicated PERF opps descriptors.
> 
> Ensure that the bad duplicates reported by the platform firmware doesn't
> get added to the opp-tables.
>

Hi Sibi,

Feel free to add my review tag when you post this as part of the next
version of the series.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

[ There appears to be something wrong with you mail setup as the patch
  had a bogus Reply-To header:

  Reply-To: 20241007060642.1978049-3-quic_sibis@quicinc.com
]

On Wed, Oct 09, 2024 at 03:39:05PM +0100, Cristian Marussi wrote:
> Buggy firmware can reply with duplicated PERF opps descriptors.
> 
> Ensure that the bad duplicates reported by the platform firmware doesn't
> get added to the opp-tables.
> 
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> A new version to include in this series that should address the limit case
> described by Sibi...not tested, of course :P

Thanks for the fix. This seems to do the trick and gets rid of the
warnings about duplicate entries on boot and resume (including the
debugfs errors).

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan

On Thu, Oct 10, 2024 at 04:43:06PM +0200, Johan Hovold wrote:
> [ There appears to be something wrong with you mail setup as the patch
>   had a bogus Reply-To header:
> 
>   Reply-To: 20241007060642.1978049-3-quic_sibis@quicinc.com
> ]
> 

Hi Johan,

unfortunately the setup that is wrong is indeed on the other side of the
screen (i.e. in my brain :D)...since I mistakenly added a --reply-to= instead
of the --in-reply-to= when trying to post with git send-email this patch
against Sibi original thread (which is the bogus message id above)

Apologies...

> On Wed, Oct 09, 2024 at 03:39:05PM +0100, Cristian Marussi wrote:
> > Buggy firmware can reply with duplicated PERF opps descriptors.
> > 
> > Ensure that the bad duplicates reported by the platform firmware doesn't
> > get added to the opp-tables.
> > 
> > Reported-by: Johan Hovold <johan+linaro@kernel.org>
> > Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > A new version to include in this series that should address the limit case
> > described by Sibi...not tested, of course :P
> 
> Thanks for the fix. This seems to do the trick and gets rid of the
> warnings about duplicate entries on boot and resume (including the
> debugfs errors).
> 
> Tested-by: Johan Hovold <johan+linaro@kernel.org>

Thanks for testing !

Cristian