[PATCH v2] drm:sprd: Correct left shift operator evaluating constant expression

[PATCH v2] drm:sprd: Correct left shift operator evaluating constant expression

[Karan Sanghavi]

The left shift operation followed by a mask with 0xf will
always result in 0. To correctly evaluate the expression for
the bitwise OR operation, use a right shift instead.

Reported by Coverity Scan CID: 1511468

Fixes: 1c66496b1391 ("drm/sprd: add Unisoc's drm mipi dsi&dphy driver")

Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>
Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
---
Coverity Scan Message:
CID 1511468: (#1 of 1): Wrong operator used (CONSTANT_EXPRESSION_RESULT)
operator_confusion: (pll->kint << 4) & 15 is always 0 regardless of the 
values of its operands. This occurs as the bitwise second operand of "|"
---
Changes in v2:
- Added the fixes tag
- Link to v1: https://lore.kernel.org/r/20241105-coverity1511468wrongoperator-v1-1-06c7513c3efc@gmail.com
---
 drivers/gpu/drm/sprd/megacores_pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sprd/megacores_pll.c b/drivers/gpu/drm/sprd/megacores_pll.c
index 3091dfdc11e3..43c10a5fc441 100644
--- a/drivers/gpu/drm/sprd/megacores_pll.c
+++ b/drivers/gpu/drm/sprd/megacores_pll.c
@@ -94,7 +94,7 @@ static void dphy_set_pll_reg(struct dphy_pll *pll, struct regmap *regmap)
 	reg_val[3] = pll->vco_band | (pll->sdm_en << 1) | (pll->refin << 2);
 	reg_val[4] = pll->kint >> 12;
 	reg_val[5] = pll->kint >> 4;
-	reg_val[6] = pll->out_sel | ((pll->kint << 4) & 0xf);
+	reg_val[6] = pll->out_sel | ((pll->kint >> 4) & 0xf);
 	reg_val[7] = 1 << 4;
 	reg_val[8] = pll->det_delay;
 

---
base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
change-id: 20241105-coverity1511468wrongoperator-20130bcd4240

Best regards,
-- 
Karan Sanghavi <karansanghvi98@gmail.com>

Re: [PATCH v2] drm:sprd: Correct left shift operator evaluating constant expression

[Dmitry Baryshkov]

On Fri, Nov 08, 2024 at 05:11:25PM +0000, Karan Sanghavi wrote:
> The left shift operation followed by a mask with 0xf will
> always result in 0. To correctly evaluate the expression for
> the bitwise OR operation, use a right shift instead.
> 
> Reported by Coverity Scan CID: 1511468
> 
> Fixes: 1c66496b1391 ("drm/sprd: add Unisoc's drm mipi dsi&dphy driver")
> 
> Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>
> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>

Please drop the empty line between tags.

Also see Documentation/process/stable-kernel-rules.rst

> ---
> Coverity Scan Message:
> CID 1511468: (#1 of 1): Wrong operator used (CONSTANT_EXPRESSION_RESULT)
> operator_confusion: (pll->kint << 4) & 15 is always 0 regardless of the 
> values of its operands. This occurs as the bitwise second operand of "|"

Is there any kind of a public link for the report? Should there be a Closes: tag?

> ---
> Changes in v2:
> - Added the fixes tag
> - Link to v1: https://lore.kernel.org/r/20241105-coverity1511468wrongoperator-v1-1-06c7513c3efc@gmail.com
> ---
>  drivers/gpu/drm/sprd/megacores_pll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sprd/megacores_pll.c b/drivers/gpu/drm/sprd/megacores_pll.c
> index 3091dfdc11e3..43c10a5fc441 100644
> --- a/drivers/gpu/drm/sprd/megacores_pll.c
> +++ b/drivers/gpu/drm/sprd/megacores_pll.c
> @@ -94,7 +94,7 @@ static void dphy_set_pll_reg(struct dphy_pll *pll, struct regmap *regmap)
>  	reg_val[3] = pll->vco_band | (pll->sdm_en << 1) | (pll->refin << 2);
>  	reg_val[4] = pll->kint >> 12;
>  	reg_val[5] = pll->kint >> 4;
> -	reg_val[6] = pll->out_sel | ((pll->kint << 4) & 0xf);
> +	reg_val[6] = pll->out_sel | ((pll->kint >> 4) & 0xf);
>  	reg_val[7] = 1 << 4;
>  	reg_val[8] = pll->det_delay;
>  
> 
> ---
> base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
> change-id: 20241105-coverity1511468wrongoperator-20130bcd4240
> 
> Best regards,
> -- 
> Karan Sanghavi <karansanghvi98@gmail.com>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm:sprd: Correct left shift operator evaluating constant expression

[Karan Sanghavi]

On Sat, Nov 09, 2024 at 09:27:36AM +0200, Dmitry Baryshkov wrote:
> On Fri, Nov 08, 2024 at 05:11:25PM +0000, Karan Sanghavi wrote:
> > The left shift operation followed by a mask with 0xf will
> > always result in 0. To correctly evaluate the expression for
> > the bitwise OR operation, use a right shift instead.
> > 
> > Reported by Coverity Scan CID: 1511468
> > 
> > Fixes: 1c66496b1391 ("drm/sprd: add Unisoc's drm mipi dsi&dphy driver")
> > 
> > Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>
> > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
> 
> Please drop the empty line between tags.
> 
> Also see Documentation/process/stable-kernel-rules.rst
> 
Sure will go through that.
> > ---
> > Coverity Scan Message:
> > CID 1511468: (#1 of 1): Wrong operator used (CONSTANT_EXPRESSION_RESULT)
> > operator_confusion: (pll->kint << 4) & 15 is always 0 regardless of the 
> > values of its operands. This occurs as the bitwise second operand of "|"
> 
> Is there any kind of a public link for the report? Should there be a Closes: tag?
>
https://scan7.scan.coverity.com/#/project-view/51975/11354?selectedIssue=1511468

This is the link for coverity scan report. 

Should I add this link in the Closes tag? 

> > ---
> > Changes in v2:
> > - Added the fixes tag
> > - Link to v1: https://lore.kernel.org/r/20241105-coverity1511468wrongoperator-v1-1-06c7513c3efc@gmail.com
> > ---
> >  drivers/gpu/drm/sprd/megacores_pll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/sprd/megacores_pll.c b/drivers/gpu/drm/sprd/megacores_pll.c
> > index 3091dfdc11e3..43c10a5fc441 100644
> > --- a/drivers/gpu/drm/sprd/megacores_pll.c
> > +++ b/drivers/gpu/drm/sprd/megacores_pll.c
> > @@ -94,7 +94,7 @@ static void dphy_set_pll_reg(struct dphy_pll *pll, struct regmap *regmap)
> >  	reg_val[3] = pll->vco_band | (pll->sdm_en << 1) | (pll->refin << 2);
> >  	reg_val[4] = pll->kint >> 12;
> >  	reg_val[5] = pll->kint >> 4;
> > -	reg_val[6] = pll->out_sel | ((pll->kint << 4) & 0xf);
> > +	reg_val[6] = pll->out_sel | ((pll->kint >> 4) & 0xf);
> >  	reg_val[7] = 1 << 4;
> >  	reg_val[8] = pll->det_delay;
> >  
> > 
> > ---
> > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
> > change-id: 20241105-coverity1511468wrongoperator-20130bcd4240
> > 
> > Best regards,
> > -- 
> > Karan Sanghavi <karansanghvi98@gmail.com>
> > 
> 
> -- 
> With best wishes
> Dmitry
Thank you,
Karan.

Re: [PATCH v2] drm:sprd: Correct left shift operator evaluating constant expression

[Karan Sanghavi]

On Sat, Nov 09, 2024 at 09:27:36AM +0200, Dmitry Baryshkov wrote:
> On Fri, Nov 08, 2024 at 05:11:25PM +0000, Karan Sanghavi wrote:
> > The left shift operation followed by a mask with 0xf will
> > always result in 0. To correctly evaluate the expression for
> > the bitwise OR operation, use a right shift instead.
> > 
> > Reported by Coverity Scan CID: 1511468
> > 
> > Fixes: 1c66496b1391 ("drm/sprd: add Unisoc's drm mipi dsi&dphy driver")
> > 
> > Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>
> > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
> 
> Please drop the empty line between tags.
> 
> Also see Documentation/process/stable-kernel-rules.rst
> 
> > ---
> > Coverity Scan Message:
> > CID 1511468: (#1 of 1): Wrong operator used (CONSTANT_EXPRESSION_RESULT)
> > operator_confusion: (pll->kint << 4) & 15 is always 0 regardless of the 
> > values of its operands. This occurs as the bitwise second operand of "|"
> 
> Is there any kind of a public link for the report? Should there be a Closes: tag?
> 
As mentioned in the earlier mail, there is no public link to this, there is only coverity scan link which would require you to login in the portal to view the whole error reported by it. 
Here is the link 
https://scan7.scan.coverity.com/#/project-view/51975/11354?selectedIssue=1511468

> > ---
> > Changes in v2:
> > - Added the fixes tag
> > - Link to v1: https://lore.kernel.org/r/20241105-coverity1511468wrongoperator-v1-1-06c7513c3efc@gmail.com
> > ---
> >  drivers/gpu/drm/sprd/megacores_pll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/sprd/megacores_pll.c b/drivers/gpu/drm/sprd/megacores_pll.c
> > index 3091dfdc11e3..43c10a5fc441 100644
> > --- a/drivers/gpu/drm/sprd/megacores_pll.c
> > +++ b/drivers/gpu/drm/sprd/megacores_pll.c
> > @@ -94,7 +94,7 @@ static void dphy_set_pll_reg(struct dphy_pll *pll, struct regmap *regmap)
> >  	reg_val[3] = pll->vco_band | (pll->sdm_en << 1) | (pll->refin << 2);
> >  	reg_val[4] = pll->kint >> 12;
> >  	reg_val[5] = pll->kint >> 4;
> > -	reg_val[6] = pll->out_sel | ((pll->kint << 4) & 0xf);
> > +	reg_val[6] = pll->out_sel | ((pll->kint >> 4) & 0xf);
> >  	reg_val[7] = 1 << 4;
> >  	reg_val[8] = pll->det_delay;
> >  
> > 
> > ---
> > base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
> > change-id: 20241105-coverity1511468wrongoperator-20130bcd4240
> > 
> > Best regards,
> > -- 
> > Karan Sanghavi <karansanghvi98@gmail.com>
> > 
> 
> -- 
> With best wishes
> Dmitry

Thank you,
Karan.