Re: [PATCH v2] fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `cancel_request` flag is erroneously cleared after acquiring `sec->lock` in `m10bmc_sec_prepare()`, causing valid firmware update cancellations to be silently ignored.
- [Medium] The `is_visible` callback for the `control` group returns `0` instead of `SYSFS_GROUP_INVISIBLE` when the device does not support image reloading, leaving an empty directory in sysfs.
--

--- Patch [1]: [PATCH v2] fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005 ---
commit d8e9e7daf1e274b64b2638b22b26c8f13aa35b4d
Author: Ionut Nechita <[email protected]>

fpga: m10bmc-sec: add image_load sysfs for N3000 and D5005

This commit adds the control/available_images and control/image_load sysfs
attributes for the N3000 and D5005 cards to allow triggering a reload of an
image already present in FLASH or EEPROM. It also introduces a mutex to
serialize image reloads against firmware uploads.

Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/linux-fpga/amGYhCKpesGH%2FyLB@yilunxu-OptiPlex-7050/

> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> index 7d23d914df3f2..06c9f9130c580 100644
> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
[ ... ]
> +static umode_t
> +m10bmc_control_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct m10bmc_sec *sec = dev_get_drvdata(kobj_to_dev(kobj));
> +
> +	if (!sec->ops->image_load)
> +		return 0;

[Severity: Medium]
Will returning 0 here for named groups leave an empty control directory in
sysfs?

If the device does not support image reloading (like N6000), returning 0 hides
the individual attributes but still creates the named group directory. Should
this return SYSFS_GROUP_INVISIBLE instead to suppress the entire directory?

> +
> +	return attr->mode;
> +}
> +
> +static struct attribute *m10bmc_control_attrs[] = {
> +	&dev_attr_available_images.attr,
> +	&dev_attr_image_load.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group m10bmc_control_attr_group = {
> +	.name = "control",
> +	.attrs = m10bmc_control_attrs,
> +	.is_visible = m10bmc_control_visible,
> +};
[ ... ]
> -static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
> -					     const u8 *data, u32 size)
> +static enum fw_upload_err m10bmc_sec_do_prepare(struct fw_upload *fwl,
> +						const u8 *data, u32 size)
>  {
>  	struct m10bmc_sec *sec = fwl->dd_handle;
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
[ ... ]
> +static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
> +					     const u8 *data, u32 size)
> +{
> +	struct m10bmc_sec *sec = fwl->dd_handle;
> +	enum fw_upload_err ret;
> +
> +	mutex_lock(&sec->lock);
> +
> +	ret = m10bmc_sec_do_prepare(fwl, data, size);

[Severity: High]
Does calling m10bmc_sec_do_prepare() overwrite a cancellation requested while
waiting for the lock?

If a firmware upload starts while an image reload is holding sec->lock,
m10bmc_sec_prepare() blocks. If a user tries to cancel the upload during this
time, it sets sec->cancel_request = true asynchronously.

When the lock is finally acquired, m10bmc_sec_do_prepare() unconditionally
sets the flag to false:

static enum fw_upload_err m10bmc_sec_do_prepare(...)
{
    ...
    sec->cancel_request = false;

Will this erase the abort state and proceed to flash the device despite the
cancellation request?

> +	if (ret != FW_UPLOAD_ERR_NONE)
> +		mutex_unlock(&sec->lock);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1