IS_ERR_VALUE() assumes that parameter is an unsigned long.
It can not be used to check if 'unsigned int' is passed insted.
Which tends to reflect an error.
In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
value is zero extended to 64 bits.
Now Problem In UCC fast protocols -: drivers/soc/fsl/qe/ucc_fast.c
/* 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)) {
printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n",
__func__);
uccf->ucc_fast_tx_virtual_fifo_base_offset = 0;
ucc_fast_free(uccf);
return -ENOMEM;
}
/* Allocate memory for Rx Virtual Fifo */
uccf->ucc_fast_rx_virtual_fifo_base_offset =
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)) {
printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n",
__func__);
uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;
ucc_fast_free(uccf);
return -ENOMEM;
}
qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long.
Return value store in a u32 (ucc_fast_tx_virtual_fifo_base_offset
and ucc_fast_rx_virtual_fifo_base_offset).If qe_muram_alloc will
return any error, Then IS_ERR_VALUE will always return 0. it'll not
call ucc_fast_free for any failure. Inside 'if code' will be a dead
code on 64bit.
This patch is to avoid this problem on 64bit machine.
Signed-off-by: Arvind Yadav <[email protected]>
---
include/soc/fsl/qe/ucc_fast.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/soc/fsl/qe/ucc_fast.h b/include/soc/fsl/qe/ucc_fast.h
index df8ea79..ada9070 100644
--- a/include/soc/fsl/qe/ucc_fast.h
+++ b/include/soc/fsl/qe/ucc_fast.h
@@ -165,10 +165,12 @@ struct ucc_fast_private {
int stopped_tx; /* Whether channel has been stopped for Tx
(STOP_TX, etc.) */
int stopped_rx; /* Whether channel has been stopped for Rx */
- u32 ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of Tx
- virtual fifo */
- u32 ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of Rx
- virtual fifo */
+ unsigned long ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of
+ * Tx virtual fifo
+ */
+ unsigned long ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of
+ * Rx virtual fifo
+ */
#ifdef STATISTICS
u32 tx_frames; /* Transmitted frames counter. */
u32 rx_frames; /* Received frames counter (only frames
--
1.9.1
On Friday 05 August 2016 02:01 AM, Arnd Bergmann wrote:
> On Thursday, August 4, 2016 10:22:43 PM CEST Arvind Yadav wrote:
>> index df8ea79..ada9070 100644
>> --- a/include/soc/fsl/qe/ucc_fast.h
>> +++ b/include/soc/fsl/qe/ucc_fast.h
>> @@ -165,10 +165,12 @@ struct ucc_fast_private {
>> int stopped_tx; /* Whether channel has been stopped for Tx
>> (STOP_TX, etc.) */
>> int stopped_rx; /* Whether channel has been stopped for Rx */
>> - u32 ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of Tx
>> - virtual fifo */
>> - u32 ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of Rx
>> - virtual fifo */
>> + unsigned long ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of
>> + * Tx virtual fifo
>> + */
>> + unsigned long ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of
>> + * Rx virtual fifo
>> + */
>> #ifdef STATISTICS
>> u32 tx_frames; /* Transmitted frames counter. */
>> u32 rx_frames; /* Received frames counter (only frames
>>
> This change seems ok, but what about the other u32 variables in ucc_geth.c
> that get checked for IS_ERR_VALUE?
>
> Arnd
> I have send separate patch for ucc_geth ans ucc_slow.
> -Arvind
On Thursday, August 4, 2016 10:22:43 PM CEST Arvind Yadav wrote:
> index df8ea79..ada9070 100644
> --- a/include/soc/fsl/qe/ucc_fast.h
> +++ b/include/soc/fsl/qe/ucc_fast.h
> @@ -165,10 +165,12 @@ struct ucc_fast_private {
> int stopped_tx; /* Whether channel has been stopped for Tx
> (STOP_TX, etc.) */
> int stopped_rx; /* Whether channel has been stopped for Rx */
> - u32 ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of Tx
> - virtual fifo */
> - u32 ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of Rx
> - virtual fifo */
> + unsigned long ucc_fast_tx_virtual_fifo_base_offset;/* pointer to base of
> + * Tx virtual fifo
> + */
> + unsigned long ucc_fast_rx_virtual_fifo_base_offset;/* pointer to base of
> + * Rx virtual fifo
> + */
> #ifdef STATISTICS
> u32 tx_frames; /* Transmitted frames counter. */
> u32 rx_frames; /* Received frames counter (only frames
>
This change seems ok, but what about the other u32 variables in ucc_geth.c
that get checked for IS_ERR_VALUE?
Arnd
From: Arvind Yadav
> Sent: 04 August 2016 17:53
> IS_ERR_VALUE() assumes that parameter is an unsigned long.
> It can not be used to check if 'unsigned int' is passed insted.
> Which tends to reflect an error.
> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
> value is zero extended to 64 bits.
>
> Now Problem In UCC fast protocols -: drivers/soc/fsl/qe/ucc_fast.c
>
> /* 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)) {
> printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n",
> __func__);
> uccf->ucc_fast_tx_virtual_fifo_base_offset = 0;
> ucc_fast_free(uccf);
> return -ENOMEM;
> }
>
> /* Allocate memory for Rx Virtual Fifo */
> uccf->ucc_fast_rx_virtual_fifo_base_offset =
> 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)) {
> printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n",
> __func__);
> uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;
> ucc_fast_free(uccf);
> return -ENOMEM;
> }
>
> qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long.
> Return value store in a u32 (ucc_fast_tx_virtual_fifo_base_offset
> and ucc_fast_rx_virtual_fifo_base_offset).If qe_muram_alloc will
> return any error, Then IS_ERR_VALUE will always return 0. it'll not
> call ucc_fast_free for any failure. Inside 'if code' will be a dead
> code on 64bit.
> This patch is to avoid this problem on 64bit machine.
That is really far too wordy for a commit message.
My suspicion is that qe_muram_alloc() always returns a value that is much
less than 2^32 - even though the return type is 'long'.
Looking further all this code is a bag of worms.
The 'fail' return value from qe_muram_alloc() (aka cpm_muram_alloc()) is
never returned to an outer level.
It might be better to return a constant CPM_MURAL_ALLOC_FAIL (say 0x7fffffff)
and have the callers check that (via a #define).
That is only the start of the problems...
It looks very likely that cpm_muram_free() will be called in tidy up paths
when cpm_muram_alloc() either failed, or hasn't been called.
Since 0 is a valid return value, and there is no check for -ENOMEM it is
all an 'accident waiting to happen'.
>From my quick scan (grep -B2 -A6) I'm not at all sure most of the error paths
at best leak memory.
David