Re: [PATCH v2 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2023 00:38, Marek Marczykowski-Górecki wrote:
> On Tue, May 30, 2023 at 02:04:26PM +0200, Jan Beulich wrote:
>> On 05.05.2023 23:25, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/drivers/char/xhci-dbc.c
>>> +++ b/xen/drivers/char/xhci-dbc.c
>>> @@ -1221,14 +1221,12 @@ static void __init cf_check 
>>> dbc_uart_init_postirq(struct serial_port *port)
>>>       * Linux's XHCI driver (as of 5.18) works without writting to the whole
>>>       * page, so keep it simple.
>>>       */
>>> -    if ( rangeset_add_range(mmio_ro_ranges,
>>> -                PFN_DOWN((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
>>> -                         uart->dbc.xhc_dbc_offset),
>>> -                PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
>>> -                       uart->dbc.xhc_dbc_offset +
>>> -                sizeof(*uart->dbc.dbc_reg)) - 1) )
>>> -        printk(XENLOG_INFO
>>> -               "Error while adding MMIO range of device to 
>>> mmio_ro_ranges\n");
>>> +    if ( subpage_mmio_ro_add(
>>> +            (uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
>>> +             uart->dbc.xhc_dbc_offset,
>>> +            sizeof(*uart->dbc.dbc_reg)) )
>>> +        printk(XENLOG_WARNING
>>> +               "Error while marking MMIO range of XHCI console as R/O\n");
>>
>> So how about falling back to just rangeset_add_range(mmio_ro_ranges, ...)
>> in this failure case? (I did mention an alternative to doing it here in
>> the comments on patch 1.)
> 
> Or fallback to XHCI_SHARE_NONE (IOW, pci_ro_device()).
> 
>> Also, doesn't the comment ahead of the construct become stale?
> 
> Indeed.
> 
>> Finally I think indentation of the function call arguments is off by one.
> 
> How is it supposed to be? Currently it's 8 spaces over the "if", should
> it be 4 spaces over the function name?

Yes. All our indentation is using a granularity of 4 spaces, of course 
taking into account aligning with respective items on earlier lines
(the length of which may not be a multiple of 4). It is always the
innermost construct relative to which the new indentation level is
determined.

Jan