[PATCH] lightnvm: change max_phys_sect to ushort

[PATCH] lightnvm: change max_phys_sect to ushort

[Matias Bjørling]

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(-)

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

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Geert Uytterhoeven]

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

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Matias Bjørling]

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.

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Linus Torvalds]

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

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Matias Bjørling]

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
>

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Thiago Farina]

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 {


-- 
Thiago Farina

Re: [PATCH] lightnvm: change max_phys_sect to ushort

[Matias Bjørling]

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 {
>
>