[v2,1/9] drm/i915: Remove superfluous NULL check

Submitted by Takashi Iwai on Dec. 1, 2015, 4:09 p.m.

Details

Message ID 1448986198-3488-2-git-send-email-tiwai@suse.de
State New
Headers show
Series "Add get_eld audio component for i915/HD-audio" ( rev: 3 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai Dec. 1, 2015, 4:09 p.m.
to_intel_crtc() always returns a non-NULL pointer.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_audio.c | 4 ----
 1 file changed, 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 4dccd9b003a1..0c38cc6c82ae 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -656,10 +656,6 @@  static int i915_audio_component_sync_audio_rate(struct device *dev,
 		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
 		if (port == intel_dig_port->port) {
 			crtc = to_intel_crtc(intel_encoder->base.crtc);
-			if (!crtc) {
-				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
-				continue;
-			}
 			pipe = crtc->pipe;
 			break;
 		}

Comments

On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> to_intel_crtc() always returns a non-NULL pointer.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_audio.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 4dccd9b003a1..0c38cc6c82ae 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
>  		if (port == intel_dig_port->port) {
>  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> -			if (!crtc) {
> -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> -				continue;
> -			}
>  			pipe = crtc->pipe;
>  			break;
>  		}
> -- 
> 2.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> to_intel_crtc() always returns a non-NULL pointer.

Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
breakage on our hands. Or maybe the atomic work has gotten rid of that
assumption, but at least we used to depend on that heavily.

> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 4dccd9b003a1..0c38cc6c82ae 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
>  		if (port == intel_dig_port->port) {
>  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> -			if (!crtc) {
> -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> -				continue;
> -			}
>  			pipe = crtc->pipe;
>  			break;
>  		}
> -- 
> 2.6.3
On Fri, 04 Dec 2015 13:16:46 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > to_intel_crtc() always returns a non-NULL pointer.
> 
> Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> breakage on our hands. Or maybe the atomic work has gotten rid of that
> assumption, but at least we used to depend on that heavily.

Well, to_intel_crtc() has been always container_of() since the very
beginning of universe:

commit 79e539453b34e35f39299a899d263b0a1f1670bd
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Nov 7 14:24:08 2008 -0800

    DRM: i915: add mode setting support
    
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_drv.h
....
+#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
....


Takashi

> 
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 4dccd9b003a1..0c38cc6c82ae 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> >  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> >  		if (port == intel_dig_port->port) {
> >  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > -			if (!crtc) {
> > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > -				continue;
> > -			}
> >  			pipe = crtc->pipe;
> >  			break;
> >  		}
> > -- 
> > 2.6.3
> 
> -- 
> Ville Syrjälä
> Intel OTC
>
On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 13:16:46 +0100,
> Ville Syrjälä wrote:
> > 
> > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > to_intel_crtc() always returns a non-NULL pointer.
> > 
> > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > assumption, but at least we used to depend on that heavily.
> 
> Well, to_intel_crtc() has been always container_of() since the very
> beginning of universe:
> 
> commit 79e539453b34e35f39299a899d263b0a1f1670bd
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Fri Nov 7 14:24:08 2008 -0800
> 
>     DRM: i915: add mode setting support
>     
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> ....
> +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> ....

Yes, but 
struct intel_crtc {
	struct drm_crtc base;
	...
}

So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.

I once suggested that someone should add a compile time assert to
make sure this always holds for us, but no one took the bait.

> 
> 
> Takashi
> 
> > 
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  drivers/gpu/drm/i915/intel_audio.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > index 4dccd9b003a1..0c38cc6c82ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > >  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> > >  		if (port == intel_dig_port->port) {
> > >  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > -			if (!crtc) {
> > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > -				continue;
> > > -			}
> > >  			pipe = crtc->pipe;
> > >  			break;
> > >  		}
> > > -- 
> > > 2.6.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> >
On Fri, 04 Dec 2015 14:07:03 +0100,
Ville Syrjälä wrote:
> 
> On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> > On Fri, 04 Dec 2015 13:16:46 +0100,
> > Ville Syrjälä wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > > to_intel_crtc() always returns a non-NULL pointer.
> > > 
> > > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > > assumption, but at least we used to depend on that heavily.
> > 
> > Well, to_intel_crtc() has been always container_of() since the very
> > beginning of universe:
> > 
> > commit 79e539453b34e35f39299a899d263b0a1f1670bd
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Fri Nov 7 14:24:08 2008 -0800
> > 
> >     DRM: i915: add mode setting support
> >     
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > ....
> > +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> > ....
> 
> Yes, but 
> struct intel_crtc {
> 	struct drm_crtc base;
> 	...
> }
> 
> So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.
> 
> I once suggested that someone should add a compile time assert to
> make sure this always holds for us, but no one took the bait.

Argh, only from the usage of container_of(), no one expects this
implicit assumption :-<  Indeed intel code has lots of these silent
rules, e.g. calling kfree() for the base class object.

Daniel, could you please take my patch back?


thanks,

Takashi

> 
> > 
> > 
> > Takashi
> > 
> > > 
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_audio.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 4dccd9b003a1..0c38cc6c82ae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > > >  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> > > >  		if (port == intel_dig_port->port) {
> > > >  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > > -			if (!crtc) {
> > > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > > -				continue;
> > > > -			}
> > > >  			pipe = crtc->pipe;
> > > >  			break;
> > > >  		}
> > > > -- 
> > > > 2.6.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
>
On Fri, Dec 04, 2015 at 02:12:14PM +0100, Takashi Iwai wrote:
> On Fri, 04 Dec 2015 14:07:03 +0100,
> Ville Syrjälä wrote:
> > 
> > On Fri, Dec 04, 2015 at 01:54:56PM +0100, Takashi Iwai wrote:
> > > On Fri, 04 Dec 2015 13:16:46 +0100,
> > > Ville Syrjälä wrote:
> > > > 
> > > > On Tue, Dec 01, 2015 at 05:09:50PM +0100, Takashi Iwai wrote:
> > > > > to_intel_crtc() always returns a non-NULL pointer.
> > > > 
> > > > Eh? to_intel_crtc(NULL) should return NULL or we might have tons of
> > > > breakage on our hands. Or maybe the atomic work has gotten rid of that
> > > > assumption, but at least we used to depend on that heavily.
> > > 
> > > Well, to_intel_crtc() has been always container_of() since the very
> > > beginning of universe:
> > > 
> > > commit 79e539453b34e35f39299a899d263b0a1f1670bd
> > > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Date:   Fri Nov 7 14:24:08 2008 -0800
> > > 
> > >     DRM: i915: add mode setting support
> > >     
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > ....
> > > +#define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
> > > ....
> > 
> > Yes, but 
> > struct intel_crtc {
> > 	struct drm_crtc base;
> > 	...
> > }
> > 
> > So offsetof(struct intel_crtc, base)==0 and NULL-0 is still NULL.
> > 
> > I once suggested that someone should add a compile time assert to
> > make sure this always holds for us, but no one took the bait.
> 
> Argh, only from the usage of container_of(), no one expects this
> implicit assumption :-<  Indeed intel code has lots of these silent
> rules, e.g. calling kfree() for the base class object.
> 
> Daniel, could you please take my patch back?

Oops. Patch reverted, sorry for the mess.
-Daniel

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > > 
> > > 
> > > Takashi
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 4dccd9b003a1..0c38cc6c82ae 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> > > > >  		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> > > > >  		if (port == intel_dig_port->port) {
> > > > >  			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > > > -			if (!crtc) {
> > > > > -				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> > > > > -				continue;
> > > > > -			}
> > > > >  			pipe = crtc->pipe;
> > > > >  			break;
> > > > >  		}
> > > > > -- 
> > > > > 2.6.3
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx