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

[Dave Stevenson]

On Wed, 4 Sept 2024 at 14:19, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Wed, Sep 4, 2024 at 6:18 AM Dave Stevenson
> <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi Rob
> >
> > On Tue, 3 Sept 2024 at 20:19, Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 2:13 PM Rob Herring (Arm) <robh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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