Message-ID: <46d8e0ef-59f8-6298-93fb-80638599af8e@intel.com>
Date:   Tue, 21 Sep 2021 11:36:39 -0700
From:   Russ Weight <russell.h.weight@...el.com>
To:     Hillf Danton <hdanton@...a.com>
CC:     <linux-fpga@...r.kernel.org>, <trix@...hat.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v15 2/6] fpga: image-load: enable image loads



On 9/11/21 7:37 PM, Hillf Danton wrote:
> On Wed,  8 Sep 2021 19:18:42 -0700 Russ Weight wrote:
>> Extend the FPGA Image Load class driver to include IOCTL support
>> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
>> The IOCTL will return immediately, and the update will begin in the
>> context of a kernel worker thread.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
> Good work overall. Nits below.
> [...]
>
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -5,18 +5,181 @@
>>   * Copyright (C) 2019-2021 Intel Corporation, Inc.
>>   */
>>  
>> +#include <linux/delay.h>
>>  #include <linux/fpga/fpga-image-load.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>> +#include <linux/uaccess.h>
>>  #include <linux/vmalloc.h>
>>  
>>  #define IMAGE_LOAD_XA_LIMIT	XA_LIMIT(0, INT_MAX)
>>  static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>>  
>>  static struct class *fpga_image_load_class;
>> +static dev_t fpga_image_devt;
>>  
>>  #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>  
>> +static void fpga_image_dev_error(struct fpga_image_load *imgld,
>> +				 enum fpga_image_err err_code)
>> +{
>> +	imgld->err_code = err_code;
>> +	imgld->lops->cancel(imgld);
>> +}
>> +
>> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>> +{
>> +	mutex_lock(&imgld->lock);
>> +	imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> +	complete_all(&imgld->update_done);
>> +	mutex_unlock(&imgld->lock);
>> +}
>> +
>> +static void fpga_image_do_load(struct work_struct *work)
>> +{
>> +	struct fpga_image_load *imgld;
>> +	enum fpga_image_err ret;
>> +	u32 size, offset = 0;
>> +
>> +	imgld = container_of(work, struct fpga_image_load, work);
>> +	size = imgld->remaining_size;
> Bail out if imgld->driver_unload is set.
I'll add this.
>
>> +
>> +	get_device(&imgld->dev);
>> +	if (!try_module_get(imgld->dev.parent->driver->owner)) {
>> +		imgld->err_code = FPGA_IMAGE_ERR_BUSY;
>> +		goto idle_exit;
>> +	}
>> +
>> +	imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>> +	ret = imgld->lops->prepare(imgld);
>> +	if (ret != FPGA_IMAGE_ERR_NONE) {
>> +		fpga_image_dev_error(imgld, ret);
>> +		goto modput_exit;
>> +	}
>> +
>> +	imgld->progress = FPGA_IMAGE_PROG_WRITING;
>> +	while (imgld->remaining_size) {
>> +		ret = imgld->lops->write_blk(imgld, offset);
>> +		if (ret != FPGA_IMAGE_ERR_NONE) {
>> +			fpga_image_dev_error(imgld, ret);
>> +			goto done;
>> +		}
>> +
>> +		offset = size - imgld->remaining_size;
> If cond_resched() is needed here then make imgld->work unbound.
>
> 	queue_work(system_unbound_wq, &imgld->work);
Given that class driver does not have control over the size of
the data or the actual implementation of the writes, it is
probably best to add these changes. I'll do this for the next
submission of theses patches.

>
>> +	}
>> +
>> +	imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>> +	ret = imgld->lops->poll_complete(imgld);
>> +	if (ret != FPGA_IMAGE_ERR_NONE)
>> +		fpga_image_dev_error(imgld, ret);
>> +
>> +done:
>> +	if (imgld->lops->cleanup)
>> +		imgld->lops->cleanup(imgld);
>> +
>> +modput_exit:
>> +	module_put(imgld->dev.parent->driver->owner);
>> +
>> +idle_exit:
>> +	/*
>> +	 * Note: imgld->remaining_size is left unmodified here to provide
>> +	 * additional information on errors. It will be reinitialized when
>> +	 * the next image load begins.
>> +	 */
>> +	vfree(imgld->data);
>> +	imgld->data = NULL;
>> +	put_device(&imgld->dev);
>> +	fpga_image_prog_complete(imgld);
>> +}
>> +
>> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> +				       unsigned long arg)
>> +{
>> +	struct fpga_image_write wb;
>> +	unsigned long minsz;
>> +	u8 *buf;
>> +
>> +	if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
>> +		return -EBUSY;
>> +
>> +	minsz = offsetofend(struct fpga_image_write, buf);
>> +	if (copy_from_user(&wb, (void __user *)arg, minsz))
>> +		return -EFAULT;
>> +
>> +	if (wb.flags)
>> +		return -EINVAL;
>> +
>> +	/* Enforce 32-bit alignment on the write data */
>> +	if (wb.size & 0x3)
>> +		return -EINVAL;
>> +
>> +	buf = vzalloc(wb.size);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
>> +		vfree(buf);
>> +		return -EFAULT;
>> +	}
>> +
>> +	imgld->data = buf;
>> +	imgld->remaining_size = wb.size;
>> +	imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> +	imgld->progress = FPGA_IMAGE_PROG_STARTING;
>> +	reinit_completion(&imgld->update_done);
>> +	schedule_work(&imgld->work);
>> +
>> +	return 0;
>> +}
>> +
>> +static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>> +				  unsigned long arg)
>> +{
>> +	struct fpga_image_load *imgld = filp->private_data;
>> +	int ret = -ENOTTY;
>> +
>> +	switch (cmd) {
>> +	case FPGA_IMAGE_LOAD_WRITE:
>> +		mutex_lock(&imgld->lock);
>> +		ret = fpga_image_load_ioctl_write(imgld, arg);
>> +		mutex_unlock(&imgld->lock);
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
>> +{
>> +	struct fpga_image_load *imgld = container_of(inode->i_cdev,
>> +						     struct fpga_image_load, cdev);
>> +
>> +	if (test_and_set_bit(0, &imgld->opened))
>> +		return -EBUSY;
>> +
>> +	filp->private_data = imgld;
>> +
>> +	return 0;
>> +}
>> +
>> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
>> +{
>> +	struct fpga_image_load *imgld = filp->private_data;
>> +
>> +	clear_bit(0, &imgld->opened);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations fpga_image_load_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = fpga_image_load_open,
>> +	.release = fpga_image_load_release,
>> +	.unlocked_ioctl = fpga_image_load_ioctl,
>> +};
>> +
>>  /**
>>   * fpga_image_load_register - create and register an FPGA Image Load Device
>>   *
>> @@ -33,11 +196,17 @@ fpga_image_load_register(struct device *parent,
>>  			 const struct fpga_image_load_ops *lops, void *priv)
>>  {
>>  	struct fpga_image_load *imgld;
>> -	int id, ret;
>> +	int ret;
>> +
>> +	if (!lops || !lops->cancel || !lops->prepare ||
>> +	    !lops->write_blk || !lops->poll_complete) {
>> +		dev_err(parent, "Attempt to register without all required ops\n");
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>>  
>>  	imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
>>  	if (!imgld)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>  
>>  	ret = xa_alloc(&fpga_image_load_xa, &imgld->dev.id, imgld, IMAGE_LOAD_XA_LIMIT,
>>  		       GFP_KERNEL);
>> @@ -48,13 +217,19 @@ fpga_image_load_register(struct device *parent,
>>  
>>  	imgld->priv = priv;
>>  	imgld->lops = lops;
>> +	imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> +	imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> +	init_completion(&imgld->update_done);
>> +	INIT_WORK(&imgld->work, fpga_image_do_load);
>>  
>>  	imgld->dev.class = fpga_image_load_class;
>>  	imgld->dev.parent = parent;
>> +	imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>>  
>> -	ret = dev_set_name(&imgld->dev, "fpga_image%d", id);
>> +	ret = dev_set_name(&imgld->dev, "fpga_image%d", imgld->dev.id);
>>  	if (ret) {
>> -		dev_err(parent, "Failed to set device name: fpga_image%d\n", id);
>> +		dev_err(parent, "Failed to set device name: fpga_image%d\n",
>> +			imgld->dev.id);
>>  		goto error_device;
>>  	}
>>  
>> @@ -64,6 +239,16 @@ fpga_image_load_register(struct device *parent,
>>  		return ERR_PTR(ret);
>>  	}
>>  
>> +	cdev_init(&imgld->cdev, &fpga_image_load_fops);
>> +	imgld->cdev.owner = parent->driver->owner;
>> +	imgld->cdev.kobj.parent = &imgld->dev.kobj;
>> +
>> +	ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
>> +	if (ret) {
>> +		put_device(&imgld->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>>  	return imgld;
>>  
>>  error_device:
>> @@ -83,9 +268,29 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
>>   *
>>   * This function is intended for use in an FPGA Image Load driver's
>>   * remove() function.
>> + *
>> + * For some devices, once authentication of the uploaded image has begun,
>> + * the hardware cannot be signaled to stop, and the driver will not exit
>> + * until the hardware signals completion.  This could be 30+ minutes of
>> + * waiting. The driver_unload flag enables a force-unload of the driver
>> + * (e.g. modprobe -r) by signaling the parent driver to exit even if the
>> + * hardware update is incomplete. The driver_unload flag also prevents
>> + * new updates from starting once the unregister process has begun.
>>   */
>>  void fpga_image_load_unregister(struct fpga_image_load *imgld)
>>  {
>> +	mutex_lock(&imgld->lock);
>> +	imgld->driver_unload = true;
>> +	if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>> +		mutex_unlock(&imgld->lock);
>> +		goto unregister;
>> +	}
>> +
>> +	mutex_unlock(&imgld->lock);
>> +	wait_for_completion(&imgld->update_done);
> You can cut update_done if wait_for_completion() is replaced with
> flush_work() or cancel_work_sync(&imgld->work).
Thanks! I'll use flush_work() and eliminate the update_done completion
instance.

Thanks for the suggestions!

- Russ
>
>> +
>> +unregister:
>> +	cdev_del(&imgld->cdev);
>>  	device_unregister(&imgld->dev);
>>  }
>>  EXPORT_SYMBOL_GPL(fpga_image_load_unregister);

Powered by blists - more mailing lists