* Re: [v2] genirq/msi: Add the address and data that show MSI/MSIX
2025-03-01 12:39 [v2] genirq/msi: Add the address and data that show MSI/MSIX Hans Zhang
@ 2025-03-02 1:12 ` kernel test robot
2025-03-02 1:12 ` kernel test robot
2025-03-02 9:01 ` Thomas Gleixner
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-03-02 1:12 UTC (permalink / raw)
To: Hans Zhang, tglx
Cc: oe-kbuild-all, manivannan.sadhasivam, kw, kwilczynski, bhelgaas,
Frank.Li, cassel, linux-pci, linux-kernel, Hans Zhang
Hi Hans,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 76544811c850a1f4c055aa182b513b7a843868ea]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/genirq-msi-Add-the-address-and-data-that-show-MSI-MSIX/20250301-204332
base: 76544811c850a1f4c055aa182b513b7a843868ea
patch link: https://lore.kernel.org/r/20250301123953.291675-1-18255117159%40163.com
patch subject: [v2] genirq/msi: Add the address and data that show MSI/MSIX
config: arm-randconfig-001-20250302 (https://download.01.org/0day-ci/archive/20250302/202503020812.PKZf7JBa-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250302/202503020812.PKZf7JBa-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503020812.PKZf7JBa-lkp@intel.com/
All warnings (new ones prefixed by >>):
kernel/irq/msi.c: In function 'msi_domain_debug_show':
kernel/irq/msi.c:770:9: error: implicit declaration of function 'seq_printf'; did you mean 'bstr_printf'? [-Wimplicit-function-declaration]
770 | seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
| ^~~~~~~~~~
| bstr_printf
kernel/irq/msi.c: At top level:
kernel/irq/msi.c:782:10: error: 'const struct irq_domain_ops' has no member named 'debug_show'
782 | .debug_show = msi_domain_debug_show,
| ^~~~~~~~~~
>> kernel/irq/msi.c:782:27: warning: excess elements in struct initializer
782 | .debug_show = msi_domain_debug_show,
| ^~~~~~~~~~~~~~~~~~~~~
kernel/irq/msi.c:782:27: note: (near initialization for 'msi_domain_ops')
vim +782 kernel/irq/msi.c
775
776 static const struct irq_domain_ops msi_domain_ops = {
777 .alloc = msi_domain_alloc,
778 .free = msi_domain_free,
779 .activate = msi_domain_activate,
780 .deactivate = msi_domain_deactivate,
781 .translate = msi_domain_translate,
> 782 .debug_show = msi_domain_debug_show,
783 };
784
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [v2] genirq/msi: Add the address and data that show MSI/MSIX
2025-03-01 12:39 [v2] genirq/msi: Add the address and data that show MSI/MSIX Hans Zhang
2025-03-02 1:12 ` kernel test robot
@ 2025-03-02 1:12 ` kernel test robot
2025-03-02 9:01 ` Thomas Gleixner
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-03-02 1:12 UTC (permalink / raw)
To: Hans Zhang, tglx
Cc: oe-kbuild-all, manivannan.sadhasivam, kw, kwilczynski, bhelgaas,
Frank.Li, cassel, linux-pci, linux-kernel, Hans Zhang
Hi Hans,
kernel test robot noticed the following build errors:
[auto build test ERROR on 76544811c850a1f4c055aa182b513b7a843868ea]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/genirq-msi-Add-the-address-and-data-that-show-MSI-MSIX/20250301-204332
base: 76544811c850a1f4c055aa182b513b7a843868ea
patch link: https://lore.kernel.org/r/20250301123953.291675-1-18255117159%40163.com
patch subject: [v2] genirq/msi: Add the address and data that show MSI/MSIX
config: x86_64-buildonly-randconfig-003-20250302 (https://download.01.org/0day-ci/archive/20250302/202503020807.c3MhmbJh-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250302/202503020807.c3MhmbJh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503020807.c3MhmbJh-lkp@intel.com/
All errors (new ones prefixed by >>):
kernel/irq/msi.c: In function 'msi_domain_debug_show':
>> kernel/irq/msi.c:770:9: error: implicit declaration of function 'seq_printf'; did you mean 'bstr_printf'? [-Werror=implicit-function-declaration]
770 | seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
| ^~~~~~~~~~
| bstr_printf
kernel/irq/msi.c: At top level:
>> kernel/irq/msi.c:782:10: error: 'const struct irq_domain_ops' has no member named 'debug_show'
782 | .debug_show = msi_domain_debug_show,
| ^~~~~~~~~~
>> kernel/irq/msi.c:782:27: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
782 | .debug_show = msi_domain_debug_show,
| ^~~~~~~~~~~~~~~~~~~~~
kernel/irq/msi.c:782:27: note: (near initialization for 'msi_domain_ops')
kernel/irq/msi.c:782:27: error: initialization of 'int (*)(struct irq_domain *, unsigned int, unsigned int, void *)' from incompatible pointer type 'void (*)(struct seq_file *, struct irq_domain *, struct irq_data *, int)' [-Werror=incompatible-pointer-types]
kernel/irq/msi.c:782:27: note: (near initialization for 'msi_domain_ops.alloc')
kernel/irq/msi.c:782:27: warning: initialized field overwritten [-Woverride-init]
kernel/irq/msi.c:782:27: note: (near initialization for 'msi_domain_ops.alloc')
cc1: some warnings being treated as errors
vim +770 kernel/irq/msi.c
758
759 static void msi_domain_debug_show(struct seq_file *m, struct irq_domain *d,
760 struct irq_data *irqd, int ind)
761 {
762 struct msi_desc *desc;
763 bool is_msix;
764
765 desc = irq_get_msi_desc(irqd->irq);
766 if (!desc)
767 return;
768
769 is_msix = desc->pci.msi_attrib.is_msix;
> 770 seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
771 seq_printf(m, "\n%*saddress_hi: 0x%08x", ind + 1, "", desc->msg.address_hi);
772 seq_printf(m, "\n%*saddress_lo: 0x%08x", ind + 1, "", desc->msg.address_lo);
773 seq_printf(m, "\n%*smsg_data: 0x%08x\n", ind + 1, "", desc->msg.data);
774 }
775
776 static const struct irq_domain_ops msi_domain_ops = {
777 .alloc = msi_domain_alloc,
778 .free = msi_domain_free,
779 .activate = msi_domain_activate,
780 .deactivate = msi_domain_deactivate,
781 .translate = msi_domain_translate,
> 782 .debug_show = msi_domain_debug_show,
783 };
784
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [v2] genirq/msi: Add the address and data that show MSI/MSIX
2025-03-01 12:39 [v2] genirq/msi: Add the address and data that show MSI/MSIX Hans Zhang
2025-03-02 1:12 ` kernel test robot
2025-03-02 1:12 ` kernel test robot
@ 2025-03-02 9:01 ` Thomas Gleixner
2025-03-02 14:45 ` Hans Zhang
2 siblings, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2025-03-02 9:01 UTC (permalink / raw)
To: Hans Zhang
Cc: manivannan.sadhasivam, kw, kwilczynski, bhelgaas, Frank.Li,
cassel, linux-pci, linux-kernel, Hans Zhang
Hans!
On Sat, Mar 01 2025 at 20:39, Hans Zhang wrote:
> The debug_show() callback function is implemented in the MSI core code.
> And assign it to the domain ops::debug_show() creation.
>
> cat /sys/kernel/debug/irq/irqs/msi_irq_num, the address and data stored
> in the MSI capability or the address and data stored in the MSIX vector
> table will be displayed.
So this explains what the patch is doing and what the output is. But it
fails to explain the _why_. Documentation gives proper guidance:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes
> e.g.
> root@root:/sys/kernel/debug/irq/irqs# cat /proc/interrupts | grep ITS
> 85: 0 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 75497472 Edge PCIe PME, aerdrv
> 86: 0 30 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021760 Edge nvme0q0
> 87: 287 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021761 Edge nvme0q1
> 88: 0 265 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021762 Edge nvme0q2
> 89: 0 0 177 0 0 0 0 0 0 0 0 0 ITS-MSI 76021763 Edge nvme0q3
> 90: 0 0 0 76 0 0 0 0 0 0 0 0 ITS-MSI 76021764 Edge nvme0q4
> 91: 0 0 0 0 161 0 0 0 0 0 0 0 ITS-MSI 76021765 Edge nvme0q5
> 92: 0 0 0 0 0 991 0 0 0 0 0 0 ITS-MSI 76021766 Edge nvme0q6
> 93: 0 0 0 0 0 0 194 0 0 0 0 0 ITS-MSI 76021767 Edge nvme0q7
> 94: 0 0 0 0 0 0 0 94 0 0 0 0 ITS-MSI 76021768 Edge nvme0q8
> 95: 0 0 0 0 0 0 0 0 148 0 0 0 ITS-MSI 76021769 Edge nvme0q9
> 96: 0 0 0 0 0 0 0 0 0 261 0 0 ITS-MSI 76021770 Edge nvme0q10
> 97: 0 0 0 0 0 0 0 0 0 0 127 0 ITS-MSI 76021771 Edge nvme0q11
> 98: 0 0 0 0 0 0 0 0 0 0 0 317 ITS-MSI 76021772 Edge nvme0q12
How is this relevant to describe the patch?
> root@root:/sys/kernel/debug/irq/irqs#
> root@root:/sys/kernel/debug/irq/irqs# cat 87
> handler: handle_fasteoi_irq
> device: 0000:91:00.0
> status: 0x00000000
> istate: 0x00004000
> ddepth: 0
> wdepth: 0
> dstate: 0x31600200
> IRQD_ACTIVATED
> IRQD_IRQ_STARTED
> IRQD_SINGLE_TARGET
> IRQD_AFFINITY_MANAGED
> IRQD_AFFINITY_ON_ACTIVATE
> IRQD_HANDLE_ENFORCE_IRQCTX
> node: 0
> affinity: 0
> effectiv: 0
> domain: :soc@0:interrupt-controller@0e001000:its@0e050000-3
> hwirq: 0x4880001
> chip: ITS-MSI
This output is from a pre 6.11 kernel...
> flags: 0x20
> IRQCHIP_ONESHOT_SAFE
> msix:
> address_hi: 0x00000000
> address_lo: 0x0e060040
> msg_data: 0x00000001
For demonstration it's enough to stop here, no?
> +static void msi_domain_debug_show(struct seq_file *m, struct irq_domain *d,
> + struct irq_data *irqd, int ind)
> +{
> + struct msi_desc *desc;
> + bool is_msix;
> +
> + desc = irq_get_msi_desc(irqd->irq);
Move this up to the declaration.
> + if (!desc)
> + return;
> +
> + is_msix = desc->pci.msi_attrib.is_msix;
That's not valid for non PCI MSI interrupts.
This function is used for all types of MSI interrupts. So for non PCI
MSI interrupts this will output random garbage. Just print the address
and be done with it. The MSI variant is visible from the chip name on
current kernels. It's either ITS-PCI-MSI or ITS-PCI-MSIX and not
ITS-MSI.
> + seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
> + seq_printf(m, "\n%*saddress_hi: 0x%08x", ind + 1, "", desc->msg.address_hi);
> + seq_printf(m, "\n%*saddress_lo: 0x%08x", ind + 1, "", desc->msg.address_lo);
> + seq_printf(m, "\n%*smsg_data: 0x%08x\n", ind + 1, "", desc->msg.data);
> +}
> +
> static const struct irq_domain_ops msi_domain_ops = {
> .alloc = msi_domain_alloc,
> .free = msi_domain_free,
> .activate = msi_domain_activate,
> .deactivate = msi_domain_deactivate,
> .translate = msi_domain_translate,
> + .debug_show = msi_domain_debug_show,
This does not build when CONFIG_GENERIC_IRQ_DEBUGFS=n.
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [v2] genirq/msi: Add the address and data that show MSI/MSIX
2025-03-02 9:01 ` Thomas Gleixner
@ 2025-03-02 14:45 ` Hans Zhang
0 siblings, 0 replies; 5+ messages in thread
From: Hans Zhang @ 2025-03-02 14:45 UTC (permalink / raw)
To: Thomas Gleixner
Cc: manivannan.sadhasivam, kw, kwilczynski, bhelgaas, Frank.Li,
cassel, linux-pci, linux-kernel
Hi Thomas(tglx),
On 2025/3/2 17:01, Thomas Gleixner wrote:
> Hans!
>
> On Sat, Mar 01 2025 at 20:39, Hans Zhang wrote:
>> The debug_show() callback function is implemented in the MSI core code.
>> And assign it to the domain ops::debug_show() creation.
>>
>> cat /sys/kernel/debug/irq/irqs/msi_irq_num, the address and data stored
>> in the MSI capability or the address and data stored in the MSIX vector
>> table will be displayed.
>
> So this explains what the patch is doing and what the output is. But it
> fails to explain the _why_. Documentation gives proper guidance:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes
Is the following explanation OK?
Displaying the address and data of the MSI/MSIX interrupt in the debugfs
helps with debugging.
>> e.g.
>> root@root:/sys/kernel/debug/irq/irqs# cat /proc/interrupts | grep ITS
>> 85: 0 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 75497472 Edge PCIe PME, aerdrv
>> 86: 0 30 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021760 Edge nvme0q0
>> 87: 287 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021761 Edge nvme0q1
>> 88: 0 265 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021762 Edge nvme0q2
>> 89: 0 0 177 0 0 0 0 0 0 0 0 0 ITS-MSI 76021763 Edge nvme0q3
>> 90: 0 0 0 76 0 0 0 0 0 0 0 0 ITS-MSI 76021764 Edge nvme0q4
>> 91: 0 0 0 0 161 0 0 0 0 0 0 0 ITS-MSI 76021765 Edge nvme0q5
>> 92: 0 0 0 0 0 991 0 0 0 0 0 0 ITS-MSI 76021766 Edge nvme0q6
>> 93: 0 0 0 0 0 0 194 0 0 0 0 0 ITS-MSI 76021767 Edge nvme0q7
>> 94: 0 0 0 0 0 0 0 94 0 0 0 0 ITS-MSI 76021768 Edge nvme0q8
>> 95: 0 0 0 0 0 0 0 0 148 0 0 0 ITS-MSI 76021769 Edge nvme0q9
>> 96: 0 0 0 0 0 0 0 0 0 261 0 0 ITS-MSI 76021770 Edge nvme0q10
>> 97: 0 0 0 0 0 0 0 0 0 0 127 0 ITS-MSI 76021771 Edge nvme0q11
>> 98: 0 0 0 0 0 0 0 0 0 0 0 317 ITS-MSI 76021772 Edge nvme0q12
>
> How is this relevant to describe the patch?
The preceding information will be deleted in v4 patch.
>
>> root@root:/sys/kernel/debug/irq/irqs#
>> root@root:/sys/kernel/debug/irq/irqs# cat 87
>> handler: handle_fasteoi_irq
>> device: 0000:91:00.0
>> status: 0x00000000
>> istate: 0x00004000
>> ddepth: 0
>> wdepth: 0
>> dstate: 0x31600200
>> IRQD_ACTIVATED
>> IRQD_IRQ_STARTED
>> IRQD_SINGLE_TARGET
>> IRQD_AFFINITY_MANAGED
>> IRQD_AFFINITY_ON_ACTIVATE
>> IRQD_HANDLE_ENFORCE_IRQCTX
>> node: 0
>> affinity: 0
>> effectiv: 0
>> domain: :soc@0:interrupt-controller@0e001000:its@0e050000-3
>> hwirq: 0x4880001
>> chip: ITS-MSI
>
> This output is from a pre 6.11 kernel...
I will delete all other information except for what my patch will display.
>
>> flags: 0x20
>> IRQCHIP_ONESHOT_SAFE
>> msix:
>> address_hi: 0x00000000
>> address_lo: 0x0e060040
>> msg_data: 0x00000001
>
> For demonstration it's enough to stop here, no?
Yes, I will change it to the following:
msix:
address_hi: 0x00000000
address_lo: 0x0e060040
msg_data: 0x00000001
>
>> +static void msi_domain_debug_show(struct seq_file *m, struct irq_domain *d,
>> + struct irq_data *irqd, int ind)
>> +{
>> + struct msi_desc *desc;
>> + bool is_msix;
>> +
>> + desc = irq_get_msi_desc(irqd->irq);
>
> Move this up to the declaration.
>
>> + if (!desc)
>> + return;
>> +
>> + is_msix = desc->pci.msi_attrib.is_msix;
>
> That's not valid for non PCI MSI interrupts.
Do you mean to remove the following two lines of code?
is_msix = desc->pci.msi_attrib.is_msix;
seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
> This function is used for all types of MSI interrupts. So for non PCI
> MSI interrupts this will output random garbage. Just print the address
> and be done with it. The MSI variant is visible from the chip name on
> current kernels. It's either ITS-PCI-MSI or ITS-PCI-MSIX and not
> ITS-MSI.
>
>> + seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
>> + seq_printf(m, "\n%*saddress_hi: 0x%08x", ind + 1, "", desc->msg.address_hi);
>> + seq_printf(m, "\n%*saddress_lo: 0x%08x", ind + 1, "", desc->msg.address_lo);
>> + seq_printf(m, "\n%*smsg_data: 0x%08x\n", ind + 1, "", desc->msg.data);
>> +}
>> +
>> static const struct irq_domain_ops msi_domain_ops = {
>> .alloc = msi_domain_alloc,
>> .free = msi_domain_free,
>> .activate = msi_domain_activate,
>> .deactivate = msi_domain_deactivate,
>> .translate = msi_domain_translate,
>> + .debug_show = msi_domain_debug_show,
>
> This does not build when CONFIG_GENERIC_IRQ_DEBUGFS=n.
>
Kernel test robot has reported a compilation error, and I have submitted
v3 patch to solve this problem. I will fix all your questions in v4 patch.
Finally, thank you very much for all your comments.
Best regards
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread