diff mbox series

[v1,03/12] fpga: expose max10 flash update counts in sysfs

Message ID 20200904235305.6254-4-russell.h.weight@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Intel FPGA Security Manager Class Driver | expand

Commit Message

Russ Weight Sept. 4, 2020, 11:52 p.m. UTC 
Extend the MAX10 BMC Security Engine driver to provide a
handler to expose the flash update count for the FPGA user
image.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Wu Hao <hao.wu@intel.com>
---
 drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Tom Rix Sept. 5, 2020, 8:39 p.m. UTC | #1 
On 9/4/20 4:52 PM, Russ Weight wrote:
> Extend the MAX10 BMC Security Engine driver to provide a
> handler to expose the flash update count for the FPGA user
> image.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Wu Hao <hao.wu@intel.com>
> ---
>  drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> index 1f86bfb694b4..b824790e43aa 100644
> --- a/drivers/fpga/intel-m10-bmc-secure.c
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -10,6 +10,7 @@
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  
>  struct m10bmc_sec {
> @@ -99,7 +100,38 @@ SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>  SYSFS_GET_REH(sr, SR_REH_ADDR)
>  SYSFS_GET_REH(pr, PR_REH_ADDR)
>  
> +#define FLASH_COUNT_SIZE 4096
This seems too high at most it should be 64.
> +#define USER_FLASH_COUNT 0x17ffb000
Why shouldn't this be in intel-m10-bmc.h ?
> +
> +static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)

what does 'qspi' mean ?

unless there are going to be several *flash_count's consider

removing this substring.

> +{
> +	struct m10bmc_sec *sec = imgr->priv;
> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> +	unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
> +	u8 *flash_buf;
> +	int ret;
> +
> +	flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
> +	if (!flash_buf)
> +		return -ENOMEM;
> +
> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
> +				   FLASH_COUNT_SIZE / stride);
> +	if (ret) {
> +		dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
> +		goto exit_free;
> +	}
> +
> +	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
Simplify ret = num_bits...
> +
> +exit_free:
> +	kfree(flash_buf);
> +
> +	return ret ? : cnt;

Then simplify

return ret;

Tom

> +}
> +
>  static const struct ifpga_sec_mgr_ops m10bmc_iops = {
> +	.user_flash_count = get_qspi_flash_count,
>  	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>  	.sr_root_entry_hash = get_sr_root_entry_hash,
>  	.pr_root_entry_hash = get_pr_root_entry_hash,
Russ Weight Sept. 16, 2020, 6:37 p.m. UTC | #2 
On 9/5/20 1:39 PM, Tom Rix wrote:
> On 9/4/20 4:52 PM, Russ Weight wrote:
>> Extend the MAX10 BMC Security Engine driver to provide a
>> handler to expose the flash update count for the FPGA user
>> image.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Reviewed-by: Wu Hao <hao.wu@intel.com>
>> ---
>>  drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>> index 1f86bfb694b4..b824790e43aa 100644
>> --- a/drivers/fpga/intel-m10-bmc-secure.c
>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/mfd/intel-m10-bmc.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/slab.h>
>>  #include <linux/vmalloc.h>
>>  
>>  struct m10bmc_sec {
>> @@ -99,7 +100,38 @@ SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>>  SYSFS_GET_REH(sr, SR_REH_ADDR)
>>  SYSFS_GET_REH(pr, PR_REH_ADDR)
>>  
>> +#define FLASH_COUNT_SIZE 4096
> This seems too high at most it should be 64.

The flash count size represents the size of the flash memory that stores the
count. The count is represented in flash as an inverted bit vector.

I suppose a comment would be helpful here...

>> +#define USER_FLASH_COUNT 0x17ffb000
> Why shouldn't this be in intel-m10-bmc.h ?

Sure - I'll move it there with the other addresses.

>> +
>> +static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
> what does 'qspi' mean ?

It means Quad-SPI, the controller type that connects to the FLASH. This term
does seem out of place here. There is also a BMC flash count. I'll change this to
"user".

>
> unless there are going to be several *flash_count's consider
>
> removing this substring.
>
>> +{
>> +	struct m10bmc_sec *sec = imgr->priv;
>> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>> +	unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
>> +	u8 *flash_buf;
>> +	int ret;
>> +
>> +	flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
>> +	if (!flash_buf)
>> +		return -ENOMEM;
>> +
>> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
>> +				   FLASH_COUNT_SIZE / stride);
>> +	if (ret) {
>> +		dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
>> +		goto exit_free;
>> +	}
>> +
>> +	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
> Simplify ret = num_bits...

yes - will do.
Thanks!

- Russ

>> +
>> +exit_free:
>> +	kfree(flash_buf);
>> +
>> +	return ret ? : cnt;
> Then simplify
>
> return ret;
>
> Tom
>
>> +}
>> +
>>  static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>> +	.user_flash_count = get_qspi_flash_count,
>>  	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>>  	.sr_root_entry_hash = get_sr_root_entry_hash,
>>  	.pr_root_entry_hash = get_pr_root_entry_hash,
diff mbox series

Patch

diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index 1f86bfb694b4..b824790e43aa 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -10,6 +10,7 @@ 
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/vmalloc.h>
 
 struct m10bmc_sec {
@@ -99,7 +100,38 @@  SYSFS_GET_REH(bmc, BMC_REH_ADDR)
 SYSFS_GET_REH(sr, SR_REH_ADDR)
 SYSFS_GET_REH(pr, PR_REH_ADDR)
 
+#define FLASH_COUNT_SIZE 4096
+#define USER_FLASH_COUNT 0x17ffb000
+
+static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
+{
+	struct m10bmc_sec *sec = imgr->priv;
+	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+	unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
+	u8 *flash_buf;
+	int ret;
+
+	flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
+	if (!flash_buf)
+		return -ENOMEM;
+
+	ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
+				   FLASH_COUNT_SIZE / stride);
+	if (ret) {
+		dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
+		goto exit_free;
+	}
+
+	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
+
+exit_free:
+	kfree(flash_buf);
+
+	return ret ? : cnt;
+}
+
 static const struct ifpga_sec_mgr_ops m10bmc_iops = {
+	.user_flash_count = get_qspi_flash_count,
 	.bmc_root_entry_hash = get_bmc_root_entry_hash,
 	.sr_root_entry_hash = get_sr_root_entry_hash,
 	.pr_root_entry_hash = get_pr_root_entry_hash,