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 <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
---
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 {
--
2.1.4
On Thu, Nov 12, 2015 at 7:35 PM, Matias Bjørling <[email protected]> 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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
> ---
> 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 -- [email protected]
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 11/12/2015 07:42 PM, Geert Uytterhoeven wrote:
> On Thu, Nov 12, 2015 at 7:35 PM, Matias Bjørling <[email protected]> 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.
On Thu, Nov 12, 2015 at 10:57 AM, Matias Bjørling <[email protected]> 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
On 11/12/2015 08:02 PM, Linus Torvalds wrote:
> On Thu, Nov 12, 2015 at 10:57 AM, Matias Bjørling <[email protected]> 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
>
On Thu, Nov 12, 2015 at 4:35 PM, Matias Bjørling <[email protected]> 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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
> ---
> 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 {
--
Thiago Farina
On 11/16/2015 12:18 PM, Thiago Farina wrote:
> On Thu, Nov 12, 2015 at 4:35 PM, Matias Bjørling <[email protected]> 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 <[email protected]>
>> Reported-by: Geert Uytterhoeven <[email protected]>
>> ---
>> 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 {
>
>