Re: [PATCH] xen: Remove trigraphs from comments

[Michal Orzel <michal.orzel@xxxxxxx>]

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