Extend the MAX10 BMC Secure Update driver to provide a sysfs file to
expose the flash update count.
Signed-off-by: Russ Weight <[email protected]>
Reviewed-by: Tom Rix <[email protected]>
---
v20:
- No change
v19:
- Change "card bmc" naming back to "m10 bmc" naming to be consistent
with the parent driver.
v18:
- No change
v17:
- Update the Date and KernelVersion for the ABI documentation to Jul 2022
and 5.19 respectively.
- Change "m10bmc" in symbol names to "cardbmc" to reflect the fact that the
future devices will not necessarily use the MAX10.
v16:
- No Change
v15:
- Updated the Dates and KernelVersions in the ABI documentation
v14:
- No change
v13:
- Updated ABI documentation date and kernel version
v12:
- Updated Date and KernelVersion fields in ABI documentation
v11:
- No change
v10:
- Changed the path expression in the sysfs documentation to
replace the n3000 reference with something more generic to
accomodate other devices that use the same driver.
v9:
- Rebased to 5.12-rc2 next
- Updated Date and KernelVersion in ABI documentation
v8:
- Previously patch 3/6, otherwise no change
v7:
- Updated Date and KernelVersion in ABI documentation
v6:
- Changed flash_count_show() parameter list to achieve
reverse-christmas tree format.
- Added WARN_ON() call for (FLASH_COUNT_SIZE / stride) to ensure
that the proper count is passed to regmap_bulk_read().
v5:
- Renamed sysfs node user_flash_count to flash_count and updated the
sysfs documentation accordingly.
v4:
- Moved the sysfs file for displaying the flash count from the
FPGA Security Manager class driver to here. The
m10bmc_user_flash_count() function is removed and the
functionality is moved into a user_flash_count_show()
function.
- Added ABI documentation for the new sysfs entry
v3:
- Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
- Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
driver"
- Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
underlying functions are now called directly.
v2:
- Renamed get_qspi_flash_count() to m10bmc_user_flash_count()
- Minor code cleanup per review comments
- Added m10bmc_ prefix to functions in m10bmc_iops structure
---
.../sysfs-driver-intel-m10-bmc-sec-update | 8 +++++
drivers/fpga/intel-m10-bmc-sec-update.c | 36 +++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
index 2bb271695e14..1132e39b2125 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
@@ -27,3 +27,11 @@ Description: Read only. Returns the root entry hash for the BMC image
"hash not programmed". This file is only visible if the
underlying device supports it.
Format: string.
+
+What: /sys/bus/platform/drivers/intel-m10bmc-sec-update/.../security/flash_count
+Date: Jul 2022
+KernelVersion: 5.19
+Contact: Russ Weight <[email protected]>
+Description: Read only. Returns number of times the secure update
+ staging area has been flashed.
+ Format: "%u".
diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index f9f39d2cfe5b..3f183202de3b 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -78,7 +78,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
+#define FLASH_COUNT_SIZE 4096 /* count stored as inverted bit vector */
+
+static ssize_t flash_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct m10bmc_sec *sec = dev_get_drvdata(dev);
+ unsigned int stride, num_bits;
+ u8 *flash_buf;
+ int cnt, ret;
+
+ stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+ num_bits = FLASH_COUNT_SIZE * 8;
+
+ flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
+ if (!flash_buf)
+ return -ENOMEM;
+
+ WARN_ON(FLASH_COUNT_SIZE % stride);
+ ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
+ flash_buf, FLASH_COUNT_SIZE / stride);
+ if (ret) {
+ dev_err(sec->dev,
+ "failed to read flash count: %x cnt %x: %d\n",
+ STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
+ goto exit_free;
+ }
+ cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
+
+exit_free:
+ kfree(flash_buf);
+
+ return ret ? : sysfs_emit(buf, "%u\n", cnt);
+}
+static DEVICE_ATTR_RO(flash_count);
+
static struct attribute *m10bmc_security_attrs[] = {
+ &dev_attr_flash_count.attr,
&dev_attr_bmc_root_entry_hash.attr,
&dev_attr_sr_root_entry_hash.attr,
&dev_attr_pr_root_entry_hash.attr,
--
2.25.1
On Mon, May 16, 2022 at 04:49:39PM -0700, Russ Weight wrote:
> Extend the MAX10 BMC Secure Update driver to provide a sysfs file to
> expose the flash update count.
>
> Signed-off-by: Russ Weight <[email protected]>
> Reviewed-by: Tom Rix <[email protected]>
> ---
> v20:
> - No change
> v19:
> - Change "card bmc" naming back to "m10 bmc" naming to be consistent
> with the parent driver.
> v18:
> - No change
> v17:
> - Update the Date and KernelVersion for the ABI documentation to Jul 2022
> and 5.19 respectively.
> - Change "m10bmc" in symbol names to "cardbmc" to reflect the fact that the
> future devices will not necessarily use the MAX10.
> v16:
> - No Change
> v15:
> - Updated the Dates and KernelVersions in the ABI documentation
> v14:
> - No change
> v13:
> - Updated ABI documentation date and kernel version
> v12:
> - Updated Date and KernelVersion fields in ABI documentation
> v11:
> - No change
> v10:
> - Changed the path expression in the sysfs documentation to
> replace the n3000 reference with something more generic to
> accomodate other devices that use the same driver.
> v9:
> - Rebased to 5.12-rc2 next
> - Updated Date and KernelVersion in ABI documentation
> v8:
> - Previously patch 3/6, otherwise no change
> v7:
> - Updated Date and KernelVersion in ABI documentation
> v6:
> - Changed flash_count_show() parameter list to achieve
> reverse-christmas tree format.
> - Added WARN_ON() call for (FLASH_COUNT_SIZE / stride) to ensure
> that the proper count is passed to regmap_bulk_read().
> v5:
> - Renamed sysfs node user_flash_count to flash_count and updated the
> sysfs documentation accordingly.
> v4:
> - Moved the sysfs file for displaying the flash count from the
> FPGA Security Manager class driver to here. The
> m10bmc_user_flash_count() function is removed and the
> functionality is moved into a user_flash_count_show()
> function.
> - Added ABI documentation for the new sysfs entry
> v3:
> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> - Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
> driver"
> - Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
> underlying functions are now called directly.
> v2:
> - Renamed get_qspi_flash_count() to m10bmc_user_flash_count()
> - Minor code cleanup per review comments
> - Added m10bmc_ prefix to functions in m10bmc_iops structure
> ---
> .../sysfs-driver-intel-m10-bmc-sec-update | 8 +++++
> drivers/fpga/intel-m10-bmc-sec-update.c | 36 +++++++++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> index 2bb271695e14..1132e39b2125 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> @@ -27,3 +27,11 @@ Description: Read only. Returns the root entry hash for the BMC image
> "hash not programmed". This file is only visible if the
> underlying device supports it.
> Format: string.
> +
> +What: /sys/bus/platform/drivers/intel-m10bmc-sec-update/.../security/flash_count
> +Date: Jul 2022
> +KernelVersion: 5.19
> +Contact: Russ Weight <[email protected]>
> +Description: Read only. Returns number of times the secure update
> + staging area has been flashed.
> + Format: "%u".
> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> index f9f39d2cfe5b..3f183202de3b 100644
> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> @@ -78,7 +78,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
> DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
> DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
>
> +#define FLASH_COUNT_SIZE 4096 /* count stored as inverted bit vector */
> +
> +static ssize_t flash_count_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct m10bmc_sec *sec = dev_get_drvdata(dev);
> + unsigned int stride, num_bits;
> + u8 *flash_buf;
> + int cnt, ret;
> +
> + stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> + num_bits = FLASH_COUNT_SIZE * 8;
> +
> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
> + if (!flash_buf)
> + return -ENOMEM;
> +
> + WARN_ON(FLASH_COUNT_SIZE % stride);
The same concern here. Stop users from getting the broken value.
> + ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
> + flash_buf, FLASH_COUNT_SIZE / stride);
> + if (ret) {
> + dev_err(sec->dev,
> + "failed to read flash count: %x cnt %x: %d\n",
> + STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
> + goto exit_free;
> + }
> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
> +
> +exit_free:
> + kfree(flash_buf);
> +
> + return ret ? : sysfs_emit(buf, "%u\n", cnt);
> +}
> +static DEVICE_ATTR_RO(flash_count);
> +
> static struct attribute *m10bmc_security_attrs[] = {
> + &dev_attr_flash_count.attr,
> &dev_attr_bmc_root_entry_hash.attr,
> &dev_attr_sr_root_entry_hash.attr,
> &dev_attr_pr_root_entry_hash.attr,
> --
> 2.25.1
On 5/16/22 21:13, Xu Yilun wrote:
> On Mon, May 16, 2022 at 04:49:39PM -0700, Russ Weight wrote:
>> Extend the MAX10 BMC Secure Update driver to provide a sysfs file to
>> expose the flash update count.
>>
>> Signed-off-by: Russ Weight <[email protected]>
>> Reviewed-by: Tom Rix <[email protected]>
>> ---
>> v20:
>> - No change
>> v19:
>> - Change "card bmc" naming back to "m10 bmc" naming to be consistent
>> with the parent driver.
>> v18:
>> - No change
>> v17:
>> - Update the Date and KernelVersion for the ABI documentation to Jul 2022
>> and 5.19 respectively.
>> - Change "m10bmc" in symbol names to "cardbmc" to reflect the fact that the
>> future devices will not necessarily use the MAX10.
>> v16:
>> - No Change
>> v15:
>> - Updated the Dates and KernelVersions in the ABI documentation
>> v14:
>> - No change
>> v13:
>> - Updated ABI documentation date and kernel version
>> v12:
>> - Updated Date and KernelVersion fields in ABI documentation
>> v11:
>> - No change
>> v10:
>> - Changed the path expression in the sysfs documentation to
>> replace the n3000 reference with something more generic to
>> accomodate other devices that use the same driver.
>> v9:
>> - Rebased to 5.12-rc2 next
>> - Updated Date and KernelVersion in ABI documentation
>> v8:
>> - Previously patch 3/6, otherwise no change
>> v7:
>> - Updated Date and KernelVersion in ABI documentation
>> v6:
>> - Changed flash_count_show() parameter list to achieve
>> reverse-christmas tree format.
>> - Added WARN_ON() call for (FLASH_COUNT_SIZE / stride) to ensure
>> that the proper count is passed to regmap_bulk_read().
>> v5:
>> - Renamed sysfs node user_flash_count to flash_count and updated the
>> sysfs documentation accordingly.
>> v4:
>> - Moved the sysfs file for displaying the flash count from the
>> FPGA Security Manager class driver to here. The
>> m10bmc_user_flash_count() function is removed and the
>> functionality is moved into a user_flash_count_show()
>> function.
>> - Added ABI documentation for the new sysfs entry
>> v3:
>> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
>> - Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
>> driver"
>> - Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
>> underlying functions are now called directly.
>> v2:
>> - Renamed get_qspi_flash_count() to m10bmc_user_flash_count()
>> - Minor code cleanup per review comments
>> - Added m10bmc_ prefix to functions in m10bmc_iops structure
>> ---
>> .../sysfs-driver-intel-m10-bmc-sec-update | 8 +++++
>> drivers/fpga/intel-m10-bmc-sec-update.c | 36 +++++++++++++++++++
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>> index 2bb271695e14..1132e39b2125 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>> @@ -27,3 +27,11 @@ Description: Read only. Returns the root entry hash for the BMC image
>> "hash not programmed". This file is only visible if the
>> underlying device supports it.
>> Format: string.
>> +
>> +What: /sys/bus/platform/drivers/intel-m10bmc-sec-update/.../security/flash_count
>> +Date: Jul 2022
>> +KernelVersion: 5.19
>> +Contact: Russ Weight <[email protected]>
>> +Description: Read only. Returns number of times the secure update
>> + staging area has been flashed.
>> + Format: "%u".
>> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
>> index f9f39d2cfe5b..3f183202de3b 100644
>> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
>> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
>> @@ -78,7 +78,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
>> DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
>> DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
>>
>> +#define FLASH_COUNT_SIZE 4096 /* count stored as inverted bit vector */
>> +
>> +static ssize_t flash_count_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct m10bmc_sec *sec = dev_get_drvdata(dev);
>> + unsigned int stride, num_bits;
>> + u8 *flash_buf;
>> + int cnt, ret;
>> +
>> + stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>> + num_bits = FLASH_COUNT_SIZE * 8;
>> +
>> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
>> + if (!flash_buf)
>> + return -ENOMEM;
>> +
>> + WARN_ON(FLASH_COUNT_SIZE % stride);
> The same concern here. Stop users from getting the broken value.
Sure - I will change this.
I was using WARN_ON() because it indicates a problem in the device or
the driver itself. It is not really validating information from userspace.
As I understand it, WARN_ON() is used to log such events to the kernel
log. If this isn't an appropriate use of WARN_ON(), then when should I
consider using it?
Thanks,
- Russ
>
>> + ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
>> + flash_buf, FLASH_COUNT_SIZE / stride);
>> + if (ret) {
>> + dev_err(sec->dev,
>> + "failed to read flash count: %x cnt %x: %d\n",
>> + STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
>> + goto exit_free;
>> + }
>> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
>> +
>> +exit_free:
>> + kfree(flash_buf);
>> +
>> + return ret ? : sysfs_emit(buf, "%u\n", cnt);
>> +}
>> +static DEVICE_ATTR_RO(flash_count);
>> +
>> static struct attribute *m10bmc_security_attrs[] = {
>> + &dev_attr_flash_count.attr,
>> &dev_attr_bmc_root_entry_hash.attr,
>> &dev_attr_sr_root_entry_hash.attr,
>> &dev_attr_pr_root_entry_hash.attr,
>> --
>> 2.25.1
On Tue, May 17, 2022 at 09:16:50AM -0700, Russ Weight wrote:
>
>
> On 5/16/22 21:13, Xu Yilun wrote:
> > On Mon, May 16, 2022 at 04:49:39PM -0700, Russ Weight wrote:
> >> Extend the MAX10 BMC Secure Update driver to provide a sysfs file to
> >> expose the flash update count.
> >>
> >> Signed-off-by: Russ Weight <[email protected]>
> >> Reviewed-by: Tom Rix <[email protected]>
> >> ---
> >> v20:
> >> - No change
> >> v19:
> >> - Change "card bmc" naming back to "m10 bmc" naming to be consistent
> >> with the parent driver.
> >> v18:
> >> - No change
> >> v17:
> >> - Update the Date and KernelVersion for the ABI documentation to Jul 2022
> >> and 5.19 respectively.
> >> - Change "m10bmc" in symbol names to "cardbmc" to reflect the fact that the
> >> future devices will not necessarily use the MAX10.
> >> v16:
> >> - No Change
> >> v15:
> >> - Updated the Dates and KernelVersions in the ABI documentation
> >> v14:
> >> - No change
> >> v13:
> >> - Updated ABI documentation date and kernel version
> >> v12:
> >> - Updated Date and KernelVersion fields in ABI documentation
> >> v11:
> >> - No change
> >> v10:
> >> - Changed the path expression in the sysfs documentation to
> >> replace the n3000 reference with something more generic to
> >> accomodate other devices that use the same driver.
> >> v9:
> >> - Rebased to 5.12-rc2 next
> >> - Updated Date and KernelVersion in ABI documentation
> >> v8:
> >> - Previously patch 3/6, otherwise no change
> >> v7:
> >> - Updated Date and KernelVersion in ABI documentation
> >> v6:
> >> - Changed flash_count_show() parameter list to achieve
> >> reverse-christmas tree format.
> >> - Added WARN_ON() call for (FLASH_COUNT_SIZE / stride) to ensure
> >> that the proper count is passed to regmap_bulk_read().
> >> v5:
> >> - Renamed sysfs node user_flash_count to flash_count and updated the
> >> sysfs documentation accordingly.
> >> v4:
> >> - Moved the sysfs file for displaying the flash count from the
> >> FPGA Security Manager class driver to here. The
> >> m10bmc_user_flash_count() function is removed and the
> >> functionality is moved into a user_flash_count_show()
> >> function.
> >> - Added ABI documentation for the new sysfs entry
> >> v3:
> >> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> >> - Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
> >> driver"
> >> - Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
> >> underlying functions are now called directly.
> >> v2:
> >> - Renamed get_qspi_flash_count() to m10bmc_user_flash_count()
> >> - Minor code cleanup per review comments
> >> - Added m10bmc_ prefix to functions in m10bmc_iops structure
> >> ---
> >> .../sysfs-driver-intel-m10-bmc-sec-update | 8 +++++
> >> drivers/fpga/intel-m10-bmc-sec-update.c | 36 +++++++++++++++++++
> >> 2 files changed, 44 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> >> index 2bb271695e14..1132e39b2125 100644
> >> --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> >> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
> >> @@ -27,3 +27,11 @@ Description: Read only. Returns the root entry hash for the BMC image
> >> "hash not programmed". This file is only visible if the
> >> underlying device supports it.
> >> Format: string.
> >> +
> >> +What: /sys/bus/platform/drivers/intel-m10bmc-sec-update/.../security/flash_count
> >> +Date: Jul 2022
> >> +KernelVersion: 5.19
> >> +Contact: Russ Weight <[email protected]>
> >> +Description: Read only. Returns number of times the secure update
> >> + staging area has been flashed.
> >> + Format: "%u".
> >> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> >> index f9f39d2cfe5b..3f183202de3b 100644
> >> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> >> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> >> @@ -78,7 +78,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
> >> DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
> >> DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
> >>
> >> +#define FLASH_COUNT_SIZE 4096 /* count stored as inverted bit vector */
> >> +
> >> +static ssize_t flash_count_show(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct m10bmc_sec *sec = dev_get_drvdata(dev);
> >> + unsigned int stride, num_bits;
> >> + u8 *flash_buf;
> >> + int cnt, ret;
> >> +
> >> + stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> >> + num_bits = FLASH_COUNT_SIZE * 8;
> >> +
> >> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
> >> + if (!flash_buf)
> >> + return -ENOMEM;
> >> +
> >> + WARN_ON(FLASH_COUNT_SIZE % stride);
> > The same concern here. Stop users from getting the broken value.
>
> Sure - I will change this.
>
> I was using WARN_ON() because it indicates a problem in the device or
> the driver itself. It is not really validating information from userspace.
Understood. So it is OK we keep the WARN_ON, or WARN_ON_ONCE？But we
should still have some code to handle the issue gracefully rather
than just passing the broken value to user.
Thanks
Yilun
>
> As I understand it, WARN_ON() is used to log such events to the kernel
> log. If this isn't an appropriate use of WARN_ON(), then when should I
> consider using it?
>
> Thanks,
> - Russ
> >
> >> + ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
> >> + flash_buf, FLASH_COUNT_SIZE / stride);
> >> + if (ret) {
> >> + dev_err(sec->dev,
> >> + "failed to read flash count: %x cnt %x: %d\n",
> >> + STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
> >> + goto exit_free;
> >> + }
> >> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
> >> +
> >> +exit_free:
> >> + kfree(flash_buf);
> >> +
> >> + return ret ? : sysfs_emit(buf, "%u\n", cnt);
> >> +}
> >> +static DEVICE_ATTR_RO(flash_count);
> >> +
> >> static struct attribute *m10bmc_security_attrs[] = {
> >> + &dev_attr_flash_count.attr,
> >> &dev_attr_bmc_root_entry_hash.attr,
> >> &dev_attr_sr_root_entry_hash.attr,
> >> &dev_attr_pr_root_entry_hash.attr,
> >> --
> >> 2.25.1
On 5/18/22 00:10, Xu Yilun wrote:
> On Tue, May 17, 2022 at 09:16:50AM -0700, Russ Weight wrote:
>>
>> On 5/16/22 21:13, Xu Yilun wrote:
>>> On Mon, May 16, 2022 at 04:49:39PM -0700, Russ Weight wrote:
>>>> Extend the MAX10 BMC Secure Update driver to provide a sysfs file to
>>>> expose the flash update count.
>>>>
>>>> Signed-off-by: Russ Weight <[email protected]>
>>>> Reviewed-by: Tom Rix <[email protected]>
>>>> ---
>>>> v20:
>>>> - No change
>>>> v19:
>>>> - Change "card bmc" naming back to "m10 bmc" naming to be consistent
>>>> with the parent driver.
>>>> v18:
>>>> - No change
>>>> v17:
>>>> - Update the Date and KernelVersion for the ABI documentation to Jul 2022
>>>> and 5.19 respectively.
>>>> - Change "m10bmc" in symbol names to "cardbmc" to reflect the fact that the
>>>> future devices will not necessarily use the MAX10.
>>>> v16:
>>>> - No Change
>>>> v15:
>>>> - Updated the Dates and KernelVersions in the ABI documentation
>>>> v14:
>>>> - No change
>>>> v13:
>>>> - Updated ABI documentation date and kernel version
>>>> v12:
>>>> - Updated Date and KernelVersion fields in ABI documentation
>>>> v11:
>>>> - No change
>>>> v10:
>>>> - Changed the path expression in the sysfs documentation to
>>>> replace the n3000 reference with something more generic to
>>>> accomodate other devices that use the same driver.
>>>> v9:
>>>> - Rebased to 5.12-rc2 next
>>>> - Updated Date and KernelVersion in ABI documentation
>>>> v8:
>>>> - Previously patch 3/6, otherwise no change
>>>> v7:
>>>> - Updated Date and KernelVersion in ABI documentation
>>>> v6:
>>>> - Changed flash_count_show() parameter list to achieve
>>>> reverse-christmas tree format.
>>>> - Added WARN_ON() call for (FLASH_COUNT_SIZE / stride) to ensure
>>>> that the proper count is passed to regmap_bulk_read().
>>>> v5:
>>>> - Renamed sysfs node user_flash_count to flash_count and updated the
>>>> sysfs documentation accordingly.
>>>> v4:
>>>> - Moved the sysfs file for displaying the flash count from the
>>>> FPGA Security Manager class driver to here. The
>>>> m10bmc_user_flash_count() function is removed and the
>>>> functionality is moved into a user_flash_count_show()
>>>> function.
>>>> - Added ABI documentation for the new sysfs entry
>>>> v3:
>>>> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
>>>> - Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
>>>> driver"
>>>> - Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
>>>> underlying functions are now called directly.
>>>> v2:
>>>> - Renamed get_qspi_flash_count() to m10bmc_user_flash_count()
>>>> - Minor code cleanup per review comments
>>>> - Added m10bmc_ prefix to functions in m10bmc_iops structure
>>>> ---
>>>> .../sysfs-driver-intel-m10-bmc-sec-update | 8 +++++
>>>> drivers/fpga/intel-m10-bmc-sec-update.c | 36 +++++++++++++++++++
>>>> 2 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>>>> index 2bb271695e14..1132e39b2125 100644
>>>> --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
>>>> @@ -27,3 +27,11 @@ Description: Read only. Returns the root entry hash for the BMC image
>>>> "hash not programmed". This file is only visible if the
>>>> underlying device supports it.
>>>> Format: string.
>>>> +
>>>> +What: /sys/bus/platform/drivers/intel-m10bmc-sec-update/.../security/flash_count
>>>> +Date: Jul 2022
>>>> +KernelVersion: 5.19
>>>> +Contact: Russ Weight <[email protected]>
>>>> +Description: Read only. Returns number of times the secure update
>>>> + staging area has been flashed.
>>>> + Format: "%u".
>>>> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
>>>> index f9f39d2cfe5b..3f183202de3b 100644
>>>> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
>>>> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
>>>> @@ -78,7 +78,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
>>>> DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
>>>> DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
>>>>
>>>> +#define FLASH_COUNT_SIZE 4096 /* count stored as inverted bit vector */
>>>> +
>>>> +static ssize_t flash_count_show(struct device *dev,
>>>> + struct device_attribute *attr, char *buf)
>>>> +{
>>>> + struct m10bmc_sec *sec = dev_get_drvdata(dev);
>>>> + unsigned int stride, num_bits;
>>>> + u8 *flash_buf;
>>>> + int cnt, ret;
>>>> +
>>>> + stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>>>> + num_bits = FLASH_COUNT_SIZE * 8;
>>>> +
>>>> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
>>>> + if (!flash_buf)
>>>> + return -ENOMEM;
>>>> +
>>>> + WARN_ON(FLASH_COUNT_SIZE % stride);
>>> The same concern here. Stop users from getting the broken value.
>> Sure - I will change this.
>>
>> I was using WARN_ON() because it indicates a problem in the device or
>> the driver itself. It is not really validating information from userspace.
> Understood. So it is OK we keep the WARN_ON, or WARN_ON_ONCE？But we
> should still have some code to handle the issue gracefully rather
> than just passing the broken value to user.
Yes, I have gone through each of these cases and made sure that they
return an error rather than garbage. I have also added kernel logging
in some cases.
Thanks,
- Russ
>
> Thanks
> Yilun
>
>> As I understand it, WARN_ON() is used to log such events to the kernel
>> log. If this isn't an appropriate use of WARN_ON(), then when should I
>> consider using it?
>>
>> Thanks,
>> - Russ
>>>> + ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
>>>> + flash_buf, FLASH_COUNT_SIZE / stride);
>>>> + if (ret) {
>>>> + dev_err(sec->dev,
>>>> + "failed to read flash count: %x cnt %x: %d\n",
>>>> + STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
>>>> + goto exit_free;
>>>> + }
>>>> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
>>>> +
>>>> +exit_free:
>>>> + kfree(flash_buf);
>>>> +
>>>> + return ret ? : sysfs_emit(buf, "%u\n", cnt);
>>>> +}
>>>> +static DEVICE_ATTR_RO(flash_count);
>>>> +
>>>> static struct attribute *m10bmc_security_attrs[] = {
>>>> + &dev_attr_flash_count.attr,
>>>> &dev_attr_bmc_root_entry_hash.attr,
>>>> &dev_attr_sr_root_entry_hash.attr,
>>>> &dev_attr_pr_root_entry_hash.attr,
>>>> --
>>>> 2.25.1