[PATCH] spi: Fix invalid sgs value

[PATCH] spi: Fix invalid sgs value

[Biju Das]

max_seg_size is unsigned int and it can have a value up to 2^32
(for eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX)
When this value is used in min_t() as an integer type, it becomes
-1 and the value of sgs becomes 0.

Fix this issue by replacing the 'int' data type with 'unsigned int'
in min_t().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4599b121d744..17dcc15379f2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1019,10 +1019,10 @@ int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
 	int i, ret;
 
 	if (vmalloced_buf || kmap_buf) {
-		desc_len = min_t(int, max_seg_size, PAGE_SIZE);
+		desc_len = min_t(unsigned int, max_seg_size, PAGE_SIZE);
 		sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
 	} else if (virt_addr_valid(buf)) {
-		desc_len = min_t(int, max_seg_size, ctlr->max_dma_len);
+		desc_len = min_t(unsigned int, max_seg_size, (unsigned int)ctlr->max_dma_len);
 		sgs = DIV_ROUND_UP(len, desc_len);
 	} else {
 		return -EINVAL;
-- 
2.17.1

Re: [PATCH] spi: Fix invalid sgs value

[Geert Uytterhoeven]

Hi Biju,

On Mon, Mar 7, 2022 at 7:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> max_seg_size is unsigned int and it can have a value up to 2^32
> (for eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX)
> When this value is used in min_t() as an integer type, it becomes
> -1 and the value of sgs becomes 0.
>
> Fix this issue by replacing the 'int' data type with 'unsigned int'
> in min_t().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1019,10 +1019,10 @@ int spi_map_buf(struct spi_controller *ctlr, struct device *dev,
>         int i, ret;
>
>         if (vmalloced_buf || kmap_buf) {
> -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> +               desc_len = min_t(unsigned int, max_seg_size, PAGE_SIZE);
>                 sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
>         } else if (virt_addr_valid(buf)) {
> -               desc_len = min_t(int, max_seg_size, ctlr->max_dma_len);
> +               desc_len = min_t(unsigned int, max_seg_size, (unsigned int)ctlr->max_dma_len);

The cast of the last parameter is not needed.

>                 sgs = DIV_ROUND_UP(len, desc_len);
>         } else {
>                 return -EINVAL;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] spi: Fix invalid sgs value

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH] spi: Fix invalid sgs value
> 
> Hi Biju,
> 
> On Mon, Mar 7, 2022 at 7:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > max_seg_size is unsigned int and it can have a value up to 2^32 (for
> > eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX) When this
> > value is used in min_t() as an integer type, it becomes
> > -1 and the value of sgs becomes 0.
> >
> > Fix this issue by replacing the 'int' data type with 'unsigned int'
> > in min_t().
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1019,10 +1019,10 @@ int spi_map_buf(struct spi_controller *ctlr,
> struct device *dev,
> >         int i, ret;
> >
> >         if (vmalloced_buf || kmap_buf) {
> > -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> > +               desc_len = min_t(unsigned int, max_seg_size,
> > + PAGE_SIZE);
> >                 sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
> >         } else if (virt_addr_valid(buf)) {
> > -               desc_len = min_t(int, max_seg_size, ctlr->max_dma_len);
> > +               desc_len = min_t(unsigned int, max_seg_size, (unsigned
> > + int)ctlr->max_dma_len);
> 
> The cast of the last parameter is not needed.

OK. I thought since last param is size_t, casting is required.
OK will drop this.

Cheers,
Biju

> 
> >                 sgs = DIV_ROUND_UP(len, desc_len);
> >         } else {
> >                 return -EINVAL;
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

Re: [PATCH] spi: Fix invalid sgs value

[Mark Brown]

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

On Mon, Mar 07, 2022 at 06:17:23PM +0000, Biju Das wrote:

> > >         if (vmalloced_buf || kmap_buf) {
> > > -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > + PAGE_SIZE);
> > >                 sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
> > >         } else if (virt_addr_valid(buf)) {
> > > -               desc_len = min_t(int, max_seg_size, ctlr->max_dma_len);
> > > +               desc_len = min_t(unsigned int, max_seg_size, (unsigned
> > > + int)ctlr->max_dma_len);

> > The cast of the last parameter is not needed.

> OK. I thought since last param is size_t, casting is required.
> OK will drop this.

In general unless you're getting a warning and are *very* clear on why
what's being done is valid casts should be avoided.

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

RE: [PATCH] spi: Fix invalid sgs value

[Biju Das]

Hi Mark,

> Subject: Re: [PATCH] spi: Fix invalid sgs value
> 
> On Mon, Mar 07, 2022 at 06:17:23PM +0000, Biju Das wrote:
> 
> > > >         if (vmalloced_buf || kmap_buf) {
> > > > -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> > > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > > + PAGE_SIZE);
> > > >                 sgs = DIV_ROUND_UP(len + offset_in_page(buf),
> desc_len);
> > > >         } else if (virt_addr_valid(buf)) {
> > > > -               desc_len = min_t(int, max_seg_size, ctlr-
> >max_dma_len);
> > > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > > + (unsigned int)ctlr->max_dma_len);
> 
> > > The cast of the last parameter is not needed.
> 
> > OK. I thought since last param is size_t, casting is required.
> > OK will drop this.
> 
> In general unless you're getting a warning and are *very* clear on why
> what's being done is valid casts should be avoided.

Agreed.

Cheers,
Biju

Re: [PATCH] spi: Fix invalid sgs value

[Geert Uytterhoeven]

Hi Biju,

On Mon, Mar 7, 2022 at 7:17 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] spi: Fix invalid sgs value
> > On Mon, Mar 7, 2022 at 7:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > max_seg_size is unsigned int and it can have a value up to 2^32 (for
> > > eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX) When this
> > > value is used in min_t() as an integer type, it becomes
> > > -1 and the value of sgs becomes 0.
> > >
> > > Fix this issue by replacing the 'int' data type with 'unsigned int'
> > > in min_t().
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -1019,10 +1019,10 @@ int spi_map_buf(struct spi_controller *ctlr,
> > struct device *dev,
> > >         int i, ret;
> > >
> > >         if (vmalloced_buf || kmap_buf) {
> > > -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > + PAGE_SIZE);
> > >                 sgs = DIV_ROUND_UP(len + offset_in_page(buf), desc_len);
> > >         } else if (virt_addr_valid(buf)) {
> > > -               desc_len = min_t(int, max_seg_size, ctlr->max_dma_len);
> > > +               desc_len = min_t(unsigned int, max_seg_size, (unsigned
> > > + int)ctlr->max_dma_len);
> >
> > The cast of the last parameter is not needed.
>
> OK. I thought since last param is size_t, casting is required.

That's exactly what min_t() does, but using an easier-to-grep-for
notation:
"min_t(type, a, b)" is equivalent to "min((type)a, (type)b)".

> OK will drop this.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] spi: Fix invalid sgs value

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH] spi: Fix invalid sgs value
> 
> Hi Biju,
> 
> On Mon, Mar 7, 2022 at 7:17 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] spi: Fix invalid sgs value On Mon, Mar 7, 2022
> > > at 7:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > max_seg_size is unsigned int and it can have a value up to 2^32
> > > > (for eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX) When
> > > > this value is used in min_t() as an integer type, it becomes
> > > > -1 and the value of sgs becomes 0.
> > > >
> > > > Fix this issue by replacing the 'int' data type with 'unsigned int'
> > > > in min_t().
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/spi/spi.c
> > > > +++ b/drivers/spi/spi.c
> > > > @@ -1019,10 +1019,10 @@ int spi_map_buf(struct spi_controller
> > > > *ctlr,
> > > struct device *dev,
> > > >         int i, ret;
> > > >
> > > >         if (vmalloced_buf || kmap_buf) {
> > > > -               desc_len = min_t(int, max_seg_size, PAGE_SIZE);
> > > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > > + PAGE_SIZE);
> > > >                 sgs = DIV_ROUND_UP(len + offset_in_page(buf),
> desc_len);
> > > >         } else if (virt_addr_valid(buf)) {
> > > > -               desc_len = min_t(int, max_seg_size, ctlr-
> >max_dma_len);
> > > > +               desc_len = min_t(unsigned int, max_seg_size,
> > > > + (unsigned int)ctlr->max_dma_len);
> > >
> > > The cast of the last parameter is not needed.
> >
> > OK. I thought since last param is size_t, casting is required.
> 
> That's exactly what min_t() does, but using an easier-to-grep-for
> notation:
> "min_t(type, a, b)" is equivalent to "min((type)a, (type)b)".

Thanks for clarification.

Cheers,
Biju

Re: [PATCH] spi: Fix invalid sgs value

[Mark Brown]

On Mon, 7 Mar 2022 18:01:16 +0000, Biju Das wrote:
> max_seg_size is unsigned int and it can have a value up to 2^32
> (for eg:-RZ_DMAC driver sets dma_set_max_seg_size as U32_MAX)
> When this value is used in min_t() as an integer type, it becomes
> -1 and the value of sgs becomes 0.
> 
> Fix this issue by replacing the 'int' data type with 'unsigned int'
> in min_t().
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-linus

Thanks!

[1/1] spi: Fix invalid sgs value
      commit: 1a4e53d2fc4f68aa654ad96d13ad042e1a8e8a7d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark