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

[Tom Zanussi <tom.zanussi@linux.intel.com>]

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