xen/arch/x86/x86_emulate/x86_emulate.h | 2 +- xen/include/public/arch-x86_64.h | 2 +- xen/include/xen/pci_regs.h | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-)
MISRA C rule 4.2 states that trigraphs (sequences of two question marks
followed by a specified third character [=/'()!<>-]) should not be used.
This applies to both code and comments. Thankfully, we do not use them
in the code, but still there are some comments where they are
accidentally used. Fix it.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
xen/arch/x86/x86_emulate/x86_emulate.h | 2 +-
xen/include/public/arch-x86_64.h | 2 +-
xen/include/xen/pci_regs.h | 12 ++++++------
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 4732855c40ed..bb7af967ffee 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -228,7 +228,7 @@ struct x86_emulate_ops
* All functions:
* @ctxt: [IN ] Emulation context info as passed to the emulator.
* All memory-access functions:
- * @seg: [IN ] Segment being dereferenced (specified as x86_seg_??).
+ * @seg: [IN ] Segment being dereferenced (specified as x86_seg_?).
* @offset:[IN ] Offset within segment.
* @p_data:[IN ] Pointer to i/o data buffer (length is @bytes)
* Read functions:
diff --git a/xen/include/public/arch-x86_64.h b/xen/include/public/arch-x86_64.h
index 5db52de69584..1c3567a20217 100644
--- a/xen/include/public/arch-x86_64.h
+++ b/xen/include/public/arch-x86_64.h
@@ -22,5 +22,5 @@
* A similar callback occurs if the segment selectors are invalid.
* failsafe_address is used as the value of eip.
*
- * On x86_64, event_selector and failsafe_selector are ignored (???).
+ * On x86_64, event_selector and failsafe_selector are ignored (?).
*/
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index ee8e82be36b4..a90aff1712ba 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -246,13 +246,13 @@
#define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */
#define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */
#define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */
-#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */
-#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */
+#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */
+#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */
#define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */
-#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */
-#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */
-#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */
-#define PCI_PM_DATA_REGISTER 7 /* (??) */
+#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */
+#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */
+#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */
+#define PCI_PM_DATA_REGISTER 7 /* (?) */
#define PCI_PM_SIZEOF 8
/* AGP registers */
--
2.25.1On 06.12.2022 11:59, Michal Orzel wrote: > --- a/xen/include/public/arch-x86_64.h > +++ b/xen/include/public/arch-x86_64.h > @@ -22,5 +22,5 @@ > * A similar callback occurs if the segment selectors are invalid. > * failsafe_address is used as the value of eip. > * > - * On x86_64, event_selector and failsafe_selector are ignored (???). > + * On x86_64, event_selector and failsafe_selector are ignored (?). May I ask that this become (?!?) instead? > --- a/xen/include/xen/pci_regs.h > +++ b/xen/include/xen/pci_regs.h > @@ -246,13 +246,13 @@ > #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */ > #define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */ > #define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */ > -#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */ > -#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */ > +#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */ > +#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */ > #define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */ > -#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */ > -#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */ > -#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */ > -#define PCI_PM_DATA_REGISTER 7 /* (??) */ > +#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */ > +#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */ > +#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */ > +#define PCI_PM_DATA_REGISTER 7 /* (?) */ > #define PCI_PM_SIZEOF 8 We've inherited all of these from Linux, and I notice Linux still has it this same way. Ideally Linux would change first, but I'd be okay with a sentence added to the description clarifying that we knowingly accept the divergence. With the adjustments: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Jan, On 06/12/2022 13:42, Jan Beulich wrote: > > > On 06.12.2022 11:59, Michal Orzel wrote: >> --- a/xen/include/public/arch-x86_64.h >> +++ b/xen/include/public/arch-x86_64.h >> @@ -22,5 +22,5 @@ >> * A similar callback occurs if the segment selectors are invalid. >> * failsafe_address is used as the value of eip. >> * >> - * On x86_64, event_selector and failsafe_selector are ignored (???). >> + * On x86_64, event_selector and failsafe_selector are ignored (?). > > May I ask that this become (?!?) instead? Sure. > >> --- a/xen/include/xen/pci_regs.h >> +++ b/xen/include/xen/pci_regs.h >> @@ -246,13 +246,13 @@ >> #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */ >> #define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */ >> #define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */ >> -#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */ >> -#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */ >> +#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */ >> +#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */ >> #define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */ >> -#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */ >> -#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */ >> -#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */ >> -#define PCI_PM_DATA_REGISTER 7 /* (??) */ >> +#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */ >> +#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */ >> +#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */ >> +#define PCI_PM_DATA_REGISTER 7 /* (?) */ >> #define PCI_PM_SIZEOF 8 > > We've inherited all of these from Linux, and I notice Linux still has it > this same way. Ideally Linux would change first, but I'd be okay with a > sentence added to the description clarifying that we knowingly accept > the divergence. This file already diverged and we are not in sync with Linux as well. Also there is no functional change being made by this patch so it is ok to change Xen and not Linux in this case (which has quite a lot of trigraphs all over the place). > > With the adjustments: > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Jan ~Michal
On 06.12.2022 14:05, Michal Orzel wrote: > On 06/12/2022 13:42, Jan Beulich wrote: >> On 06.12.2022 11:59, Michal Orzel wrote: >>> --- a/xen/include/xen/pci_regs.h >>> +++ b/xen/include/xen/pci_regs.h >>> @@ -246,13 +246,13 @@ >>> #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */ >>> #define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */ >>> #define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */ >>> -#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */ >>> -#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */ >>> +#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */ >>> +#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */ >>> #define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */ >>> -#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */ >>> -#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */ >>> -#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */ >>> -#define PCI_PM_DATA_REGISTER 7 /* (??) */ >>> +#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */ >>> +#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */ >>> +#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */ >>> +#define PCI_PM_DATA_REGISTER 7 /* (?) */ >>> #define PCI_PM_SIZEOF 8 >> >> We've inherited all of these from Linux, and I notice Linux still has it >> this same way. Ideally Linux would change first, but I'd be okay with a >> sentence added to the description clarifying that we knowingly accept >> the divergence. > This file already diverged and we are not in sync with Linux as well. Of course - that's the case for the majority of files we've taken from somewhere. But a Linux patch dropping the (??) parts of the comment (perhaps once whoever had put them there convinced themselves that the names of the constants and/or the comments are valid / applicable) would then no longer apply cleanly if we wanted to carry it over. Hence my request for amending the description. > Also there is no functional change being made by this patch so it is ok > to change Xen and not Linux in this case (which has quite a lot of trigraphs all over the place). Based on what criteria are you saying this is ok (unconditionally)? Jan
On 06/12/2022 14:46, Jan Beulich wrote: > > > On 06.12.2022 14:05, Michal Orzel wrote: >> On 06/12/2022 13:42, Jan Beulich wrote: >>> On 06.12.2022 11:59, Michal Orzel wrote: >>>> --- a/xen/include/xen/pci_regs.h >>>> +++ b/xen/include/xen/pci_regs.h >>>> @@ -246,13 +246,13 @@ >>>> #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */ >>>> #define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */ >>>> #define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */ >>>> -#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */ >>>> -#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */ >>>> +#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */ >>>> +#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */ >>>> #define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */ >>>> -#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */ >>>> -#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */ >>>> -#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */ >>>> -#define PCI_PM_DATA_REGISTER 7 /* (??) */ >>>> +#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */ >>>> +#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */ >>>> +#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */ >>>> +#define PCI_PM_DATA_REGISTER 7 /* (?) */ >>>> #define PCI_PM_SIZEOF 8 >>> >>> We've inherited all of these from Linux, and I notice Linux still has it >>> this same way. Ideally Linux would change first, but I'd be okay with a >>> sentence added to the description clarifying that we knowingly accept >>> the divergence. >> This file already diverged and we are not in sync with Linux as well. > > Of course - that's the case for the majority of files we've taken from > somewhere. But a Linux patch dropping the (??) parts of the comment > (perhaps once whoever had put them there convinced themselves that the > names of the constants and/or the comments are valid / applicable) > would then no longer apply cleanly if we wanted to carry it over. > Hence my request for amending the description. I'm totally fine to add an extra sentence. > >> Also there is no functional change being made by this patch so it is ok >> to change Xen and not Linux in this case (which has quite a lot of trigraphs all over the place). > > Based on what criteria are you saying this is ok (unconditionally)? I said that it is ok to change Xen and not Linux because this file already diverged, so we cannot assume that future backports will apply cleanly. If we change a file that did not diverge, then we are required to modify the origin first and then do the backport. > > Jan ~Michal
On 06.12.2022 15:05, Michal Orzel wrote: > On 06/12/2022 14:46, Jan Beulich wrote: >> On 06.12.2022 14:05, Michal Orzel wrote: >>> Also there is no functional change being made by this patch so it is ok >>> to change Xen and not Linux in this case (which has quite a lot of trigraphs all over the place). >> >> Based on what criteria are you saying this is ok (unconditionally)? > > I said that it is ok to change Xen and not Linux because this file already diverged, > so we cannot assume that future backports will apply cleanly. If we change a file > that did not diverge, then we are required to modify the origin first and then > do the backport. A file might have diverged, yet commits to be pulled in from the original tree may still apply cleanly. That why, in the general case, we aim at limiting the amount of divergence, and we prefer to pull in changes bringing us back closer to the (meanwhile evolved) original file. Jan
> On 6 Dec 2022, at 10:59, Michal Orzel <michal.orzel@amd.com> wrote: > > MISRA C rule 4.2 states that trigraphs (sequences of two question marks > followed by a specified third character [=/'()!<>-]) should not be used. > This applies to both code and comments. Thankfully, we do not use them > in the code, but still there are some comments where they are > accidentally used. Fix it. > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/x86/x86_emulate/x86_emulate.h | 2 +- > xen/include/public/arch-x86_64.h | 2 +- > xen/include/xen/pci_regs.h | 12 ++++++------ > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h > index 4732855c40ed..bb7af967ffee 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -228,7 +228,7 @@ struct x86_emulate_ops > * All functions: > * @ctxt: [IN ] Emulation context info as passed to the emulator. > * All memory-access functions: > - * @seg: [IN ] Segment being dereferenced (specified as x86_seg_??). > + * @seg: [IN ] Segment being dereferenced (specified as x86_seg_?). > * @offset:[IN ] Offset within segment. > * @p_data:[IN ] Pointer to i/o data buffer (length is @bytes) > * Read functions: > diff --git a/xen/include/public/arch-x86_64.h b/xen/include/public/arch-x86_64.h > index 5db52de69584..1c3567a20217 100644 > --- a/xen/include/public/arch-x86_64.h > +++ b/xen/include/public/arch-x86_64.h > @@ -22,5 +22,5 @@ > * A similar callback occurs if the segment selectors are invalid. > * failsafe_address is used as the value of eip. > * > - * On x86_64, event_selector and failsafe_selector are ignored (???). > + * On x86_64, event_selector and failsafe_selector are ignored (?). > */ > diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h > index ee8e82be36b4..a90aff1712ba 100644 > --- a/xen/include/xen/pci_regs.h > +++ b/xen/include/xen/pci_regs.h > @@ -246,13 +246,13 @@ > #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */ > #define PCI_PM_CTRL_NO_SOFT_RESET 0x0008 /* No reset for D3hot->D0 */ > #define PCI_PM_CTRL_PME_ENABLE 0x0100 /* PME pin enable */ > -#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (??) */ > -#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (??) */ > +#define PCI_PM_CTRL_DATA_SEL_MASK 0x1e00 /* Data select (?) */ > +#define PCI_PM_CTRL_DATA_SCALE_MASK 0x6000 /* Data scale (?) */ > #define PCI_PM_CTRL_PME_STATUS 0x8000 /* PME pin status */ > -#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (??) */ > -#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (??) */ > -#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (??) */ > -#define PCI_PM_DATA_REGISTER 7 /* (??) */ > +#define PCI_PM_PPB_EXTENSIONS 6 /* PPB support extensions (?) */ > +#define PCI_PM_PPB_B2_B3 0x40 /* Stop clock when in D3hot (?) */ > +#define PCI_PM_BPCC_ENABLE 0x80 /* Bus power/clock control enable (?) */ > +#define PCI_PM_DATA_REGISTER 7 /* (?) */ > #define PCI_PM_SIZEOF 8 > > /* AGP registers */ > -- > 2.25.1 > >
© 2016 - 2024 Red Hat, Inc.