[bug report] drm/vc4: dsi: Fix bridge chain handling

[bug report] drm/vc4: dsi: Fix bridge chain handling

[Dan Carpenter]

Hello Boris Brezillon,

The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
from Dec 27, 2019, leads to the following static checker warning:

	drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
	warn: iterator used outside loop: 'iter'

drivers/gpu/drm/vc4/vc4_dsi.c
   743  static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
   744  {
   745          struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
   746          struct vc4_dsi *dsi = vc4_encoder->dsi;
   747          struct device *dev = &dsi->pdev->dev;
   748          struct drm_bridge *iter;
   749  
   750          list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
   751                  if (iter->funcs->disable)
   752                          iter->funcs->disable(iter);
   753          }

This loops backwards until iter is parked on the list head.

   754  
   755          vc4_dsi_ulps(dsi, true);
   756  
   757          list_for_each_entry_from(iter, &dsi->bridge_chain, chain_node) {

But then this "continues" until the iter is parked on the list head.
Since we ended with the iterator already on the list head then we never
enter this loop and it is a no-op.

Am I missing something?

   758                  if (iter->funcs->post_disable)
   759                          iter->funcs->post_disable(iter);
   760          }
   761  
   762          clk_disable_unprepare(dsi->pll_phy_clock);
   763          clk_disable_unprepare(dsi->escape_clock);
   764          clk_disable_unprepare(dsi->pixel_clock);
   765  
   766          pm_runtime_put(dev);
   767  }

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/vc4: dsi: Fix bridge chain handling

[Boris Brezillon]

Hello Dan,

On Wed, 24 Jun 2020 20:58:06 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> Hello Boris Brezillon,
> 
> The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
> from Dec 27, 2019, leads to the following static checker warning:
> 
> 	drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
> 	warn: iterator used outside loop: 'iter'
> 
> drivers/gpu/drm/vc4/vc4_dsi.c
>    743  static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
>    744  {
>    745          struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
>    746          struct vc4_dsi *dsi = vc4_encoder->dsi;
>    747          struct device *dev = &dsi->pdev->dev;
>    748          struct drm_bridge *iter;
>    749  
>    750          list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
>    751                  if (iter->funcs->disable)
>    752                          iter->funcs->disable(iter);
>    753          }
> 
> This loops backwards until iter is parked on the list head.
> 
>    754  
>    755          vc4_dsi_ulps(dsi, true);
>    756  
>    757          list_for_each_entry_from(iter, &dsi->bridge_chain, chain_node) {
> 
> But then this "continues" until the iter is parked on the list head.
> Since we ended with the iterator already on the list head then we never
> enter this loop and it is a no-op.
> 
> Am I missing something?

It should definitely be list_for_each_entry() here. Thanks for
this report. Would you mind sending a patch?

Regards,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/vc4: dsi: Fix bridge chain handling

[Dan Carpenter]

On Wed, Jun 24, 2020 at 08:30:25PM +0200, Boris Brezillon wrote:
> Hello Dan,
> 
> On Wed, 24 Jun 2020 20:58:06 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Hello Boris Brezillon,
> > 
> > The patch 033bfe7538a1: "drm/vc4: dsi: Fix bridge chain handling"
> > from Dec 27, 2019, leads to the following static checker warning:
> > 
> > 	drivers/gpu/drm/vc4/vc4_dsi.c:758 vc4_dsi_encoder_disable()
> > 	warn: iterator used outside loop: 'iter'
> > 
> > drivers/gpu/drm/vc4/vc4_dsi.c
> >    743  static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
> >    744  {
> >    745          struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
> >    746          struct vc4_dsi *dsi = vc4_encoder->dsi;
> >    747          struct device *dev = &dsi->pdev->dev;
> >    748          struct drm_bridge *iter;
> >    749  
> >    750          list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
> >    751                  if (iter->funcs->disable)
> >    752                          iter->funcs->disable(iter);
> >    753          }
> > 
> > This loops backwards until iter is parked on the list head.
> > 
> >    754  
> >    755          vc4_dsi_ulps(dsi, true);
> >    756  
> >    757          list_for_each_entry_from(iter, &dsi->bridge_chain, chain_node) {
> > 
> > But then this "continues" until the iter is parked on the list head.
> > Since we ended with the iterator already on the list head then we never
> > enter this loop and it is a no-op.
> > 
> > Am I missing something?
> 
> It should definitely be list_for_each_entry() here. Thanks for
> this report. Would you mind sending a patch?

Yeah.  I can do that tomorrow.

I'm working on a new Smatch check and so I wasn't super familiar with
some of these list_for_each() loops.  Thanks!

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel