diff mbox series

[v10,14/14] dmaengine: idxd: Add support for device/wq defaults

Message ID 20231127202704.1263376-15-tom.zanussi@linux.intel.com
State New
Headers
Series crypto: Add Intel Analytics Accelerator (IAA) crypto compression driver |

Commit Message

Tom Zanussi Nov. 27, 2023, 8:27 p.m. UTC 
  Add a load_device_defaults() function pointer to struct
idxd_driver_data, which if defined, will be called when an idxd device
is probed and will allow the idxd device to be configured with default
values.

The load_device_defaults() function is passed an idxd device to work
with to set specific device attributes.

Also add a load_device_defaults() implementation IAA devices; future
patches would add default functions for other device types such as
DSA.

The way idxd device probing works, if the device configuration is
valid at that point e.g. at least one workqueue and engine is properly
configured then the device will be enabled and ready to go.

The IAA implementation, idxd_load_iaa_device_defaults(), configures a
single workqueue (wq0) for each device with the following default
values:

      mode     	        "dedicated"
      threshold		0
      size		16
      priority		10
      type		IDXD_WQT_KERNEL
      group		0
      name              "iaa_crypto"
      driver_name       "crypto"

Note that this now adds another configuration step for any users that
want to configure their own devices/workqueus with something different
in that they'll first need to disable (in the case of IAA) wq0 and the
device itself before they can set their own attributes and re-enable,
since they've been already been auto-enabled.  Note also that in order
for the new configuration to be applied to the deflate-iaa crypto
algorithm the iaa_crypto module needs to unregister the old version,
which is accomplished by removing the iaa_crypto module, and
re-registering it with the new configuration by reinserting the
iaa_crypto module.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 drivers/dma/idxd/Makefile   |  2 +-
 drivers/dma/idxd/defaults.c | 53 +++++++++++++++++++++++++++++++++++++
 drivers/dma/idxd/idxd.h     |  4 +++
 drivers/dma/idxd/init.c     |  7 +++++
 4 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma/idxd/defaults.c

Comments

Dave Jiang Nov. 27, 2023, 10:14 p.m. UTC | #1 
On 11/27/23 13:27, Tom Zanussi wrote:
> Add a load_device_defaults() function pointer to struct
> idxd_driver_data, which if defined, will be called when an idxd device
> is probed and will allow the idxd device to be configured with default
> values.
> 
> The load_device_defaults() function is passed an idxd device to work
> with to set specific device attributes.
> 
> Also add a load_device_defaults() implementation IAA devices; future
> patches would add default functions for other device types such as
> DSA.
> 
> The way idxd device probing works, if the device configuration is
> valid at that point e.g. at least one workqueue and engine is properly
> configured then the device will be enabled and ready to go.
> 
> The IAA implementation, idxd_load_iaa_device_defaults(), configures a
> single workqueue (wq0) for each device with the following default
> values:
> 
>       mode     	        "dedicated"
>       threshold		0
>       size		16
>       priority		10
>       type		IDXD_WQT_KERNEL
>       group		0
>       name              "iaa_crypto"
>       driver_name       "crypto"
> 
> Note that this now adds another configuration step for any users that
> want to configure their own devices/workqueus with something different
> in that they'll first need to disable (in the case of IAA) wq0 and the
> device itself before they can set their own attributes and re-enable,
> since they've been already been auto-enabled.  Note also that in order
> for the new configuration to be applied to the deflate-iaa crypto
> algorithm the iaa_crypto module needs to unregister the old version,
> which is accomplished by removing the iaa_crypto module, and
> re-registering it with the new configuration by reinserting the
> iaa_crypto module.
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/dma/idxd/Makefile   |  2 +-
>  drivers/dma/idxd/defaults.c | 53 +++++++++++++++++++++++++++++++++++++
>  drivers/dma/idxd/idxd.h     |  4 +++
>  drivers/dma/idxd/init.c     |  7 +++++
>  4 files changed, 65 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/dma/idxd/defaults.c
> 
> diff --git a/drivers/dma/idxd/Makefile b/drivers/dma/idxd/Makefile
> index c5e679070e46..2b4a0d406e1e 100644
> --- a/drivers/dma/idxd/Makefile
> +++ b/drivers/dma/idxd/Makefile
> @@ -4,7 +4,7 @@ obj-$(CONFIG_INTEL_IDXD_BUS) += idxd_bus.o
>  idxd_bus-y := bus.o
>  
>  obj-$(CONFIG_INTEL_IDXD) += idxd.o
> -idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o
> +idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o defaults.o
>  
>  idxd-$(CONFIG_INTEL_IDXD_PERFMON) += perfmon.o
>  
> diff --git a/drivers/dma/idxd/defaults.c b/drivers/dma/idxd/defaults.c
> new file mode 100644
> index 000000000000..a0c9faad8efe
> --- /dev/null
> +++ b/drivers/dma/idxd/defaults.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Intel Corporation. All rights rsvd. */
> +#include <linux/kernel.h>
> +#include "idxd.h"
> +
> +int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
> +{
> +	struct idxd_engine *engine;
> +	struct idxd_group *group;
> +	struct idxd_wq *wq;
> +
> +	if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
> +		return 0;
> +
> +	wq = idxd->wqs[0];
> +
> +	if (wq->state != IDXD_WQ_DISABLED)
> +		return -EPERM;
> +
> +	/* set mode to "dedicated" */
> +	set_bit(WQ_FLAG_DEDICATED, &wq->flags);
> +	wq->threshold = 0;
> +
> +	/* set size to 16 */
> +	wq->size = 16;
> +
> +	/* set priority to 10 */
> +	wq->priority = 10;
> +
> +	/* set type to "kernel" */
> +	wq->type = IDXD_WQT_KERNEL;
> +
> +	/* set wq group to 0 */
> +	group = idxd->groups[0];
> +	wq->group = group;
> +	group->num_wqs++;
> +
> +	/* set name to "iaa_crypto" */
> +	memset(wq->name, 0, WQ_NAME_SIZE + 1);
> +	strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
> +
> +	/* set driver_name to "crypto" */
> +	memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> +	strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);
> +
> +	engine = idxd->engines[0];
> +
> +	/* set engine group to 0 */
> +	engine->group = idxd->groups[0];
> +	engine->group->num_engines++;
> +
> +	return 0;
> +}
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index 62ea21b25906..47de3f93ff1e 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -277,6 +277,8 @@ struct idxd_dma_dev {
>  	struct dma_device dma;
>  };
>  
> +typedef int (*load_device_defaults_fn_t) (struct idxd_device *idxd);
> +
>  struct idxd_driver_data {
>  	const char *name_prefix;
>  	enum idxd_type type;
> @@ -286,6 +288,7 @@ struct idxd_driver_data {
>  	int evl_cr_off;
>  	int cr_status_off;
>  	int cr_result_off;
> +	load_device_defaults_fn_t load_device_defaults;
>  };
>  
>  struct idxd_evl {
> @@ -730,6 +733,7 @@ void idxd_unregister_devices(struct idxd_device *idxd);
>  void idxd_wqs_quiesce(struct idxd_device *idxd);
>  bool idxd_queue_int_handle_resubmit(struct idxd_desc *desc);
>  void multi_u64_to_bmap(unsigned long *bmap, u64 *val, int count);
> +int idxd_load_iaa_device_defaults(struct idxd_device *idxd);
>  
>  /* device interrupt control */
>  irqreturn_t idxd_misc_thread(int vec, void *data);
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 0eb1c827a215..14df1f1347a8 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -59,6 +59,7 @@ static struct idxd_driver_data idxd_driver_data[] = {
>  		.evl_cr_off = offsetof(struct iax_evl_entry, cr),
>  		.cr_status_off = offsetof(struct iax_completion_record, status),
>  		.cr_result_off = offsetof(struct iax_completion_record, error_code),
> +		.load_device_defaults = idxd_load_iaa_device_defaults,
>  	},
>  };
>  
> @@ -745,6 +746,12 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err;
>  	}
>  
> +	if (data->load_device_defaults) {
> +		rc = data->load_device_defaults(idxd);
> +		if (rc)
> +			dev_warn(dev, "IDXD loading device defaults failed\n");
> +	}
> +
>  	rc = idxd_register_devices(idxd);
>  	if (rc) {
>  		dev_err(dev, "IDXD sysfs setup failed\n");
Tom Zanussi Nov. 27, 2023, 10:42 p.m. UTC | #2 
On Mon, 2023-11-27 at 15:14 -0700, Dave Jiang wrote:
> 
> 
> On 11/27/23 13:27, Tom Zanussi wrote:
> > Add a load_device_defaults() function pointer to struct
> > idxd_driver_data, which if defined, will be called when an idxd
> > device
> > is probed and will allow the idxd device to be configured with
> > default
> > values.
> > 
> > The load_device_defaults() function is passed an idxd device to
> > work
> > with to set specific device attributes.
> > 
> > Also add a load_device_defaults() implementation IAA devices;
> > future
> > patches would add default functions for other device types such as
> > DSA.
> > 
> > The way idxd device probing works, if the device configuration is
> > valid at that point e.g. at least one workqueue and engine is
> > properly
> > configured then the device will be enabled and ready to go.
> > 
> > The IAA implementation, idxd_load_iaa_device_defaults(), configures
> > a
> > single workqueue (wq0) for each device with the following default
> > values:
> > 
> >       mode              "dedicated"
> >       threshold         0
> >       size              16
> >       priority          10
> >       type              IDXD_WQT_KERNEL
> >       group             0
> >       name              "iaa_crypto"
> >       driver_name       "crypto"
> > 
> > Note that this now adds another configuration step for any users
> > that
> > want to configure their own devices/workqueus with something
> > different
> > in that they'll first need to disable (in the case of IAA) wq0 and
> > the
> > device itself before they can set their own attributes and re-
> > enable,
> > since they've been already been auto-enabled.  Note also that in
> > order
> > for the new configuration to be applied to the deflate-iaa crypto
> > algorithm the iaa_crypto module needs to unregister the old
> > version,
> > which is accomplished by removing the iaa_crypto module, and
> > re-registering it with the new configuration by reinserting the
> > iaa_crypto module.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Thanks, Dave!

Tom

> 
> > ---
> >  drivers/dma/idxd/Makefile   |  2 +-
> >  drivers/dma/idxd/defaults.c | 53
> > +++++++++++++++++++++++++++++++++++++
> >  drivers/dma/idxd/idxd.h     |  4 +++
> >  drivers/dma/idxd/init.c     |  7 +++++
> >  4 files changed, 65 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/dma/idxd/defaults.c
> > 
> > diff --git a/drivers/dma/idxd/Makefile b/drivers/dma/idxd/Makefile
> > index c5e679070e46..2b4a0d406e1e 100644
> > --- a/drivers/dma/idxd/Makefile
> > +++ b/drivers/dma/idxd/Makefile
> > @@ -4,7 +4,7 @@ obj-$(CONFIG_INTEL_IDXD_BUS) += idxd_bus.o
> >  idxd_bus-y := bus.o
> >  
> >  obj-$(CONFIG_INTEL_IDXD) += idxd.o
> > -idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o
> > debugfs.o
> > +idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o
> > debugfs.o defaults.o
> >  
> >  idxd-$(CONFIG_INTEL_IDXD_PERFMON) += perfmon.o
> >  
> > diff --git a/drivers/dma/idxd/defaults.c
> > b/drivers/dma/idxd/defaults.c
> > new file mode 100644
> > index 000000000000..a0c9faad8efe
> > --- /dev/null
> > +++ b/drivers/dma/idxd/defaults.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright(c) 2023 Intel Corporation. All rights rsvd. */
> > +#include <linux/kernel.h>
> > +#include "idxd.h"
> > +
> > +int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
> > +{
> > +       struct idxd_engine *engine;
> > +       struct idxd_group *group;
> > +       struct idxd_wq *wq;
> > +
> > +       if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
> > +               return 0;
> > +
> > +       wq = idxd->wqs[0];
> > +
> > +       if (wq->state != IDXD_WQ_DISABLED)
> > +               return -EPERM;
> > +
> > +       /* set mode to "dedicated" */
> > +       set_bit(WQ_FLAG_DEDICATED, &wq->flags);
> > +       wq->threshold = 0;
> > +
> > +       /* set size to 16 */
> > +       wq->size = 16;
> > +
> > +       /* set priority to 10 */
> > +       wq->priority = 10;
> > +
> > +       /* set type to "kernel" */
> > +       wq->type = IDXD_WQT_KERNEL;
> > +
> > +       /* set wq group to 0 */
> > +       group = idxd->groups[0];
> > +       wq->group = group;
> > +       group->num_wqs++;
> > +
> > +       /* set name to "iaa_crypto" */
> > +       memset(wq->name, 0, WQ_NAME_SIZE + 1);
> > +       strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
> > +
> > +       /* set driver_name to "crypto" */
> > +       memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> > +       strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);
> > +
> > +       engine = idxd->engines[0];
> > +
> > +       /* set engine group to 0 */
> > +       engine->group = idxd->groups[0];
> > +       engine->group->num_engines++;
> > +
> > +       return 0;
> > +}
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index 62ea21b25906..47de3f93ff1e 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -277,6 +277,8 @@ struct idxd_dma_dev {
> >         struct dma_device dma;
> >  };
> >  
> > +typedef int (*load_device_defaults_fn_t) (struct idxd_device
> > *idxd);
> > +
> >  struct idxd_driver_data {
> >         const char *name_prefix;
> >         enum idxd_type type;
> > @@ -286,6 +288,7 @@ struct idxd_driver_data {
> >         int evl_cr_off;
> >         int cr_status_off;
> >         int cr_result_off;
> > +       load_device_defaults_fn_t load_device_defaults;
> >  };
> >  
> >  struct idxd_evl {
> > @@ -730,6 +733,7 @@ void idxd_unregister_devices(struct idxd_device
> > *idxd);
> >  void idxd_wqs_quiesce(struct idxd_device *idxd);
> >  bool idxd_queue_int_handle_resubmit(struct idxd_desc *desc);
> >  void multi_u64_to_bmap(unsigned long *bmap, u64 *val, int count);
> > +int idxd_load_iaa_device_defaults(struct idxd_device *idxd);
> >  
> >  /* device interrupt control */
> >  irqreturn_t idxd_misc_thread(int vec, void *data);
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index 0eb1c827a215..14df1f1347a8 100644
> > --- a/drivers/dma/idxd/init.c
> > +++ b/drivers/dma/idxd/init.c
> > @@ -59,6 +59,7 @@ static struct idxd_driver_data idxd_driver_data[]
> > = {
> >                 .evl_cr_off = offsetof(struct iax_evl_entry, cr),
> >                 .cr_status_off = offsetof(struct
> > iax_completion_record, status),
> >                 .cr_result_off = offsetof(struct
> > iax_completion_record, error_code),
> > +               .load_device_defaults =
> > idxd_load_iaa_device_defaults,
> >         },
> >  };
> >  
> > @@ -745,6 +746,12 @@ static int idxd_pci_probe(struct pci_dev
> > *pdev, const struct pci_device_id *id)
> >                 goto err;
> >         }
> >  
> > +       if (data->load_device_defaults) {
> > +               rc = data->load_device_defaults(idxd);
> > +               if (rc)
> > +                       dev_warn(dev, "IDXD loading device defaults
> > failed\n");
> > +       }
> > +
> >         rc = idxd_register_devices(idxd);
> >         if (rc) {
> >                 dev_err(dev, "IDXD sysfs setup failed\n");
Fenghua Yu Nov. 28, 2023, 8:31 p.m. UTC | #3 
Hi, Tom,

On 11/27/23 12:27, Tom Zanussi wrote:
> Add a load_device_defaults() function pointer to struct
> idxd_driver_data, which if defined, will be called when an idxd device
> is probed and will allow the idxd device to be configured with default
> values.
> 
> The load_device_defaults() function is passed an idxd device to work
> with to set specific device attributes.
> 
> Also add a load_device_defaults() implementation IAA devices; future
> patches would add default functions for other device types such as
> DSA.
> 
> The way idxd device probing works, if the device configuration is
> valid at that point e.g. at least one workqueue and engine is properly
> configured then the device will be enabled and ready to go.
> 
> The IAA implementation, idxd_load_iaa_device_defaults(), configures a
> single workqueue (wq0) for each device with the following default
> values:
> 
>        mode     	        "dedicated"
>        threshold		0
>        size		16

Why is it 16?

If wqcap supports less than 16, configuring wq size 16 will fail.
If wqcap supports more than 16, wq size 16 uses less wq size than 
capable and less performance.

Is it better to set wq size as total wq size reported in wqcap?
>        priority		10
>        type		IDXD_WQT_KERNEL
>        group		0
>        name              "iaa_crypto"
>        driver_name       "crypto"
> 
> Note that this now adds another configuration step for any users that
> want to configure their own devices/workqueus with something different
> in that they'll first need to disable (in the case of IAA) wq0 and the
> device itself before they can set their own attributes and re-enable,
> since they've been already been auto-enabled.  Note also that in order
> for the new configuration to be applied to the deflate-iaa crypto
> algorithm the iaa_crypto module needs to unregister the old version,
> which is accomplished by removing the iaa_crypto module, and
> re-registering it with the new configuration by reinserting the
> iaa_crypto module.
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---
>   drivers/dma/idxd/Makefile   |  2 +-
>   drivers/dma/idxd/defaults.c | 53 +++++++++++++++++++++++++++++++++++++
>   drivers/dma/idxd/idxd.h     |  4 +++
>   drivers/dma/idxd/init.c     |  7 +++++
>   4 files changed, 65 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/dma/idxd/defaults.c
> 
> diff --git a/drivers/dma/idxd/Makefile b/drivers/dma/idxd/Makefile
> index c5e679070e46..2b4a0d406e1e 100644
> --- a/drivers/dma/idxd/Makefile
> +++ b/drivers/dma/idxd/Makefile
> @@ -4,7 +4,7 @@ obj-$(CONFIG_INTEL_IDXD_BUS) += idxd_bus.o
>   idxd_bus-y := bus.o
>   
>   obj-$(CONFIG_INTEL_IDXD) += idxd.o
> -idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o
> +idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o defaults.o
>   
>   idxd-$(CONFIG_INTEL_IDXD_PERFMON) += perfmon.o
>   
> diff --git a/drivers/dma/idxd/defaults.c b/drivers/dma/idxd/defaults.c
> new file mode 100644
> index 000000000000..a0c9faad8efe
> --- /dev/null
> +++ b/drivers/dma/idxd/defaults.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Intel Corporation. All rights rsvd. */
> +#include <linux/kernel.h>
> +#include "idxd.h"
> +
> +int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
> +{
> +	struct idxd_engine *engine;
> +	struct idxd_group *group;
> +	struct idxd_wq *wq;
> +
> +	if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
> +		return 0;
> +
> +	wq = idxd->wqs[0];
> +
> +	if (wq->state != IDXD_WQ_DISABLED)
> +		return -EPERM;
> +
> +	/* set mode to "dedicated" */
> +	set_bit(WQ_FLAG_DEDICATED, &wq->flags);
> +	wq->threshold = 0;
> +
> +	/* set size to 16 */
> +	wq->size = 16;
> +
> +	/* set priority to 10 */
> +	wq->priority = 10;
> +
> +	/* set type to "kernel" */
> +	wq->type = IDXD_WQT_KERNEL;
> +
> +	/* set wq group to 0 */
> +	group = idxd->groups[0];
> +	wq->group = group;
> +	group->num_wqs++;
> +
> +	/* set name to "iaa_crypto" */
> +	memset(wq->name, 0, WQ_NAME_SIZE + 1);
> +	strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);

Is strcpy(wq->name, "iaa_crypto") simpler than memset() and strscpy()?

> +
> +	/* set driver_name to "crypto" */
> +	memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> +	strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);

Is strcpy(wq->driver_name, "crypto") simpler?

> +
> +	engine = idxd->engines[0];
> +
> +	/* set engine group to 0 */
> +	engine->group = idxd->groups[0];
> +	engine->group->num_engines++;
> +
> +	return 0;
> +}
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index 62ea21b25906..47de3f93ff1e 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -277,6 +277,8 @@ struct idxd_dma_dev {
>   	struct dma_device dma;
>   };
>   
> +typedef int (*load_device_defaults_fn_t) (struct idxd_device *idxd);
> +
>   struct idxd_driver_data {
>   	const char *name_prefix;
>   	enum idxd_type type;
> @@ -286,6 +288,7 @@ struct idxd_driver_data {
>   	int evl_cr_off;
>   	int cr_status_off;
>   	int cr_result_off;
> +	load_device_defaults_fn_t load_device_defaults;
>   };
>   
>   struct idxd_evl {
> @@ -730,6 +733,7 @@ void idxd_unregister_devices(struct idxd_device *idxd);
>   void idxd_wqs_quiesce(struct idxd_device *idxd);
>   bool idxd_queue_int_handle_resubmit(struct idxd_desc *desc);
>   void multi_u64_to_bmap(unsigned long *bmap, u64 *val, int count);
> +int idxd_load_iaa_device_defaults(struct idxd_device *idxd);
>   
>   /* device interrupt control */
>   irqreturn_t idxd_misc_thread(int vec, void *data);
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 0eb1c827a215..14df1f1347a8 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -59,6 +59,7 @@ static struct idxd_driver_data idxd_driver_data[] = {
>   		.evl_cr_off = offsetof(struct iax_evl_entry, cr),
>   		.cr_status_off = offsetof(struct iax_completion_record, status),
>   		.cr_result_off = offsetof(struct iax_completion_record, error_code),
> +		.load_device_defaults = idxd_load_iaa_device_defaults,
>   	},
>   };
>   
> @@ -745,6 +746,12 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		goto err;
>   	}
>   
> +	if (data->load_device_defaults) {
> +		rc = data->load_device_defaults(idxd);
> +		if (rc)
> +			dev_warn(dev, "IDXD loading device defaults failed\n");
> +	}
> +
>   	rc = idxd_register_devices(idxd);
>   	if (rc) {
>   		dev_err(dev, "IDXD sysfs setup failed\n");

Thanks.

-Fenghua
Tom Zanussi Nov. 28, 2023, 9:09 p.m. UTC | #4 
Hi Fenghua,

On Tue, 2023-11-28 at 12:31 -0800, Fenghua Yu wrote:
> Hi, Tom,
> 
> On 11/27/23 12:27, Tom Zanussi wrote:
> > Add a load_device_defaults() function pointer to struct
> > idxd_driver_data, which if defined, will be called when an idxd
> > device
> > is probed and will allow the idxd device to be configured with
> > default
> > values.
> > 
> > The load_device_defaults() function is passed an idxd device to
> > work
> > with to set specific device attributes.
> > 
> > Also add a load_device_defaults() implementation IAA devices;
> > future
> > patches would add default functions for other device types such as
> > DSA.
> > 
> > The way idxd device probing works, if the device configuration is
> > valid at that point e.g. at least one workqueue and engine is
> > properly
> > configured then the device will be enabled and ready to go.
> > 
> > The IAA implementation, idxd_load_iaa_device_defaults(), configures
> > a
> > single workqueue (wq0) for each device with the following default
> > values:
> > 
> >        mode                     "dedicated"
> >        threshold                0
> >        size             16
> 
> Why is it 16?
> 
> If wqcap supports less than 16, configuring wq size 16 will fail.
> If wqcap supports more than 16, wq size 16 uses less wq size than 
> capable and less performance.
> 
> Is it better to set wq size as total wq size reported in wqcap?

Yes, good point.  I'll calculate this based on wqcap.

> >        priority         10
> >        type             IDXD_WQT_KERNEL
> >        group            0
> >        name              "iaa_crypto"
> >        driver_name       "crypto"
> > 
> > Note that this now adds another configuration step for any users
> > that
> > want to configure their own devices/workqueus with something
> > different
> > in that they'll first need to disable (in the case of IAA) wq0 and
> > the
> > device itself before they can set their own attributes and re-
> > enable,
> > since they've been already been auto-enabled.  Note also that in
> > order
> > for the new configuration to be applied to the deflate-iaa crypto
> > algorithm the iaa_crypto module needs to unregister the old
> > version,
> > which is accomplished by removing the iaa_crypto module, and
> > re-registering it with the new configuration by reinserting the
> > iaa_crypto module.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > ---
> >   drivers/dma/idxd/Makefile   |  2 +-
> >   drivers/dma/idxd/defaults.c | 53
> > +++++++++++++++++++++++++++++++++++++
> >   drivers/dma/idxd/idxd.h     |  4 +++
> >   drivers/dma/idxd/init.c     |  7 +++++
> >   4 files changed, 65 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/dma/idxd/defaults.c
> > 
> > diff --git a/drivers/dma/idxd/Makefile b/drivers/dma/idxd/Makefile
> > index c5e679070e46..2b4a0d406e1e 100644
> > --- a/drivers/dma/idxd/Makefile
> > +++ b/drivers/dma/idxd/Makefile
> > @@ -4,7 +4,7 @@ obj-$(CONFIG_INTEL_IDXD_BUS) += idxd_bus.o
> >   idxd_bus-y := bus.o
> >   
> >   obj-$(CONFIG_INTEL_IDXD) += idxd.o
> > -idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o
> > debugfs.o
> > +idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o
> > debugfs.o defaults.o
> >   
> >   idxd-$(CONFIG_INTEL_IDXD_PERFMON) += perfmon.o
> >   
> > diff --git a/drivers/dma/idxd/defaults.c
> > b/drivers/dma/idxd/defaults.c
> > new file mode 100644
> > index 000000000000..a0c9faad8efe
> > --- /dev/null
> > +++ b/drivers/dma/idxd/defaults.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright(c) 2023 Intel Corporation. All rights rsvd. */
> > +#include <linux/kernel.h>
> > +#include "idxd.h"
> > +
> > +int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
> > +{
> > +       struct idxd_engine *engine;
> > +       struct idxd_group *group;
> > +       struct idxd_wq *wq;
> > +
> > +       if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
> > +               return 0;
> > +
> > +       wq = idxd->wqs[0];
> > +
> > +       if (wq->state != IDXD_WQ_DISABLED)
> > +               return -EPERM;
> > +
> > +       /* set mode to "dedicated" */
> > +       set_bit(WQ_FLAG_DEDICATED, &wq->flags);
> > +       wq->threshold = 0;
> > +
> > +       /* set size to 16 */
> > +       wq->size = 16;
> > +
> > +       /* set priority to 10 */
> > +       wq->priority = 10;
> > +
> > +       /* set type to "kernel" */
> > +       wq->type = IDXD_WQT_KERNEL;
> > +
> > +       /* set wq group to 0 */
> > +       group = idxd->groups[0];
> > +       wq->group = group;
> > +       group->num_wqs++;
> > +
> > +       /* set name to "iaa_crypto" */
> > +       memset(wq->name, 0, WQ_NAME_SIZE + 1);
> > +       strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
> 
> Is strcpy(wq->name, "iaa_crypto") simpler than memset() and
> strscpy()?

That's what I originally had, but checkpatch complained about it,
suggesting strscpy, so I changed it to make checkpatch happy.

> 
> > +
> > +       /* set driver_name to "crypto" */
> > +       memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> > +       strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);
> 
> Is strcpy(wq->driver_name, "crypto") simpler?

Same here.

Thanks,

Tom

> 
> > +
> > +       engine = idxd->engines[0];
> > +
> > +       /* set engine group to 0 */
> > +       engine->group = idxd->groups[0];
> > +       engine->group->num_engines++;
> > +
> > +       return 0;
> > +}
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index 62ea21b25906..47de3f93ff1e 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -277,6 +277,8 @@ struct idxd_dma_dev {
> >         struct dma_device dma;
> >   };
> >   
> > +typedef int (*load_device_defaults_fn_t) (struct idxd_device
> > *idxd);
> > +
> >   struct idxd_driver_data {
> >         const char *name_prefix;
> >         enum idxd_type type;
> > @@ -286,6 +288,7 @@ struct idxd_driver_data {
> >         int evl_cr_off;
> >         int cr_status_off;
> >         int cr_result_off;
> > +       load_device_defaults_fn_t load_device_defaults;
> >   };
> >   
> >   struct idxd_evl {
> > @@ -730,6 +733,7 @@ void idxd_unregister_devices(struct idxd_device
> > *idxd);
> >   void idxd_wqs_quiesce(struct idxd_device *idxd);
> >   bool idxd_queue_int_handle_resubmit(struct idxd_desc *desc);
> >   void multi_u64_to_bmap(unsigned long *bmap, u64 *val, int count);
> > +int idxd_load_iaa_device_defaults(struct idxd_device *idxd);
> >   
> >   /* device interrupt control */
> >   irqreturn_t idxd_misc_thread(int vec, void *data);
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index 0eb1c827a215..14df1f1347a8 100644
> > --- a/drivers/dma/idxd/init.c
> > +++ b/drivers/dma/idxd/init.c
> > @@ -59,6 +59,7 @@ static struct idxd_driver_data idxd_driver_data[]
> > = {
> >                 .evl_cr_off = offsetof(struct iax_evl_entry, cr),
> >                 .cr_status_off = offsetof(struct
> > iax_completion_record, status),
> >                 .cr_result_off = offsetof(struct
> > iax_completion_record, error_code),
> > +               .load_device_defaults =
> > idxd_load_iaa_device_defaults,
> >         },
> >   };
> >   
> > @@ -745,6 +746,12 @@ static int idxd_pci_probe(struct pci_dev
> > *pdev, const struct pci_device_id *id)
> >                 goto err;
> >         }
> >   
> > +       if (data->load_device_defaults) {
> > +               rc = data->load_device_defaults(idxd);
> > +               if (rc)
> > +                       dev_warn(dev, "IDXD loading device defaults
> > failed\n");
> > +       }
> > +
> >         rc = idxd_register_devices(idxd);
> >         if (rc) {
> >                 dev_err(dev, "IDXD sysfs setup failed\n");
> 
> Thanks.
> 
> -Fenghua
Fenghua Yu Nov. 30, 2023, 12:31 a.m. UTC | #5 
Hi, Tom,

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > +       /* set name to "iaa_crypto" */
> > > +       memset(wq->name, 0, WQ_NAME_SIZE + 1);
> > > +       strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
> >
> > Is strcpy(wq->name, "iaa_crypto") simpler than memset() and strscpy()?
> 
> That's what I originally had, but checkpatch complained about it, suggesting
> strscpy, so I changed it to make checkpatch happy.

Why is size WQ_NAME_SIZE+1 instead of WQ_NAME_SIZE? Will WQ_NAME_SIZE+1 cause mem corruption because wq->name is defined as a string with WQ_NAME_SIZE?
> 
> >
> > > +
> > > +       /* set driver_name to "crypto" */
> > > +       memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> > > +       strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);
> >
> > Is strcpy(wq->driver_name, "crypto") simpler?
> 
> Same here.

Ditto.

Thanks.

-Fenghua
Tom Zanussi Nov. 30, 2023, 3:18 p.m. UTC | #6 
Hi Fenghua,

On Thu, 2023-11-30 at 00:31 +0000, Yu, Fenghua wrote:
> Hi, Tom,
> 
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > > +       /* set name to "iaa_crypto" */
> > > > +       memset(wq->name, 0, WQ_NAME_SIZE + 1);
> > > > +       strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
> > > 
> > > Is strcpy(wq->name, "iaa_crypto") simpler than memset() and
> > > strscpy()?
> > 
> > That's what I originally had, but checkpatch complained about it,
> > suggesting
> > strscpy, so I changed it to make checkpatch happy.
> 
> Why is size WQ_NAME_SIZE+1 instead of WQ_NAME_SIZE? Will
> WQ_NAME_SIZE+1 cause mem corruption because wq->name is defined as a
> string with WQ_NAME_SIZE?

No, wq->name actually is:

        char name[WQ_NAME_SIZE + 1];

This code is doing the same thing as elsewhere in the idxd driver
except instead of sprintf() it uses strscpy().

> > 
> > > 
> > > > +
> > > > +       /* set driver_name to "crypto" */
> > > > +       memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
> > > > +       strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE +
> > > > 1);
> > > 
> > > Is strcpy(wq->driver_name, "crypto") simpler?
> > 
> > Same here.
> 
> Ditto.
> 

Same.

Tom

> Thanks.
> 
> -Fenghua
diff mbox series

Patch

diff --git a/drivers/dma/idxd/Makefile b/drivers/dma/idxd/Makefile
index c5e679070e46..2b4a0d406e1e 100644
--- a/drivers/dma/idxd/Makefile
+++ b/drivers/dma/idxd/Makefile
@@ -4,7 +4,7 @@  obj-$(CONFIG_INTEL_IDXD_BUS) += idxd_bus.o
 idxd_bus-y := bus.o
 
 obj-$(CONFIG_INTEL_IDXD) += idxd.o
-idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o
+idxd-y := init.o irq.o device.o sysfs.o submit.o dma.o cdev.o debugfs.o defaults.o
 
 idxd-$(CONFIG_INTEL_IDXD_PERFMON) += perfmon.o
 
diff --git a/drivers/dma/idxd/defaults.c b/drivers/dma/idxd/defaults.c
new file mode 100644
index 000000000000..a0c9faad8efe
--- /dev/null
+++ b/drivers/dma/idxd/defaults.c
@@ -0,0 +1,53 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2023 Intel Corporation. All rights rsvd. */
+#include <linux/kernel.h>
+#include "idxd.h"
+
+int idxd_load_iaa_device_defaults(struct idxd_device *idxd)
+{
+	struct idxd_engine *engine;
+	struct idxd_group *group;
+	struct idxd_wq *wq;
+
+	if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
+		return 0;
+
+	wq = idxd->wqs[0];
+
+	if (wq->state != IDXD_WQ_DISABLED)
+		return -EPERM;
+
+	/* set mode to "dedicated" */
+	set_bit(WQ_FLAG_DEDICATED, &wq->flags);
+	wq->threshold = 0;
+
+	/* set size to 16 */
+	wq->size = 16;
+
+	/* set priority to 10 */
+	wq->priority = 10;
+
+	/* set type to "kernel" */
+	wq->type = IDXD_WQT_KERNEL;
+
+	/* set wq group to 0 */
+	group = idxd->groups[0];
+	wq->group = group;
+	group->num_wqs++;
+
+	/* set name to "iaa_crypto" */
+	memset(wq->name, 0, WQ_NAME_SIZE + 1);
+	strscpy(wq->name, "iaa_crypto", WQ_NAME_SIZE + 1);
+
+	/* set driver_name to "crypto" */
+	memset(wq->driver_name, 0, DRIVER_NAME_SIZE + 1);
+	strscpy(wq->driver_name, "crypto", DRIVER_NAME_SIZE + 1);
+
+	engine = idxd->engines[0];
+
+	/* set engine group to 0 */
+	engine->group = idxd->groups[0];
+	engine->group->num_engines++;
+
+	return 0;
+}
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 62ea21b25906..47de3f93ff1e 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -277,6 +277,8 @@  struct idxd_dma_dev {
 	struct dma_device dma;
 };
 
+typedef int (*load_device_defaults_fn_t) (struct idxd_device *idxd);
+
 struct idxd_driver_data {
 	const char *name_prefix;
 	enum idxd_type type;
@@ -286,6 +288,7 @@  struct idxd_driver_data {
 	int evl_cr_off;
 	int cr_status_off;
 	int cr_result_off;
+	load_device_defaults_fn_t load_device_defaults;
 };
 
 struct idxd_evl {
@@ -730,6 +733,7 @@  void idxd_unregister_devices(struct idxd_device *idxd);
 void idxd_wqs_quiesce(struct idxd_device *idxd);
 bool idxd_queue_int_handle_resubmit(struct idxd_desc *desc);
 void multi_u64_to_bmap(unsigned long *bmap, u64 *val, int count);
+int idxd_load_iaa_device_defaults(struct idxd_device *idxd);
 
 /* device interrupt control */
 irqreturn_t idxd_misc_thread(int vec, void *data);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 0eb1c827a215..14df1f1347a8 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -59,6 +59,7 @@  static struct idxd_driver_data idxd_driver_data[] = {
 		.evl_cr_off = offsetof(struct iax_evl_entry, cr),
 		.cr_status_off = offsetof(struct iax_completion_record, status),
 		.cr_result_off = offsetof(struct iax_completion_record, error_code),
+		.load_device_defaults = idxd_load_iaa_device_defaults,
 	},
 };
 
@@ -745,6 +746,12 @@  static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err;
 	}
 
+	if (data->load_device_defaults) {
+		rc = data->load_device_defaults(idxd);
+		if (rc)
+			dev_warn(dev, "IDXD loading device defaults failed\n");
+	}
+
 	rc = idxd_register_devices(idxd);
 	if (rc) {
 		dev_err(dev, "IDXD sysfs setup failed\n");