[linux-sunxi] Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup
On Mon, Dec 16, 2019 at 04:43:48PM +0200, Stefan Mavrodiev wrote: > It's possible hdmi->connector and hdmi->encoder divices to be NULL. The wording is broken and s/divices/devices/. I'd write: It's possible that the parent devices of the connector and/or encoder are NULL. This makes drm_connector_cleanup and drm_encoder_cleanup respectively trigger a NULL pointer exeception: <...log here...> This is reproducible by <...receipt here...> So add a check for NULL before calling these functions. Of course this doesn't address the reservations by Maxime. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20191217072625.rqzl2udojrxcxqd4%40pengutronix.de.
[linux-sunxi] Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup
Hi, On 12/16/19 6:12 PM, Maxime Ripard wrote: Hi, On Mon, Dec 16, 2019 at 04:43:48PM +0200, Stefan Mavrodiev wrote: It's possible hdmi->connector and hdmi->encoder divices to be NULL. This can happen when building as kernel module and you try to remove the module. This patch make simple null check, before calling the cleanup functions. Signed-off-by: Stefan Mavrodiev --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index a7c4654445c7..b61e00f2ecb8 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -685,8 +685,10 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master, struct sun4i_hdmi *hdmi = dev_get_drvdata(dev); cec_unregister_adapter(hdmi->cec_adap); - drm_connector_cleanup(>connector); - drm_encoder_cleanup(>encoder); + if (hdmi->connector.dev) + drm_connector_cleanup(>connector); + if (hdmi->encoder.dev) + drm_encoder_cleanup(>encoder); Hmmm, this doesn't look right. Do you have more information on how you can reproduce it? Just build sun4i_drm_hdmi as module (CONFIG_DRM_SUN4I_HDMI=m). Then try to unload the module: # rmmod sun4i_drm_hdmi And you get this: Unable to handle kernel NULL pointer dereference at virtual address pgd = 6b032436 [] *pgd= Internal error: Oops: 5 [#1] SMP ARM Modules linked in: sun4i_drm_hdmi(-) CPU: 0 PID: 1081 Comm: rmmod Not tainted 5.5.0-rc1-00030-g6ec417030d93 #33 Hardware name: Allwinner sun7i (A20) Family PC is at drm_connector_cleanup+0x40/0x208 LR is at sun4i_hdmi_unbind+0x10/0x54 [sun4i_drm_hdmi] ... I've tested that with sunxi/for-next branch on A20-OLinuXino board. Best regards, Stefan Maxime -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/cebda755-2649-79a1-fd08-79b13edef1a5%40olimex.com.
[linux-sunxi] Re: [PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding
Hi,
On 12/16/19 8:04 AM, Maxime Ripard wrote:
> Hi,
>
> On Sat, Dec 14, 2019 at 10:24:49PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
>> sun50i SoCs. Add a device tree binding for it. As it has only been
>> tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
>> included.
>>
>> Signed-off-by: Samuel Holland
>> ---
>> .../mailbox/allwinner,sun6i-a31-msgbox.yaml | 78 +++
>> 1 file changed, 78 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>>
>> diff --git
>> a/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> new file mode 100644
>> index ..dd746e07acfd
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mailbox/allwinner,sun6i-a31-msgbox.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Allwinner sunxi Message Box
>> +
>> +maintainers:
>> + - Samuel Holland
>> +
>> +description: |
>> + The hardware message box on sun6i, sun8i, sun9i, and sun50i SoCs is a
>> + two-user mailbox controller containing 8 unidirectional FIFOs. An
>> interrupt
>> + is raised for received messages, but software must poll to know when a
>> + transmitted message has been acknowledged by the remote user. Each FIFO
>> can
>> + hold four 32-bit messages; when a FIFO is full, clients must wait before
>> + attempting more transmissions.
>> +
>> + Refer to ./mailbox.txt for generic information about mailbox device-tree
>> + bindings.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - allwinner,sun8i-a83t-msgbox
>> + - allwinner,sun8i-h3-msgbox
>> + - allwinner,sun50i-a64-msgbox
>> + - allwinner,sun50i-h6-msgbox
>> + - const: allwinner,sun6i-a31-msgbox
>
> This will fail for the A31, since it won't have two compatibles but
> just one.
You asked me earlier to only include compatibles that had been tested, so I did.
This hasn't been tested on the A31, so there's no A31-only compatible.
> You can have something like this if you want to do it with an enum:
>
> compatible:
> oneOf:
> - const: allwinner,sun6i-a31-msgbox
> - items:
> - enum:
> - allwinner,sun8i-a83t-msgbox
> - allwinner,sun8i-h3-msgbox
> - allwinner,sun50i-a64-msgbox
> - allwinner,sun50i-h6-msgbox
> - const: allwinner,sun6i-a31-msgbox
>
>> + reg:
>> +items:
>> + - description: MMIO register range
>
> There's no need for an obvious description like this.
> Just set it to maxItems: 1
Will do for v6.
>> +
>> + clocks:
>> +maxItems: 1
>> +description: bus clock
>> +
>> + resets:
>> +maxItems: 1
>> +description: bus reset
>> +
>> + interrupts:
>> +maxItems: 1
>> +description: controller interrupt
>
> Ditto, you can drop the description here.
Will do for v6.
>> + '#mbox-cells':
>> +const: 1
>
> However, you should document what the argument is about?
Ok. "Number of cells used to encode a mailbox specifier" should work.
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - resets
>> + - interrupts
>> + - '#mbox-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> +#include
>> +#include
>> +#include
>> +
>> +msgbox: mailbox@1c17000 {
>> +compatible = "allwinner,sun8i-h3-msgbox",
>> + "allwinner,sun6i-a31-msgbox";
>> +reg = <0x01c17000 0x1000>;
>> +clocks = < CLK_BUS_MSGBOX>;
>> +resets = < RST_BUS_MSGBOX>;
>> +interrupts = ;
>> +#mbox-cells = <1>;
>> +};
>
> Look good otherwise, thanks!
> Maxime
>
Thanks,
Samuel
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/d3a1c7c2-953a-cbfe-970e-c00f9a9f5742%40sholland.org.
[linux-sunxi] Re: [PATCH v12 1/7] dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller
On Mon, Dec 16, 2019 at 4:50 PM Maxime Ripard wrote: > > On Mon, Dec 16, 2019 at 04:37:20PM +0530, Jagan Teki wrote: > > On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard wrote: > > > > > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > > > to have separate compatible for A64 on the same driver. > > > > > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > > > is not required for A64. > > > > > > > > On that note > > > > - A64 require minimum of 1 clock like the bus clock > > > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > > > > > So, update dt-bindings so-that it can document both A33, > > > > A64 bindings requirements. > > > > > > > > Reviewed-by: Rob Herring > > > > Signed-off-by: Jagan Teki > > > > --- > > > > Changes for v12: > > > > - Use 'enum' instead of oneOf+const > > > > > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +-- > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > > > > > b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > index dafc0980c4fa..b91446475f35 100644 > > > > --- > > > > a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > +++ > > > > b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > @@ -15,7 +15,9 @@ properties: > > > >"#size-cells": true > > > > > > > >compatible: > > > > -const: allwinner,sun6i-a31-mipi-dsi > > > > +enum: > > > > + - allwinner,sun6i-a31-mipi-dsi > > > > + - allwinner,sun50i-a64-mipi-dsi > > > > > > > >reg: > > > > maxItems: 1 > > > > @@ -24,6 +26,8 @@ properties: > > > > maxItems: 1 > > > > > > > >clocks: > > > > +minItems: 1 > > > > +maxItems: 2 > > > > items: > > > >- description: Bus Clock > > > >- description: Module Clock > > > > @@ -63,13 +67,25 @@ required: > > > >- reg > > > >- interrupts > > > >- clocks > > > > - - clock-names > > > >- phys > > > >- phy-names > > > >- resets > > > >- vcc-dsi-supply > > > >- port > > > > > > > > +allOf: > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: allwinner,sun6i-a31-mipi-dsi > > > > + then: > > > > +properties: > > > > + clocks: > > > > +minItems: 2 > > > > +required: > > > > + - clock-names > > > > + > > > > > > Your else condition should check that the number of clocks items is 1 > > > on the A64 > > > > But the minItems mentioned as 1 in clocks, which is unchanged number > > by default. doesn't it sufficient? > > In the main schema, it's said that the clocks property can have one or > two elements (to cover the A31 case that has one, and the A64 case > that has 2). > > This is fine. > > Later on, you enforce that the A64 has two elements, and this is fine > too. Actually A31 case has 2 and A64 case has 1. > > However, you never check that on the A31 you only have one clock, and > you could very well have two and no one would notice. I did check A31 case for 2 but not in A64. this is what you mean? so adding A64 check like below would fine? allOf: - if: properties: compatible: contains: const: allwinner,sun6i-a31-mipi-dsi then: properties: clocks: minItems: 2 required: - clock-names - if: properties: compatible: contains: const: allwinner,sun50i-a64-mipi-dsi then: properties: clocks: minItems: 1 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAMty3ZBU-XaxR_vM5L2yVbhR5ftfbtDn3jP00qCxBF%2BowVyqDQ%40mail.gmail.com.
[linux-sunxi] [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup
It's possible hdmi->connector and hdmi->encoder divices to be NULL. This can happen when building as kernel module and you try to remove the module. This patch make simple null check, before calling the cleanup functions. Signed-off-by: Stefan Mavrodiev --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index a7c4654445c7..b61e00f2ecb8 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -685,8 +685,10 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master, struct sun4i_hdmi *hdmi = dev_get_drvdata(dev); cec_unregister_adapter(hdmi->cec_adap); - drm_connector_cleanup(>connector); - drm_encoder_cleanup(>encoder); + if (hdmi->connector.dev) + drm_connector_cleanup(>connector); + if (hdmi->encoder.dev) + drm_encoder_cleanup(>encoder); i2c_del_adapter(hdmi->i2c); i2c_put_adapter(hdmi->ddc_i2c); clk_disable_unprepare(hdmi->mod_clk); -- 2.17.1 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20191216144348.7540-1-stefan%40olimex.com.
[linux-sunxi] Re: [PATCH v12 1/7] dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller
On Wed, Dec 4, 2019 at 7:06 PM Maxime Ripard wrote: > > On Tue, Dec 03, 2019 at 07:18:10PM +0530, Jagan Teki wrote: > > The MIPI DSI controller in Allwinner A64 is similar to A33. > > > > But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid > > to have separate compatible for A64 on the same driver. > > > > DSI_SCLK uses mod clock-names on dt-bindings, so the same > > is not required for A64. > > > > On that note > > - A64 require minimum of 1 clock like the bus clock > > - A33 require minimum of 2 clocks like both bus, mod clocks > > > > So, update dt-bindings so-that it can document both A33, > > A64 bindings requirements. > > > > Reviewed-by: Rob Herring > > Signed-off-by: Jagan Teki > > --- > > Changes for v12: > > - Use 'enum' instead of oneOf+const > > > > .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > > > b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > index dafc0980c4fa..b91446475f35 100644 > > --- > > a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > +++ > > b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml > > @@ -15,7 +15,9 @@ properties: > >"#size-cells": true > > > >compatible: > > -const: allwinner,sun6i-a31-mipi-dsi > > +enum: > > + - allwinner,sun6i-a31-mipi-dsi > > + - allwinner,sun50i-a64-mipi-dsi > > > >reg: > > maxItems: 1 > > @@ -24,6 +26,8 @@ properties: > > maxItems: 1 > > > >clocks: > > +minItems: 1 > > +maxItems: 2 > > items: > >- description: Bus Clock > >- description: Module Clock > > @@ -63,13 +67,25 @@ required: > >- reg > >- interrupts > >- clocks > > - - clock-names > >- phys > >- phy-names > >- resets > >- vcc-dsi-supply > >- port > > > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: allwinner,sun6i-a31-mipi-dsi > > + then: > > +properties: > > + clocks: > > +minItems: 2 > > +required: > > + - clock-names > > + > > Your else condition should check that the number of clocks items is 1 > on the A64 But the minItems mentioned as 1 in clocks, which is unchanged number by default. doesn't it sufficient? -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAMty3ZDU57Hj3ZSBC6sSMFWN9-HQadA03hmXUNUVS1W0UQQ3DA%40mail.gmail.com.
