iommu/mediatek: Remove a unnecessary checking for larbid

[PATCH] iommu/mediatek: Remove a unnecessary checking for larbid

Posted by Yong Wu 1 year, 2 months ago

Fix a coverity issue:

>>assignment: Assigning: larbid = (fwspec->ids[0] >> 5) & 0x1fU.
	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>between: At condition larbid >= 32U, the value of larbid must be between
>>0 and 31.
>>dead_error_condition: The condition larbid >= 32U cannot be true.
	if (larbid >= MTK_LARB_NR_MAX)
>>CID 11306470 (#1 of 1): Logically dead code (DEADCODE)
>>dead_error_line: Execution cannot reach this statement:
>>return ERR_PTR(-22L);
        	return ERR_PTR(-EINVAL);

The checking "if (larbid >= MTK_LARB_NR_MAX)" is unnecessary.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
Rebase on v6.4-rc1.
---
 drivers/iommu/mtk_iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index aecc7d154f28..67caa90b481b 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -838,9 +838,6 @@ static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
 	 * All the ports in each a device should be in the same larbs.
 	 */
 	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
-	if (larbid >= MTK_LARB_NR_MAX)
-		return ERR_PTR(-EINVAL);
-
 	for (i = 1; i < fwspec->num_ids; i++) {
 		larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
 		if (larbid != larbidx) {
-- 
2.25.1

Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid

Posted by AngeloGioacchino Del Regno 1 year, 2 months ago

Il 04/07/23 13:56, Yong Wu ha scritto:
> Fix a coverity issue:
> 
>>> assignment: Assigning: larbid = (fwspec->ids[0] >> 5) & 0x1fU.
> 	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>> between: At condition larbid >= 32U, the value of larbid must be between
>>> 0 and 31.
>>> dead_error_condition: The condition larbid >= 32U cannot be true.
> 	if (larbid >= MTK_LARB_NR_MAX)
>>> CID 11306470 (#1 of 1): Logically dead code (DEADCODE)
>>> dead_error_line: Execution cannot reach this statement:
>>> return ERR_PTR(-22L);
>          	return ERR_PTR(-EINVAL);
> 
> The checking "if (larbid >= MTK_LARB_NR_MAX)" is unnecessary.
> 

I agree with the coverity tool in that after the transformation (going through
the definition of MTK_M4U_TO_LARB) the check is pointless, but I think that the
right fix here is to check for validity of fwspec->ids[0] instead of simply
removing validation.

Having no validation after mtk_iommu_probe_device() is fine, but that's
because we assume that *this* function performs all validation steps.

Regards,
Angelo

> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> Rebase on v6.4-rc1.
> ---
>   drivers/iommu/mtk_iommu.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index aecc7d154f28..67caa90b481b 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -838,9 +838,6 @@ static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
>   	 * All the ports in each a device should be in the same larbs.
>   	 */
>   	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> -	if (larbid >= MTK_LARB_NR_MAX)
> -		return ERR_PTR(-EINVAL);
> -
>   	for (i = 1; i < fwspec->num_ids; i++) {
>   		larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
>   		if (larbid != larbidx) {

Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid

Posted by Yong Wu (吴勇) 1 year, 2 months ago

On Tue, 2023-07-04 at 14:19 +0200, AngeloGioacchino Del Regno wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Il 04/07/23 13:56, Yong Wu ha scritto:
> > Fix a coverity issue:
> > 
> >>> assignment: Assigning: larbid = (fwspec->ids[0] >> 5) & 0x1fU.
> > larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> >>> between: At condition larbid >= 32U, the value of larbid must be
> between
> >>> 0 and 31.
> >>> dead_error_condition: The condition larbid >= 32U cannot be true.
> > if (larbid >= MTK_LARB_NR_MAX)
> >>> CID 11306470 (#1 of 1): Logically dead code (DEADCODE)
> >>> dead_error_line: Execution cannot reach this statement:
> >>> return ERR_PTR(-22L);
> >          return ERR_PTR(-EINVAL);
> > 
> > The checking "if (larbid >= MTK_LARB_NR_MAX)" is unnecessary.
> > 
> 
> I agree with the coverity tool in that after the transformation
> (going through
> the definition of MTK_M4U_TO_LARB) the check is pointless, but I
> think that the
> right fix here is to check for validity of fwspec->ids[0] instead of
> simply
> removing validation.
> 
> Having no validation after mtk_iommu_probe_device() is fine, but
> that's
> because we assume that *this* function performs all validation steps.

There already is validation code at the point later in this function.

"if (!larbdev) return ERR_PTR(-EINVAL);" //if the larbid is invalid.

This patch just removes a deadcode.

> 
> Regards,
> Angelo
> 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > Rebase on v6.4-rc1.
> > ---
> >   drivers/iommu/mtk_iommu.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index aecc7d154f28..67caa90b481b 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -838,9 +838,6 @@ static struct iommu_device
> *mtk_iommu_probe_device(struct device *dev)
> >    * All the ports in each a device should be in the same larbs.
> >    */
> >   larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> > -if (larbid >= MTK_LARB_NR_MAX)
> > -return ERR_PTR(-EINVAL);
> > -
> >   for (i = 1; i < fwspec->num_ids; i++) {
> >   larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
> >   if (larbid != larbidx) {
> 
>

Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid

Posted by Robin Murphy 1 year, 2 months ago

On 2023-07-05 07:49, Yong Wu (吴勇) wrote:
> On Tue, 2023-07-04 at 14:19 +0200, AngeloGioacchino Del Regno wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   Il 04/07/23 13:56, Yong Wu ha scritto:
>>> Fix a coverity issue:
>>>
>>>>> assignment: Assigning: larbid = (fwspec->ids[0] >> 5) & 0x1fU.
>>> larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>>>> between: At condition larbid >= 32U, the value of larbid must be
>> between
>>>>> 0 and 31.
>>>>> dead_error_condition: The condition larbid >= 32U cannot be true.
>>> if (larbid >= MTK_LARB_NR_MAX)
>>>>> CID 11306470 (#1 of 1): Logically dead code (DEADCODE)
>>>>> dead_error_line: Execution cannot reach this statement:
>>>>> return ERR_PTR(-22L);
>>>           return ERR_PTR(-EINVAL);
>>>
>>> The checking "if (larbid >= MTK_LARB_NR_MAX)" is unnecessary.
>>>
>>
>> I agree with the coverity tool in that after the transformation
>> (going through
>> the definition of MTK_M4U_TO_LARB) the check is pointless, but I
>> think that the
>> right fix here is to check for validity of fwspec->ids[0] instead of
>> simply
>> removing validation.
>>
>> Having no validation after mtk_iommu_probe_device() is fine, but
>> that's
>> because we assume that *this* function performs all validation steps.
> 
> There already is validation code at the point later in this function.
> 
> "if (!larbdev) return ERR_PTR(-EINVAL);" //if the larbid is invalid.
> 
> This patch just removes a deadcode.

Right, if the fwspec value was out of range then the truncated value 
might happen to map to a valid LARB, but then the fwspec could equally 
have an in-range value for a valid (but incorrect) LARB; in general a 
driver can't validate the overall correctness of data from the DT (and 
if it could, that data wouldn't need to be in the DT anyway).

 From the history, the intent of this check doesn't appear to have been 
anything other than protecting the dereference of the data->larb_imu 
array, and it's never had any functional effect, so we don't lose 
anything by removing it. FWIW,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Cheers,
Robin.

> 
>>
>> Regards,
>> Angelo
>>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>> ---
>>> Rebase on v6.4-rc1.
>>> ---
>>>    drivers/iommu/mtk_iommu.c | 3 ---
>>>    1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index aecc7d154f28..67caa90b481b 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -838,9 +838,6 @@ static struct iommu_device
>> *mtk_iommu_probe_device(struct device *dev)
>>>     * All the ports in each a device should be in the same larbs.
>>>     */
>>>    larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>> -if (larbid >= MTK_LARB_NR_MAX)
>>> -return ERR_PTR(-EINVAL);
>>> -
>>>    for (i = 1; i < fwspec->num_ids; i++) {
>>>    larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
>>>    if (larbid != larbidx) {
>>
>>

Re: [PATCH] iommu/mediatek: Remove a unnecessary checking for larbid

Posted by AngeloGioacchino Del Regno 1 year, 2 months ago

Il 05/07/23 10:26, Robin Murphy ha scritto:
> On 2023-07-05 07:49, Yong Wu (吴勇) wrote:
>> On Tue, 2023-07-04 at 14:19 +0200, AngeloGioacchino Del Regno wrote:
>>>
>>> External email : Please do not click links or open attachments until
>>> you have verified the sender or the content.
>>>   Il 04/07/23 13:56, Yong Wu ha scritto:
>>>> Fix a coverity issue:
>>>>
>>>>>> assignment: Assigning: larbid = (fwspec->ids[0] >> 5) & 0x1fU.
>>>> larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>>>>> between: At condition larbid >= 32U, the value of larbid must be
>>> between
>>>>>> 0 and 31.
>>>>>> dead_error_condition: The condition larbid >= 32U cannot be true.
>>>> if (larbid >= MTK_LARB_NR_MAX)
>>>>>> CID 11306470 (#1 of 1): Logically dead code (DEADCODE)
>>>>>> dead_error_line: Execution cannot reach this statement:
>>>>>> return ERR_PTR(-22L);
>>>>           return ERR_PTR(-EINVAL);
>>>>
>>>> The checking "if (larbid >= MTK_LARB_NR_MAX)" is unnecessary.
>>>>
>>>
>>> I agree with the coverity tool in that after the transformation
>>> (going through
>>> the definition of MTK_M4U_TO_LARB) the check is pointless, but I
>>> think that the
>>> right fix here is to check for validity of fwspec->ids[0] instead of
>>> simply
>>> removing validation.
>>>
>>> Having no validation after mtk_iommu_probe_device() is fine, but
>>> that's
>>> because we assume that *this* function performs all validation steps.
>>
>> There already is validation code at the point later in this function.
>>
>> "if (!larbdev) return ERR_PTR(-EINVAL);" //if the larbid is invalid.
>>
>> This patch just removes a deadcode.
> 
> Right, if the fwspec value was out of range then the truncated value might happen 
> to map to a valid LARB, but then the fwspec could equally have an in-range value 
> for a valid (but incorrect) LARB; in general a driver can't validate the overall 
> correctness of data from the DT (and if it could, that data wouldn't need to be in 
> the DT anyway).
> 
>  From the history, the intent of this check doesn't appear to have been anything 
> other than protecting the dereference of the data->larb_imu array, and it's never 
> had any functional effect, so we don't lose anything by removing it. FWIW,
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 

Yong, you're right. My bad for not noticing the later validation code.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> Cheers,
> Robin.
> 
>>
>>>
>>> Regards,
>>> Angelo
>>>
>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>> ---
>>>> Rebase on v6.4-rc1.
>>>> ---
>>>>    drivers/iommu/mtk_iommu.c | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>>> index aecc7d154f28..67caa90b481b 100644
>>>> --- a/drivers/iommu/mtk_iommu.c
>>>> +++ b/drivers/iommu/mtk_iommu.c
>>>> @@ -838,9 +838,6 @@ static struct iommu_device
>>> *mtk_iommu_probe_device(struct device *dev)
>>>>     * All the ports in each a device should be in the same larbs.
>>>>     */
>>>>    larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>>>> -if (larbid >= MTK_LARB_NR_MAX)
>>>> -return ERR_PTR(-EINVAL);
>>>> -
>>>>    for (i = 1; i < fwspec->num_ids; i++) {
>>>>    larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
>>>>    if (larbid != larbidx) {
>>>
>>>