All of lore.kernel.org
 help / color / mirror / Atom feed
From: Neil Armstrong <narmstrong@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: dri-devel@lists.freedesktop.org,
	linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Stefan Agner <stefan@agner.ch>
Subject: Re: [PATCH] drm/meson: fix shutdown crash when component not probed
Date: Tue, 25 May 2021 09:19:29 +0200	[thread overview]
Message-ID: <6a350607-3ccf-1af0-be17-8a31cc937e57@baylibre.com> (raw)
In-Reply-To: <CAFBinCC0aaMUbBkJ4bjhFa0A+sZH1muyW6kqAQYfjjXOkrNPGg@mail.gmail.com>

Hi Martin,

On 20/05/2021 22:25, Martin Blumenstingl wrote:
> Hi Neil,
> 
> since this has not received any Reviewed-by yet I tried my best to
> review it myself
> 
> On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev,
>>  static void meson_drv_shutdown(struct platform_device *pdev)
>>  {
>>         struct meson_drm *priv = dev_get_drvdata(&pdev->dev);
> this part made it hard for me because I was wondering where the
> matching dev_set_drvdata call is
> it turns out platform_set_drvdata is used instead, meaning for me it
> would have been easier to understand if platform_get_drvdata was used
> here
> that's however nothing which has changed with this patch

OK sure, indeed, but since it's the same... but yeah it may be an issue if platform_set_drvdata changes.

> 
>> -       struct drm_device *drm = priv->drm;
>>
>> -       DRM_DEBUG_DRIVER("\n");
>> -       drm_kms_helper_poll_fini(drm);
>> -       drm_atomic_helper_shutdown(drm);
>> +       if (!priv)
>> +               return;
>> +
>> +       drm_kms_helper_poll_fini(priv->drm);
>> +       drm_atomic_helper_shutdown(priv->drm);
>>  }
> then this part finally made sense to me (as non-drm person), as
> platform_set_drvdata comes near the end of meson_drv_bind_master (so
> any errors would cause the drvdata to be NULL).
> 
> with this I can also give me:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> in addition to my:
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks !

> 
> Can you please queue this up for -fixes or do we need to ask someone to do it?

Yes, the drm-misc-next & drm-misc-fixes are aleays opened and merged each week in the corresponding drm-next and drm-fixes
branches, so we can push patches at anytime without thinking about the merge window (except drm-misc-fixes merged back with -rc1).

Neil

> 
> 
> Best regards,
> Martin
> 


WARNING: multiple messages have this Message-ID (diff)
From: Neil Armstrong <narmstrong@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: dri-devel@lists.freedesktop.org,
	linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Stefan Agner <stefan@agner.ch>
Subject: Re: [PATCH] drm/meson: fix shutdown crash when component not probed
Date: Tue, 25 May 2021 09:19:29 +0200	[thread overview]
Message-ID: <6a350607-3ccf-1af0-be17-8a31cc937e57@baylibre.com> (raw)
In-Reply-To: <CAFBinCC0aaMUbBkJ4bjhFa0A+sZH1muyW6kqAQYfjjXOkrNPGg@mail.gmail.com>

Hi Martin,

On 20/05/2021 22:25, Martin Blumenstingl wrote:
> Hi Neil,
> 
> since this has not received any Reviewed-by yet I tried my best to
> review it myself
> 
> On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev,
>>  static void meson_drv_shutdown(struct platform_device *pdev)
>>  {
>>         struct meson_drm *priv = dev_get_drvdata(&pdev->dev);
> this part made it hard for me because I was wondering where the
> matching dev_set_drvdata call is
> it turns out platform_set_drvdata is used instead, meaning for me it
> would have been easier to understand if platform_get_drvdata was used
> here
> that's however nothing which has changed with this patch

OK sure, indeed, but since it's the same... but yeah it may be an issue if platform_set_drvdata changes.

> 
>> -       struct drm_device *drm = priv->drm;
>>
>> -       DRM_DEBUG_DRIVER("\n");
>> -       drm_kms_helper_poll_fini(drm);
>> -       drm_atomic_helper_shutdown(drm);
>> +       if (!priv)
>> +               return;
>> +
>> +       drm_kms_helper_poll_fini(priv->drm);
>> +       drm_atomic_helper_shutdown(priv->drm);
>>  }
> then this part finally made sense to me (as non-drm person), as
> platform_set_drvdata comes near the end of meson_drv_bind_master (so
> any errors would cause the drvdata to be NULL).
> 
> with this I can also give me:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> in addition to my:
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks !

> 
> Can you please queue this up for -fixes or do we need to ask someone to do it?

Yes, the drm-misc-next & drm-misc-fixes are aleays opened and merged each week in the corresponding drm-next and drm-fixes
branches, so we can push patches at anytime without thinking about the merge window (except drm-misc-fixes merged back with -rc1).

Neil

> 
> 
> Best regards,
> Martin
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Neil Armstrong <narmstrong@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/meson: fix shutdown crash when component not probed
Date: Tue, 25 May 2021 09:19:29 +0200	[thread overview]
Message-ID: <6a350607-3ccf-1af0-be17-8a31cc937e57@baylibre.com> (raw)
In-Reply-To: <CAFBinCC0aaMUbBkJ4bjhFa0A+sZH1muyW6kqAQYfjjXOkrNPGg@mail.gmail.com>

Hi Martin,

On 20/05/2021 22:25, Martin Blumenstingl wrote:
> Hi Neil,
> 
> since this has not received any Reviewed-by yet I tried my best to
> review it myself
> 
> On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev,
>>  static void meson_drv_shutdown(struct platform_device *pdev)
>>  {
>>         struct meson_drm *priv = dev_get_drvdata(&pdev->dev);
> this part made it hard for me because I was wondering where the
> matching dev_set_drvdata call is
> it turns out platform_set_drvdata is used instead, meaning for me it
> would have been easier to understand if platform_get_drvdata was used
> here
> that's however nothing which has changed with this patch

OK sure, indeed, but since it's the same... but yeah it may be an issue if platform_set_drvdata changes.

> 
>> -       struct drm_device *drm = priv->drm;
>>
>> -       DRM_DEBUG_DRIVER("\n");
>> -       drm_kms_helper_poll_fini(drm);
>> -       drm_atomic_helper_shutdown(drm);
>> +       if (!priv)
>> +               return;
>> +
>> +       drm_kms_helper_poll_fini(priv->drm);
>> +       drm_atomic_helper_shutdown(priv->drm);
>>  }
> then this part finally made sense to me (as non-drm person), as
> platform_set_drvdata comes near the end of meson_drv_bind_master (so
> any errors would cause the drvdata to be NULL).
> 
> with this I can also give me:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> in addition to my:
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks !

> 
> Can you please queue this up for -fixes or do we need to ask someone to do it?

Yes, the drm-misc-next & drm-misc-fixes are aleays opened and merged each week in the corresponding drm-next and drm-fixes
branches, so we can push patches at anytime without thinking about the merge window (except drm-misc-fixes merged back with -rc1).

Neil

> 
> 
> Best regards,
> Martin
> 


WARNING: multiple messages have this Message-ID (diff)
From: Neil Armstrong <narmstrong@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: dri-devel@lists.freedesktop.org,
	linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Stefan Agner <stefan@agner.ch>
Subject: Re: [PATCH] drm/meson: fix shutdown crash when component not probed
Date: Tue, 25 May 2021 09:19:29 +0200	[thread overview]
Message-ID: <6a350607-3ccf-1af0-be17-8a31cc937e57@baylibre.com> (raw)
In-Reply-To: <CAFBinCC0aaMUbBkJ4bjhFa0A+sZH1muyW6kqAQYfjjXOkrNPGg@mail.gmail.com>

Hi Martin,

On 20/05/2021 22:25, Martin Blumenstingl wrote:
> Hi Neil,
> 
> since this has not received any Reviewed-by yet I tried my best to
> review it myself
> 
> On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> [...]
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev,
>>  static void meson_drv_shutdown(struct platform_device *pdev)
>>  {
>>         struct meson_drm *priv = dev_get_drvdata(&pdev->dev);
> this part made it hard for me because I was wondering where the
> matching dev_set_drvdata call is
> it turns out platform_set_drvdata is used instead, meaning for me it
> would have been easier to understand if platform_get_drvdata was used
> here
> that's however nothing which has changed with this patch

OK sure, indeed, but since it's the same... but yeah it may be an issue if platform_set_drvdata changes.

> 
>> -       struct drm_device *drm = priv->drm;
>>
>> -       DRM_DEBUG_DRIVER("\n");
>> -       drm_kms_helper_poll_fini(drm);
>> -       drm_atomic_helper_shutdown(drm);
>> +       if (!priv)
>> +               return;
>> +
>> +       drm_kms_helper_poll_fini(priv->drm);
>> +       drm_atomic_helper_shutdown(priv->drm);
>>  }
> then this part finally made sense to me (as non-drm person), as
> platform_set_drvdata comes near the end of meson_drv_bind_master (so
> any errors would cause the drvdata to be NULL).
> 
> with this I can also give me:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> in addition to my:
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks !

> 
> Can you please queue this up for -fixes or do we need to ask someone to do it?

Yes, the drm-misc-next & drm-misc-fixes are aleays opened and merged each week in the corresponding drm-next and drm-fixes
branches, so we can push patches at anytime without thinking about the merge window (except drm-misc-fixes merged back with -rc1).

Neil

> 
> 
> Best regards,
> Martin
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply	other threads:[~2021-05-25  7:19 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-30  8:27 [PATCH] drm/meson: fix shutdown crash when component not probed Neil Armstrong
2021-04-30  8:27 ` Neil Armstrong
2021-04-30  8:27 ` Neil Armstrong
2021-04-30  8:27 ` Neil Armstrong
2021-05-02 23:04 ` Martin Blumenstingl
2021-05-02 23:04   ` Martin Blumenstingl
2021-05-02 23:04   ` Martin Blumenstingl
2021-05-02 23:04   ` Martin Blumenstingl
2021-05-20 20:25 ` Martin Blumenstingl
2021-05-20 20:25   ` Martin Blumenstingl
2021-05-20 20:25   ` Martin Blumenstingl
2021-05-20 20:25   ` Martin Blumenstingl
2021-05-25  7:19   ` Neil Armstrong [this message]
2021-05-25  7:19     ` Neil Armstrong
2021-05-25  7:19     ` Neil Armstrong
2021-05-25  7:19     ` Neil Armstrong
2021-05-25  7:27 ` Neil Armstrong
2021-05-25  7:27   ` Neil Armstrong
2021-05-25  7:27   ` Neil Armstrong
2021-05-25  7:27   ` Neil Armstrong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6a350607-3ccf-1af0-be17-8a31cc937e57@baylibre.com \
    --to=narmstrong@baylibre.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=stefan@agner.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.