Re: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

Re: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250303105902.215009-7-akshay.gupta@amd.com>
References: <20250303105902.215009-7-akshay.gupta@amd.com>
TO: Akshay Gupta <akshay.gupta@amd.com>
TO: linux-hwmon@vger.kernel.org
TO: linux-kernel@vger.kernel.org
CC: linux@roeck-us.net
CC: gregkh@linuxfoundation.org
CC: arnd@arndb.de
CC: shyam-sundar.s-k@amd.com
CC: gautham.shenoy@amd.com
CC: mario.limonciello@amd.com
CC: naveenkrishna.chatradhi@amd.com
CC: Akshay Gupta <akshay.gupta@amd.com>

Hi Akshay,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus groeck-staging/hwmon-next soc/for-next linus/master v6.14-rc5 next-20250306]
[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/Akshay-Gupta/hwmon-misc-amd-sbi-Move-core-sbrmi-from-hwmon-to-misc/20250303-190830
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20250303105902.215009-7-akshay.gupta%40amd.com
patch subject: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: riscv-randconfig-r072-20250306 (https://download.01.org/0day-ci/archive/20250307/202503071145.fhmAEjMr-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)

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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202503071145.fhmAEjMr-lkp@intel.com/

smatch warnings:
drivers/misc/amd-sbi/rmi-core.c:110 sbrmi_ioctl() warn: can 'data' even be NULL?

vim +/data +110 drivers/misc/amd-sbi/rmi-core.c

dd49a6be4bc20c Akshay Gupta 2025-03-03  100  
dd49a6be4bc20c Akshay Gupta 2025-03-03  101  static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
dd49a6be4bc20c Akshay Gupta 2025-03-03  102  {
dd49a6be4bc20c Akshay Gupta 2025-03-03  103  	int __user *arguser = (int  __user *)arg;
dd49a6be4bc20c Akshay Gupta 2025-03-03  104  	struct apml_message msg = { 0 };
dd49a6be4bc20c Akshay Gupta 2025-03-03  105  	bool read = false;
dd49a6be4bc20c Akshay Gupta 2025-03-03  106  	int ret;
dd49a6be4bc20c Akshay Gupta 2025-03-03  107  
dd49a6be4bc20c Akshay Gupta 2025-03-03  108  	struct sbrmi_data *data = container_of(fp->private_data, struct sbrmi_data,
dd49a6be4bc20c Akshay Gupta 2025-03-03  109  					       sbrmi_misc_dev);
dd49a6be4bc20c Akshay Gupta 2025-03-03 @110  	if (!data)
dd49a6be4bc20c Akshay Gupta 2025-03-03  111  		return -ENODEV;
dd49a6be4bc20c Akshay Gupta 2025-03-03  112  
dd49a6be4bc20c Akshay Gupta 2025-03-03  113  	/* Copy the structure from user */
dd49a6be4bc20c Akshay Gupta 2025-03-03  114  	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
dd49a6be4bc20c Akshay Gupta 2025-03-03  115  				  sizeof(struct apml_message)))
dd49a6be4bc20c Akshay Gupta 2025-03-03  116  		return -EFAULT;
dd49a6be4bc20c Akshay Gupta 2025-03-03  117  
dd49a6be4bc20c Akshay Gupta 2025-03-03  118  	/* Is this a read/monitor/get request */
dd49a6be4bc20c Akshay Gupta 2025-03-03  119  	if (msg.data_in.reg_in[AMD_SBI_RD_FLAG_INDEX])
dd49a6be4bc20c Akshay Gupta 2025-03-03  120  		read = true;
dd49a6be4bc20c Akshay Gupta 2025-03-03  121  
dd49a6be4bc20c Akshay Gupta 2025-03-03  122  	switch (msg.cmd) {
dd49a6be4bc20c Akshay Gupta 2025-03-03  123  	case 0 ... 0x999:
dd49a6be4bc20c Akshay Gupta 2025-03-03  124  		/* Mailbox protocol */
dd49a6be4bc20c Akshay Gupta 2025-03-03  125  		ret = rmi_mailbox_xfer(data, &msg);
dd49a6be4bc20c Akshay Gupta 2025-03-03  126  		break;
dd49a6be4bc20c Akshay Gupta 2025-03-03  127  	default:
dd49a6be4bc20c Akshay Gupta 2025-03-03  128  		return -EINVAL;
dd49a6be4bc20c Akshay Gupta 2025-03-03  129  	}
dd49a6be4bc20c Akshay Gupta 2025-03-03  130  
dd49a6be4bc20c Akshay Gupta 2025-03-03  131  	/* Copy results back to user only for get/monitor commands and firmware failures */
dd49a6be4bc20c Akshay Gupta 2025-03-03  132  	if ((read && !ret) || ret == -EPROTOTYPE) {
dd49a6be4bc20c Akshay Gupta 2025-03-03  133  		if (copy_to_user(arguser, &msg, sizeof(struct apml_message)))
dd49a6be4bc20c Akshay Gupta 2025-03-03  134  			ret = -EFAULT;
dd49a6be4bc20c Akshay Gupta 2025-03-03  135  	}
dd49a6be4bc20c Akshay Gupta 2025-03-03  136  	return ret;
dd49a6be4bc20c Akshay Gupta 2025-03-03  137  }
dd49a6be4bc20c Akshay Gupta 2025-03-03  138  

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

[PATCH v5 00/11] misc: Move AMD side band interface(SBI) functionality

[Akshay Gupta]

At present, sbrmi driver under hwmon subsystem, is probed as an i2c driver,
fetches data using APML specified protocol and reports through hwmon power sensor.

AMD provides additional information using custom protocols, which cannot be
enumerated as hwmon sensors. Hence, move the existing functionality from hwmon/
to misc/ and add support for following custom protocols
  - read Processor feature capabilities and configuration information
    through side band.
  - read Machine Check Architecture(MCA) registers over sideband.
    The information is accessed for range of MCA registers by passing
    register address and thread ID to the protocol.

NOTE: AMD defines Advanced Platform Management Link (APML) interface which provides
system management functionality access to the baseboard management
controller (BMC).

This patchset is an attempt to keep all APML core functionality in one place,
provide hwmon and IOCTL interface to user space
1. [Patch 1] Move the i2c client probe, hwmon sensors and sbrmi core functionality
   from drivers/hwmon to drivers/misc/
2. [Patch 2] Move sbrmi core functionality to new core file to export core functionality
3. [Patch 3] Move hwmon device sensor as separate entity
4. [Patch 4] Convert i2c to regmap which provides multiple benefits
   over direct smbus APIs.
    a. i2c/i3c support and 
    b. 1 byte/2 byte RMI register size addressing
5. [Patch 5] Optimize wait condition with regmap API regmap_read_poll_timeout as per
   suggestion from Arnd
6. [Patch 6, 7] Register a misc device which provides
    a. An ioctl interface through node /dev/sbrmiX
    b. Register sets is common across APML protocols. IOCTL is providing
       synchronization among protocols as transactions may create
       race condition.
7. [Subsequent patches 8, 9 and 10] add support for AMD custom protocols
    a. CPUID
    b. MCAMSR
    c. Register xfer
8. [Patch 11] AMD side band description document

Open-sourced and widely used [1]_ will continue to provide user-space programmable API.

.. [1] https://github.com/amd/esmi_oob_library

Akshay Gupta (11):
  hwmon/misc: amd-sbi: Move core sbrmi from hwmon to misc
  misc: amd-sbi: Move protocol functionality to core file
  misc: amd-sbi: Move hwmon device sensor as separate entity
  misc: amd-sbi: Use regmap subsystem
  misc: amd-sbi: Optimize the wait condition for mailbox command
    completion
  misc: amd-sbi: Add support for AMD_SBI IOCTL
  misc: amd-sbi: Add support for mailbox error codes
  misc: amd-sbi: Add support for CPUID protocol
  misc: amd-sbi: Add support for read MCA register protocol
  misc: amd-sbi: Add support for register xfer
  misc: amd-sbi: Add document for AMD SB IOCTL description

 Documentation/misc-devices/amd-sbi.rst        |  87 ++++
 Documentation/misc-devices/index.rst          |   1 +
 .../userspace-api/ioctl/ioctl-number.rst      |   2 +
 drivers/hwmon/Kconfig                         |  10 -
 drivers/hwmon/sbrmi.c                         | 357 --------------
 drivers/misc/Kconfig                          |   1 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/amd-sbi/Kconfig                  |   9 +
 drivers/misc/amd-sbi/Makefile                 |   3 +
 drivers/misc/amd-sbi/rmi-core.c               | 445 ++++++++++++++++++
 drivers/misc/amd-sbi/rmi-core.h               |  67 +++
 drivers/misc/amd-sbi/rmi-hwmon.c              | 122 +++++
 drivers/misc/amd-sbi/rmi-i2c.c                | 133 ++++++
 include/uapi/misc/amd-apml.h                  |  97 ++++
 14 files changed, 968 insertions(+), 367 deletions(-)
 create mode 100644 Documentation/misc-devices/amd-sbi.rst
 delete mode 100644 drivers/hwmon/sbrmi.c
 create mode 100644 drivers/misc/amd-sbi/Kconfig
 create mode 100644 drivers/misc/amd-sbi/Makefile
 create mode 100644 drivers/misc/amd-sbi/rmi-core.c
 create mode 100644 drivers/misc/amd-sbi/rmi-core.h
 create mode 100644 drivers/misc/amd-sbi/rmi-hwmon.c
 create mode 100644 drivers/misc/amd-sbi/rmi-i2c.c
 create mode 100644 include/uapi/misc/amd-apml.h

-- 
2.25.1

[PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

[Akshay Gupta]

The present sbrmi module only support reporting power via hwmon.
However, AMD data center range of processors support various
system management functionality using custom protocols defined in
Advanced Platform Management Link (APML) specification.

Register a miscdevice, which creates a device /dev/sbrmiX with an IOCTL
interface for the user space to invoke the APML Mailbox protocol, which
is already defined in sbrmi_mailbox_xfer().

The APML protocols depend on a set of RMI registers. Having an IOCTL
as a single entry point will help in providing synchronization among
these protocols as multiple transactions on RMI register set may
create race condition.
Support for other protocols will be added in subsequent patches.

Open-sourced and widely used https://github.com/amd/esmi_oob_library
will continue to provide user-space programmable API.

Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
---
Changes since v4:
- Address review comment
 - Address Greg review comments
 - Not initialize ret
 - return on error
- Previously patch 4
- Fix documentation warning

Changes since v3:
- Previously patch 3
- Documentation and comments changes

Changes since v2:
- update the MACROS name as per feedback

Changes since v1:
- Previously patch 5
- Add IOCTL description in ioctl-number.rst
- Split patch as per suggestion.

 Documentation/misc-devices/index.rst          |  1 +
 .../userspace-api/ioctl/ioctl-number.rst      |  2 +
 drivers/misc/amd-sbi/rmi-core.c               | 81 +++++++++++++++++--
 drivers/misc/amd-sbi/rmi-core.h               | 15 ++--
 drivers/misc/amd-sbi/rmi-hwmon.c              | 15 ++--
 drivers/misc/amd-sbi/rmi-i2c.c                | 25 +++++-
 include/uapi/misc/amd-apml.h                  | 66 +++++++++++++++
 7 files changed, 178 insertions(+), 27 deletions(-)
 create mode 100644 include/uapi/misc/amd-apml.h

diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
index 8c5b226d8313..081e79415e38 100644
--- a/Documentation/misc-devices/index.rst
+++ b/Documentation/misc-devices/index.rst
@@ -12,6 +12,7 @@ fit into other categories.
    :maxdepth: 2
 
    ad525x_dpot
+   amd-sbi
    apds990x
    bh1770glc
    c2port
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 6d1465315df3..5692b50b3c6f 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -392,6 +392,8 @@ Code  Seq#    Include File                                           Comments
                                                                      <mailto:mathieu.desnoyers@efficios.com>
 0xF8  all    arch/x86/include/uapi/asm/amd_hsmp.h                    AMD HSMP EPYC system management interface driver
                                                                      <mailto:nchatrad@amd.com>
+0xF9  00-0F  uapi/misc/amd-apml.h		                     AMD side band system management interface driver
+                                                                     <mailto:naveenkrishna.chatradhi@amd.com>
 0xFD  all    linux/dm-ioctl.h
 0xFE  all    linux/isst_if.h
 ====  =====  ======================================================= ================================================================
diff --git a/drivers/misc/amd-sbi/rmi-core.c b/drivers/misc/amd-sbi/rmi-core.c
index 1d5e2556ab88..c39a29d90c27 100644
--- a/drivers/misc/amd-sbi/rmi-core.c
+++ b/drivers/misc/amd-sbi/rmi-core.c
@@ -7,7 +7,10 @@
  */
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/fs.h>
 #include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
 #include "rmi-core.h"
@@ -20,7 +23,7 @@
 #define TRIGGER_MAILBOX	0x01
 
 int rmi_mailbox_xfer(struct sbrmi_data *data,
-		     struct sbrmi_mailbox_msg *msg)
+		     struct apml_message *msg)
 {
 	unsigned int bytes;
 	int i, ret;
@@ -44,8 +47,8 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * Command Data In[31:0] to SBRMI::InBndMsg_inst[4:1]
 	 * SBRMI_x3C(MSB):SBRMI_x39(LSB)
 	 */
-	for (i = 0; i < 4; i++) {
-		byte = (msg->data_in >> i * 8) & 0xff;
+	for (i = 0; i < AMD_SBI_MB_DATA_SIZE; i++) {
+		byte = msg->data_in.reg_in[i];
 		ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte);
 		if (ret < 0)
 			goto exit_unlock;
@@ -74,13 +77,13 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 * response Command Data Out[31:0] from SBRMI::OutBndMsg_inst[4:1]
 	 * {SBRMI_x34(MSB):SBRMI_x31(LSB)}.
 	 */
-	if (msg->read) {
-		for (i = 0; i < 4; i++) {
+	if (msg->data_in.reg_in[AMD_SBI_RD_FLAG_INDEX]) {
+		for (i = 0; i < AMD_SBI_MB_DATA_SIZE; i++) {
 			ret = regmap_read(data->regmap,
 					  SBRMI_OUTBNDMSG1 + i, &bytes);
 			if (ret < 0)
-				goto exit_unlock;
-			msg->data_out |= bytes << i * 8;
+				break;
+			msg->data_out.reg_out[i] = bytes;
 		}
 	}
 
@@ -90,8 +93,70 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	 */
 	ret = regmap_write(data->regmap, SBRMI_STATUS,
 			   sw_status | SW_ALERT_MASK);
-
 exit_unlock:
 	mutex_unlock(&data->lock);
 	return ret;
 }
+
+static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	int __user *arguser = (int  __user *)arg;
+	struct apml_message msg = { 0 };
+	bool read = false;
+	int ret;
+
+	struct sbrmi_data *data = container_of(fp->private_data, struct sbrmi_data,
+					       sbrmi_misc_dev);
+	if (!data)
+		return -ENODEV;
+
+	/* Copy the structure from user */
+	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
+				  sizeof(struct apml_message)))
+		return -EFAULT;
+
+	/* Is this a read/monitor/get request */
+	if (msg.data_in.reg_in[AMD_SBI_RD_FLAG_INDEX])
+		read = true;
+
+	switch (msg.cmd) {
+	case 0 ... 0x999:
+		/* Mailbox protocol */
+		ret = rmi_mailbox_xfer(data, &msg);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Copy results back to user only for get/monitor commands and firmware failures */
+	if ((read && !ret) || ret == -EPROTOTYPE) {
+		if (copy_to_user(arguser, &msg, sizeof(struct apml_message)))
+			ret = -EFAULT;
+	}
+	return ret;
+}
+
+static const struct file_operations sbrmi_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= sbrmi_ioctl,
+	.compat_ioctl	= sbrmi_ioctl,
+};
+
+int create_misc_rmi_device(struct sbrmi_data *data,
+			   struct device *dev)
+{
+	data->sbrmi_misc_dev.name	= devm_kasprintf(dev,
+							 GFP_KERNEL,
+							 "sbrmi-%x",
+							 data->dev_static_addr);
+	data->sbrmi_misc_dev.minor	= MISC_DYNAMIC_MINOR;
+	data->sbrmi_misc_dev.fops	= &sbrmi_fops;
+	data->sbrmi_misc_dev.parent	= dev;
+	data->sbrmi_misc_dev.nodename	= devm_kasprintf(dev,
+							 GFP_KERNEL,
+							 "sbrmi-%x",
+							 data->dev_static_addr);
+	data->sbrmi_misc_dev.mode	= 0600;
+
+	return misc_register(&data->sbrmi_misc_dev);
+}
diff --git a/drivers/misc/amd-sbi/rmi-core.h b/drivers/misc/amd-sbi/rmi-core.h
index bbb6bb1cefde..e3a11575d19e 100644
--- a/drivers/misc/amd-sbi/rmi-core.h
+++ b/drivers/misc/amd-sbi/rmi-core.h
@@ -6,10 +6,12 @@
 #ifndef _SBRMI_CORE_H_
 #define _SBRMI_CORE_H_
 
+#include <linux/miscdevice.h>
 #include <linux/mutex.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <uapi/misc/amd-apml.h>
 
 /* SB-RMI registers */
 enum sbrmi_reg {
@@ -48,18 +50,15 @@ enum sbrmi_msg_id {
 
 /* Each client has this additional data */
 struct sbrmi_data {
+	struct miscdevice sbrmi_misc_dev;
 	struct regmap *regmap;
+	/* Mutex locking */
 	struct mutex lock;
 	u32 pwr_limit_max;
+	u8 dev_static_addr;
 };
 
-struct sbrmi_mailbox_msg {
-	u8 cmd;
-	bool read;
-	u32 data_in;
-	u32 data_out;
-};
-
-int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg);
+int rmi_mailbox_xfer(struct sbrmi_data *data, struct apml_message *msg);
 int create_hwmon_sensor_device(struct device *dev, struct sbrmi_data *data);
+int create_misc_rmi_device(struct sbrmi_data *data, struct device *dev);
 #endif /*_SBRMI_CORE_H_*/
diff --git a/drivers/misc/amd-sbi/rmi-hwmon.c b/drivers/misc/amd-sbi/rmi-hwmon.c
index 720e800db1f0..ee0c3b72174c 100644
--- a/drivers/misc/amd-sbi/rmi-hwmon.c
+++ b/drivers/misc/amd-sbi/rmi-hwmon.c
@@ -6,6 +6,7 @@
  */
 #include <linux/err.h>
 #include <linux/hwmon.h>
+#include <uapi/misc/amd-apml.h>
 #include "rmi-core.h"
 
 /* Do not allow setting negative power limit */
@@ -15,7 +16,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long *val)
 {
 	struct sbrmi_data *data = dev_get_drvdata(dev);
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 	int ret;
 
 	if (!data)
@@ -24,7 +25,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 	if (type != hwmon_power)
 		return -EINVAL;
 
-	msg.read = true;
+	msg.data_in.reg_in[AMD_SBI_RD_FLAG_INDEX] = 1;
 	switch (attr) {
 	case hwmon_power_input:
 		msg.cmd = SBRMI_READ_PKG_PWR_CONSUMPTION;
@@ -35,7 +36,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 		ret = rmi_mailbox_xfer(data, &msg);
 		break;
 	case hwmon_power_cap_max:
-		msg.data_out = data->pwr_limit_max;
+		msg.data_out.mb_out[AMD_SBI_RD_WR_DATA_INDEX] = data->pwr_limit_max;
 		ret = 0;
 		break;
 	default:
@@ -44,7 +45,7 @@ static int sbrmi_read(struct device *dev, enum hwmon_sensor_types type,
 	if (ret < 0)
 		return ret;
 	/* hwmon power attributes are in microWatt */
-	*val = (long)msg.data_out * 1000;
+	*val = (long)msg.data_out.mb_out[AMD_SBI_RD_WR_DATA_INDEX] * 1000;
 	return ret;
 }
 
@@ -52,7 +53,7 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel, long val)
 {
 	struct sbrmi_data *data = dev_get_drvdata(dev);
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 
 	if (!data)
 		return -ENODEV;
@@ -68,8 +69,8 @@ static int sbrmi_write(struct device *dev, enum hwmon_sensor_types type,
 	val = clamp_val(val, SBRMI_PWR_MIN, data->pwr_limit_max);
 
 	msg.cmd = SBRMI_WRITE_PKG_PWR_LIMIT;
-	msg.data_in = val;
-	msg.read = false;
+	msg.data_in.mb_in[AMD_SBI_RD_WR_DATA_INDEX] = val;
+	msg.data_in.reg_in[AMD_SBI_RD_FLAG_INDEX] = 0;
 
 	return rmi_mailbox_xfer(data, &msg);
 }
diff --git a/drivers/misc/amd-sbi/rmi-i2c.c b/drivers/misc/amd-sbi/rmi-i2c.c
index 7a9801273a4c..919ff7f61225 100644
--- a/drivers/misc/amd-sbi/rmi-i2c.c
+++ b/drivers/misc/amd-sbi/rmi-i2c.c
@@ -38,15 +38,15 @@ static int sbrmi_enable_alert(struct sbrmi_data *data)
 
 static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
 {
-	struct sbrmi_mailbox_msg msg = { 0 };
+	struct apml_message msg = { 0 };
 	int ret;
 
 	msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
-	msg.read = true;
+	msg.data_in.reg_in[AMD_SBI_RD_FLAG_INDEX] = 1;
 	ret = rmi_mailbox_xfer(data, &msg);
 	if (ret < 0)
 		return ret;
-	data->pwr_limit_max = msg.data_out;
+	data->pwr_limit_max = msg.data_out.mb_out[AMD_SBI_RD_WR_DATA_INDEX];
 
 	return ret;
 }
@@ -81,8 +81,24 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	data->dev_static_addr = client->addr;
 	dev_set_drvdata(dev, data);
-	return create_hwmon_sensor_device(dev, data);
+	ret = create_hwmon_sensor_device(dev, data);
+	if (ret < 0)
+		return ret;
+	return create_misc_rmi_device(data, dev);
+}
+
+static void sbrmi_i2c_remove(struct i2c_client *client)
+{
+	struct sbrmi_data *data = dev_get_drvdata(&client->dev);
+
+	misc_deregister(&data->sbrmi_misc_dev);
+	/* Assign fops and parent of misc dev to NULL */
+	data->sbrmi_misc_dev.fops = NULL;
+	data->sbrmi_misc_dev.parent = NULL;
+	dev_info(&client->dev, "Removed sbrmi-i2c driver\n");
+	return;
 }
 
 static const struct i2c_device_id sbrmi_id[] = {
@@ -105,6 +121,7 @@ static struct i2c_driver sbrmi_driver = {
 		.of_match_table = of_match_ptr(sbrmi_of_match),
 	},
 	.probe = sbrmi_i2c_probe,
+	.remove = sbrmi_i2c_remove,
 	.id_table = sbrmi_id,
 };
 
diff --git a/include/uapi/misc/amd-apml.h b/include/uapi/misc/amd-apml.h
new file mode 100644
index 000000000000..5721aaa0c6bd
--- /dev/null
+++ b/include/uapi/misc/amd-apml.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2021-2024 Advanced Micro Devices, Inc.
+ */
+#ifndef _AMD_APML_H_
+#define _AMD_APML_H_
+
+#include <linux/types.h>
+
+/* These are byte indexes into data_in and data_out arrays */
+#define AMD_SBI_RD_WR_DATA_INDEX	0
+#define AMD_SBI_REG_OFF_INDEX		0
+#define AMD_SBI_REG_VAL_INDEX		4
+#define AMD_SBI_RD_FLAG_INDEX		7
+
+#define AMD_SBI_MB_DATA_SIZE		4
+
+struct apml_message {
+	/* message ids:
+	 * Mailbox Messages:	0x0 ... 0x999
+	 */
+	__u32 cmd;
+
+	/*
+	 * 8 bit data for reg read,
+	 * 32 bit data in case of mailbox,
+	 */
+	union {
+		__u32 mb_out[2];
+		__u8 reg_out[8];
+	} data_out;
+
+	/*
+	 * [0]...[3] mailbox 32bit input
+	 * [7] read/write functionality
+	 */
+	union {
+		__u32 mb_in[2];
+		__u8 reg_in[8];
+	} data_in;
+} __attribute__((packed));
+
+/*
+ * AMD sideband interface base IOCTL
+ */
+#define SB_BASE_IOCTL_NR	0xF9
+
+/**
+ * DOC: SBRMI_IOCTL_CMD
+ *
+ * @Parameters
+ *
+ * @struct apml_message
+ *	Pointer to the &struct apml_message that will contain the protocol
+ *	information
+ *
+ * @Description
+ * IOCTL command for APML messages using generic _IOWR
+ * The IOCTL provides userspace access to AMD sideband protocols
+ * The APML RMI module checks whether the cmd is
+ * - Mailbox message read/write(0x0~0x999)
+ * - returning "-EFAULT" if none of the above
+ */
+#define SBRMI_IOCTL_CMD		_IOWR(SB_BASE_IOCTL_NR, 0, struct apml_message)
+
+#endif /*_AMD_APML_H_*/
-- 
2.25.1

Re: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

[Arnd Bergmann]

On Mon, Mar 3, 2025, at 11:58, Akshay Gupta wrote:

> +static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned 
> long arg)
> +{
> +	int __user *arguser = (int  __user *)arg;
> +	struct apml_message msg = { 0 };
> +	bool read = false;
> +	int ret;
> +
> +	struct sbrmi_data *data = container_of(fp->private_data, struct 
> sbrmi_data,
> +					       sbrmi_misc_dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	/* Copy the structure from user */
> +	if (copy_struct_from_user(&msg, sizeof(msg), arguser,
> +				  sizeof(struct apml_message)))
> +		return -EFAULT;

This is not how ioctl commands work: you need to check the
'cmd' argument, which includes the length of the data.

copy_struct_from_user() makes no sense here since the length
is fixed (for a given command).

> +	switch (msg.cmd) {
> +	case 0 ... 0x999:
> +		/* Mailbox protocol */
> +		ret = rmi_mailbox_xfer(data, &msg);
> +		break;

This looks like you are blindly passing through any command
from userspace, which is generally not the preferred way.

Usually this should be a known set of high-level commands
accepted by the driver.

> +static const struct file_operations sbrmi_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= sbrmi_ioctl,
> +	.compat_ioctl	= sbrmi_ioctl,

Change this to

.compat_ioctl = compat_ptr_ioctl,


> +	data->sbrmi_misc_dev.name	= devm_kasprintf(dev,
> +							 GFP_KERNEL,
> +							 "sbrmi-%x",
> +							 data->dev_static_addr);
> +	data->sbrmi_misc_dev.minor	= MISC_DYNAMIC_MINOR;
> +	data->sbrmi_misc_dev.fops	= &sbrmi_fops;
> +	data->sbrmi_misc_dev.parent	= dev;
> +	data->sbrmi_misc_dev.nodename	= devm_kasprintf(dev,
> +							 GFP_KERNEL,
> +							 "sbrmi-%x",
> +							 data->dev_static_addr);
> +	data->sbrmi_misc_dev.mode	= 0600;
> +
> +	return misc_register(&data->sbrmi_misc_dev);

What is 'dev_static_addr'? Usually you want a miscdevice to 
have a constant name and a static structure definition, not
dynamic allocation.

Are there multiple devices of this type in a given system?

> +struct apml_message {
> +	/* message ids:
> +	 * Mailbox Messages:	0x0 ... 0x999
> +	 */
> +	__u32 cmd;
> +
> +	/*
> +	 * 8 bit data for reg read,
> +	 * 32 bit data in case of mailbox,
> +	 */
> +	union {
> +		__u32 mb_out[2];
> +		__u8 reg_out[8];
> +	} data_out;
> +
> +	/*
> +	 * [0]...[3] mailbox 32bit input
> +	 * [7] read/write functionality
> +	 */
> +	union {
> +		__u32 mb_in[2];
> +		__u8 reg_in[8];
> +	} data_in;
> +} __attribute__((packed));

You normally want to have the in-kernel data aligned. Even
if userspace has it at a misaligned offset, it will still
work without the __packed.

     Arnd

Re: [PATCH v5 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

[Gupta, Akshay]

On 3/3/2025 9:48 PM, Arnd Bergmann wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, Mar 3, 2025, at 11:58, Akshay Gupta wrote:
>
>> +static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned
>> long arg)
>> +{
>> +     int __user *arguser = (int  __user *)arg;
>> +     struct apml_message msg = { 0 };
>> +     bool read = false;
>> +     int ret;
>> +
>> +     struct sbrmi_data *data = container_of(fp->private_data, struct
>> sbrmi_data,
>> +                                            sbrmi_misc_dev);
>> +     if (!data)
>> +             return -ENODEV;
>> +
>> +     /* Copy the structure from user */
>> +     if (copy_struct_from_user(&msg, sizeof(msg), arguser,
>> +                               sizeof(struct apml_message)))
>> +             return -EFAULT;
> This is not how ioctl commands work: you need to check the
> 'cmd' argument, which includes the length of the data.
will add check in ioctl for 'cmd'. Thank you.
>
> copy_struct_from_user() makes no sense here since the length
> is fixed (for a given command).
will modify it to use copy_from_user and size using the _IOC_SIZE(cmd)
>
>> +     switch (msg.cmd) {
>> +     case 0 ... 0x999:
>> +             /* Mailbox protocol */
>> +             ret = rmi_mailbox_xfer(data, &msg);
>> +             break;
> This looks like you are blindly passing through any command
> from userspace, which is generally not the preferred way.
>
> Usually this should be a known set of high-level commands
> accepted by the driver.
This will be addressed
>
>> +static const struct file_operations sbrmi_fops = {
>> +     .owner          = THIS_MODULE,
>> +     .unlocked_ioctl = sbrmi_ioctl,
>> +     .compat_ioctl   = sbrmi_ioctl,
> Change this to
>
> .compat_ioctl = compat_ptr_ioctl,
sure.
>
>
>> +     data->sbrmi_misc_dev.name       = devm_kasprintf(dev,
>> +                                                      GFP_KERNEL,
>> +                                                      "sbrmi-%x",
>> +                                                      data->dev_static_addr);
>> +     data->sbrmi_misc_dev.minor      = MISC_DYNAMIC_MINOR;
>> +     data->sbrmi_misc_dev.fops       = &sbrmi_fops;
>> +     data->sbrmi_misc_dev.parent     = dev;
>> +     data->sbrmi_misc_dev.nodename   = devm_kasprintf(dev,
>> +                                                      GFP_KERNEL,
>> +                                                      "sbrmi-%x",
>> +                                                      data->dev_static_addr);
>> +     data->sbrmi_misc_dev.mode       = 0600;
>> +
>> +     return misc_register(&data->sbrmi_misc_dev);
> What is 'dev_static_addr'? Usually you want a miscdevice to
> have a constant name and a static structure definition, not
> dynamic allocation.
>
> Are there multiple devices of this type in a given system?

Yes, there can be multiple devices on the basis of number of nodes.

>
>> +struct apml_message {
>> +     /* message ids:
>> +      * Mailbox Messages:    0x0 ... 0x999
>> +      */
>> +     __u32 cmd;
>> +
>> +     /*
>> +      * 8 bit data for reg read,
>> +      * 32 bit data in case of mailbox,
>> +      */
>> +     union {
>> +             __u32 mb_out[2];
>> +             __u8 reg_out[8];
>> +     } data_out;
>> +
>> +     /*
>> +      * [0]...[3] mailbox 32bit input
>> +      * [7] read/write functionality
>> +      */
>> +     union {
>> +             __u32 mb_in[2];
>> +             __u8 reg_in[8];
>> +     } data_in;
>> +} __attribute__((packed));
> You normally want to have the in-kernel data aligned. Even
> if userspace has it at a misaligned offset, it will still
> work without the __packed.
>
>       Arnd
Thank you, will update.