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

[Russ Weight]

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@xxxxxxxxx>
>> Reviewed-by: Wu Hao <hao.wu@xxxxxxxxx>
>> ---
>>  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,