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@xxxxxxxxxxxxxxx>
> > ---
> > 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
