| Message ID | 1447353300-26027-1-git-send-email-m@bjorling.me (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jens Axboe |
| Headers | show |
On Thu, Nov 12, 2015 at 7:35 PM, Matias Bjørling <m@bjorling.me> wrote: > The max_phys_sect variable is defined as a char. We do a boundary check > to maximally allow 256 physical page descriptors per command. As we are > not indexing from zero. This expression is always in false. Bump the > max_phys_sect to an unsigned short to support the range check. unsigned int? > > Signed-off-by: Matias Bjørling <m@bjorling.me> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > include/linux/lightnvm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h > index 69c9057..4b1cd3d 100644 > --- a/include/linux/lightnvm.h > +++ b/include/linux/lightnvm.h > @@ -220,7 +220,7 @@ struct nvm_dev_ops { > nvm_dev_dma_alloc_fn *dev_dma_alloc; > nvm_dev_dma_free_fn *dev_dma_free; > > - uint8_t max_phys_sect; > + unsigned int max_phys_sect; 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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/2015 07:42 PM, Geert Uytterhoeven wrote: > On Thu, Nov 12, 2015 at 7:35 PM, Matias Bjørling <m@bjorling.me> wrote: >> The max_phys_sect variable is defined as a char. We do a boundary check >> to maximally allow 256 physical page descriptors per command. As we are >> not indexing from zero. This expression is always in false. Bump the >> max_phys_sect to an unsigned short to support the range check. > > unsigned int? Grah, I need to be more careful. I sent the wrong patch after I had fixed it to unsigned short. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 12, 2015 at 10:57 AM, Matias Bjørling <m@bjorling.me> wrote: > > Grah, I need to be more careful. I sent the wrong patch after I had fixed it > to unsigned short. Actually, I think "unsigned int" was better. You're not saving any space with "unsigned short" (the size of the structure will be rounded up to the alignment of it anyway), and we should generally strive to avoid 16-bit accesses unless there is some real reason for them, because they are often slower than either "char" or "int". Several architectures have weak support for 16-bit accesses (eg alpha), and even on x86 you end up having operand size overrides etc. So unless there is a clear *reason* to use "short" - just don't. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/12/2015 08:02 PM, Linus Torvalds wrote: > On Thu, Nov 12, 2015 at 10:57 AM, Matias Bjørling <m@bjorling.me> wrote: >> >> Grah, I need to be more careful. I sent the wrong patch after I had fixed it >> to unsigned short. > > Actually, I think "unsigned int" was better. > > You're not saving any space with "unsigned short" (the size of the > structure will be rounded up to the alignment of it anyway), and we > should generally strive to avoid 16-bit accesses unless there is some > real reason for them, because they are often slower than either "char" > or "int". Several architectures have weak support for 16-bit accesses > (eg alpha), and even on x86 you end up having operand size overrides > etc. > Thanks > So unless there is a clear *reason* to use "short" - just don't. > > Linus > -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 12, 2015 at 4:35 PM, Matias Bjørling <m@bjorling.me> wrote: > The max_phys_sect variable is defined as a char. We do a boundary check > to maximally allow 256 physical page descriptors per command. As we are > not indexing from zero. This expression is always in false. Bump the > max_phys_sect to an unsigned short to support the range check. > You might have to change the commit message to match the code change. s/unsigned short/unsigned int. RIght? > Signed-off-by: Matias Bjørling <m@bjorling.me> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > include/linux/lightnvm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h > index 69c9057..4b1cd3d 100644 > --- a/include/linux/lightnvm.h > +++ b/include/linux/lightnvm.h > @@ -220,7 +220,7 @@ struct nvm_dev_ops { > nvm_dev_dma_alloc_fn *dev_dma_alloc; > nvm_dev_dma_free_fn *dev_dma_free; > > - uint8_t max_phys_sect; > + unsigned int max_phys_sect; > }; > > struct nvm_lun {
On 11/16/2015 12:18 PM, Thiago Farina wrote: > On Thu, Nov 12, 2015 at 4:35 PM, Matias Bjørling <m@bjorling.me> wrote: >> The max_phys_sect variable is defined as a char. We do a boundary check >> to maximally allow 256 physical page descriptors per command. As we are >> not indexing from zero. This expression is always in false. Bump the >> max_phys_sect to an unsigned short to support the range check. >> > You might have to change the commit message to match the code change. > s/unsigned short/unsigned int. RIght? You're right. It has been fixed. > >> Signed-off-by: Matias Bjørling <m@bjorling.me> >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> --- >> include/linux/lightnvm.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h >> index 69c9057..4b1cd3d 100644 >> --- a/include/linux/lightnvm.h >> +++ b/include/linux/lightnvm.h >> @@ -220,7 +220,7 @@ struct nvm_dev_ops { >> nvm_dev_dma_alloc_fn *dev_dma_alloc; >> nvm_dev_dma_free_fn *dev_dma_free; >> >> - uint8_t max_phys_sect; >> + unsigned int max_phys_sect; >> }; >> >> struct nvm_lun { > > -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h index 69c9057..4b1cd3d 100644 --- a/include/linux/lightnvm.h +++ b/include/linux/lightnvm.h @@ -220,7 +220,7 @@ struct nvm_dev_ops { nvm_dev_dma_alloc_fn *dev_dma_alloc; nvm_dev_dma_free_fn *dev_dma_free; - uint8_t max_phys_sect; + unsigned int max_phys_sect; }; struct nvm_lun {
The max_phys_sect variable is defined as a char. We do a boundary check to maximally allow 256 physical page descriptors per command. As we are not indexing from zero. This expression is always in false. Bump the max_phys_sect to an unsigned short to support the range check. Signed-off-by: Matias Bjørling <m@bjorling.me> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> --- include/linux/lightnvm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)