| Message ID | 20230309150334.216760-5-james.clark@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | devres: Provide krealloc_array | expand |
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 > --- > 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; > }
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. 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; >> } >
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; > >> } > >
On 18/03/2023 17:34, Jonathan Cameron wrote: > 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). Ok that makes sense to me. I can add a comment instead this patch to change this one. >> >> 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; >>>> } >>> >
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; }
Now that it exists, use it instead of doing the multiplication manually. Signed-off-by: James Clark <james.clark@arm.com> --- drivers/tty/serial/qcom_geni_serial.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)