Received-SPF: pass (google.com: domain of linux-kernel+bounces-65853-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 14 Feb 2024 10:59:25 -0800
From: Elliot Berman <quic_eberman@quicinc.com>
To: Bjorn Andersson <andersson@kernel.org>
CC: Maulik Shah <quic_mkshah@quicinc.com>,
        Konrad Dybcio
	<konrad.dybcio@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_collinsd@quicinc.com>, <quic_lsrao@quicinc.com>,
        <stable@vger.kernel.org>
Subject: Re: Re: [PATCH v3] soc: qcom: rpmh-rsc: Enhance check for VRM
 in-flight request
Message-ID: <20240214105605460-0800.eberman@hu-eberman-lv.qualcomm.com>
Mail-Followup-To: Bjorn Andersson <andersson@kernel.org>, 
	Maulik Shah <quic_mkshah@quicinc.com>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, quic_collinsd@quicinc.com, 
	quic_lsrao@quicinc.com, stable@vger.kernel.org
References: <20240212-rpmh-rsc-fixes-v3-1-1be0d705dbb5@quicinc.com>
 <sizizst7xkexl3dd26sssgxtjhk7mcrawswbs76vdutsxsm6qh@mvilvzwydjpm>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <sizizst7xkexl3dd26sssgxtjhk7mcrawswbs76vdutsxsm6qh@mvilvzwydjpm>

On Wed, Feb 14, 2024 at 12:08:43AM -0600, Bjorn Andersson wrote:
> On Mon, Feb 12, 2024 at 10:18:08AM +0530, Maulik Shah wrote:
> > Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte aligned
> > addresses associated with it. These control voltage, enable state, mode,
> > and in legacy targets, voltage headroom. The current in-flight request
> > checking logic looks for exact address matches. Requests for different
> > addresses of the same RPMh resource as thus not detected as in-flight.
> > 
> > Add new cmd-db API cmd_db_match_resource_addr() to enhance the in-flight
> > request check for VRM requests by ignoring the address offset.
> > 
> > This ensures that only one request is allowed to be in-flight for a given
> > VRM resource. This is needed to avoid scenarios where request commands are
> > carried out by RPMh hardware out-of-order leading to LDO regulator
> > over-current protection triggering.
> > 
> > Fixes: 658628e7ef78 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs")
> > cc: stable@vger.kernel.org
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
> 
> This says, "Elliot first certified the origin of the patch, then Maulik
> took and certified the origin of the patch". But according to the From:
> the author of the patch is you, Maulik.
> 
> How was Elliot able to certify the patch's origin before you, when
> you're the author?
> 
> If the two of you collaborated, also add Co-developed-by: Elliot above
> his s-o-b.
> 

Even my Co-developed-by is being generous :-) All I had done was copy
the commit and resolve couple merge conflicts.

Maulik, you can swap my S-o-B for a:

Tested-by: Elliot Berman <quic_eberman@quicinc.com> # sm8650-qrd

> > ---
> > Changes in v3:
> > - Fix s-o-b chain
> > - Add cmd-db API to compare addresses
> > - Reuse already defined resource types in cmd-db
> > - Add Fixes tag and Cc to stable
> > - Retain Reviewed-by tag of v2
> > - Link to v2: https://lore.kernel.org/r/20240119-rpmh-rsc-fixes-v2-1-e42c0a9e36f0@quicinc.com
> > Changes in v2:
> > - Use GENMASK() and FIELD_GET()
> > - Link to v1: https://lore.kernel.org/r/20240117-rpmh-rsc-fixes-v1-1-71ee4f8f72a4@quicinc.com
> > ---
> >  drivers/soc/qcom/cmd-db.c   | 41 +++++++++++++++++++++++++++++++++++------
> >  drivers/soc/qcom/rpmh-rsc.c |  3 ++-
> >  include/soc/qcom/cmd-db.h   | 10 +++++++++-
> >  3 files changed, 46 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> > index a5fd68411bed..e87682b9755e 100644
> > --- a/drivers/soc/qcom/cmd-db.c
> > +++ b/drivers/soc/qcom/cmd-db.c
> > @@ -1,6 +1,10 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > -/* Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved. */
> > +/*
> > + * Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -15,8 +19,8 @@
> >  
> >  #define NUM_PRIORITY		2
> >  #define MAX_SLV_ID		8
> > -#define SLAVE_ID_MASK		0x7
> > -#define SLAVE_ID_SHIFT		16
> > +#define SLAVE_ID(addr)		FIELD_GET(GENMASK(19, 16), addr)
> > +#define VRM_ADDR(addr)		FIELD_GET(GENMASK(19, 4), addr)
> >  
> >  /**
> >   * struct entry_header: header for each entry in cmddb
> > @@ -221,9 +225,34 @@ const void *cmd_db_read_aux_data(const char *id, size_t *len)
> >  EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
> >  
> >  /**
> > - * cmd_db_read_slave_id - Get the slave ID for a given resource address
> > + * cmd_db_match_resource_addr - Compare if both Resource addresses are same
> 
> () after the function name, please.
> 
> > + *
> > + * @addr1: Resource address to compare
> > + * @addr2: Resource address to compare
> > + *
> > + * Return: true on matching addresses, false otherwise
> 
> "Return: true if the two addresses refer to the same resource"
> 
> > + */
> > +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
> > +{
> > +	/*
> > +	 * Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte
> > +	 * aligned addresses associated with it. Ignore the offset to check
> > +	 * for VRM requests.
> > +	 */
> > +	if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
> > +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
> 
> One line please, it's just 83 characters.
> 
> > +		return true;
> > +	else if (addr1 == addr2)
> > +		return true;
> > +	else
> > +		return false;
> > +}
> > +EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
> > +
> > +/**
> > + * cmd_db_read_slave_id - Get the slave ID for a given resource name
> >   *
> > - * @id: Resource id to query the DB for version
> > + * @id: Resource id to query the DB for slave id
> 
> Although trivial, it's unrelated to the newly introduced logic. Please
> submit a separate patch. Please also then add the () after the function
> name.
> 
> Regards,
> Bjorn
> 
> >   *
> >   * Return: cmd_db_hw_type enum on success, CMD_DB_HW_INVALID on error
> >   */
> > @@ -238,7 +267,7 @@ enum cmd_db_hw_type cmd_db_read_slave_id(const char *id)
> >  		return CMD_DB_HW_INVALID;
> >  
> >  	addr = le32_to_cpu(ent->addr);
> > -	return (addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
> > +	return SLAVE_ID(addr);
> >  }
> >  EXPORT_SYMBOL_GPL(cmd_db_read_slave_id);
> >  
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index a021dc71807b..daf64be966fe 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> >   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
> >   */
> >  
> >  #define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
> > @@ -557,7 +558,7 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> >  		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> >  			addr = read_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_ADDR], i, j);
> >  			for (k = 0; k < msg->num_cmds; k++) {
> > -				if (addr == msg->cmds[k].addr)
> > +				if (cmd_db_match_resource_addr(msg->cmds[k].addr, addr))
> >  					return -EBUSY;
> >  			}
> >  		}
> > diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
> > index c8bb56e6852a..47a6cab75e63 100644
> > --- a/include/soc/qcom/cmd-db.h
> > +++ b/include/soc/qcom/cmd-db.h
> > @@ -1,5 +1,8 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > -/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. */
> > +/*
> > + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> >  
> >  #ifndef __QCOM_COMMAND_DB_H__
> >  #define __QCOM_COMMAND_DB_H__
> > @@ -21,6 +24,8 @@ u32 cmd_db_read_addr(const char *resource_id);
> >  
> >  const void *cmd_db_read_aux_data(const char *resource_id, size_t *len);
> >  
> > +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2);
> > +
> >  enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id);
> >  
> >  int cmd_db_ready(void);
> > @@ -31,6 +36,9 @@ static inline u32 cmd_db_read_addr(const char *resource_id)
> >  static inline const void *cmd_db_read_aux_data(const char *resource_id, size_t *len)
> >  { return ERR_PTR(-ENODEV); }
> >  
> > +static inline bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
> > +{ return false; }
> > +
> >  static inline enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id)
> >  { return -ENODEV; }
> >  
> > 
> > ---
> > base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
> > change-id: 20240210-rpmh-rsc-fixes-372a79ab364b
> > 
> > Best regards,
> > -- 
> > Maulik Shah <quic_mkshah@quicinc.com>
> >