If drm_edid_to_speaker_allocation returns a count of 0 sadb is not
initialized and should not get kfreed. Bail out like the other two
callers.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/gpu/drm/radeon/dce3_1_afmt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c b/drivers/gpu/drm/radeon/dce3_1_afmt.c
index 51800e3..cb76074 100644
--- a/drivers/gpu/drm/radeon/dce3_1_afmt.c
+++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c
@@ -48,7 +48,7 @@ static void dce3_2_afmt_write_speaker_allocation(struct drm_encoder *encoder)
}
sad_count = drm_edid_to_speaker_allocation(radeon_connector->edid, &sadb);
- if (sad_count < 0) {
+ if (sad_count <= 0) {
DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n", sad_count);
return;
}
--
2.0.4
Removing a code-path that can never be executed ... and its copies. If
drm_edid_to_speaker_allocation returns 0 the callers return. There is no
need to check that condition again.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/gpu/drm/radeon/dce3_1_afmt.c | 5 +----
drivers/gpu/drm/radeon/dce6_afmt.c | 5 +----
drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 +----
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c b/drivers/gpu/drm/radeon/dce3_1_afmt.c
index cb76074..6d31ed8 100644
--- a/drivers/gpu/drm/radeon/dce3_1_afmt.c
+++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c
@@ -58,10 +58,7 @@ static void dce3_2_afmt_write_speaker_allocation(struct drm_encoder *encoder)
tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
/* set HDMI mode */
tmp |= HDMI_CONNECTION;
- if (sad_count)
- tmp |= SPEAKER_ALLOCATION(sadb[0]);
- else
- tmp |= SPEAKER_ALLOCATION(5); /* stereo */
+ tmp |= SPEAKER_ALLOCATION(sadb[0]);
WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
kfree(sadb);
diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c b/drivers/gpu/drm/radeon/dce6_afmt.c
index ab29f95..e6b2750 100644
--- a/drivers/gpu/drm/radeon/dce6_afmt.c
+++ b/drivers/gpu/drm/radeon/dce6_afmt.c
@@ -186,10 +186,7 @@ void dce6_afmt_write_speaker_allocation(struct drm_encoder *encoder)
tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
/* set HDMI mode */
tmp |= HDMI_CONNECTION;
- if (sad_count)
- tmp |= SPEAKER_ALLOCATION(sadb[0]);
- else
- tmp |= SPEAKER_ALLOCATION(5); /* stereo */
+ tmp |= SPEAKER_ALLOCATION(sadb[0]);
WREG32_ENDPOINT(offset, AZ_F0_CODEC_PIN_CONTROL_CHANNEL_SPEAKER, tmp);
kfree(sadb);
diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c b/drivers/gpu/drm/radeon/evergreen_hdmi.c
index 278c7a1..11a6b65 100644
--- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
+++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
@@ -128,10 +128,7 @@ static void dce4_afmt_write_speaker_allocation(struct drm_encoder *encoder)
tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
/* set HDMI mode */
tmp |= HDMI_CONNECTION;
- if (sad_count)
- tmp |= SPEAKER_ALLOCATION(sadb[0]);
- else
- tmp |= SPEAKER_ALLOCATION(5); /* stereo */
+ tmp |= SPEAKER_ALLOCATION(sadb[0]);
WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
kfree(sadb);
--
2.0.4
Well i am not sure what the code actually is supposed to do. That is up to you
because you know the hardware. I just wanted to point out that right now some
code can never be reached.
I ran into the whole thing because the pointer (sadp) that gets kfreed was never
initialized (in one of the three cases). My first fix was to initialize it to
NULL, that way the kfree did not fail even if the allocation did not do
anything. Then i found the other two copies and decided to follow their scheme.
In your patch you have to make kfree depend on (sad_count > 0) or sadp needs to
be initialized with NULL. Otherwise you will get the dangling pointer kfree that
lead me to read this code.
Henning
> On October 13, 2014 at 6:08 PM "Deucher, Alexander"
> <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Henning Schild [mailto:[email protected]]
> > Sent: Saturday, October 11, 2014 8:51 AM
> > To: [email protected]
> > Cc: Henning Schild; Deucher, Alexander; Rafał Miłecki
> > Subject: [PATCH] drm/radeon: remove code that can never get executed
> >
> > Removing a code-path that can never be executed ... and its copies. If
> > drm_edid_to_speaker_allocation returns 0 the callers return. There is no
> > need to check that condition again.
>
> I think we actually want to set the speaker allocation setup to stereo if the
> speaker allocation block is not present so I think the attached patch is
> probably the proper fix.
>
> Alex
>
> >
> > Signed-off-by: Henning Schild <[email protected]>
> > ---
> > drivers/gpu/drm/radeon/dce3_1_afmt.c | 5 +----
> > drivers/gpu/drm/radeon/dce6_afmt.c | 5 +----
> > drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 +----
> > 3 files changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > index cb76074..6d31ed8 100644
> > --- a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > +++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > @@ -58,10 +58,7 @@ static void
> > dce3_2_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > /* set HDMI mode */
> > tmp |= HDMI_CONNECTION;
> > - if (sad_count)
> > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > - else
> > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
> >
> > kfree(sadb);
> > diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c
> > b/drivers/gpu/drm/radeon/dce6_afmt.c
> > index ab29f95..e6b2750 100644
> > --- a/drivers/gpu/drm/radeon/dce6_afmt.c
> > +++ b/drivers/gpu/drm/radeon/dce6_afmt.c
> > @@ -186,10 +186,7 @@ void dce6_afmt_write_speaker_allocation(struct
> > drm_encoder *encoder)
> > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > /* set HDMI mode */
> > tmp |= HDMI_CONNECTION;
> > - if (sad_count)
> > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > - else
> > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > WREG32_ENDPOINT(offset,
> > AZ_F0_CODEC_PIN_CONTROL_CHANNEL_SPEAKER, tmp);
> >
> > kfree(sadb);
> > diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > index 278c7a1..11a6b65 100644
> > --- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > +++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > @@ -128,10 +128,7 @@ static void
> > dce4_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > /* set HDMI mode */
> > tmp |= HDMI_CONNECTION;
> > - if (sad_count)
> > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > - else
> > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
> >
> > kfree(sadb);
> > --
> > 2.0.4
>
> -----Original Message-----
> From: Henning Schild [mailto:[email protected]]
> Sent: Monday, October 13, 2014 12:44 PM
> To: Deucher, Alexander; [email protected]
> Cc: Rafał Miłecki
> Subject: RE: [PATCH] drm/radeon: remove code that can never get executed
>
> Well i am not sure what the code actually is supposed to do. That is up to you
> because you know the hardware. I just wanted to point out that right now
> some
> code can never be reached.
>
> I ran into the whole thing because the pointer (sadp) that gets kfreed was
> never
> initialized (in one of the three cases). My first fix was to initialize it to
> NULL, that way the kfree did not fail even if the allocation did not do
> anything. Then i found the other two copies and decided to follow their
> scheme.
>
> In your patch you have to make kfree depend on (sad_count > 0) or sadp
> needs to
> be initialized with NULL. Otherwise you will get the dangling pointer kfree
> that
> lead me to read this code.
>
Ah, ok. The attached patches should do the trick then.
Alex
> Henning
>
> > On October 13, 2014 at 6:08 PM "Deucher, Alexander"
> > <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Henning Schild [mailto:[email protected]]
> > > Sent: Saturday, October 11, 2014 8:51 AM
> > > To: [email protected]
> > > Cc: Henning Schild; Deucher, Alexander; Rafał Miłecki
> > > Subject: [PATCH] drm/radeon: remove code that can never get executed
> > >
> > > Removing a code-path that can never be executed ... and its copies. If
> > > drm_edid_to_speaker_allocation returns 0 the callers return. There is no
> > > need to check that condition again.
> >
> > I think we actually want to set the speaker allocation setup to stereo if the
> > speaker allocation block is not present so I think the attached patch is
> > probably the proper fix.
> >
> > Alex
> >
> > >
> > > Signed-off-by: Henning Schild <[email protected]>
> > > ---
> > > drivers/gpu/drm/radeon/dce3_1_afmt.c | 5 +----
> > > drivers/gpu/drm/radeon/dce6_afmt.c | 5 +----
> > > drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 +----
> > > 3 files changed, 3 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > > b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > > index cb76074..6d31ed8 100644
> > > --- a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > > +++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> > > @@ -58,10 +58,7 @@ static void
> > > dce3_2_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> > > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > > /* set HDMI mode */
> > > tmp |= HDMI_CONNECTION;
> > > - if (sad_count)
> > > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > - else
> > > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
> > >
> > > kfree(sadb);
> > > diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c
> > > b/drivers/gpu/drm/radeon/dce6_afmt.c
> > > index ab29f95..e6b2750 100644
> > > --- a/drivers/gpu/drm/radeon/dce6_afmt.c
> > > +++ b/drivers/gpu/drm/radeon/dce6_afmt.c
> > > @@ -186,10 +186,7 @@ void dce6_afmt_write_speaker_allocation(struct
> > > drm_encoder *encoder)
> > > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > > /* set HDMI mode */
> > > tmp |= HDMI_CONNECTION;
> > > - if (sad_count)
> > > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > - else
> > > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > WREG32_ENDPOINT(offset,
> > > AZ_F0_CODEC_PIN_CONTROL_CHANNEL_SPEAKER, tmp);
> > >
> > > kfree(sadb);
> > > diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > > b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > > index 278c7a1..11a6b65 100644
> > > --- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > > +++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> > > @@ -128,10 +128,7 @@ static void
> > > dce4_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> > > tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> > > /* set HDMI mode */
> > > tmp |= HDMI_CONNECTION;
> > > - if (sad_count)
> > > - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > - else
> > > - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> > > + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> > > WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
> > >
> > > kfree(sadb);
> > > --
> > > 2.0.4
>
> >
> -----Original Message-----
> From: Henning Schild [mailto:[email protected]]
> Sent: Saturday, October 11, 2014 8:51 AM
> To: [email protected]
> Cc: Henning Schild; Deucher, Alexander; Rafa? Mi?ecki
> Subject: [PATCH] drm/radeon: remove code that can never get executed
>
> Removing a code-path that can never be executed ... and its copies. If
> drm_edid_to_speaker_allocation returns 0 the callers return. There is no
> need to check that condition again.
I think we actually want to set the speaker allocation setup to stereo if the speaker allocation block is not present so I think the attached patch is probably the proper fix.
Alex
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/gpu/drm/radeon/dce3_1_afmt.c | 5 +----
> drivers/gpu/drm/radeon/dce6_afmt.c | 5 +----
> drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 +----
> 3 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> index cb76074..6d31ed8 100644
> --- a/drivers/gpu/drm/radeon/dce3_1_afmt.c
> +++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c
> @@ -58,10 +58,7 @@ static void
> dce3_2_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> /* set HDMI mode */
> tmp |= HDMI_CONNECTION;
> - if (sad_count)
> - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> - else
> - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
>
> kfree(sadb);
> diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c
> b/drivers/gpu/drm/radeon/dce6_afmt.c
> index ab29f95..e6b2750 100644
> --- a/drivers/gpu/drm/radeon/dce6_afmt.c
> +++ b/drivers/gpu/drm/radeon/dce6_afmt.c
> @@ -186,10 +186,7 @@ void dce6_afmt_write_speaker_allocation(struct
> drm_encoder *encoder)
> tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> /* set HDMI mode */
> tmp |= HDMI_CONNECTION;
> - if (sad_count)
> - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> - else
> - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> WREG32_ENDPOINT(offset,
> AZ_F0_CODEC_PIN_CONTROL_CHANNEL_SPEAKER, tmp);
>
> kfree(sadb);
> diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> index 278c7a1..11a6b65 100644
> --- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
> +++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
> @@ -128,10 +128,7 @@ static void
> dce4_afmt_write_speaker_allocation(struct drm_encoder *encoder)
> tmp &= ~(DP_CONNECTION | SPEAKER_ALLOCATION_MASK);
> /* set HDMI mode */
> tmp |= HDMI_CONNECTION;
> - if (sad_count)
> - tmp |= SPEAKER_ALLOCATION(sadb[0]);
> - else
> - tmp |= SPEAKER_ALLOCATION(5); /* stereo */
> + tmp |= SPEAKER_ALLOCATION(sadb[0]);
> WREG32(AZ_F0_CODEC_PIN0_CONTROL_CHANNEL_SPEAKER, tmp);
>
> kfree(sadb);
> --
> 2.0.4
On Mon, 13 Oct 2014 17:31:12 +0000
"Deucher, Alexander" <[email protected]> wrote:
> Ah, ok. The attached patches should do the trick then.
Yes that would do the trick. But i suggest doing the NULL initialization
in drm_edid_to_speaker_allocation.
The kernel doc suggests that whatever drm_edid_to_speaker_allocation
returns should be freed. Depending on which version you prefer, the
doc might have to be changed as well.
Henning