linux-kernel - Re: [PATCH v7 2/5] cdx: Export Symbols for MCDI RPC and Initialization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250613201034.GC171759@yaz-khff2.amd.com>
Date: Fri, 13 Jun 2025 16:10:34 -0400
From: Yazen Ghannam <yazen.ghannam@....com>
To: Shubhrajyoti Datta <shubhrajyoti.datta@....com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-edac@...r.kernel.org, git@....com,
	Krzysztof Kozlowski <krzk@...nel.org>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Borislav Petkov <bp@...en8.de>, Tony Luck <tony.luck@...el.com>,
	James Morse <james.morse@....com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Robert Richter <rric@...nel.org>, Nipun Gupta <nipun.gupta@....com>,
	Nikhil Agarwal <nikhil.agarwal@....com>
Subject: Re: [PATCH v7 2/5] cdx: Export Symbols for MCDI RPC and
 Initialization

On Thu, May 29, 2025 at 12:30:14PM +0530, Shubhrajyoti Datta wrote:
> The cdx_mcdi_init, cdx_mcdi_process_cmd, and cdx_mcdi_rpc functions are
> needed by VersalNET EDAC modules that interact with the MCDI (Management
> Controller Direct Interface) framework. These functions facilitate
> communication between different hardware components by enabling command
> execution and status management.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@....com>
> ---
> 
> Changes in v7:
> - Add the kernel doc description
> - Add the prototype from first patch to here
> 
> Changes in v6:
> - Update commit description
> 
> Changes in v2:
> - Export the symbols for module compilation
> 
>  drivers/cdx/controller/mcdi.c | 29 +++++++++++++++++++++++++++++
>  include/linux/cdx/mcdi.h      |  6 ++++++

You've added the function prototypes to this new global header.

But you didn't remove them from the local header.
  drivers/cdx/controller/mcdi.h

Also, you haven't included the new global header in the cdx/controller
code.

Even though this does compile, it doesn't seem proper.

I expect you would want to do the following:

1) Add the common code to the global header.
2) Remove the common code from the local header.
3) Include the global header everywhere the common code is needed.

Keeping the diff below for reference.

Thanks,
Yazen

>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/cdx/controller/mcdi.c b/drivers/cdx/controller/mcdi.c
> index e760f8d347cc..f3cca4c884ff 100644
> --- a/drivers/cdx/controller/mcdi.c
> +++ b/drivers/cdx/controller/mcdi.c
> @@ -99,6 +99,19 @@ static unsigned long cdx_mcdi_rpc_timeout(struct cdx_mcdi *cdx, unsigned int cmd
>  		return cdx->mcdi_ops->mcdi_rpc_timeout(cdx, cmd);
>  }
>  
> +/**
> + * cdx_mcdi_init - Initialize MCDI (Management Controller Driver Interface) state
> + * @cdx: NIC through which to issue the command
> + *
> + * This function allocates and initializes internal MCDI structures and resources
> + * for the CDX device, including the workqueue, locking primitives, and command
> + * tracking mechanisms. It sets the initial operating mode and prepares the device
> + * for MCDI operations.
> + *
> + * Return:
> + * * 0        - on success
> + * * -ENOMEM  - if memory allocation or workqueue creation fails
> + */
>  int cdx_mcdi_init(struct cdx_mcdi *cdx)
>  {
>  	struct cdx_mcdi_iface *mcdi;
> @@ -128,6 +141,7 @@ int cdx_mcdi_init(struct cdx_mcdi *cdx)
>  fail:
>  	return rc;
>  }
> +EXPORT_SYMBOL_GPL(cdx_mcdi_init);
>  
>  void cdx_mcdi_finish(struct cdx_mcdi *cdx)
>  {
> @@ -553,6 +567,19 @@ static void cdx_mcdi_start_or_queue(struct cdx_mcdi_iface *mcdi,
>  			cdx_mcdi_cmd_start_or_queue(mcdi, cmd);
>  }
>  
> +/**
> + * cdx_mcdi_process_cmd - Process an incoming MCDI response
> + * @cdx: NIC through which to issue the command
> + * @outbuf:  Pointer to the response buffer received from the management controller
> + * @len:     Length of the response buffer in bytes
> + *
> + * This function handles a response from the management controller. It locates the
> + * corresponding command using the sequence number embedded in the header,
> + * completes the command if it is still pending, and initiates any necessary cleanup.
> + *
> + * The function assumes that the response buffer is well-formed and at least one
> + * dword in size.
> + */
>  void cdx_mcdi_process_cmd(struct cdx_mcdi *cdx, struct cdx_dword *outbuf, int len)
>  {
>  	struct cdx_mcdi_iface *mcdi;
> @@ -590,6 +617,7 @@ void cdx_mcdi_process_cmd(struct cdx_mcdi *cdx, struct cdx_dword *outbuf, int le
>  
>  	cdx_mcdi_process_cleanup_list(mcdi->cdx, &cleanup_list);
>  }
> +EXPORT_SYMBOL_GPL(cdx_mcdi_process_cmd);
>  
>  static void cdx_mcdi_cmd_work(struct work_struct *context)
>  {
> @@ -757,6 +785,7 @@ int cdx_mcdi_rpc(struct cdx_mcdi *cdx, unsigned int cmd,
>  	return cdx_mcdi_rpc_sync(cdx, cmd, inbuf, inlen, outbuf, outlen,
>  				 outlen_actual, false);
>  }
> +EXPORT_SYMBOL_GPL(cdx_mcdi_rpc);
>  
>  /**
>   * cdx_mcdi_rpc_async - Schedule an MCDI command to run asynchronously
> diff --git a/include/linux/cdx/mcdi.h b/include/linux/cdx/mcdi.h
> index 46e3f63b062a..1344119e9a2c 100644
> --- a/include/linux/cdx/mcdi.h
> +++ b/include/linux/cdx/mcdi.h
> @@ -169,6 +169,12 @@ struct cdx_mcdi_data {
>  	u32 fn_flags;
>  };
>  
> +int cdx_mcdi_init(struct cdx_mcdi *cdx);
> +void cdx_mcdi_process_cmd(struct cdx_mcdi *cdx, struct cdx_dword *outbuf, int len);
> +int cdx_mcdi_rpc(struct cdx_mcdi *cdx, unsigned int cmd,
> +		 const struct cdx_dword *inbuf, size_t inlen,
> +		 struct cdx_dword *outbuf, size_t outlen, size_t *outlen_actual);
> +
>  /*
>   * We expect that 16- and 32-bit fields in MCDI requests and responses
>   * are appropriately aligned, but 64-bit fields are only
> -- 
> 2.34.1
>