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

[Dave Stevenson]

On Fri, 6 Sept 2024 at 20:15, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Fri, Sep 6, 2024 at 9:24 AM Dave Stevenson
> <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> >
> > 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.
>
> 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