Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

On Thu, Aug 19, 2021 at 05:57:21PM +0200, Jan Beulich wrote:
> On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote:
> > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
> >      }
> >  }
> >  
> > +static void enable_exar_enhanced_bits(struct ns16550 *uart)
> 
> Afaics the parameter can be pointer-to-const.
> 
> > +{
> > +#ifdef NS16550_PCI
> > +    if ( uart->bar &&
> > +         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
> > +                         uart->ps_bdf[2]), PCI_VENDOR_ID) == PCI_VENDOR_ID_EXAR )
> > +    {
> > +        u8 devid = ns_read_reg(uart, UART_XR_DVID);
> > +        u8 efr;
> > +        /*
> > +         * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control)
> > +         * unless "Enhanced control bits" is enabled.
> > +         * The below checks for a 2, 4 or 8 port UART, following Linux driver.
> > +         */
> > +        if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) {
> 
> Hmm, now I'm in trouble as to a question you did ask earlier (once
> on irc and I think also once in email): You asked whether to use
> the PCI device ID _or_ the DVID register. Now you're using both,
> albeit in a way not really making the access here safe: You assume
> that you can access the byte at offset 0x8d on all Exar devices. I
> don't know whether there is anything written anywhere telling you
> this is safe. With the earlier vendor/device ID match, it would feel
> safer to me if you replaced vendor ID and DVID checks here by a
> check of uart->param: While you must not deref that pointer, you can
> still check that it points at one of the three new entries you add
> to uart_param[]. Perhaps via "switch ( uart->param - uart_param )".

Ok, indeed with checking just uart->param - uart_param, I can get rid of
pci_conf_read here entirely. And so the #ifdef won't be necessary
either.

> Also there are a number of style nits:
> - opening braces go on their own lines (except after "do"),
> - blank lines are wanted between declarations and statements,
> - we try to move away from u<N> and want new code to use uint<N>_t,
> - with "devid" declared in the narrowest possible scope, please do
>   so also for "efr" (albeit this logic may get rearranged enough to
>   make this point moot).
> 
> Jan
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]