[PATCH v4 0/3] MCTP over PCC

[PATCH v4 0/3] MCTP over PCC

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

This series adds support for the Management Control Transport Protocol (MCTP)
over the Platform Communication Channel (PCC) mechanism.

MCTP defines a communication model intended to
facilitate communication between Management controllers
and other management controllers, and between Management
controllers and management devices

PCC is a mechanism for communication between components within
the  Platform.  It is a composed of shared memory regions,
interrupt registers, and status registers.

The MCTP over PCC driver makes use of two PCC channels. For
sending messages, it uses a Type 3 channel, and for receiving
messages it uses the paired Type 4 channel.  The device
and corresponding channels are specified via ACPI.

The first patch in the series implements a mechanism to allow the driver
to indicate whether an ACK should be sent back to the caller
after processing the interrupt.  This is an optional feature in
the PCC code, but has been made explicitly required in another driver.
The implementation here maintains the backwards compatibility of that
driver.

The second patch in the series is the required change from ACPICA
code that will be imported into the Linux kernel when synchronized
with the ACPICA repository. It ahs already merged there and will
be merged in as is.  It is included here so that the patch series
can run and be tested prior to that merge.

Previous Version:
https://lore.kernel.org/20240619200552.119080-1-admiyo@os.amperecomputing.com/

Changes in V4
- Read flags out of shared buffer to trigger ACK for Type 4 RX
- Remove list of netdevs and cleanup from devices only
- tag PCCT protocol headers as little endian
- Remove unused constants

Changes in V3
- removed unused header
- removed spurious space
- removed spurious semis after functiomns
- removed null assignment for init
- remove redundant set of device on skb
- tabify constant declarations
- added  rtnl_link_stats64 function
- set MTU to minimum to start
- clean up logic on driver removal
- remove cast on void * assignment
- call cleanup function directly
- check received length before allocating skb
- introduce symbolic constatn for ACK FLAG MASK
- symbolic constant for PCC header flag.
- Add namespace ID to PCC magic
- replaced readls with copy from io of PCC header
- replaced custom modules init and cleanup with ACPI version

Changes in V2

- All Variable Declarations are in reverse Xmass Tree Format
- All Checkpatch Warnings Are Fixed
- Removed Dead code
- Added packet tx/rx stats
- Removed network physical address.  This is still in
  disucssion in the spec, and will be added once there
  is consensus. The protocol can be used with out it.
  This also lead to the removal of the Big Endian
  conversions.
- Avoided using non volatile pointers in copy to and from io space
- Reorderd the patches to put the ACK check for the PCC Mailbox
  as a pre-requisite.  The corresponding change for the MCTP
  driver has been inlined in the main patch.
- Replaced magic numbers with constants, fixed typos, and other
  minor changes from code review.

Code Review Change not made

- Did not change the module init unload function to use the
  ACPI equivalent as they do not remove all devices prior
  to unload, leading to dangling references and seg faults.

Adam Young (3):
  mctp pcc: Check before sending MCTP PCC response ACK
  mctp pcc: Allow PCC Data Type in MCTP resource.
  mctp pcc: Implement MCTP over PCC Transport

 drivers/acpi/acpica/rsaddr.c |   2 +-
 drivers/mailbox/pcc.c        |  31 +++-
 drivers/net/mctp/Kconfig     |  13 ++
 drivers/net/mctp/Makefile    |   1 +
 drivers/net/mctp/mctp-pcc.c  | 322 +++++++++++++++++++++++++++++++++++
 include/acpi/pcc.h           |   8 +
 6 files changed, 368 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.34.1

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

[admiyo]

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;
 	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 *
-- 
2.34.1

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

[Simon Horman]

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
> 
> 

-- 
pw-bot: changes-requested

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

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jeremy Kerr]

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

[PATCH v4 2/3] mctp pcc: Allow PCC Data Type in MCTP resource.

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

Note that this patch is for code that will be merged
in via ACPICA changes.  The corresponding patch in ACPCA
has already merged. Thus, no changes can be made to this patch.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/acpi/acpica/rsaddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/rsaddr.c b/drivers/acpi/acpica/rsaddr.c
index fff48001d7ef..7932c2a6ddde 100644
--- a/drivers/acpi/acpica/rsaddr.c
+++ b/drivers/acpi/acpica/rsaddr.c
@@ -282,7 +282,7 @@ acpi_rs_get_address_common(struct acpi_resource *resource,
 
 	/* Validate the Resource Type */
 
-	if ((address.resource_type > 2) && (address.resource_type < 0xC0)) {
+	if ((address.resource_type > 2) && (address.resource_type < 0xC0) && (address.resource_type != 0x0A)) {
 		return (FALSE);
 	}
 
-- 
2.34.1

[PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

Implementation of network driver for
Management Control Transport Protocol(MCTP) over
Platform Communication Channel(PCC)

DMTF DSP:0292

MCTP devices are specified by entries in DSDT/SDST and
reference channels specified in the PCCT.

Communication with other devices use the PCC based
doorbell mechanism.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/net/mctp/Kconfig    |  13 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-pcc.c | 322 ++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index ce9d2d2ccf3b..ff4effd8e99c 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C
 	  A MCTP protocol network device is created for each I3C bus
 	  having a "mctp-controller" devicetree property.
 
+config MCTP_TRANSPORT_PCC
+	tristate "MCTP  PCC transport"
+	select ACPI
+	help
+	  Provides a driver to access MCTP devices over PCC transport,
+	  A MCTP protocol network device is created via ACPI for each
+	  entry in the DST/SDST that matches the identifier. The Platform
+	  commuinucation channels are selected from the corresponding
+	  entries in the PCCT.
+
+	  Say y here if you need to connect to MCTP endpoints over PCC. To
+	  compile as a module, use m; the module will be called mctp-pcc.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index e1cb99ced54a..492a9e47638f 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
new file mode 100644
index 000000000000..50ebad0e0cbe
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mctp-pcc.c - Driver for MCTP over PCC.
+ * Copyright (c) 2024, Ampere Computing LLC
+ */
+
+#include <linux/acpi.h>
+#include <linux/if_arp.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acrestyp.h>
+#include <acpi/actbl.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+#include <acpi/pcc.h>
+
+#define MCTP_PAYLOAD_LENGTH	256
+#define MCTP_CMD_LENGTH		4
+#define MCTP_PCC_VERSION	0x1 /* DSP0253 defines a single version: 1 */
+#define MCTP_SIGNATURE		"MCTP"
+#define SIGNATURE_LENGTH	4
+#define MCTP_HEADER_LENGTH	12
+#define MCTP_MIN_MTU		68
+#define PCC_MAGIC		0x50434300
+#define PCC_HEADER_FLAG_REQ_INT	0x1
+#define PCC_HEADER_FLAGS	PCC_HEADER_FLAG_REQ_INT
+#define PCC_DWORD_TYPE		0x0c
+#define PCC_ACK_FLAG_MASK	0x1
+
+struct mctp_pcc_hdr {
+	u32 signature;
+	u32 flags;
+	u32 length;
+	char mctp_signature[4];
+};
+
+struct mctp_pcc_hw_addr {
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+/* The netdev structure. One of these per PCC adapter. */
+struct mctp_pcc_ndev {
+	struct list_head next;
+	/* spinlock to serialize access to pcc buffer and registers*/
+	spinlock_t lock;
+	struct mctp_dev mdev;
+	struct acpi_device *acpi_device;
+	struct pcc_mbox_chan *in_chan;
+	struct pcc_mbox_chan *out_chan;
+	struct mbox_client outbox_client;
+	struct mbox_client inbox_client;
+	void __iomem *pcc_comm_inbox_addr;
+	void __iomem *pcc_comm_outbox_addr;
+	struct mctp_pcc_hw_addr hw_addr;
+};
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
+{
+	struct mctp_pcc_ndev *mctp_pcc_dev;
+	struct mctp_pcc_hdr mctp_pcc_hdr;
+	struct mctp_skb_cb *cb;
+	struct sk_buff *skb;
+	void *skb_buf;
+	u32 data_len;
+
+	mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
+	memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
+		      sizeof(struct mctp_pcc_hdr));
+	data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
+
+	if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
+		mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
+		return;
+	}
+
+	skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
+	if (!skb) {
+		mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
+		return;
+	}
+	mctp_pcc_dev->mdev.dev->stats.rx_packets++;
+	mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
+	skb->protocol = htons(ETH_P_MCTP);
+	skb_buf = skb_put(skb, data_len);
+	memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
+
+	skb_reset_mac_header(skb);
+	skb_pull(skb, sizeof(struct mctp_pcc_hdr));
+	skb_reset_network_header(skb);
+	cb = __mctp_cb(skb);
+	cb->halen = 0;
+	netif_rx(skb);
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_pcc_hdr pcc_header;
+	struct mctp_pcc_ndev *mpnd;
+	void __iomem *buffer;
+	unsigned long flags;
+
+	ndev->stats.tx_bytes += skb->len;
+	ndev->stats.tx_packets++;
+	mpnd = netdev_priv(ndev);
+
+	spin_lock_irqsave(&mpnd->lock, flags);
+	buffer = mpnd->pcc_comm_outbox_addr;
+	pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index;
+	pcc_header.flags = PCC_HEADER_FLAGS;
+	memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
+	pcc_header.length = skb->len + SIGNATURE_LENGTH;
+	memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
+	memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
+	mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
+						    NULL);
+	spin_unlock_irqrestore(&mpnd->lock, flags);
+
+	dev_consume_skb_any(skb);
+	return NETDEV_TX_OK;
+}
+
+static void
+mctp_pcc_net_stats(struct net_device *net_dev,
+		   struct rtnl_link_stats64 *stats)
+{
+	struct mctp_pcc_ndev *mpnd;
+
+	mpnd = netdev_priv(net_dev);
+	stats->rx_errors = 0;
+	stats->rx_packets = mpnd->mdev.dev->stats.rx_packets;
+	stats->tx_packets = mpnd->mdev.dev->stats.tx_packets;
+	stats->rx_dropped = 0;
+	stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes;
+	stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_start_xmit = mctp_pcc_tx,
+	.ndo_get_stats64 = mctp_pcc_net_stats,
+};
+
+static void  mctp_pcc_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->hard_header_len = 0;
+	ndev->addr_len = 0;
+	ndev->tx_queue_len = 0;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_pcc_netdev_ops;
+	ndev->needs_free_netdev = true;
+}
+
+struct lookup_context {
+	int index;
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+				       void *context)
+{
+	struct acpi_resource_address32 *addr;
+	struct lookup_context *luc = context;
+
+	switch (ares->type) {
+	case PCC_DWORD_TYPE:
+		break;
+	default:
+		return AE_OK;
+	}
+
+	addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
+	switch (luc->index) {
+	case 0:
+		luc->outbox_index = addr[0].address.minimum;
+		break;
+	case 1:
+		luc->inbox_index = addr[0].address.minimum;
+		break;
+	}
+	luc->index++;
+	return AE_OK;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+	struct lookup_context context = {0, 0, 0};
+	struct mctp_pcc_ndev *mctp_pcc_dev;
+	struct net_device *ndev;
+	acpi_handle dev_handle;
+	acpi_status status;
+	struct device *dev;
+	int mctp_pcc_mtu;
+	int outbox_index;
+	int inbox_index;
+	char name[32];
+	int rc;
+
+	dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
+		acpi_device_hid(acpi_dev));
+	dev_handle = acpi_device_handle(acpi_dev);
+	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+				     &context);
+	if (!ACPI_SUCCESS(status)) {
+		dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
+		return -EINVAL;
+	}
+	inbox_index = context.inbox_index;
+	outbox_index = context.outbox_index;
+	dev = &acpi_dev->dev;
+
+	snprintf(name, sizeof(name), "mctpipcc%d", inbox_index);
+	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_ENUM,
+			    mctp_pcc_setup);
+	if (!ndev)
+		return -ENOMEM;
+	mctp_pcc_dev = netdev_priv(ndev);
+	INIT_LIST_HEAD(&mctp_pcc_dev->next);
+	spin_lock_init(&mctp_pcc_dev->lock);
+
+	mctp_pcc_dev->hw_addr.inbox_index = inbox_index;
+	mctp_pcc_dev->hw_addr.outbox_index = outbox_index;
+	mctp_pcc_dev->inbox_client.rx_callback = mctp_pcc_client_rx_callback;
+	mctp_pcc_dev->out_chan =
+		pcc_mbox_request_channel(&mctp_pcc_dev->outbox_client,
+					 outbox_index);
+	if (IS_ERR(mctp_pcc_dev->out_chan)) {
+		rc = PTR_ERR(mctp_pcc_dev->out_chan);
+		goto free_netdev;
+	}
+	mctp_pcc_dev->in_chan =
+		pcc_mbox_request_channel(&mctp_pcc_dev->inbox_client,
+					 inbox_index);
+	if (IS_ERR(mctp_pcc_dev->in_chan)) {
+		rc = PTR_ERR(mctp_pcc_dev->in_chan);
+		goto cleanup_out_channel;
+	}
+	mctp_pcc_dev->pcc_comm_inbox_addr =
+		devm_ioremap(dev, mctp_pcc_dev->in_chan->shmem_base_addr,
+			     mctp_pcc_dev->in_chan->shmem_size);
+	if (!mctp_pcc_dev->pcc_comm_inbox_addr) {
+		rc = -EINVAL;
+		goto cleanup_in_channel;
+	}
+	mctp_pcc_dev->pcc_comm_outbox_addr =
+		devm_ioremap(dev, mctp_pcc_dev->out_chan->shmem_base_addr,
+			     mctp_pcc_dev->out_chan->shmem_size);
+	if (!mctp_pcc_dev->pcc_comm_outbox_addr) {
+		rc = -EINVAL;
+		goto cleanup_in_channel;
+	}
+	mctp_pcc_dev->acpi_device = acpi_dev;
+	mctp_pcc_dev->inbox_client.dev = dev;
+	mctp_pcc_dev->outbox_client.dev = dev;
+	mctp_pcc_dev->mdev.dev = ndev;
+	acpi_dev->driver_data = mctp_pcc_dev;
+
+	/* There is no clean way to pass the MTU
+	 * to the callback function used for registration,
+	 * so set the values ahead of time.
+	 */
+	mctp_pcc_mtu = mctp_pcc_dev->out_chan->shmem_size -
+		sizeof(struct mctp_pcc_hdr);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	rc = register_netdev(ndev);
+	if (rc)
+		goto cleanup_in_channel;
+	return 0;
+
+cleanup_in_channel:
+	pcc_mbox_free_channel(mctp_pcc_dev->in_chan);
+cleanup_out_channel:
+	pcc_mbox_free_channel(mctp_pcc_dev->out_chan);
+free_netdev:
+	unregister_netdev(ndev);
+	free_netdev(ndev);
+	return rc;
+}
+
+static void mctp_pcc_driver_remove(struct acpi_device *adev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
+
+	pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
+	pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
+	mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
+}
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+	{ "DMT0001", 0},
+	{ "", 0},
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+	.name = "mctp_pcc",
+	.class = "Unknown",
+	.ids = mctp_pcc_device_ids,
+	.ops = {
+		.add = mctp_pcc_driver_add,
+		.remove = mctp_pcc_driver_remove,
+	},
+	.owner = THIS_MODULE,
+};
+
+module_acpi_driver(mctp_pcc_driver);
+
+MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
+
+MODULE_DESCRIPTION("MCTP PCC device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
-- 
2.34.1

Re: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[Simon Horman]

On Tue, Jul 02, 2024 at 06:58:45PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Implementation of network driver for
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC)
> 
> DMTF DSP:0292
> 
> MCTP devices are specified by entries in DSDT/SDST and
> reference channels specified in the PCCT.
> 
> Communication with other devices use the PCC based
> doorbell mechanism.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/net/mctp/Kconfig    |  13 ++
>  drivers/net/mctp/Makefile   |   1 +
>  drivers/net/mctp/mctp-pcc.c | 322 ++++++++++++++++++++++++++++++++++++

...

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c

...

> +static struct acpi_driver mctp_pcc_driver = {
> +	.name = "mctp_pcc",
> +	.class = "Unknown",
> +	.ids = mctp_pcc_device_ids,
> +	.ops = {
> +		.add = mctp_pcc_driver_add,
> +		.remove = mctp_pcc_driver_remove,
> +	},
> +	.owner = THIS_MODULE,

Hi Adam,

Perhaps net-next isn't the appropriate tree to apply this patch.
But, due to [1] the owner field is not present in net-next and
thus this fails to build when applied there.

[1] cc85f9c05bba ("ACPI: drop redundant owner from acpi_driver")

> +};
> +
> +module_acpi_driver(mctp_pcc_driver);
> +
> +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
> +
> +MODULE_DESCRIPTION("MCTP PCC device");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
> -- 
> 2.34.1
> 
> 

Re: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR 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-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240704/202407041311.rvCLf6kZ-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041311.rvCLf6kZ-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/202407041311.rvCLf6kZ-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/net/mctp/mctp-pcc.c:17:
   include/acpi/acpi_drivers.h:72:43: warning: 'struct acpi_pci_root' declared inside parameter list will not be visible outside of this definition or declaration
      72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
         |                                           ^~~~~~~~~~~~~
   In file included from include/linux/printk.h:570,
                    from include/asm-generic/bug.h:22,
                    from arch/sh/include/asm/bug.h:112,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/sh/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from drivers/net/mctp/mctp-pcc.c:7:
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_add':
   drivers/net/mctp/mctp-pcc.c:207:26: error: invalid use of undefined type 'struct acpi_device'
     207 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |                          ^~
   include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls'
     224 |                 func(&id, ##__VA_ARGS__);                       \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls'
     250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call'
     273 |         _dynamic_func_call(fmt, __dynamic_dev_dbg,              \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
     165 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:207:9: note: in expansion of macro 'dev_dbg'
     207 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:208:17: error: implicit declaration of function 'acpi_device_hid'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
     208 |                 acpi_device_hid(acpi_dev));
         |                 ^~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:224:29: note: in definition of macro '__dynamic_func_call_cls'
     224 |                 func(&id, ##__VA_ARGS__);                       \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:250:9: note: in expansion of macro '_dynamic_func_call_cls'
     250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:273:9: note: in expansion of macro '_dynamic_func_call'
     273 |         _dynamic_func_call(fmt, __dynamic_dev_dbg,              \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:165:9: note: in expansion of macro 'dynamic_dev_dbg'
     165 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:207:9: note: in expansion of macro 'dev_dbg'
     207 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |         ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:209:22: error: implicit declaration of function 'acpi_device_handle'; did you mean 'acpi_device_dep'? [-Werror=implicit-function-declaration]
     209 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^~~~~~~~~~~~~~~~~~
         |                      acpi_device_dep
   drivers/net/mctp/mctp-pcc.c:209:20: warning: assignment to 'acpi_handle' {aka 'void *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     209 |         dev_handle = acpi_device_handle(acpi_dev);
         |                    ^
   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:14:
   drivers/net/mctp/mctp-pcc.c:213:34: error: invalid use of undefined type 'struct acpi_device'
     213 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                                  ^~
   include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   drivers/net/mctp/mctp-pcc.c:213:17: note: in expansion of macro 'dev_err'
     213 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                 ^~~~~~~
   drivers/net/mctp/mctp-pcc.c:218:24: error: invalid use of undefined type 'struct acpi_device'
     218 |         dev = &acpi_dev->dev;
         |                        ^~
   drivers/net/mctp/mctp-pcc.c:264:17: error: invalid use of undefined type 'struct acpi_device'
     264 |         acpi_dev->driver_data = mctp_pcc_dev;
         |                 ^~
   drivers/net/mctp/mctp-pcc.c: In function 'mctp_pcc_driver_remove':
>> drivers/net/mctp/mctp-pcc.c:293:47: error: implicit declaration of function 'acpi_driver_data'; did you mean 'acpi_get_data'? [-Werror=implicit-function-declaration]
     293 |         struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
         |                                               ^~~~~~~~~~~~~~~~
         |                                               acpi_get_data
>> drivers/net/mctp/mctp-pcc.c:293:47: warning: initialization of 'struct mctp_pcc_ndev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
   drivers/net/mctp/mctp-pcc.c: At top level:
   drivers/net/mctp/mctp-pcc.c:305:15: error: variable 'mctp_pcc_driver' has initializer but incomplete type
     305 | static struct acpi_driver mctp_pcc_driver = {
         |               ^~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:306:10: error: 'struct acpi_driver' has no member named 'name'
     306 |         .name = "mctp_pcc",
         |          ^~~~
   drivers/net/mctp/mctp-pcc.c:306:17: warning: excess elements in struct initializer
     306 |         .name = "mctp_pcc",
         |                 ^~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:306:17: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:307:10: error: 'struct acpi_driver' has no member named 'class'
     307 |         .class = "Unknown",
         |          ^~~~~
   drivers/net/mctp/mctp-pcc.c:307:18: warning: excess elements in struct initializer
     307 |         .class = "Unknown",
         |                  ^~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:307:18: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:308:10: error: 'struct acpi_driver' has no member named 'ids'
     308 |         .ids = mctp_pcc_device_ids,
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:308:16: warning: excess elements in struct initializer
     308 |         .ids = mctp_pcc_device_ids,
         |                ^~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:308:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:309:10: error: 'struct acpi_driver' has no member named 'ops'
     309 |         .ops = {
         |          ^~~
   drivers/net/mctp/mctp-pcc.c:309:16: error: extra brace group at end of initializer
     309 |         .ops = {
         |                ^
   drivers/net/mctp/mctp-pcc.c:309:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:309:16: warning: excess elements in struct initializer
   drivers/net/mctp/mctp-pcc.c:309:16: note: (near initialization for 'mctp_pcc_driver')
   drivers/net/mctp/mctp-pcc.c:313:10: error: 'struct acpi_driver' has no member named 'owner'
     313 |         .owner = THIS_MODULE,
         |          ^~~~~
   In file included from arch/sh/include/asm/cache.h:12,
                    from include/linux/cache.h:6,
                    from include/linux/slab.h:15:
   include/linux/init.h:180:21: warning: excess elements in struct initializer
     180 | #define THIS_MODULE (&__this_module)
         |                     ^
   drivers/net/mctp/mctp-pcc.c:313:18: note: in expansion of macro 'THIS_MODULE'
     313 |         .owner = THIS_MODULE,
         |                  ^~~~~~~~~~~
   include/linux/init.h:180:21: note: (near initialization for 'mctp_pcc_driver')
     180 | #define THIS_MODULE (&__this_module)
         |                     ^
   drivers/net/mctp/mctp-pcc.c:313:18: note: in expansion of macro 'THIS_MODULE'
     313 |         .owner = THIS_MODULE,
         |                  ^~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:316:1: warning: data definition has no type or storage class
     316 | module_acpi_driver(mctp_pcc_driver);
         | ^~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:316:1: error: type defaults to 'int' in declaration of 'module_acpi_driver' [-Werror=implicit-int]
   drivers/net/mctp/mctp-pcc.c:316:1: warning: parameter names (without types) in function declaration
   drivers/net/mctp/mctp-pcc.c:305:27: error: storage size of 'mctp_pcc_driver' isn't known
     305 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:305:27: warning: 'mctp_pcc_driver' defined but not used [-Wunused-variable]
   cc1: some warnings being treated as errors


vim +293 drivers/net/mctp/mctp-pcc.c

   290	
   291	static void mctp_pcc_driver_remove(struct acpi_device *adev)
   292	{
 > 293		struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
   294	
   295		pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
   296		pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
   297		mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
   298	}
   299	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR 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-4-admiyo%40os.amperecomputing.com
patch subject: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240704/202407041340.ve4NQPsF-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 326ba38a991250a8587a399a260b0f7af2c9166a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240704/202407041340.ve4NQPsF-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/202407041340.ve4NQPsF-lkp@intel.com/

All errors (new ones prefixed by >>):

         |                                                   ^
   In file included from drivers/net/mctp/mctp-pcc.c:8:
   In file included from include/linux/if_arp.h:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/net/mctp/mctp-pcc.c:8:
   In file included from include/linux/if_arp.h:22:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   In file included from drivers/net/mctp/mctp-pcc.c:17:
   include/acpi/acpi_drivers.h:72:43: warning: declaration of 'struct acpi_pci_root' will not be visible outside of this function [-Wvisibility]
      72 | struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root);
         |                                           ^
   drivers/net/mctp/mctp-pcc.c:207:19: error: incomplete definition of type 'struct acpi_device'
     207 |         dev_dbg(&acpi_dev->dev, "Adding mctp_pcc device for HID  %s\n",
         |                  ~~~~~~~~^
   include/linux/dev_printk.h:165:18: note: expanded from macro 'dev_dbg'
     165 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                         ^~~
   include/linux/dynamic_debug.h:274:7: note: expanded from macro 'dynamic_dev_dbg'
     274 |                            dev, fmt, ##__VA_ARGS__)
         |                            ^~~
   include/linux/dynamic_debug.h:250:59: note: expanded from macro '_dynamic_func_call'
     250 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |                                                                  ^~~~~~~~~~~
   include/linux/dynamic_debug.h:248:65: note: expanded from macro '_dynamic_func_call_cls'
     248 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
         |                                                                        ^~~~~~~~~~~
   include/linux/dynamic_debug.h:224:15: note: expanded from macro '__dynamic_func_call_cls'
     224 |                 func(&id, ##__VA_ARGS__);                       \
         |                             ^~~~~~~~~~~
   include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
     793 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:208:3: error: call to undeclared function 'acpi_device_hid'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     208 |                 acpi_device_hid(acpi_dev));
         |                 ^
   drivers/net/mctp/mctp-pcc.c:208:3: note: did you mean 'acpi_device_dep'?
   include/acpi/acpi_bus.h:41:6: note: 'acpi_device_dep' declared here
      41 | bool acpi_device_dep(acpi_handle target, acpi_handle match);
         |      ^
   drivers/net/mctp/mctp-pcc.c:209:15: error: call to undeclared function 'acpi_device_handle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     209 |         dev_handle = acpi_device_handle(acpi_dev);
         |                      ^
   drivers/net/mctp/mctp-pcc.c:209:13: error: incompatible integer to pointer conversion assigning to 'acpi_handle' (aka 'void *') from 'int' [-Wint-conversion]
     209 |         dev_handle = acpi_device_handle(acpi_dev);
         |                    ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:213:20: error: incomplete definition of type 'struct acpi_device'
     213 |                 dev_err(&acpi_dev->dev, "FAILURE to lookup PCC indexes from CRS");
         |                          ~~~~~~~~^
   include/linux/dev_printk.h:154:44: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                   ^~~
   include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
     793 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:218:17: error: incomplete definition of type 'struct acpi_device'
     218 |         dev = &acpi_dev->dev;
         |                ~~~~~~~~^
   include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
     793 | struct acpi_device;
         |        ^
   drivers/net/mctp/mctp-pcc.c:264:10: error: incomplete definition of type 'struct acpi_device'
     264 |         acpi_dev->driver_data = mctp_pcc_dev;
         |         ~~~~~~~~^
   include/linux/acpi.h:793:8: note: forward declaration of 'struct acpi_device'
     793 | struct acpi_device;
         |        ^
>> drivers/net/mctp/mctp-pcc.c:293:40: error: call to undeclared function 'acpi_driver_data'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     293 |         struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
         |                                               ^
>> drivers/net/mctp/mctp-pcc.c:293:24: error: incompatible integer to pointer conversion initializing 'struct mctp_pcc_ndev *' with an expression of type 'int' [-Wint-conversion]
     293 |         struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
         |                               ^               ~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/mctp/mctp-pcc.c:305:27: error: variable has incomplete type 'struct acpi_driver'
     305 | static struct acpi_driver mctp_pcc_driver = {
         |                           ^
   drivers/net/mctp/mctp-pcc.c:305:15: note: forward declaration of 'struct acpi_driver'
     305 | static struct acpi_driver mctp_pcc_driver = {
         |               ^
   drivers/net/mctp/mctp-pcc.c:316:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
     316 | module_acpi_driver(mctp_pcc_driver);
         | ^
         | int
   drivers/net/mctp/mctp-pcc.c:316:20: error: a parameter list without types is only allowed in a function definition
     316 | module_acpi_driver(mctp_pcc_driver);
         |                    ^
   8 warnings and 12 errors generated.


vim +/acpi_driver_data +293 drivers/net/mctp/mctp-pcc.c

   290	
   291	static void mctp_pcc_driver_remove(struct acpi_device *adev)
   292	{
 > 293		struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
   294	
   295		pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
   296		pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
   297		mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
   298	}
   299	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr]

Hi Adam,

Still some minor things, and locking query, inline.

You'll want to address the kernel-test-robot failure too; it looks like
the config it's hitting ends up without the acpi definitions available;
possibly CONFIG_ACPI is not set. Maybe you need a depends instead of a
select?

> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index ce9d2d2ccf3b..ff4effd8e99c 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C
>           A MCTP protocol network device is created for each I3C bus
>           having a "mctp-controller" devicetree property.
>  
> +config MCTP_TRANSPORT_PCC
> +       tristate "MCTP  PCC transport"

Nit: you have two spaces there.

> +struct mctp_pcc_hdr {
> +       u32 signature;
> +       u32 flags;
> +       u32 length;
> +       char mctp_signature[4];
> +};

I see you've added the __le annotations that I mentioned, but to a
different patch in the series. Was that intentional?

> +
> +struct mctp_pcc_hw_addr {
> +       u32 inbox_index;
> +       u32 outbox_index;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> +       struct list_head next;

This is now unused.

> +       /* spinlock to serialize access to pcc buffer and registers*/
> +       spinlock_t lock;

The addition of the comment is good, but I'm still not clear on what
data this lock is serialising access to. Does "pcc buffer" refer to both
the pcc_comm_inbox_addr and pcc_comm_outbox_addr buffer?

If so, you may be missing locking on the client_rx_callback.

And what are the "registers" referring to here? everything seems to be
accessed through the mbox interface. Does that require serialisation?

(if you can list actual struct members that are only accessed under the
lock, that may make things more clear - but it can be a bit of a balance
in not going *too* overboard with the description!)

> +       struct mctp_dev mdev;
> +       struct acpi_device *acpi_device;
> +       struct pcc_mbox_chan *in_chan;
> +       struct pcc_mbox_chan *out_chan;
> +       struct mbox_client outbox_client;
> +       struct mbox_client inbox_client;
> +       void __iomem *pcc_comm_inbox_addr;
> +       void __iomem *pcc_comm_outbox_addr;
> +       struct mctp_pcc_hw_addr hw_addr;
> +};
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct mctp_pcc_hdr mctp_pcc_hdr;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       void *skb_buf;
> +       u32 data_len;
> +
> +       mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> +       memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
> +                     sizeof(struct mctp_pcc_hdr));
> +       data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +
> +       if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
> +               mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +               return;
> +       }

Shouldn't this be comparing on the currently-set MTU, rather than the
max?

[in either case, this assumes that we want to enforce RX packets to be
within the transmit limit, which may be reasonable, but maybe not
strictly necessary]

> +
> +       skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
> +       if (!skb) {
> +               mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +               return;
> +       }
> +       mctp_pcc_dev->mdev.dev->stats.rx_packets++;
> +       mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
> +       skb->protocol = htons(ETH_P_MCTP);
> +       skb_buf = skb_put(skb, data_len);
> +       memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, data_len);
> +
> +       skb_reset_mac_header(skb);
> +       skb_pull(skb, sizeof(struct mctp_pcc_hdr));
> +       skb_reset_network_header(skb);
> +       cb = __mctp_cb(skb);
> +       cb->halen = 0;
> +       netif_rx(skb);
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct mctp_pcc_hdr pcc_header;
> +       struct mctp_pcc_ndev *mpnd;
> +       void __iomem *buffer;
> +       unsigned long flags;
> +
> +       ndev->stats.tx_bytes += skb->len;
> +       ndev->stats.tx_packets++;
> +       mpnd = netdev_priv(ndev);
> +
> +       spin_lock_irqsave(&mpnd->lock, flags);
> +       buffer = mpnd->pcc_comm_outbox_addr;
> +       pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index;
> +       pcc_header.flags = PCC_HEADER_FLAGS;
> +       memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> +       pcc_header.length = skb->len + SIGNATURE_LENGTH;

Are there any constraints on this length?

> +       memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
> +       memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
> +       mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> +                                                   NULL);
> +       spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> +       dev_consume_skb_any(skb);
> +       return NETDEV_TX_OK;
> +}
> +
> +static void
> +mctp_pcc_net_stats(struct net_device *net_dev,
> +                  struct rtnl_link_stats64 *stats)
> +{
> +       struct mctp_pcc_ndev *mpnd;
> +
> +       mpnd = netdev_priv(net_dev);
> +       stats->rx_errors = 0;
> +       stats->rx_packets = mpnd->mdev.dev->stats.rx_packets;
> +       stats->tx_packets = mpnd->mdev.dev->stats.tx_packets;
> +       stats->rx_dropped = 0;
> +       stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes;
> +       stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes;

Isn't mpnd->mdev.dev just net_dev?

Should we be doing this (as well as the stats updates) with mpnd->lock
held? 

> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
> +
> +       pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
> +       pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
> +       mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
> +}

Nice!

Cheers,


Jeremy

Re: [PATCH v4 3/3] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

On 7/4/24 06:23, Jeremy Kerr wrote:
>> +struct mctp_pcc_hdr {
>> +       u32 signature;
>> +       u32 flags;
>> +       u32 length;
>> +       char mctp_signature[4];
>> +};
> I see you've added the __le annotations that I mentioned, but to a
> different patch in the series. Was that intentional?


Yes.  Since the header is used by the Mailbox implementation now, in 
order to check the ACK flag.

It made sense to move the structure to a common location, but the change 
was based on your last review.

Re: [PATCH v4 0/3] MCTP over PCC

[Dan Williams]

admiyo@ wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> This series adds support for the Management Control Transport Protocol (MCTP)
> over the Platform Communication Channel (PCC) mechanism.
> 
> MCTP defines a communication model intended to
> facilitate communication between Management controllers
> and other management controllers, and between Management
> controllers and management devices
> 
> PCC is a mechanism for communication between components within
> the  Platform.  It is a composed of shared memory regions,
> interrupt registers, and status registers.
> 
> The MCTP over PCC driver makes use of two PCC channels. For
> sending messages, it uses a Type 3 channel, and for receiving
> messages it uses the paired Type 4 channel.  The device
> and corresponding channels are specified via ACPI.
> 
> The first patch in the series implements a mechanism to allow the driver
> to indicate whether an ACK should be sent back to the caller
> after processing the interrupt.  This is an optional feature in
> the PCC code, but has been made explicitly required in another driver.
> The implementation here maintains the backwards compatibility of that
> driver.
> 
> The second patch in the series is the required change from ACPICA
> code that will be imported into the Linux kernel when synchronized
> with the ACPICA repository. It ahs already merged there and will
> be merged in as is.  It is included here so that the patch series
> can run and be tested prior to that merge.

This cover letter looks woefully insufficient.

What is the end user visible effect of merging these patches, or not
merging these patches?  I.e. what does Linux gain by merging them, what
pressing end user need goes unsatisfied if these are not merged? What is
the security model for these commands, i.e. how does a distro judge
whether this facility allows bypass of Kernel Lockdown protections?

The Kconfig does not help either. All this patch says is "communication
path exists, plumb it direct to userspace", with no discussion of
intended use cases, assumptions, or tradeoffs.

Re: [PATCH v4 0/3] MCTP over PCC

[Adam Young]

Apologies for not addressing these concerns before updating.  If there 
is a V6 (am sure there will be) I will update the cover.

MCTP is a general purpose  protocol so  it would  be impossible to 
enumerate all the use cases, but some of the ones that are most topical 
are attestation and RAS support.  There are a handful of protocols built 
on top of MCTP, to include PLDM and SPDM, both specified by the DMTF.

https://www.dmtf.org/sites/default/files/standards/documents/DSP0240_1.0.0.pdf
https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.3.0.pd

SPDM entails various usages, including device identity collection, 
device authentication, measurement collection, and device secure session 
establishment.

PLDM is more likely to be used  for hardware support: temperature, 
voltage, or fan sensor control.

At least two companies have devices that can make use of the mechanism. 
One is Ampere Computing, my employer.

The mechanism it uses is called Platform Communication Channels is part 
of the ACPI spec: 
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html

Since it is a socket interface, the system administrator also has  the 
ability to ignore an MCTP link that they do not want to enable.  This 
link would be exposed to the end user, but would not be usable.

If MCTP support is disabled  in the Kernel, this driver would also be 
disabled.

PCC is based on a shared buffer and a set of I/O mapped memory locations 
that the Spec calls registers.  This mechanism exists regardless of the 
existence of the driver. Thus, if the user has the ability to map these  
physical location to virtual locations, they have the ability to drive 
the hardware.  Thus, there is a security aspect to this mechanism that 
extends beyond the responsibilities of the operating system.

If the hardware does not expose the PCC in the ACPI table, this device 
will never be enabled.  Thus it is only an issue on hard that does 
support PCC.  In that case, it is up to the remote controller to 
sanitize communication; MCTP will be exposed as a socket interface, and 
userland can send any crafted packet it wants.  It would thus also be 
incumbent on the hardware manufacturer to allow the end user to disable 
MCTP over PCC communication if they did not want to expose it.

Does this cover you concerns?


On 7/11/24 12:57, Dan Williams wrote:

> admiyo@ wrote:
>> From: Adam Young<admiyo@os.amperecomputing.com>
>>
>> This series adds support for the Management Control Transport Protocol (MCTP)
>> over the Platform Communication Channel (PCC) mechanism.
>>
>> MCTP defines a communication model intended to
>> facilitate communication between Management controllers
>> and other management controllers, and between Management
>> controllers and management devices
>>
>> PCC is a mechanism for communication between components within
>> the  Platform.  It is a composed of shared memory regions,
>> interrupt registers, and status registers.
>>
>> The MCTP over PCC driver makes use of two PCC channels. For
>> sending messages, it uses a Type 3 channel, and for receiving
>> messages it uses the paired Type 4 channel.  The device
>> and corresponding channels are specified via ACPI.
>>
>> The first patch in the series implements a mechanism to allow the driver
>> to indicate whether an ACK should be sent back to the caller
>> after processing the interrupt.  This is an optional feature in
>> the PCC code, but has been made explicitly required in another driver.
>> The implementation here maintains the backwards compatibility of that
>> driver.
>>
>> The second patch in the series is the required change from ACPICA
>> code that will be imported into the Linux kernel when synchronized
>> with the ACPICA repository. It ahs already merged there and will
>> be merged in as is.  It is included here so that the patch series
>> can run and be tested prior to that merge.
> This cover letter looks woefully insufficient.
>
> What is the end user visible effect of merging these patches, or not
> merging these patches?  I.e. what does Linux gain by merging them, what
> pressing end user need goes unsatisfied if these are not merged? What is
> the security model for these commands, i.e. how does a distro judge
> whether this facility allows bypass of Kernel Lockdown protections?
>
> The Kconfig does not help either. All this patch says is "communication
> path exists, plumb it direct to userspace", with no discussion of
> intended use cases, assumptions, or tradeoffs.