Re: [PATCH v3 5/8] misc: amd-sbi: Add support for CPUID protocol

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Aug 14, 2024, at 11:59, Akshay Gupta wrote:

> +/* input for bulk write to CPUID protocol */
> +struct cpu_msr_indata {
> +	u8 wr_len;	/* const value */
> +	u8 rd_len;	/* const value */
> +	u8 proto_cmd;	/* const value */
> +	u8 thread;	/* thread number */
> +	union {
> +		u8 reg_offset[4];	/* input value */
> +		u32 value;
> +	};
> +	u8 ext; /* extended function */
> +} __packed;

You cannot have a fully aligned union inside of a misaligned
struct. Since the only member is the inner 'u32 value',
I think it would make more sense to make that one
__packed instead of the structure.

> +/* output for bulk read from CPUID protocol */
> +struct cpu_msr_outdata {
> +	u8 num_bytes;	/* number of bytes return */
> +	u8 status;	/* Protocol status code */
> +	union {
> +		u64 value;
> +		u8 reg_data[8];
> +	};
> +} __packed;

Same here.

> +#define prepare_cpuid_input_message(input, thread_id, func, ext_func)	\
> +	input.rd_len = CPUID_RD_DATA_LEN,				\
> +	input.wr_len = CPUID_WR_DATA_LEN,				\
> +	input.proto_cmd = RD_CPUID_CMD,					\
> +	input.thread = thread_id << 1,					\
> +	input.value =  func,						\
> +	input.ext =  ext_func

This can be an inline function.

> +/*
> + * For Mailbox command software alert status bit is set by firmware
> + * to indicate command completion
> + * For RMI Rev 0x20, new h/w status bit is introduced. which is used
> + * by firmware to indicate completion of commands (0x71, 0x72, 0x73).
> + * wait for the status bit to be set by the firmware before
> + * reading the data out.
> + */
> +static int sbrmi_wait_status(struct sbrmi_data *data,
> +			     int *status, int mask)
> +{
> +	int ret, retry = 100;
> +
> +	do {
> +		ret = regmap_read(data->regmap, SBRMI_STATUS, status);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (*status & mask)
> +			break;
> +
> +		/* Wait 1~2 second for firmware to return data out */
> +		if (retry > 95)
> +			usleep_range(50, 100);
> +		else
> +			usleep_range(10000, 20000);
> +	} while (retry--);

This loop is likely to take much longer than 2 seconds if it
times out because of all the rounding etc.

You can probably change this to regmap_read_poll_timeout(),
which handles timeouts correctly.

> +/* command ID to identify CPUID protocol */
> +#define APML_CPUID	0x1000
>  /* 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_THREAD_LOW_INDEX	4
> +#define AMD_SBI_THREAD_HI_INDEX		5
> +#define AMD_SBI_EXT_FUNC_INDEX		6
> 
>  #define AMD_SBI_MB_DATA_SIZE		4
> 
>  struct apml_message {
>  	/* message ids:
>  	 * Mailbox Messages:	0x0 ... 0x999
> +	 * APML_CPUID:		0x1000
>  	 */
>  	__u32 cmd;
> 
>  	/*
>  	 * 8 bit data for reg read,
>  	 * 32 bit data in case of mailbox,
> +	 * up to 64 bit in case of cpuid
>  	 */
>  	union {
> +		__u64 cpu_msr_out;
>  		__u32 mb_out[2];
>  		__u8 reg_out[8];
>  	} data_out;
> 
>  	/*
>  	 * [0]...[3] mailbox 32bit input
> +	 *	     cpuid,
> +	 * [4][5] cpuid: thread
> +	 * [6] cpuid: ext function & read eax/ebx or ecx/edx
> +	 *	[7:0] -> bits [7:4] -> ext function &
> +	 *	bit [0] read eax/ebx or ecx/edx
>  	 * [7] read/write functionality
>  	 */
>  	union {
> +		__u64 cpu_msr_in;
>  		__u32 mb_in[2];
>  		__u8 reg_in[8];
>  	} data_in;
>  	/*
> +	 * Status code is returned in case of CPUID access
>  	 * Error code is returned in case of soft mailbox
>  	 */
>  	__u32 fw_ret_code;

Low-level protocols like this are rarely a good idea to be
exported to userspace. I can't see what the exact data is
that you can get in and out, but you probably want higher-level
interfaces for the individual things the platform interface
can do, either hooking up to existing kernel subsystems or
as separate user space interfaces.

     Arnd