mbox series

[0/4] Add debugfs pm_status for qat driver

Message ID 20230817143352.132583-1-lucas.segarra.fernandez@intel.com
Headers show
Series Add debugfs pm_status for qat driver | expand

Message

Lucas Segarra Fernandez Aug. 17, 2023, 2:33 p.m. UTC 
Add debugfs pm_status.

Expose power management info by providing the "pm_status" file under
debugfs.

Patch #1 has been added to this pathcset in order to add
ARRAY_SIZE_OF_FIELD() (Patch #2) without expanding kernel.h

---

Alejandro Colomar (1):
  linux/array_size.h: Move ARRAY_SIZE(arr) to a separate header

Lucas Segarra Fernandez (3):
  linux/array_size.h: Add ARRAY_SIZE_OF_FIELD()
  crypto: qat - refactor included headers
  crypto: qat - add pm_status debugfs file

 Documentation/ABI/testing/debugfs-driver-qat  |   9 +
 drivers/crypto/intel/qat/qat_common/Makefile  |   1 +
 .../intel/qat/qat_common/adf_accel_devices.h  |  13 +
 .../crypto/intel/qat/qat_common/adf_admin.c   |  25 ++
 .../intel/qat/qat_common/adf_common_drv.h     |   1 +
 .../crypto/intel/qat/qat_common/adf_dbgfs.c   |   3 +
 .../crypto/intel/qat/qat_common/adf_gen4_pm.c | 265 ++++++++++++++++++
 .../crypto/intel/qat/qat_common/adf_gen4_pm.h |  38 ++-
 .../intel/qat/qat_common/adf_pm_dbgfs.c       |  47 ++++
 .../intel/qat/qat_common/adf_pm_dbgfs.h       |  12 +
 .../qat/qat_common/icp_qat_fw_init_admin.h    |  33 +++
 include/linux/array_size.h                    |  21 ++
 include/linux/clk-provider.h                  |   1 +
 include/linux/counter.h                       |   1 +
 include/linux/genl_magic_func.h               |   1 +
 include/linux/hashtable.h                     |   1 +
 include/linux/kernel.h                        |   7 +-
 include/linux/kfifo.h                         |   1 +
 include/linux/kvm_host.h                      |   1 +
 include/linux/moduleparam.h                   |   2 +
 include/linux/mtd/rawnand.h                   |   1 +
 include/linux/netfilter.h                     |   1 +
 include/linux/pagemap.h                       |   1 +
 include/linux/phy.h                           |   1 +
 include/linux/pinctrl/machine.h               |   2 +-
 include/linux/property.h                      |   1 +
 include/linux/rcupdate_wait.h                 |   1 +
 include/linux/regmap.h                        |   1 +
 include/linux/skmsg.h                         |   1 +
 include/linux/string.h                        |   1 +
 include/linux/surface_aggregator/controller.h |   1 +
 31 files changed, 487 insertions(+), 8 deletions(-)
 create mode 100644 drivers/crypto/intel/qat/qat_common/adf_pm_dbgfs.c
 create mode 100644 drivers/crypto/intel/qat/qat_common/adf_pm_dbgfs.h
 create mode 100644 include/linux/array_size.h

Comments

Alejandro Colomar Aug. 17, 2023, 8:24 p.m. UTC | #1 
Hi Lucas,

On 2023-08-17 16:33, Lucas Segarra Fernandez wrote:
> From: Alejandro Colomar <alx.manpages@gmail.com>
> 
> Touching files so used for the kernel,
> forces 'make' to recompile most of the kernel.
> 
> Having those definitions in more granular files
> helps avoid recompiling so much of the kernel.
> 
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Heh!  This is really really old, isn't it?  :p

Would you mind updating my email to use the kernel.org address?

From: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

Thanks for taking care of this patch!

Cheers,
Alex

> ---
>  include/linux/array_size.h                    | 13 +++++++++++++
>  include/linux/clk-provider.h                  |  1 +
>  include/linux/counter.h                       |  1 +
>  include/linux/genl_magic_func.h               |  1 +
>  include/linux/hashtable.h                     |  1 +
>  include/linux/kernel.h                        |  7 +------
>  include/linux/kfifo.h                         |  1 +
>  include/linux/kvm_host.h                      |  1 +
>  include/linux/moduleparam.h                   |  2 ++
>  include/linux/mtd/rawnand.h                   |  1 +
>  include/linux/netfilter.h                     |  1 +
>  include/linux/pagemap.h                       |  1 +
>  include/linux/phy.h                           |  1 +
>  include/linux/pinctrl/machine.h               |  2 +-
>  include/linux/property.h                      |  1 +
>  include/linux/rcupdate_wait.h                 |  1 +
>  include/linux/regmap.h                        |  1 +
>  include/linux/skmsg.h                         |  1 +
>  include/linux/string.h                        |  1 +
>  include/linux/surface_aggregator/controller.h |  1 +
>  20 files changed, 33 insertions(+), 7 deletions(-)
>  create mode 100644 include/linux/array_size.h
> 
> diff --git a/include/linux/array_size.h b/include/linux/array_size.h
> new file mode 100644
> index 000000000000..06d7d83196ca
> --- /dev/null
> +++ b/include/linux/array_size.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_ARRAY_SIZE_H
> +#define _LINUX_ARRAY_SIZE_H
> +
> +#include <linux/compiler.h>
> +
> +/**
> + * ARRAY_SIZE - get the number of elements in array @arr
> + * @arr: array to be sized
> + */
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> +
> +#endif  /* _LINUX_ARRAY_SIZE_H */
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 0f0cd01906b4..4f4d4f4af0a6 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -6,6 +6,7 @@
>  #ifndef __LINUX_CLK_PROVIDER_H
>  #define __LINUX_CLK_PROVIDER_H
>  
> +#include <linux/array_size.h>
>  #include <linux/of.h>
>  #include <linux/of_clk.h>
>  
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index b63746637de2..baf4ffcd8d18 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -6,6 +6,7 @@
>  #ifndef _COUNTER_H_
>  #define _COUNTER_H_
>  
> +#include <linux/array_size.h>
>  #include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> diff --git a/include/linux/genl_magic_func.h b/include/linux/genl_magic_func.h
> index 2984b0cb24b1..cec9cae51f0b 100644
> --- a/include/linux/genl_magic_func.h
> +++ b/include/linux/genl_magic_func.h
> @@ -2,6 +2,7 @@
>  #ifndef GENL_MAGIC_FUNC_H
>  #define GENL_MAGIC_FUNC_H
>  
> +#include <linux/array_size.h>
>  #include <linux/build_bug.h>
>  #include <linux/genl_magic_struct.h>
>  
> diff --git a/include/linux/hashtable.h b/include/linux/hashtable.h
> index f6c666730b8c..09c5f1522b06 100644
> --- a/include/linux/hashtable.h
> +++ b/include/linux/hashtable.h
> @@ -7,6 +7,7 @@
>  #ifndef _LINUX_HASHTABLE_H
>  #define _LINUX_HASHTABLE_H
>  
> +#include <linux/array_size.h>
>  #include <linux/list.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 0d91e0af0125..7195c6f27a22 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -11,6 +11,7 @@
>  #ifndef _LINUX_KERNEL_H
>  #define _LINUX_KERNEL_H
>  
> +#include <linux/array_size.h>
>  #include <linux/stdarg.h>
>  #include <linux/align.h>
>  #include <linux/limits.h>
> @@ -49,12 +50,6 @@
>  #define READ			0
>  #define WRITE			1
>  
> -/**
> - * ARRAY_SIZE - get the number of elements in array @arr
> - * @arr: array to be sized
> - */
> -#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> -
>  #define PTR_IF(cond, ptr)	((cond) ? (ptr) : NULL)
>  
>  #define u64_to_user_ptr(x) (		\
> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> index 0b35a41440ff..b6fdd92ccb56 100644
> --- a/include/linux/kfifo.h
> +++ b/include/linux/kfifo.h
> @@ -36,6 +36,7 @@
>   * to lock the reader.
>   */
>  
> +#include <linux/array_size.h>
>  #include <linux/kernel.h>
>  #include <linux/spinlock.h>
>  #include <linux/stddef.h>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9d3ac7720da9..a629b398a592 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -3,6 +3,7 @@
>  #define __KVM_HOST_H
>  
>  
> +#include <linux/array_size.h>
>  #include <linux/types.h>
>  #include <linux/hardirq.h>
>  #include <linux/list.h>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 962cd41a2cb5..3cecef5fa1cf 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -2,6 +2,8 @@
>  #ifndef _LINUX_MODULE_PARAMS_H
>  #define _LINUX_MODULE_PARAMS_H
>  /* (C) Copyright 2001, 2002 Rusty Russell IBM Corporation */
> +
> +#include <linux/array_size.h>
>  #include <linux/init.h>
>  #include <linux/stringify.h>
>  #include <linux/kernel.h>
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 5159d692f9ce..cd27ef633a4f 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -13,6 +13,7 @@
>  #ifndef __LINUX_MTD_RAWNAND_H
>  #define __LINUX_MTD_RAWNAND_H
>  
> +#include <linux/array_size.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/mtd/flashchip.h>
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index d4fed4c508ca..f9ca506c4261 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -2,6 +2,7 @@
>  #ifndef __LINUX_NETFILTER_H
>  #define __LINUX_NETFILTER_H
>  
> +#include <linux/array_size.h>
>  #include <linux/init.h>
>  #include <linux/skbuff.h>
>  #include <linux/net.h>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 716953ee1ebd..7a3de980ed9d 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -5,6 +5,7 @@
>  /*
>   * Copyright 1995 Linus Torvalds
>   */
> +#include <linux/array_size.h>
>  #include <linux/mm.h>
>  #include <linux/fs.h>
>  #include <linux/list.h>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 11c1e91563d4..39e88b570ead 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -11,6 +11,7 @@
>  #ifndef __PHY_H
>  #define __PHY_H
>  
> +#include <linux/array_size.h>
>  #include <linux/compiler.h>
>  #include <linux/spinlock.h>
>  #include <linux/ethtool.h>
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index 0639b36f43c5..ee8803f6ad07 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -11,7 +11,7 @@
>  #ifndef __LINUX_PINCTRL_MACHINE_H
>  #define __LINUX_PINCTRL_MACHINE_H
>  
> -#include <linux/kernel.h>	/* ARRAY_SIZE() */
> +#include <linux/array_size.h>
>  
>  #include <linux/pinctrl/pinctrl-state.h>
>  
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 8c3c6685a2ae..f7889c7c3a66 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -10,6 +10,7 @@
>  #ifndef _LINUX_PROPERTY_H_
>  #define _LINUX_PROPERTY_H_
>  
> +#include <linux/array_size.h>
>  #include <linux/bits.h>
>  #include <linux/fwnode.h>
>  #include <linux/stddef.h>
> diff --git a/include/linux/rcupdate_wait.h b/include/linux/rcupdate_wait.h
> index 699b938358bf..a321404eeec0 100644
> --- a/include/linux/rcupdate_wait.h
> +++ b/include/linux/rcupdate_wait.h
> @@ -6,6 +6,7 @@
>   * RCU synchronization types and methods:
>   */
>  
> +#include <linux/array_size.h>
>  #include <linux/rcupdate.h>
>  #include <linux/completion.h>
>  
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 8fc0b3ebce44..af0430dc0945 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -10,6 +10,7 @@
>   * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
>   */
>  
> +#include <linux/array_size.h>
>  #include <linux/list.h>
>  #include <linux/rbtree.h>
>  #include <linux/ktime.h>
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 054d7911bfc9..d7e3b9f46d58 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -4,6 +4,7 @@
>  #ifndef _LINUX_SKMSG_H
>  #define _LINUX_SKMSG_H
>  
> +#include <linux/array_size.h>
>  #include <linux/bpf.h>
>  #include <linux/filter.h>
>  #include <linux/scatterlist.h>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index dbfc66400050..3c920b6d609b 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -2,6 +2,7 @@
>  #ifndef _LINUX_STRING_H_
>  #define _LINUX_STRING_H_
>  
> +#include <linux/array_size.h>
>  #include <linux/compiler.h>	/* for inline */
>  #include <linux/types.h>	/* for size_t */
>  #include <linux/stddef.h>	/* for NULL */
> diff --git a/include/linux/surface_aggregator/controller.h b/include/linux/surface_aggregator/controller.h
> index cb7980805920..dcce0b663a3a 100644
> --- a/include/linux/surface_aggregator/controller.h
> +++ b/include/linux/surface_aggregator/controller.h
> @@ -12,6 +12,7 @@
>  #ifndef _LINUX_SURFACE_AGGREGATOR_CONTROLLER_H
>  #define _LINUX_SURFACE_AGGREGATOR_CONTROLLER_H
>  
> +#include <linux/array_size.h>
>  #include <linux/completion.h>
>  #include <linux/device.h>
>  #include <linux/types.h>
Herbert Xu Aug. 18, 2023, 5:28 a.m. UTC | #2 
On Thu, Aug 17, 2023 at 04:33:17PM +0200, Lucas Segarra Fernandez wrote:
>
> +static struct pm_status_row pm_event_rows[] = {
> +	PM_INFO_REGSET_ENTRY32(event_log[0], EVENT0),
> +	PM_INFO_REGSET_ENTRY32(event_log[1], EVENT1),
> +	PM_INFO_REGSET_ENTRY32(event_log[2], EVENT2),
> +	PM_INFO_REGSET_ENTRY32(event_log[3], EVENT3),
> +	PM_INFO_REGSET_ENTRY32(event_log[4], EVENT4),
> +	PM_INFO_REGSET_ENTRY32(event_log[5], EVENT5),
> +	PM_INFO_REGSET_ENTRY32(event_log[6], EVENT6),
> +	PM_INFO_REGSET_ENTRY32(event_log[7], EVENT7),
> +};
> +
> +static_assert(ARRAY_SIZE_OF_FIELD(struct icp_qat_fw_init_admin_pm_info, event_log) ==
> +	      ARRAY_SIZE(pm_event_rows));

Was all of that churn just for this one line?

How about simply declaring a macro

	#define QAT_NUMBER_OF_PM_EVENTS 8

and then use it for the two arrays:

	static struct pm_status_row pm_event_rows[QAT_NUMBER_OF_PM_EVENTS] = {

	__u32 event_log[QAT_NUMBER_OF_PM_EVENTS];

What am I missing?

Cheers,
Andy Shevchenko Aug. 18, 2023, 8:46 a.m. UTC | #3 
On Fri, Aug 18, 2023 at 01:28:42PM +0800, Herbert Xu wrote:
> On Thu, Aug 17, 2023 at 04:33:17PM +0200, Lucas Segarra Fernandez wrote:
> >
> > +static struct pm_status_row pm_event_rows[] = {
> > +	PM_INFO_REGSET_ENTRY32(event_log[0], EVENT0),
> > +	PM_INFO_REGSET_ENTRY32(event_log[1], EVENT1),
> > +	PM_INFO_REGSET_ENTRY32(event_log[2], EVENT2),
> > +	PM_INFO_REGSET_ENTRY32(event_log[3], EVENT3),
> > +	PM_INFO_REGSET_ENTRY32(event_log[4], EVENT4),
> > +	PM_INFO_REGSET_ENTRY32(event_log[5], EVENT5),
> > +	PM_INFO_REGSET_ENTRY32(event_log[6], EVENT6),
> > +	PM_INFO_REGSET_ENTRY32(event_log[7], EVENT7),
> > +};
> > +
> > +static_assert(ARRAY_SIZE_OF_FIELD(struct icp_qat_fw_init_admin_pm_info, event_log) ==
> > +	      ARRAY_SIZE(pm_event_rows));
> 
> Was all of that churn just for this one line?
> 
> How about simply declaring a macro
> 
> 	#define QAT_NUMBER_OF_PM_EVENTS 8
> 
> and then use it for the two arrays:
> 
> 	static struct pm_status_row pm_event_rows[QAT_NUMBER_OF_PM_EVENTS] = {
> 
> 	__u32 event_log[QAT_NUMBER_OF_PM_EVENTS];
> 
> What am I missing?

Splitting ARRAY_SIZE() is very beneficial on its own.
The static assert is slightly more robust for the big code then defining
something that at some point can be missed or miscalculated. Yet we can
survive with a macro if you thinks it's better.
Herbert Xu Aug. 18, 2023, 8:51 a.m. UTC | #4 
On Fri, Aug 18, 2023 at 11:46:55AM +0300, Andy Shevchenko wrote:
>
> Splitting ARRAY_SIZE() is very beneficial on its own.
> The static assert is slightly more robust for the big code then defining
> something that at some point can be missed or miscalculated. Yet we can
> survive with a macro if you thinks it's better.

Yes please go ahead with just a macro.

If the ARRYA_SIZE thing turns out to be useful for other people I'm
sure it'll be added.  But I don't see much value in this particular
use.

Thanks,
Andy Shevchenko Aug. 21, 2023, 11:18 a.m. UTC | #5 
On Sun, Aug 20, 2023 at 09:52:22PM +0200, Alejandro Colomar wrote:
> On 2023-08-18 10:46, Andy Shevchenko wrote:
> > On Fri, Aug 18, 2023 at 01:28:42PM +0800, Herbert Xu wrote:
> >> On Thu, Aug 17, 2023 at 04:33:17PM +0200, Lucas Segarra Fernandez wrote:

...

> Many xxxof_{member,field}() macros make use of the same construction to
> refer to a member of a struct without needing a variable of the
> structure type.
> 
> memberof(T, m) simplifies all of those, avoids possible mistakes in
> repetition, adds a meaningful name to the construction, and improves
> readability by avoiding too many parentheses together.
> 
> It uses a compound literal, which should optimized out by the compiler.
> It's a bit simpler to read than the dereference of a casted null
> pointer, due to having less parentheses in the implementation.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Lucas Segarra Fernandez <lucas.segarra.fernandez@intel.com>
> Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> ---
>  include/linux/container_of.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 713890c867be..5e762025c780 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -5,7 +5,9 @@
>  #include <linux/build_bug.h>
>  #include <linux/stddef.h>
>  
> -#define typeof_member(T, m)	typeof(((T*)0)->m)
> +
> +#define memberof(T, member)  ((T){}.member)

I'm not sure. This seems to me utilization of compound literal, while above
uses direct struct member pointer calculations.

> +#define typeof_member(T, m)  typeof(memberof(T, m))
Alejandro Colomar Aug. 21, 2023, 11:23 a.m. UTC | #6 
Hi Andy,

On 2023-08-21 13:18, Andy Shevchenko wrote:
> On Sun, Aug 20, 2023 at 09:52:22PM +0200, Alejandro Colomar wrote:
>>  
>> -#define typeof_member(T, m)	typeof(((T*)0)->m)
>> +
>> +#define memberof(T, member)  ((T){}.member)
> 
> I'm not sure. This seems to me utilization of compound literal, while above
> uses direct struct member pointer calculations.

Both can be used in most cases.  The only exception is offsetof(3), where
you need the pointer calculation.  The good thing about the compound
literal is that it's farther away from causing UB, but if that's not a
concern --using sizeof() or typeof() will usually make things safe from
UB, as there's really no dereference, but just to be a little paranoic--,
I could change the definition of memberof() to use the pointer thing.

Should I send a v2 with the pointer thing?

[I'll take some time, as I need to restore my USB with keys, which just
died yesterday.  I didn't sign this email due to that.]

Cheers,
Alex

> 
>> +#define typeof_member(T, m)  typeof(memberof(T, m))
>