| Message ID | 20240731191312.1710417-12-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: vc4: Use of_property_present() | expand |
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 >
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 > >
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
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
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
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
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;
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(-)