* [PATCH] Remove lots of IS_ERR_VALUE abuses and compilation warning.
@ 2016-07-07 15:27 Arvind Yadav
2016-07-07 15:51 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Arvind Yadav @ 2016-07-07 15:27 UTC (permalink / raw)
To: scottwood; +Cc: qiang.zhao, linuxppc-dev, linux-kernel, Arvind Yadav
Passing value in IS_ERR_VALUE() is wrong, as they
pass an 'int' into a function that takes an 'unsigned long'
argument. This happens to work because the type is sign-extended
on 64-bit architectures before it gets converted into an
unsigned type.
Passing an 'unsigned short' or 'unsigned int'
argument into IS_ERR_VALUE() is guaranteed to be broken, as are
8-bit integers and types that are wider than 'unsigned long'.
Any user will get compilation warning for that do not pass an
'unsigned long' argument.
Commit '287980e49f; - This change is alreday fixes lots of other
worst abusers.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/soc/fsl/qe/ucc_fast.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/qe/ucc_fast.c b/drivers/soc/fsl/qe/ucc_fast.c
index a768931..7cc783c 100644
--- a/drivers/soc/fsl/qe/ucc_fast.c
+++ b/drivers/soc/fsl/qe/ucc_fast.c
@@ -268,7 +268,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
/* Allocate memory for Tx Virtual Fifo */
uccf->ucc_fast_tx_virtual_fifo_base_offset =
qe_muram_alloc(uf_info->utfs, UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
- if (IS_ERR_VALUE(uccf->ucc_fast_tx_virtual_fifo_base_offset)) {
+ if (uccf->ucc_fast_tx_virtual_fifo_base_offset < 0) {
printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n",
__func__);
uccf->ucc_fast_tx_virtual_fifo_base_offset = 0;
@@ -281,7 +281,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
qe_muram_alloc(uf_info->urfs +
UCC_FAST_RECEIVE_VIRTUAL_FIFO_SIZE_FUDGE_FACTOR,
UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
- if (IS_ERR_VALUE(uccf->ucc_fast_rx_virtual_fifo_base_offset)) {
+ if (uccf->ucc_fast_rx_virtual_fifo_base_offset < 0) {
printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n",
__func__);
uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: Remove lots of IS_ERR_VALUE abuses and compilation warning.
2016-07-07 15:27 [PATCH] Remove lots of IS_ERR_VALUE abuses and compilation warning Arvind Yadav
@ 2016-07-07 15:51 ` Guenter Roeck
2016-07-07 17:06 ` arvind Yadav
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2016-07-07 15:51 UTC (permalink / raw)
To: Arvind Yadav; +Cc: scottwood, qiang.zhao, linuxppc-dev, linux-kernel
On Thu, Jul 07, 2016 at 08:57:29PM +0530, Arvind Yadav wrote:
> Passing value in IS_ERR_VALUE() is wrong, as they
> pass an 'int' into a function that takes an 'unsigned long'
> argument. This happens to work because the type is sign-extended
> on 64-bit architectures before it gets converted into an
> unsigned type.
>
> Passing an 'unsigned short' or 'unsigned int'
> argument into IS_ERR_VALUE() is guaranteed to be broken, as are
> 8-bit integers and types that are wider than 'unsigned long'.
>
> Any user will get compilation warning for that do not pass an
> 'unsigned long' argument.
>
> Commit '287980e49f; - This change is alreday fixes lots of other
> worst abusers.
>
Couple of generic comments:
- Your patch subject lines don't include the affected drivers/modules.
As such, most of them will be ignored because maintainers won't realize
that you are talking with them. Some may ask you to resubmit with proper
subject lines.
Commit 287980e49f is different; it addresses the problem in several
drivers in a single commit.
- If you patch a single file, I think it would be better to adjust the
description accordingly. In this patch, the offending variable type is
u32. The patch description is therefore misleading; the code here simply does
not work.
- When you resubmit a patch, you don't include a version, not a change log.
This means additional work for maintainers, who have to figure out which
patch to apply.
Specific comment:
The allocator in question returns -ENOMEM in an unsigned long. This is assigned
to u32. A proper fix would be to assign the return value to an unsigned
long and to use IS_ER_VALUE() to check if it reports an error, and to only
assign it to ucc_fast_rx_virtual_fifo_base_offset if there was no error.
Also, unless I am missing something - since ucc_fast_rx_virtual_fifo_base_offset
is defined as u32, it is somewhat unlikely that it is ever < 0.
Guenter
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> drivers/soc/fsl/qe/ucc_fast.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/ucc_fast.c b/drivers/soc/fsl/qe/ucc_fast.c
> index a768931..7cc783c 100644
> --- a/drivers/soc/fsl/qe/ucc_fast.c
> +++ b/drivers/soc/fsl/qe/ucc_fast.c
> @@ -268,7 +268,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
> /* Allocate memory for Tx Virtual Fifo */
> uccf->ucc_fast_tx_virtual_fifo_base_offset =
> qe_muram_alloc(uf_info->utfs, UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
> - if (IS_ERR_VALUE(uccf->ucc_fast_tx_virtual_fifo_base_offset)) {
> + if (uccf->ucc_fast_tx_virtual_fifo_base_offset < 0) {
> printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n",
> __func__);
> uccf->ucc_fast_tx_virtual_fifo_base_offset = 0;
> @@ -281,7 +281,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
> qe_muram_alloc(uf_info->urfs +
> UCC_FAST_RECEIVE_VIRTUAL_FIFO_SIZE_FUDGE_FACTOR,
> UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
> - if (IS_ERR_VALUE(uccf->ucc_fast_rx_virtual_fifo_base_offset)) {
> + if (uccf->ucc_fast_rx_virtual_fifo_base_offset < 0) {
> printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n",
> __func__);
> uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Remove lots of IS_ERR_VALUE abuses and compilation warning.
2016-07-07 15:51 ` Guenter Roeck
@ 2016-07-07 17:06 ` arvind Yadav
0 siblings, 0 replies; 3+ messages in thread
From: arvind Yadav @ 2016-07-07 17:06 UTC (permalink / raw)
To: Guenter Roeck; +Cc: scottwood, qiang.zhao, linuxppc-dev, linux-kernel
Yes, You are right,
-Now Return type of 'qe_muram_alloc' is 'unsigned long', That Was trying
to assigned in ucc_fast_tx_virtual_fifo_base_offset and
ucc_fast_rx_virtual_fifo_base_offset. These variable are 'unsigned int'.
So before assignment need a proper type casting.
-Passing value in IS_ERR_VALUE() is wrong. So this is also need a proper
type casting before passing an argument.
I have done the changes and re-submitted anther patch, Please review It.
Thanks,
Arvind
On Thursday 07 July 2016 09:21 PM, Guenter Roeck wrote:
> On Thu, Jul 07, 2016 at 08:57:29PM +0530, Arvind Yadav wrote:
>> Passing value in IS_ERR_VALUE() is wrong, as they
>> pass an 'int' into a function that takes an 'unsigned long'
>> argument. This happens to work because the type is sign-extended
>> on 64-bit architectures before it gets converted into an
>> unsigned type.
>>
>> Passing an 'unsigned short' or 'unsigned int'
>> argument into IS_ERR_VALUE() is guaranteed to be broken, as are
>> 8-bit integers and types that are wider than 'unsigned long'.
>>
>> Any user will get compilation warning for that do not pass an
>> 'unsigned long' argument.
>>
>> Commit '287980e49f; - This change is alreday fixes lots of other
>> worst abusers.
>>
> Couple of generic comments:
>
> - Your patch subject lines don't include the affected drivers/modules.
> As such, most of them will be ignored because maintainers won't realize
> that you are talking with them. Some may ask you to resubmit with proper
> subject lines.
> Commit 287980e49f is different; it addresses the problem in several
> drivers in a single commit.
> - If you patch a single file, I think it would be better to adjust the
> description accordingly. In this patch, the offending variable type is
> u32. The patch description is therefore misleading; the code here simply does
> not work.
> - When you resubmit a patch, you don't include a version, not a change log.
> This means additional work for maintainers, who have to figure out which
> patch to apply.
>
> Specific comment:
>
> The allocator in question returns -ENOMEM in an unsigned long. This is assigned
> to u32. A proper fix would be to assign the return value to an unsigned
> long and to use IS_ER_VALUE() to check if it reports an error, and to only
> assign it to ucc_fast_rx_virtual_fifo_base_offset if there was no error.
>
> Also, unless I am missing something - since ucc_fast_rx_virtual_fifo_base_offset
> is defined as u32, it is somewhat unlikely that it is ever < 0.
>
> Guenter
>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>> drivers/soc/fsl/qe/ucc_fast.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/fsl/qe/ucc_fast.c b/drivers/soc/fsl/qe/ucc_fast.c
>> index a768931..7cc783c 100644
>> --- a/drivers/soc/fsl/qe/ucc_fast.c
>> +++ b/drivers/soc/fsl/qe/ucc_fast.c
>> @@ -268,7 +268,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
>> /* Allocate memory for Tx Virtual Fifo */
>> uccf->ucc_fast_tx_virtual_fifo_base_offset =
>> qe_muram_alloc(uf_info->utfs, UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
>> - if (IS_ERR_VALUE(uccf->ucc_fast_tx_virtual_fifo_base_offset)) {
>> + if (uccf->ucc_fast_tx_virtual_fifo_base_offset < 0) {
>> printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n",
>> __func__);
>> uccf->ucc_fast_tx_virtual_fifo_base_offset = 0;
>> @@ -281,7 +281,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc
>> qe_muram_alloc(uf_info->urfs +
>> UCC_FAST_RECEIVE_VIRTUAL_FIFO_SIZE_FUDGE_FACTOR,
>> UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT);
>> - if (IS_ERR_VALUE(uccf->ucc_fast_rx_virtual_fifo_base_offset)) {
>> + if (uccf->ucc_fast_rx_virtual_fifo_base_offset < 0) {
>> printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n",
>> __func__);
>> uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-07 17:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-07 15:27 [PATCH] Remove lots of IS_ERR_VALUE abuses and compilation warning Arvind Yadav
2016-07-07 15:51 ` Guenter Roeck
2016-07-07 17:06 ` arvind Yadav
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).