| Message ID | 1448986198-3488-2-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; }
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(-)