[PATCH] drm: vc4: Use of_property_present()

[PATCH] drm: vc4: Use of_property_present()

[Rob Herring (Arm)]

Use of_property_present() to test for property presence rather than
of_find_property(). This is part of a larger effort to remove callers
of of_find_property() and similar functions. of_find_property() leaks
the DT struct property and data pointers which is a problem for
dynamically allocated nodes which may be freed.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d57c4a5948c8..049de06460d5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
 	struct device *dev = &vc4_hdmi->pdev->dev;
 	struct platform_device *codec_pdev;
 	const __be32 *addr;
-	int index, len;
+	int index;
 	int ret;
 
 	/*
@@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
 	BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
 	BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
 
-	if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
+	if (!of_property_present(dev->of_node, "dmas")) {
 		dev_warn(dev,
 			 "'dmas' DT property is missing or empty, no HDMI audio\n");
 		return 0;
-- 
2.43.0

Re: [PATCH] drm: vc4: Use of_property_present()

[Rob Herring]

On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> Use of_property_present() to test for property presence rather than
> of_find_property(). This is part of a larger effort to remove callers
> of of_find_property() and similar functions. of_find_property() leaks
> the DT struct property and data pointers which is a problem for
> dynamically allocated nodes which may be freed.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Ping!

>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d57c4a5948c8..049de06460d5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
>         struct device *dev = &vc4_hdmi->pdev->dev;
>         struct platform_device *codec_pdev;
>         const __be32 *addr;
> -       int index, len;
> +       int index;
>         int ret;
>
>         /*
> @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
>         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
>         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
>
> -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> +       if (!of_property_present(dev->of_node, "dmas")) {
>                 dev_warn(dev,
>                          "'dmas' DT property is missing or empty, no HDMI audio\n");
>                 return 0;
> --
> 2.43.0
>

Re: [PATCH] drm: vc4: Use of_property_present()

[Dave Stevenson]

Hi Rob

On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > Use of_property_present() to test for property presence rather than
> > of_find_property(). This is part of a larger effort to remove callers
> > of of_find_property() and similar functions. of_find_property() leaks
> > the DT struct property and data pointers which is a problem for
> > dynamically allocated nodes which may be freed.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Ping!

Sorry, this fell through the cracks.

> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index d57c4a5948c8..049de06460d5 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> >         struct device *dev = &vc4_hdmi->pdev->dev;
> >         struct platform_device *codec_pdev;
> >         const __be32 *addr;
> > -       int index, len;
> > +       int index;
> >         int ret;
> >
> >         /*
> > @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> >         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
> >         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
> >
> > -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> > +       if (!of_property_present(dev->of_node, "dmas")) {

The existing conditional is true if the property is not present or is 0 length.
Your new one is only true if the property isn't present, so it isn't the same.

Is there a more appropriate of_ call to return the length of the property?

Thanks
  Dave

> >                 dev_warn(dev,
> >                          "'dmas' DT property is missing or empty, no HDMI audio\n");
> >                 return 0;
> > --
> > 2.43.0
> >

Re: [PATCH] drm: vc4: Use of_property_present()

[Rob Herring]

On Wed, Sep 4, 2024 at 6:18 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Rob
>
> On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > >
> > > Use of_property_present() to test for property presence rather than
> > > of_find_property(). This is part of a larger effort to remove callers
> > > of of_find_property() and similar functions. of_find_property() leaks
> > > the DT struct property and data pointers which is a problem for
> > > dynamically allocated nodes which may be freed.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Ping!
>
> Sorry, this fell through the cracks.
>
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index d57c4a5948c8..049de06460d5 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > >         struct device *dev = &vc4_hdmi->pdev->dev;
> > >         struct platform_device *codec_pdev;
> > >         const __be32 *addr;
> > > -       int index, len;
> > > +       int index;
> > >         int ret;
> > >
> > >         /*
> > > @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
> > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
> > >
> > > -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> > > +       if (!of_property_present(dev->of_node, "dmas")) {
>
> The existing conditional is true if the property is not present or is 0 length.
> Your new one is only true if the property isn't present, so it isn't the same.

It is not the kernel's job to validate the DT. It does a terrible job
of it and we have better tools for that now (schemas (though RPi
platforms are in a pretty sad state for schemas)). I'm pretty sure a
zero length or otherwise malformed "dmas" property would also cause a
dtc warning as well. Non-zero length is hardly a complete test
anyways. Any bogus value of "dmas" would pass. Or it can be completely
valid, but the DMA driver is not enabled (whether you even probe
depends on fw_devlinks).

The kernel should just parse the properties it wants and handle any errors then.

>
> Is there a more appropriate of_ call to return the length of the property?

There are several which are all based on the data type (string, u32,
u8, phandle+args, etc.). This case would be
of_count_phandle_with_args(). Unless you required something like 2
dmas entries instead of 1, I wouldn't use that here though.

Rob

Re: [PATCH] drm: vc4: Use of_property_present()

[Dave Stevenson]

On Wed, 4 Sept 2024 at 14:19, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 4, 2024 at 6:18 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Rob
> >
> > On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > >
> > > > Use of_property_present() to test for property presence rather than
> > > > of_find_property(). This is part of a larger effort to remove callers
> > > > of of_find_property() and similar functions. of_find_property() leaks
> > > > the DT struct property and data pointers which is a problem for
> > > > dynamically allocated nodes which may be freed.
> > > >
> > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Ping!
> >
> > Sorry, this fell through the cracks.
> >
> > > >
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > index d57c4a5948c8..049de06460d5 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > >         struct device *dev = &vc4_hdmi->pdev->dev;
> > > >         struct platform_device *codec_pdev;
> > > >         const __be32 *addr;
> > > > -       int index, len;
> > > > +       int index;
> > > >         int ret;
> > > >
> > > >         /*
> > > > @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
> > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
> > > >
> > > > -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> > > > +       if (!of_property_present(dev->of_node, "dmas")) {
> >
> > The existing conditional is true if the property is not present or is 0 length.
> > Your new one is only true if the property isn't present, so it isn't the same.
>
> It is not the kernel's job to validate the DT. It does a terrible job
> of it and we have better tools for that now (schemas (though RPi
> platforms are in a pretty sad state for schemas)). I'm pretty sure a
> zero length or otherwise malformed "dmas" property would also cause a
> dtc warning as well. Non-zero length is hardly a complete test
> anyways. Any bogus value of "dmas" would pass. Or it can be completely
> valid, but the DMA driver is not enabled (whether you even probe
> depends on fw_devlinks).
>
> The kernel should just parse the properties it wants and handle any errors then.

I've followed up over the rationale of this.

The base DT enables HDMI audio.
On some systems there is a need to use the DMA channels for other
purposes and no need for HDMI audio.
As we understand it, an overlay can't remove a property from the base
DT, but it can set it to being empty. (Please correct us if there is a
way to delete an existing property).

The current check therefore allows an overlay to disable the HDMI
audio that is enabled in the base DT. It doesn't care how long the
property actually is, just whether it is totally empty or not as an
alternative to being present.

I understand that you may consider that abuse of DT, but that is the
reasoning behind it. We can drop it to a downstream patch if
necessary.

  Dave

> >
> > Is there a more appropriate of_ call to return the length of the property?
>
> There are several which are all based on the data type (string, u32,
> u8, phandle+args, etc.). This case would be
> of_count_phandle_with_args(). Unless you required something like 2
> dmas entries instead of 1, I wouldn't use that here though.
>
> Rob

Re: [PATCH] drm: vc4: Use of_property_present()

[Rob Herring]

On Fri, Sep 6, 2024 at 9:24 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Wed, 4 Sept 2024 at 14:19, Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Sep 4, 2024 at 6:18 AM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > >
> > > Hi Rob
> > >
> > > On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > > >
> > > > > Use of_property_present() to test for property presence rather than
> > > > > of_find_property(). This is part of a larger effort to remove callers
> > > > > of of_find_property() and similar functions. of_find_property() leaks
> > > > > the DT struct property and data pointers which is a problem for
> > > > > dynamically allocated nodes which may be freed.
> > > > >
> > > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > Ping!
> > >
> > > Sorry, this fell through the cracks.
> > >
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > index d57c4a5948c8..049de06460d5 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > > >         struct device *dev = &vc4_hdmi->pdev->dev;
> > > > >         struct platform_device *codec_pdev;
> > > > >         const __be32 *addr;
> > > > > -       int index, len;
> > > > > +       int index;
> > > > >         int ret;
> > > > >
> > > > >         /*
> > > > > @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
> > > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
> > > > >
> > > > > -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> > > > > +       if (!of_property_present(dev->of_node, "dmas")) {
> > >
> > > The existing conditional is true if the property is not present or is 0 length.
> > > Your new one is only true if the property isn't present, so it isn't the same.
> >
> > It is not the kernel's job to validate the DT. It does a terrible job
> > of it and we have better tools for that now (schemas (though RPi
> > platforms are in a pretty sad state for schemas)). I'm pretty sure a
> > zero length or otherwise malformed "dmas" property would also cause a
> > dtc warning as well. Non-zero length is hardly a complete test
> > anyways. Any bogus value of "dmas" would pass. Or it can be completely
> > valid, but the DMA driver is not enabled (whether you even probe
> > depends on fw_devlinks).
> >
> > The kernel should just parse the properties it wants and handle any errors then.
>
> I've followed up over the rationale of this.
>
> The base DT enables HDMI audio.
> On some systems there is a need to use the DMA channels for other
> purposes and no need for HDMI audio.

If that's a user decision, I wouldn't use overlays to decide that, but
make it a run-time OS decision...

> As we understand it, an overlay can't remove a property from the base
> DT, but it can set it to being empty. (Please correct us if there is a
> way to delete an existing property).

There isn't currently.

> The current check therefore allows an overlay to disable the HDMI
> audio that is enabled in the base DT. It doesn't care how long the
> property actually is, just whether it is totally empty or not as an
> alternative to being present.
>
> I understand that you may consider that abuse of DT, but that is the
> reasoning behind it. We can drop it to a downstream patch if
> necessary.

I guess it's going to be use of_count_phandle_with_args() instead.

Rob

Re: [PATCH] drm: vc4: Use of_property_present()

[Dave Stevenson]

On Fri, 6 Sept 2024 at 20:15, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Sep 6, 2024 at 9:24 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > On Wed, 4 Sept 2024 at 14:19, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Sep 4, 2024 at 6:18 AM Dave Stevenson
> > > <dave.stevenson@raspberrypi.com> wrote:
> > > >
> > > > Hi Rob
> > > >
> > > > On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > > > >
> > > > > > Use of_property_present() to test for property presence rather than
> > > > > > of_find_property(). This is part of a larger effort to remove callers
> > > > > > of of_find_property() and similar functions. of_find_property() leaks
> > > > > > the DT struct property and data pointers which is a problem for
> > > > > > dynamically allocated nodes which may be freed.
> > > > > >
> > > > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > Ping!
> > > >
> > > > Sorry, this fell through the cracks.
> > > >
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > index d57c4a5948c8..049de06460d5 100644
> > > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > @@ -2211,7 +2211,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > > > >         struct device *dev = &vc4_hdmi->pdev->dev;
> > > > > >         struct platform_device *codec_pdev;
> > > > > >         const __be32 *addr;
> > > > > > -       int index, len;
> > > > > > +       int index;
> > > > > >         int ret;
> > > > > >
> > > > > >         /*
> > > > > > @@ -2234,7 +2234,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> > > > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0);
> > > > > >         BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0);
> > > > > >
> > > > > > -       if (!of_find_property(dev->of_node, "dmas", &len) || !len) {
> > > > > > +       if (!of_property_present(dev->of_node, "dmas")) {
> > > >
> > > > The existing conditional is true if the property is not present or is 0 length.
> > > > Your new one is only true if the property isn't present, so it isn't the same.
> > >
> > > It is not the kernel's job to validate the DT. It does a terrible job
> > > of it and we have better tools for that now (schemas (though RPi
> > > platforms are in a pretty sad state for schemas)). I'm pretty sure a
> > > zero length or otherwise malformed "dmas" property would also cause a
> > > dtc warning as well. Non-zero length is hardly a complete test
> > > anyways. Any bogus value of "dmas" would pass. Or it can be completely
> > > valid, but the DMA driver is not enabled (whether you even probe
> > > depends on fw_devlinks).
> > >
> > > The kernel should just parse the properties it wants and handle any errors then.
> >
> > I've followed up over the rationale of this.
> >
> > The base DT enables HDMI audio.
> > On some systems there is a need to use the DMA channels for other
> > purposes and no need for HDMI audio.
>
> If that's a user decision, I wouldn't use overlays to decide that, but
> make it a run-time OS decision...

Raspberry Pi users tend to mess far more with the hardware
configuration than on most other platforms, so they do want to make
these changes when adding extra external peripherals, but they aren't
DT experts.
AFAIK configuring audio availability like that is not an option that
can really be pushed fully into userspace through any current API,
which would only leave a module parameter, and those generally seem to
be frowned upon these days.

> > As we understand it, an overlay can't remove a property from the base
> > DT, but it can set it to being empty. (Please correct us if there is a
> > way to delete an existing property).
>
> There isn't currently.
>
> > The current check therefore allows an overlay to disable the HDMI
> > audio that is enabled in the base DT. It doesn't care how long the
> > property actually is, just whether it is totally empty or not as an
> > alternative to being present.
> >
> > I understand that you may consider that abuse of DT, but that is the
> > reasoning behind it. We can drop it to a downstream patch if
> > necessary.
>
> I guess it's going to be use of_count_phandle_with_args() instead.

I'm happy with that if you are.

Thanks
  Dave

> Rob