linux-kernel - Re: [PATCH v2 5/6] i3c: master: Add support for SETAASA CCC

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <583c5381-11fe-4fc0-83b4-512c2aae50bd@linux.intel.com>
Date: Mon, 4 Nov 2024 17:00:50 +0200
From: Jarkko Nikula <jarkko.nikula@...ux.intel.com>
To: Shyam Sundar S K <Shyam-sundar.S-k@....com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: Sanket.Goswami@....com, linux-i3c@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] i3c: master: Add support for SETAASA CCC

On 10/23/24 8:51 AM, Shyam Sundar S K wrote:
> I3C devices like DIMMs over SPD use SETAASA for bus discovery instead of
> SETDASA. Add a new routine for I3C host controller drivers to use. If the
> I3C slave on the bus is an SPD device, skip the regular DAA process.
> 
> According to the SPD spec[1], use SETAASA for bus discovery, and avoid
> sending RSTDAA and DISEC, as they are considered illegal. Skip this entire
> process if the slave is SPD-compliant, as indicated by the "jdec_spd" flag
> from the BIOS.
> 
> [1] https://www.jedec.org/system/files/docs/JESD300-5B.01.pdf
> (section 2.4 and 2.6.3)
> 
SETAASA seems to come in MIPI v1.1.1 specification. I think worth to 
mention in commit log.

> Co-developed-by: Sanket Goswami <Sanket.Goswami@....com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@....com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> ---
>   drivers/i3c/master.c               | 32 +++++++++++++++++++++++++++++-
>   drivers/i3c/master/dw-i3c-master.c |  1 +
>   include/linux/i3c/ccc.h            |  1 +
>   include/linux/i3c/master.h         |  1 +
>   4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index ba6f17cb8aa6..1596efd6d82a 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1657,6 +1657,21 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>   	}
>   }
>   
> +static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
> +{
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
> +	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +
>   static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
>   				  struct i3c_dev_boardinfo *boardinfo)
>   {
> @@ -1684,6 +1699,10 @@ static int i3c_master_add_spd_dev(struct i3c_master_controller *master,
>   		i3cdev->info.pid = i3cdev->boardinfo->pid;
>   		i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
>   
> +		ret = i3c_master_setaasa_locked(master);
> +		if (ret)
> +			goto err_free_dev;
> +
>   		i3c_bus_normaluse_lock(&master->bus);
>   		i3c_master_register_new_i3c_devs(master);
>   		i3c_bus_normaluse_unlock(&master->bus);
> @@ -1907,7 +1926,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>   		goto err_bus_cleanup;
>   	}
>   
> -	i3c_master_add_spd_dev(master, i3cboardinfo);
> +	/*
> +	 * If the I3C slave on the bus is SPD device, then do not follow the regular
> +	 * DAA process. Also, as per SPD spec SETAASA is required for the bus discovery
> +	 * and sending RSTDAA and DISEC is considered as illegal. So skip the entire process
> +	 * if the jdec_spd flag has been identified from the BIOS.
> +	 */
> +	if (master->jdec_spd)
> +		return i3c_master_add_spd_dev(master, i3cboardinfo);
>   
>   	if (master->ops->set_speed) {
>   		ret = master->ops->set_speed(master, I3C_OPEN_DRAIN_SLOW_SPEED);
> @@ -2311,6 +2337,10 @@ static int i3c_acpi_configure_master(struct i3c_master_controller *master)
>   		return -ENODEV;
>   	}
>   
> +	status = acpi_evaluate_object(master->ahandle, "_STR", NULL, NULL);
> +	if (ACPI_SUCCESS(status))
> +		master->jdec_spd = true;
> +

Am I right "_STR" object should carry a string "jdec_spd"? But this code 
is not actually checking it, only the existence of _STR?

>   	num_dev = device_get_child_node_count(dev);
>   	if (!num_dev) {
>   		dev_err(&master->dev, "Error: no child node present\n");
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index f683e2a398ad..90a43209e55e 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -282,6 +282,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
>   	case I3C_CCC_GETSTATUS:
>   	case I3C_CCC_GETMXDS:
>   	case I3C_CCC_GETHDRCAP:
> +	case I3C_CCC_SETAASA:
>   		return true;
>   	default:
>   		return false;
> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
> index ad59a4ae60d1..a145d766ab6f 100644
> --- a/include/linux/i3c/ccc.h
> +++ b/include/linux/i3c/ccc.h
> @@ -32,6 +32,7 @@
>   #define I3C_CCC_DEFSLVS			I3C_CCC_ID(0x8, true)
>   #define I3C_CCC_ENTTM			I3C_CCC_ID(0xb, true)
>   #define I3C_CCC_ENTHDR(x)		I3C_CCC_ID(0x20 + (x), true)
> +#define I3C_CCC_SETAASA			I3C_CCC_ID(0x29, true)
>   
>   /* Unicast-only commands */
>   #define I3C_CCC_SETDASA			I3C_CCC_ID(0x7, false)
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 367faf7c4bf3..cd8390d8b469 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -516,6 +516,7 @@ struct i3c_master_controller {
>   	const struct i3c_master_controller_ops *ops;
>   	unsigned int secondary : 1;
>   	unsigned int init_done : 1;
> +	unsigned int jdec_spd : 1;
>   	unsigned int hotjoin: 1;
>   	struct {
>   		struct list_head i3c;