Received: by 2002:a05:6358:d09b:b0:dc:cd0c:909e with SMTP id jc27csp6431546rwb; Tue, 22 Nov 2022 13:19:18 -0800 (PST) X-Google-Smtp-Source: AA0mqf6FlLxR/TTtvKvm0Xmk3tfu+sgK8AQMTzaYBrZNILhscpOg3Y5g/A3oKkFEGvXxjNafFsrs X-Received: by 2002:a17:907:986c:b0:7a0:b505:e8f9 with SMTP id ko12-20020a170907986c00b007a0b505e8f9mr10473968ejc.216.1669151958695; Tue, 22 Nov 2022 13:19:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669151958; cv=none; d=google.com; s=arc-20160816; b=Fs7lvtB48qYp4oISvwb+GR5XHq+CYF0zno+dwqU6Fvhqnyfew6kzyYzuZ4Id6bkYRJ FCPDVhB5l24uMfGBpd+Z/s/9KJMW0M1JIETV7MO1cHUZgepYo/f96ud3a0Cfq6UjY3nQ AqZ8M2m7VvTzM+QUAmScB/ICnuTZO0w0kltPcTO/D1WQYymhgZ7KuMvn9kzoRl4Ol5EZ LB4lKeHhsiRwEl1m8PmyTlFHrFXNbXLDQ9DlMLA+ahExEkTIMKb049FZbf+X8RefhUgK USS5TF6uX4IPDuu9WtkZo2N+OfMPI9QEN+FvYjuVM/spnIdLuWoNYZw3brf8kayGP8dg VECQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=TEsBoWd98Bx8Szc4KH9G5PjRUA8sYnqA0FuOpnSyCE4=; b=RlTFcu62MQnKXGmzXq0RgTO6RzbN0RdpV6bT05suOSVT1mOnkQyTiz+FcFCe9/4LJd TepjuFZ3Iiro3ZhSBmRABkFGO0hRJbyp/eYAGDaCTHyY6NcoRcXgzNfhl6aJcKwTore+ l6ps29jJDHVgIKc0hS6D82qX7xZXQUXageb7G3Kb8/MCIul2kboZYrgP63Tt7ifo9LjC Vt7jj9jEkLAgW5Mf+QpvWuMS5/RwQIoEGxPepFOhkDtVdMu5Cf7uRzqrE8YnX6ScLrcP doSysQ4xxAFqtI1/dAC9L336kwGpx7u5xT4+wZ7wTTrxZp2QW7E3Jd6xq6MZTs9qNKPH OAFg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=MbzoJUg9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id w11-20020a056402268b00b00468f681ea48si13614506edd.116.2022.11.22.13.18.56; Tue, 22 Nov 2022 13:19:18 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=MbzoJUg9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233941AbiKVVFM (ORCPT + 90 others); Tue, 22 Nov 2022 16:05:12 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37402 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbiKVVFK (ORCPT ); Tue, 22 Nov 2022 16:05:10 -0500 Received: from mail-qt1-x829.google.com (mail-qt1-x829.google.com [IPv6:2607:f8b0:4864:20::829]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1520B7AF7C; Tue, 22 Nov 2022 13:05:09 -0800 (PST) Received: by mail-qt1-x829.google.com with SMTP id a27so10109884qtw.10; Tue, 22 Nov 2022 13:05:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=TEsBoWd98Bx8Szc4KH9G5PjRUA8sYnqA0FuOpnSyCE4=; b=MbzoJUg9Xw4Kaq0Thn1IUiQ5Cfiaa1GTlYPmlYvrOj4CS2MucM2OiL4ItLv3jljspa W51mbsmeDNe8/30KbGxMxBycIvJuCQTX+8I4OGIeri+RN4+PMxookm+lS1WVxRc037jP NUZ4ejkfmpDXB8OQTFnW+JP4X3EBOlqvN8jOSD6jB9z5uQyqQyOWSgyjxSeGPo3hR0Cw qYkkicUG3kNBJjOCyN1gcO/gp56/dTp7H0R5OKaSO70xTUOGQ16wJHOkX/64nxoYmhLN sjhdPgZTxbgecegbhnrPo6usbnG20HsKimk2cvzVtWsJtkp1XI2eRZdSh/rAN5TWizj7 swQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=TEsBoWd98Bx8Szc4KH9G5PjRUA8sYnqA0FuOpnSyCE4=; b=pFzA2hKQ+1udyc5iBfstW7T/HKMfhK4u6TuGGUibZ/IO8qav58R2P7PUj/FRW/rrcu H74aKHyuR7epCyw+rbZ5gkYAlNsvCXMB2QBsjEaG1HG5wjUR3DYmaJV/E43jXDwTEFLw Cx2KILmLflJkmfHj5CKWdN6EIiXeIZb1i5oFoAvJeyDAlY4tRd8G7AZ10sS9S4tB/vMz waNas9g/YdfBGRi1CQVSekAwG9tXB/6ynzo1Xp1xj0KGtuQ7x6mQslmKyItfsd2zmofC qz0IDcLrY9eIUKEIluBRM5n8Xd57Q4iYzA4FXpTWg+cbLPY/2AGvUq8EOuwtrht4Y5NP LvCg== X-Gm-Message-State: ANoB5pkd+yp2UDd+m5xQ7m/NHx5p0QGHHh/D6BJoaSGttqjdytp8aQEj y50sO49jMsRKjWpgmxjRGts= X-Received: by 2002:ac8:41cf:0:b0:3a5:868a:5b86 with SMTP id o15-20020ac841cf000000b003a5868a5b86mr23729271qtm.488.1669151108037; Tue, 22 Nov 2022 13:05:08 -0800 (PST) Received: from errol.ini.cmu.edu (pool-72-77-81-136.pitbpa.fios.verizon.net. [72.77.81.136]) by smtp.gmail.com with ESMTPSA id j6-20020a05620a410600b006fa2dde9db8sm10986864qko.95.2022.11.22.13.05.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Nov 2022 13:05:07 -0800 (PST) Date: Tue, 22 Nov 2022 16:05:05 -0500 From: "Gabriel L. Somlo" To: Jiri Slaby Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, gregkh@linuxfoundation.org, kgugala@antmicro.com, mholenko@antmicro.com, joel@jms.id.au, david.abdurachmanov@gmail.com, florent@enjoy-digital.fr, geert@linux-m68k.org, ilpo.jarvinen@linux.intel.com Subject: Re: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types Message-ID: References: <20221118145512.509950-1-gsomlo@gmail.com> <20221118145512.509950-10-gsomlo@gmail.com> <44bf21b6-cbe4-4d73-0883-a9bcbd7d5971@kernel.org> <1b5a963c-2a5b-54fe-784e-fcc4d06c347e@kernel.org> <2f242291-99b6-a50f-cd52-e7dfd8c88c8f@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2f242291-99b6-a50f-cd52-e7dfd8c88c8f@kernel.org> X-Clacks-Overhead: GNU Terry Pratchett X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 22, 2022 at 08:37:58AM +0100, Jiri Slaby wrote: > On 21. 11. 22, 14:55, Gabriel L. Somlo wrote: > > Hi Jiri, > > > > Thanks for the feedback! > > > > On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote: > > > On 21. 11. 22, 9:37, Jiri Slaby wrote: > > > > On 18. 11. 22, 15:55, Gabriel Somlo wrote: > > > > > Update variable types to match the signature of uart_insert_char() > > > > > which consumes them. > > > > > > > > > > Signed-off-by: Gabriel Somlo > > > > > Reviewed-by: Ilpo J?rvinen > > > > > --- > > > > > ? drivers/tty/serial/liteuart.c | 3 +-- > > > > > ? 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/tty/serial/liteuart.c > > > > > b/drivers/tty/serial/liteuart.c > > > > > index 81aa7c1da73c..42ac9aee050a 100644 > > > > > --- a/drivers/tty/serial/liteuart.c > > > > > +++ b/drivers/tty/serial/liteuart.c > > > > > @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t) > > > > > ????? struct liteuart_port *uart = from_timer(uart, t, timer); > > > > > ????? struct uart_port *port = &uart->port; > > > > > ????? unsigned char __iomem *membase = port->membase; > > > > > -??? int ch; > > > > > -??? unsigned long status; > > > > > +??? unsigned int status, ch; > > > > > > > > These should be u8 after all, right? > > > > OK, so: > > > > - I can hard-code `status` as `1`, like so: > > > > while(!litex_read8(membase + OFF_RXEMPTY) { > > ... > > if (!(uart_handle_sysrq_char(port, ch))) > > uart_insert_char(port, 1, 0, ch, TTY_NORMAL); > > > > ... since `status` would always be `1` inside the loop. So I'm > > basically going to get rid of it altogether. > > Yes, I had that in my mind. Except passing 1 to uart_insert_char() when > overflow is hardwired to 0 makes no sense IMO :). So, looking at what uart_insert_char() does, I could simply do this instead: while(!litex_read8(membase + OFF_RXEMPTY) { ... /* LiteUART does not provide overrun bits */ if (!(uart_handle_sysrq_char(port, ch) || tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL))) ++port->icount.buf_overrun; That is, `tty_insert_flip_char() is the portion of `uart_insert_char()` that actually gets executed if status is 1 and overrun is 0... I'm not quite confident about whether this is an improvement in legibility and/or code quality, but please let me know what *you* think... :) > > - `ch` is indeed *produced* by `litex_read8()`, which would make it > > `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()` > > and `uart_insert_char()`, which both expect `unsigned int`. > > Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed one > day. It should really be u8. All down the call chain (it magically turns > into int in the sysrq handlers, then char is expected). > > > If you think it's better to go with the type when the value is > > produced (as opposed to when it's consumed), I'm OK with that for > > the upcoming v6 of the series... :) > > Yes, please. We should slowly convert _all_ of them. OK, u8 it is, then. > > > And can you change membase to u8 * too 8-)? > > > > Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the > > `membase` field is declared as: > > > > unsigned char __iomem *membase; > > > > which is why I'm thinking we should leave it as-is? Unless there are > > plans (or a pending patch I'm unaware of) to switch the field in > > include/linux/serial_core.h to `u8` as well? -- Please advise. > > Ah, then keep it. I somehow thought it's void *. And yes, even this should > be u8 __iomem *, eventually. OK, it should/will get updated in bulk once that change is made across the entire subsystem, leaving as-is for now. Thanks again for all your help and advice! --Gabriel