| Message ID | 20220307180116.8388-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: Fix invalid sgs value | expand |
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
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
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.
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
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
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
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
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;