From: Jonathan Cameron <jic23@kernel.org> To: James Clark <james.clark@arm.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, linux-kernel@vger.kernel.org, linux@roeck-us.net, michal.simek@amd.com, Jonathan Corbet <corbet@lwn.net>, Jean Delvare <jdelvare@suse.com>, Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>, Lars-Peter Clausen <lars@metafoo.de>, Michal Simek <michal.simek@xilinx.com>, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, linux-doc@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-serial@vger.kernel.org Subject: Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array Date: Sat, 18 Mar 2023 17:34:21 +0000 [thread overview] Message-ID: <20230318173402.20a4f60d@jic23-huawei> (raw) In-Reply-To: <74d8b579-6ea8-d6f3-170f-ea13534b4565@arm.com> On Fri, 17 Mar 2023 11:34:49 +0000 James Clark <james.clark@arm.com> wrote: > On 11/03/2023 19:18, Jonathan Cameron wrote: > > On Thu, 9 Mar 2023 15:03:33 +0000 > > James Clark <james.clark@arm.com> wrote: > > > >> Now that it exists, use it instead of doing the multiplication manually. > >> > >> Signed-off-by: James Clark <james.clark@arm.com> > > > > Hmm. I've stared at the users of this for a bit, and it's not actually obvious > > that it's being used as an array of u32. The only typed user of this is as > > the 2nd parameter of > > tty_insert_flip_string() which is an unsigned char * > > > > I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming > > from even if it has the right value? Perhaps the fifo depth is just a multiple of 4? > > > > Jonathan > > > > The commit that added it (b8caf69a6946) seems to hint that something > reads from it in words. And I see this: > > /* We always configure 4 bytes per FIFO word */ > #define BYTES_PER_FIFO_WORD 4U > > Perhaps sizeof(u32) isn't as accurate of a description as using > BYTES_PER_FIFO_WORD but I'd be reluctant to make a change because I > don't really understand the implications. Agreed with your analysis. + fully understand why you don't want to change it. I'd be tempted to take the view that whilst it's allocated in 4 byte chunks because it's accessed elsewhere as a set of 1 byte entries, krealloc_array isn't appropriate and so just leave it with devm_krealloc() Risk is that a steady stream of patches will turn up 'fixing' this as it will be easy for people to find with a script. Maybe better to just add a comment (either with or without your patch). > > There is also this in handle_rx_console(): > > unsigned char buf[sizeof(u32)]; > > James > > > > > > >> --- > >> drivers/tty/serial/qcom_geni_serial.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > >> index d69592e5e2ec..23fc33d182ac 100644 > >> --- a/drivers/tty/serial/qcom_geni_serial.c > >> +++ b/drivers/tty/serial/qcom_geni_serial.c > >> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port) > >> (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE; > >> > >> if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) { > >> - port->rx_buf = devm_krealloc(uport->dev, port->rx_buf, > >> - port->rx_fifo_depth * sizeof(u32), > >> - GFP_KERNEL); > >> + port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf, > >> + port->rx_fifo_depth, sizeof(u32), > >> + GFP_KERNEL); > >> if (!port->rx_buf) > >> return -ENOMEM; > >> } > >
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <jic23@kernel.org> To: James Clark <james.clark@arm.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, linux-kernel@vger.kernel.org, linux@roeck-us.net, michal.simek@amd.com, Jonathan Corbet <corbet@lwn.net>, Jean Delvare <jdelvare@suse.com>, Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>, Lars-Peter Clausen <lars@metafoo.de>, Michal Simek <michal.simek@xilinx.com>, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, linux-doc@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-serial@vger.kernel.org Subject: Re: [PATCH v2 4/4] serial: qcom_geni: Use devm_krealloc_array Date: Sat, 18 Mar 2023 17:34:21 +0000 [thread overview] Message-ID: <20230318173402.20a4f60d@jic23-huawei> (raw) In-Reply-To: <74d8b579-6ea8-d6f3-170f-ea13534b4565@arm.com> On Fri, 17 Mar 2023 11:34:49 +0000 James Clark <james.clark@arm.com> wrote: > On 11/03/2023 19:18, Jonathan Cameron wrote: > > On Thu, 9 Mar 2023 15:03:33 +0000 > > James Clark <james.clark@arm.com> wrote: > > > >> Now that it exists, use it instead of doing the multiplication manually. > >> > >> Signed-off-by: James Clark <james.clark@arm.com> > > > > Hmm. I've stared at the users of this for a bit, and it's not actually obvious > > that it's being used as an array of u32. The only typed user of this is as > > the 2nd parameter of > > tty_insert_flip_string() which is an unsigned char * > > > > I wonder if that sizeof(u32) isn't a 'correct' description of where the 4 is coming > > from even if it has the right value? Perhaps the fifo depth is just a multiple of 4? > > > > Jonathan > > > > The commit that added it (b8caf69a6946) seems to hint that something > reads from it in words. And I see this: > > /* We always configure 4 bytes per FIFO word */ > #define BYTES_PER_FIFO_WORD 4U > > Perhaps sizeof(u32) isn't as accurate of a description as using > BYTES_PER_FIFO_WORD but I'd be reluctant to make a change because I > don't really understand the implications. Agreed with your analysis. + fully understand why you don't want to change it. I'd be tempted to take the view that whilst it's allocated in 4 byte chunks because it's accessed elsewhere as a set of 1 byte entries, krealloc_array isn't appropriate and so just leave it with devm_krealloc() Risk is that a steady stream of patches will turn up 'fixing' this as it will be easy for people to find with a script. Maybe better to just add a comment (either with or without your patch). > > There is also this in handle_rx_console(): > > unsigned char buf[sizeof(u32)]; > > James > > > > > > >> --- > >> drivers/tty/serial/qcom_geni_serial.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > >> index d69592e5e2ec..23fc33d182ac 100644 > >> --- a/drivers/tty/serial/qcom_geni_serial.c > >> +++ b/drivers/tty/serial/qcom_geni_serial.c > >> @@ -1056,9 +1056,9 @@ static int setup_fifos(struct qcom_geni_serial_port *port) > >> (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE; > >> > >> if (port->rx_buf && (old_rx_fifo_depth != port->rx_fifo_depth) && port->rx_fifo_depth) { > >> - port->rx_buf = devm_krealloc(uport->dev, port->rx_buf, > >> - port->rx_fifo_depth * sizeof(u32), > >> - GFP_KERNEL); > >> + port->rx_buf = devm_krealloc_array(uport->dev, port->rx_buf, > >> + port->rx_fifo_depth, sizeof(u32), > >> + GFP_KERNEL); > >> if (!port->rx_buf) > >> return -ENOMEM; > >> } > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-03-18 17:19 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-03-09 15:03 [PATCH v2 0/4] devres: Provide krealloc_array James Clark 2023-03-09 15:03 ` James Clark 2023-03-09 15:03 ` [PATCH v2 1/4] " James Clark 2023-03-09 15:03 ` James Clark 2023-03-11 19:02 ` Jonathan Cameron 2023-03-11 19:02 ` Jonathan Cameron 2023-03-09 15:03 ` [PATCH v2 2/4] hwmon: pmbus: Use devm_krealloc_array James Clark 2023-03-09 15:03 ` James Clark 2023-03-11 19:06 ` Jonathan Cameron 2023-03-11 19:06 ` Jonathan Cameron 2023-03-09 15:03 ` [PATCH v2 3/4] iio: adc: " James Clark 2023-03-09 15:03 ` James Clark 2023-03-11 19:05 ` Jonathan Cameron 2023-03-11 19:05 ` Jonathan Cameron 2023-03-09 15:03 ` [PATCH v2 4/4] serial: qcom_geni: " James Clark 2023-03-09 15:03 ` James Clark 2023-03-11 19:18 ` Jonathan Cameron 2023-03-11 19:18 ` Jonathan Cameron 2023-03-17 11:34 ` James Clark 2023-03-17 11:34 ` James Clark 2023-03-18 17:34 ` Jonathan Cameron [this message] 2023-03-18 17:34 ` Jonathan Cameron 2023-03-20 10:03 ` James Clark 2023-03-20 10:03 ` James Clark
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230318173402.20a4f60d@jic23-huawei \ --to=jic23@kernel.org \ --cc=agross@kernel.org \ --cc=anand.ashok.dumbre@xilinx.com \ --cc=andersson@kernel.org \ --cc=corbet@lwn.net \ --cc=gregkh@linuxfoundation.org \ --cc=james.clark@arm.com \ --cc=jdelvare@suse.com \ --cc=jirislaby@kernel.org \ --cc=konrad.dybcio@linaro.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=lars@metafoo.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-hwmon@vger.kernel.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=michal.simek@amd.com \ --cc=michal.simek@xilinx.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.