diff mbox series

bus: mhi: host: Read PK HASH dynamically

Message ID 20231208165938.1340587-1-quic_jhugo@quicinc.com
State Superseded
Headers show
Series bus: mhi: host: Read PK HASH dynamically | expand

Commit Message

Jeffrey Hugo Dec. 8, 2023, 4:59 p.m. UTC
The OEM PK HASH registers in the BHI region are read once during firmware
load (boot), cached, and displayed on demand via sysfs. This has a few
problems - if firmware load is skipped, the registers will not be read and
if the register values change over the life of the device the local cache
will be out of sync.

Qualcomm Cloud AI 100 can expose both these problems. It is possible for
mhi_async_power_up() to be invoked while the device is in AMSS EE, which
would bypass firmware loading. Also, Qualcomm Cloud AI 100 has 5 PK HASH
slots which can be dynamically provisioned while the device is active,
which would result in the values changing and users may want to know what
keys are active.

Address these concerns by reading the PK HASH registers on-demand during
the sysfs read. This will result in showing the most current information.

Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
---
 drivers/bus/mhi/host/boot.c | 11 +----------
 drivers/bus/mhi/host/init.c | 16 ++++++++++++----
 include/linux/mhi.h         |  2 --
 3 files changed, 13 insertions(+), 16 deletions(-)

Comments

Manivannan Sadhasivam Jan. 4, 2024, 6:46 a.m. UTC | #1
On Fri, Dec 08, 2023 at 09:59:38AM -0700, Jeffrey Hugo wrote:
> The OEM PK HASH registers in the BHI region are read once during firmware
> load (boot), cached, and displayed on demand via sysfs. This has a few
> problems - if firmware load is skipped, the registers will not be read and
> if the register values change over the life of the device the local cache
> will be out of sync.
> 
> Qualcomm Cloud AI 100 can expose both these problems. It is possible for
> mhi_async_power_up() to be invoked while the device is in AMSS EE, which
> would bypass firmware loading. Also, Qualcomm Cloud AI 100 has 5 PK HASH
> slots which can be dynamically provisioned while the device is active,
> which would result in the values changing and users may want to know what
> keys are active.
> 
> Address these concerns by reading the PK HASH registers on-demand during
> the sysfs read. This will result in showing the most current information.
> 
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> ---
>  drivers/bus/mhi/host/boot.c | 11 +----------
>  drivers/bus/mhi/host/init.c | 16 ++++++++++++----
>  include/linux/mhi.h         |  2 --
>  3 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index edc0ec5a0933..dedd29ca8db3 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -395,7 +395,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	void *buf;
>  	dma_addr_t dma_addr;
>  	size_t size, fw_sz;
> -	int i, ret;
> +	int ret;
>  
>  	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
>  		dev_err(dev, "Device MHI is not in valid state\n");
> @@ -408,15 +408,6 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	if (ret)
>  		dev_err(dev, "Could not capture serial number via BHI\n");
>  
> -	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++) {
> -		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i),
> -				   &mhi_cntrl->oem_pk_hash[i]);
> -		if (ret) {
> -			dev_err(dev, "Could not capture OEM PK HASH via BHI\n");
> -			break;
> -		}
> -	}
> -
>  	/* wait for ready on pass through or any other execution environment */
>  	if (!MHI_FW_LOAD_CAPABLE(mhi_cntrl->ee))
>  		goto fw_load_ready_state;
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 65ceac1837f9..e2c2f510b04f 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -97,11 +97,19 @@ static ssize_t oem_pk_hash_show(struct device *dev,
>  {
>  	struct mhi_device *mhi_dev = to_mhi_device(dev);
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> -	int i, cnt = 0;
> +	u32 hash_segment[MHI_MAX_OEM_PK_HASH_SEGMENTS];
> +	int i, cnt = 0, ret;
>  
> -	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
> -		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n",
> -				i, mhi_cntrl->oem_pk_hash[i]);
> +	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++) {
> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i), &hash_segment[i]);
> +		if (ret) {
> +			dev_err(dev, "Could not capture OEM PK HASH\n");
> +			return cnt;

return ret?

- Mani

> +		}
> +	}
> +
> +	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++)
> +		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n", i, hash_segment[i]);
>  
>  	return cnt;
>  }
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index d0f9b522f328..474d32cb0520 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -325,7 +325,6 @@ struct mhi_controller_config {
>   * @major_version: MHI controller major revision number
>   * @minor_version: MHI controller minor revision number
>   * @serial_number: MHI controller serial number obtained from BHI
> - * @oem_pk_hash: MHI controller OEM PK Hash obtained from BHI
>   * @mhi_event: MHI event ring configurations table
>   * @mhi_cmd: MHI command ring configurations table
>   * @mhi_ctxt: MHI device context, shared memory between host and device
> @@ -413,7 +412,6 @@ struct mhi_controller {
>  	u32 major_version;
>  	u32 minor_version;
>  	u32 serial_number;
> -	u32 oem_pk_hash[MHI_MAX_OEM_PK_HASH_SEGMENTS];
>  
>  	struct mhi_event *mhi_event;
>  	struct mhi_cmd *mhi_cmd;
> -- 
> 2.34.1
> 
>
Jeffrey Hugo Jan. 4, 2024, 3:54 p.m. UTC | #2
On 1/3/2024 11:46 PM, Manivannan Sadhasivam wrote:
> On Fri, Dec 08, 2023 at 09:59:38AM -0700, Jeffrey Hugo wrote:
>> The OEM PK HASH registers in the BHI region are read once during firmware
>> load (boot), cached, and displayed on demand via sysfs. This has a few
>> problems - if firmware load is skipped, the registers will not be read and
>> if the register values change over the life of the device the local cache
>> will be out of sync.
>>
>> Qualcomm Cloud AI 100 can expose both these problems. It is possible for
>> mhi_async_power_up() to be invoked while the device is in AMSS EE, which
>> would bypass firmware loading. Also, Qualcomm Cloud AI 100 has 5 PK HASH
>> slots which can be dynamically provisioned while the device is active,
>> which would result in the values changing and users may want to know what
>> keys are active.
>>
>> Address these concerns by reading the PK HASH registers on-demand during
>> the sysfs read. This will result in showing the most current information.
>>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/boot.c | 11 +----------
>>   drivers/bus/mhi/host/init.c | 16 ++++++++++++----
>>   include/linux/mhi.h         |  2 --
>>   3 files changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>> index edc0ec5a0933..dedd29ca8db3 100644
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -395,7 +395,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>   	void *buf;
>>   	dma_addr_t dma_addr;
>>   	size_t size, fw_sz;
>> -	int i, ret;
>> +	int ret;
>>   
>>   	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
>>   		dev_err(dev, "Device MHI is not in valid state\n");
>> @@ -408,15 +408,6 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>   	if (ret)
>>   		dev_err(dev, "Could not capture serial number via BHI\n");
>>   
>> -	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++) {
>> -		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i),
>> -				   &mhi_cntrl->oem_pk_hash[i]);
>> -		if (ret) {
>> -			dev_err(dev, "Could not capture OEM PK HASH via BHI\n");
>> -			break;
>> -		}
>> -	}
>> -
>>   	/* wait for ready on pass through or any other execution environment */
>>   	if (!MHI_FW_LOAD_CAPABLE(mhi_cntrl->ee))
>>   		goto fw_load_ready_state;
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index 65ceac1837f9..e2c2f510b04f 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -97,11 +97,19 @@ static ssize_t oem_pk_hash_show(struct device *dev,
>>   {
>>   	struct mhi_device *mhi_dev = to_mhi_device(dev);
>>   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> -	int i, cnt = 0;
>> +	u32 hash_segment[MHI_MAX_OEM_PK_HASH_SEGMENTS];
>> +	int i, cnt = 0, ret;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
>> -		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n",
>> -				i, mhi_cntrl->oem_pk_hash[i]);
>> +	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++) {
>> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i), &hash_segment[i]);
>> +		if (ret) {
>> +			dev_err(dev, "Could not capture OEM PK HASH\n");
>> +			return cnt;
> 
> return ret?

Sure.

> 
> - Mani
> 
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++)
>> +		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n", i, hash_segment[i]);
>>   
>>   	return cnt;
>>   }
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index d0f9b522f328..474d32cb0520 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -325,7 +325,6 @@ struct mhi_controller_config {
>>    * @major_version: MHI controller major revision number
>>    * @minor_version: MHI controller minor revision number
>>    * @serial_number: MHI controller serial number obtained from BHI
>> - * @oem_pk_hash: MHI controller OEM PK Hash obtained from BHI
>>    * @mhi_event: MHI event ring configurations table
>>    * @mhi_cmd: MHI command ring configurations table
>>    * @mhi_ctxt: MHI device context, shared memory between host and device
>> @@ -413,7 +412,6 @@ struct mhi_controller {
>>   	u32 major_version;
>>   	u32 minor_version;
>>   	u32 serial_number;
>> -	u32 oem_pk_hash[MHI_MAX_OEM_PK_HASH_SEGMENTS];
>>   
>>   	struct mhi_event *mhi_event;
>>   	struct mhi_cmd *mhi_cmd;
>> -- 
>> 2.34.1
>>
>>
>
Jeffrey Hugo Jan. 30, 2024, 4:05 p.m. UTC | #3
On 1/30/2024 1:21 AM, Manivannan Sadhasivam wrote:
> On Fri, Dec 08, 2023 at 09:59:38AM -0700, Jeffrey Hugo wrote:
>> The OEM PK HASH registers in the BHI region are read once during firmware
>> load (boot), cached, and displayed on demand via sysfs. This has a few
>> problems - if firmware load is skipped, the registers will not be read and
>> if the register values change over the life of the device the local cache
>> will be out of sync.
>>
>> Qualcomm Cloud AI 100 can expose both these problems. It is possible for
>> mhi_async_power_up() to be invoked while the device is in AMSS EE, which
>> would bypass firmware loading. Also, Qualcomm Cloud AI 100 has 5 PK HASH
>> slots which can be dynamically provisioned while the device is active,
>> which would result in the values changing and users may want to know what
>> keys are active.
>>
>> Address these concerns by reading the PK HASH registers on-demand during
>> the sysfs read. This will result in showing the most current information.
>>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> 
> Applied to mhi-next!
> 
> - Mani

I hope you applied v2 since this is a reply to v1.  I don't see mhi-next 
with this change published yet, so I can't check.

-Jeff
Manivannan Sadhasivam Jan. 30, 2024, 6:21 p.m. UTC | #4
On Tue, Jan 30, 2024 at 09:05:06AM -0700, Jeffrey Hugo wrote:
> On 1/30/2024 1:21 AM, Manivannan Sadhasivam wrote:
> > On Fri, Dec 08, 2023 at 09:59:38AM -0700, Jeffrey Hugo wrote:
> > > The OEM PK HASH registers in the BHI region are read once during firmware
> > > load (boot), cached, and displayed on demand via sysfs. This has a few
> > > problems - if firmware load is skipped, the registers will not be read and
> > > if the register values change over the life of the device the local cache
> > > will be out of sync.
> > > 
> > > Qualcomm Cloud AI 100 can expose both these problems. It is possible for
> > > mhi_async_power_up() to be invoked while the device is in AMSS EE, which
> > > would bypass firmware loading. Also, Qualcomm Cloud AI 100 has 5 PK HASH
> > > slots which can be dynamically provisioned while the device is active,
> > > which would result in the values changing and users may want to know what
> > > keys are active.
> > > 
> > > Address these concerns by reading the PK HASH registers on-demand during
> > > the sysfs read. This will result in showing the most current information.
> > > 
> > > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > 
> > Applied to mhi-next!
> > 
> > - Mani
> 
> I hope you applied v2 since this is a reply to v1.  I don't see mhi-next
> with this change published yet, so I can't check.
> 

Sorry, I did apply v2.

- Mani
diff mbox series

Patch

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index edc0ec5a0933..dedd29ca8db3 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -395,7 +395,7 @@  void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	void *buf;
 	dma_addr_t dma_addr;
 	size_t size, fw_sz;
-	int i, ret;
+	int ret;
 
 	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
 		dev_err(dev, "Device MHI is not in valid state\n");
@@ -408,15 +408,6 @@  void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	if (ret)
 		dev_err(dev, "Could not capture serial number via BHI\n");
 
-	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++) {
-		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i),
-				   &mhi_cntrl->oem_pk_hash[i]);
-		if (ret) {
-			dev_err(dev, "Could not capture OEM PK HASH via BHI\n");
-			break;
-		}
-	}
-
 	/* wait for ready on pass through or any other execution environment */
 	if (!MHI_FW_LOAD_CAPABLE(mhi_cntrl->ee))
 		goto fw_load_ready_state;
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 65ceac1837f9..e2c2f510b04f 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -97,11 +97,19 @@  static ssize_t oem_pk_hash_show(struct device *dev,
 {
 	struct mhi_device *mhi_dev = to_mhi_device(dev);
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
-	int i, cnt = 0;
+	u32 hash_segment[MHI_MAX_OEM_PK_HASH_SEGMENTS];
+	int i, cnt = 0, ret;
 
-	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
-		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n",
-				i, mhi_cntrl->oem_pk_hash[i]);
+	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++) {
+		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i), &hash_segment[i]);
+		if (ret) {
+			dev_err(dev, "Could not capture OEM PK HASH\n");
+			return cnt;
+		}
+	}
+
+	for (i = 0; i < MHI_MAX_OEM_PK_HASH_SEGMENTS; i++)
+		cnt += sysfs_emit_at(buf, cnt, "OEMPKHASH[%d]: 0x%x\n", i, hash_segment[i]);
 
 	return cnt;
 }
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index d0f9b522f328..474d32cb0520 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -325,7 +325,6 @@  struct mhi_controller_config {
  * @major_version: MHI controller major revision number
  * @minor_version: MHI controller minor revision number
  * @serial_number: MHI controller serial number obtained from BHI
- * @oem_pk_hash: MHI controller OEM PK Hash obtained from BHI
  * @mhi_event: MHI event ring configurations table
  * @mhi_cmd: MHI command ring configurations table
  * @mhi_ctxt: MHI device context, shared memory between host and device
@@ -413,7 +412,6 @@  struct mhi_controller {
 	u32 major_version;
 	u32 minor_version;
 	u32 serial_number;
-	u32 oem_pk_hash[MHI_MAX_OEM_PK_HASH_SEGMENTS];
 
 	struct mhi_event *mhi_event;
 	struct mhi_cmd *mhi_cmd;