diff mbox series

[v4,1/3] mctp pcc: Check before sending MCTP PCC response ACK

Message ID 20240702225845.322234-2-admiyo@os.amperecomputing.com (mailing list archive)
State Changes Requested
Headers show
Series MCTP over PCC | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 839 this patch: 840
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: rafael@kernel.org linux-acpi@vger.kernel.org acpica-devel@lists.linux.dev
netdev/build_clang fail Errors and warnings before: 846 this patch: 847
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 853 this patch: 855
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 75 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 0 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

admiyo@os.amperecomputing.com July 2, 2024, 10:58 p.m. UTC 
From: Adam Young <admiyo@os.amperecomputing.com>

Type 4 PCC channels have an option to send back a response
to the platform when they are done processing the request.
The flag to indicate whether or not to respond is inside
the message body, and thus is not available to the pcc
mailbox.

In order to read the flag, this patch maps the shared
buffer to virtual memory.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/mailbox/pcc.c | 31 +++++++++++++++++++++++--------
 include/acpi/pcc.h    |  8 ++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

Comments

Simon Horman July 3, 2024, 7:52 p.m. UTC | #1 
On Tue, Jul 02, 2024 at 06:58:43PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Type 4 PCC channels have an option to send back a response
> to the platform when they are done processing the request.
> The flag to indicate whether or not to respond is inside
> the message body, and thus is not available to the pcc
> mailbox.
> 
> In order to read the flag, this patch maps the shared
> buffer to virtual memory.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/mailbox/pcc.c | 31 +++++++++++++++++++++++--------
>  include/acpi/pcc.h    |  8 ++++++++
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..cad6b5bc4b04 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -107,6 +107,7 @@ struct pcc_chan_info {
>  	struct pcc_chan_reg cmd_complete;
>  	struct pcc_chan_reg cmd_update;
>  	struct pcc_chan_reg error;
> +	void __iomem *shmem_base_addr;

Hi Adam,

Please add an entry for shmem_base_addr to the Kernel doc for this
structure, which appears just above the structure.

Flagged by ./scripts/kernel-doc -none

>  	int plat_irq;
>  	u8 type;
>  	unsigned int plat_irq_flags;
> @@ -269,6 +270,24 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>  	return !!val;
>  }
>  
> +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +{
> +	struct pcc_extended_type_hdr pcc_hdr;
> +
> +	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		return;
> +	memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
> +		      sizeof(struct pcc_extended_type_hdr));
> +	/*
> +	 * The PCC slave subspace channel needs to set the command complete bit
> +	 * and ring doorbell after processing message.
> +	 *
> +	 * The PCC master subspace channel clears chan_in_use to free channel.
> +	 */
> +	if (!!le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)

Should this be:

	if (!!(le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK))

In which case I think it can be more simply expressed as:

	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)

Flagged by Smatch and Sparse.

> +		pcc_send_data(chan, NULL);
> +}
> +
>  /**
>   * pcc_mbox_irq - PCC mailbox interrupt handler
>   * @irq:	interrupt number

...

> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h

...

> @@ -31,6 +38,7 @@ struct pcc_mbox_chan {
>  #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
>  
>  #define MAX_PCC_SUBSPACES	256
> +#define PCC_ACK_FLAG_MASK       0x1

nit: For consistency please follow the white-space pattern of the preceding
     line: please use a tab rathe rthan spaces between ...MASK and 0x1.

>  
>  #ifdef CONFIG_PCC
>  extern struct pcc_mbox_chan *
> -- 
> 2.34.1
> 
>
kernel test robot July 4, 2024, 2:44 a.m. UTC | #2 
Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc6 next-20240703]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240703-163558
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240702225845.322234-2-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 1/3] mctp pcc: Check before sending MCTP PCC response ACK
config: i386-buildonly-randconfig-004-20240704 (https://download.01.org/0day-ci/archive/20240704/202407041052.926ISbwR-lkp@intel.com/config)
compiler: gcc-12 (Ubuntu 12.3.0-9ubuntu2) 12.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041052.926ISbwR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407041052.926ISbwR-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mailbox/pcc.c:115: warning: Function parameter or struct member 'shmem_base_addr' not described in 'pcc_chan_info'


vim +115 drivers/mailbox/pcc.c

800cda7b63f22b Sudeep Holla 2021-09-17   82  
80b2bdde002c52 Sudeep Holla 2021-09-17   83  /**
80b2bdde002c52 Sudeep Holla 2021-09-17   84   * struct pcc_chan_info - PCC channel specific information
80b2bdde002c52 Sudeep Holla 2021-09-17   85   *
0f2591e21b2e85 Sudeep Holla 2021-09-17   86   * @chan: PCC channel information with Shared Memory Region info
bf18123e78f4d1 Sudeep Holla 2021-09-17   87   * @db: PCC register bundle for the doorbell register
bf18123e78f4d1 Sudeep Holla 2021-09-17   88   * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
bf18123e78f4d1 Sudeep Holla 2021-09-17   89   *	register
c45ded7e11352d Sudeep Holla 2021-09-17   90   * @cmd_complete: PCC register bundle for the command complete check register
c45ded7e11352d Sudeep Holla 2021-09-17   91   * @cmd_update: PCC register bundle for the command complete update register
c45ded7e11352d Sudeep Holla 2021-09-17   92   * @error: PCC register bundle for the error status register
f92ae90e52bb09 Sudeep Holla 2021-09-17   93   * @plat_irq: platform interrupt
60c40b06fa6869 Huisong Li   2023-08-01   94   * @type: PCC subspace type
3db174e478cb0b Huisong Li   2023-08-01   95   * @plat_irq_flags: platform interrupt flags
3db174e478cb0b Huisong Li   2023-08-01   96   * @chan_in_use: this flag is used just to check if the interrupt needs
3db174e478cb0b Huisong Li   2023-08-01   97   *		handling when it is shared. Since only one transfer can occur
3db174e478cb0b Huisong Li   2023-08-01   98   *		at a time and mailbox takes care of locking, this flag can be
3db174e478cb0b Huisong Li   2023-08-01   99   *		accessed without a lock. Note: the type only support the
3db174e478cb0b Huisong Li   2023-08-01  100   *		communication from OSPM to Platform, like type3, use it, and
3db174e478cb0b Huisong Li   2023-08-01  101   *		other types completely ignore it.
80b2bdde002c52 Sudeep Holla 2021-09-17  102   */
80b2bdde002c52 Sudeep Holla 2021-09-17  103  struct pcc_chan_info {
0f2591e21b2e85 Sudeep Holla 2021-09-17  104  	struct pcc_mbox_chan chan;
bf18123e78f4d1 Sudeep Holla 2021-09-17  105  	struct pcc_chan_reg db;
bf18123e78f4d1 Sudeep Holla 2021-09-17  106  	struct pcc_chan_reg plat_irq_ack;
c45ded7e11352d Sudeep Holla 2021-09-17  107  	struct pcc_chan_reg cmd_complete;
c45ded7e11352d Sudeep Holla 2021-09-17  108  	struct pcc_chan_reg cmd_update;
c45ded7e11352d Sudeep Holla 2021-09-17  109  	struct pcc_chan_reg error;
ddd70a454be0e5 Adam Young   2024-07-02  110  	void __iomem *shmem_base_addr;
f92ae90e52bb09 Sudeep Holla 2021-09-17  111  	int plat_irq;
60c40b06fa6869 Huisong Li   2023-08-01  112  	u8 type;
3db174e478cb0b Huisong Li   2023-08-01  113  	unsigned int plat_irq_flags;
3db174e478cb0b Huisong Li   2023-08-01  114  	bool chan_in_use;
80b2bdde002c52 Sudeep Holla 2021-09-17 @115  };
80b2bdde002c52 Sudeep Holla 2021-09-17  116
kernel test robot July 4, 2024, 8:53 a.m. UTC | #3 
Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc6 next-20240703]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/admiyo-os-amperecomputing-com/mctp-pcc-Check-before-sending-MCTP-PCC-response-ACK/20240703-163558
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240702225845.322234-2-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 1/3] mctp pcc: Check before sending MCTP PCC response ACK
config: i386-randconfig-061-20240704 (https://download.01.org/0day-ci/archive/20240704/202407041613.PPjZeDpu-lkp@intel.com/config)
compiler: gcc-12 (Ubuntu 12.3.0-9ubuntu2) 12.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041613.PPjZeDpu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407041613.PPjZeDpu-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/mailbox/pcc.c:287:44: sparse: sparse: dubious: !x & y

vim +287 drivers/mailbox/pcc.c

   272	
   273	static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
   274	{
   275		struct pcc_extended_type_hdr pcc_hdr;
   276	
   277		if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
   278			return;
   279		memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
   280			      sizeof(struct pcc_extended_type_hdr));
   281		/*
   282		 * The PCC slave subspace channel needs to set the command complete bit
   283		 * and ring doorbell after processing message.
   284		 *
   285		 * The PCC master subspace channel clears chan_in_use to free channel.
   286		 */
 > 287		if (!!le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
   288			pcc_send_data(chan, NULL);
   289	}
   290
Jeremy Kerr July 4, 2024, 9:40 a.m. UTC | #4 
Hi Adam,

> +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +{
> +       struct pcc_extended_type_hdr pcc_hdr;
> +
> +       if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +               return;
> +       memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
> +                     sizeof(struct pcc_extended_type_hdr));
> +       /*
> +        * The PCC slave subspace channel needs to set the command complete bit
> +        * and ring doorbell after processing message.
> +        *
> +        * The PCC master subspace channel clears chan_in_use to free channel.
> +        */
> +       if (!!le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)

This should be:

       if (!!(le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK))

- otherwise you're bitwise testing the result of the '!!'.

Cheers,


Jeremy
diff mbox series

Patch

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..cad6b5bc4b04 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -107,6 +107,7 @@  struct pcc_chan_info {
 	struct pcc_chan_reg cmd_complete;
 	struct pcc_chan_reg cmd_update;
 	struct pcc_chan_reg error;
+	void __iomem *shmem_base_addr;
 	int plat_irq;
 	u8 type;
 	unsigned int plat_irq_flags;
@@ -269,6 +270,24 @@  static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
 	return !!val;
 }
 
+static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
+{
+	struct pcc_extended_type_hdr pcc_hdr;
+
+	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		return;
+	memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
+		      sizeof(struct pcc_extended_type_hdr));
+	/*
+	 * The PCC slave subspace channel needs to set the command complete bit
+	 * and ring doorbell after processing message.
+	 *
+	 * The PCC master subspace channel clears chan_in_use to free channel.
+	 */
+	if (!!le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
+		pcc_send_data(chan, NULL);
+}
+
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -306,14 +325,7 @@  static irqreturn_t pcc_mbox_irq(int irq, void *p)
 
 	mbox_chan_received_data(chan, NULL);
 
-	/*
-	 * The PCC slave subspace channel needs to set the command complete bit
-	 * and ring doorbell after processing message.
-	 *
-	 * The PCC master subspace channel clears chan_in_use to free channel.
-	 */
-	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
-		pcc_send_data(chan, NULL);
+	check_and_ack(pchan, chan);
 	pchan->chan_in_use = false;
 
 	return IRQ_HANDLED;
@@ -352,6 +364,9 @@  pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (rc)
 		return ERR_PTR(rc);
 
+	pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
+					      pchan->chan.shmem_base_addr,
+					      pchan->chan.shmem_size);
 	return &pchan->chan;
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 9b373d172a77..6cb21f29d343 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -18,6 +18,13 @@  struct pcc_mbox_chan {
 	u16 min_turnaround_time;
 };
 
+struct pcc_extended_type_hdr {
+	__le32 signature;
+	__le32 flags;
+	__le32 length;
+	char command[4];
+};
+
 /* Generic Communications Channel Shared Memory Region */
 #define PCC_SIGNATURE			0x50434300
 /* Generic Communications Channel Command Field */
@@ -31,6 +38,7 @@  struct pcc_mbox_chan {
 #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
 
 #define MAX_PCC_SUBSPACES	256
+#define PCC_ACK_FLAG_MASK       0x1
 
 #ifdef CONFIG_PCC
 extern struct pcc_mbox_chan *