[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 19 Aug 2021 17:57:21 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=/OudPeNClVLWA6zpCts9qVt5MgxZnutp1aJsxK/p6jg=; b=A94G/OztqHOf3zr+cG+YdPXY9Z1/i0rfj/tdsvXuYO8k2BZQb3W4YuFquxxv5iNKm7nGVXSNqPpTCSYpuq4kTQWqGs5TEeqa57XCGV1/SIX+rBp3i22GBo9C+4ZjL5l3Nd0c+lTj3BMXO00Yg1+H+3GUYT+qwtjEB4CfOQwPfjDglN3aZ2A9gDEmV0kUPAClGGiTbGp7R4fc0tA6NsU2gytHDBO4UIH2Zxanla5WCX9SksjPA+UHeW/s7x5S8a4AsXHdtRuOMQwRNpOTvsmHQ3UCdIbZ0Q19Vd+kOl1SsIoWUdnVDCjf7MyaqQhFvYWXJhAK1x6vozG9lUTsrOZmPQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ElyBCnoMiwp6Rl6xp7sm9Y5110KHaWh5C7UguoJBe7ET0H5KrXSdfcRJ2kmGR9bNyq/35N6fldIf9wxUPM6wrZPVrfaIHo8cdmLQBydqbkAV9Uos7f/Ti04kc5ie/LrhwMApNljdubySZj0BergrRhy6lPV79xG79y6qV6IneZIxRNg7RoBHNu+eXA6vrDiR5QERM9keiF1hSqty0yMOjoQgMzZQjZ5v799ks9FFs28jvWXUGPQ1FMcbsx9Z3zsEGvt7CyxfPabqqQ/gFcA0nCINdt60fDIuPbfQ0UVnUnt+tb1GReE4K+obAPs1/OhLaBx50T+nzaNOHWjqVLQgyA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 19 Aug 2021 15:57:36 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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) == 
> +    {
> +        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 )".

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).




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.