* [PATCH] drm/irq: Add irq as false detection
@ 2020-11-02 11:19 Tian Tao
2020-11-02 12:09 ` Thomas Zimmermann
2020-11-02 18:30 ` [kbuild] " Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Tian Tao @ 2020-11-02 11:19 UTC (permalink / raw)
To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
dri-devel, linux-kernel
Add the detection of false for irq, so that the EINVAL is not
returned when dev->irq_enabled is false.
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
drivers/gpu/drm/drm_irq.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 09d6e9e..7537a3d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
bool irq_enabled;
int i;
+ if (!dev->irq_enabled || !dev)
+ return 0;
+
irq_enabled = dev->irq_enabled;
dev->irq_enabled = false;
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/irq: Add irq as false detection
2020-11-02 11:19 [PATCH] drm/irq: Add irq as false detection Tian Tao
@ 2020-11-02 12:09 ` Thomas Zimmermann
2020-11-02 12:32 ` tiantao (H)
2020-11-02 12:40 ` Thomas Zimmermann
2020-11-02 18:30 ` [kbuild] " Dan Carpenter
1 sibling, 2 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2020-11-02 12:09 UTC (permalink / raw)
To: Tian Tao, maarten.lankhorst, mripard, airlied, daniel, dri-devel,
linux-kernel
[-- Attachment #1.1.1: Type: text/plain, Size: 1464 bytes --]
Hi
Am 02.11.20 um 12:19 schrieb Tian Tao:
> Add the detection of false for irq, so that the EINVAL is not
> returned when dev->irq_enabled is false.
>
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
> drivers/gpu/drm/drm_irq.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 09d6e9e..7537a3d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
> bool irq_enabled;
> int i;
>
> + if (!dev->irq_enabled || !dev)
> + return 0;
> +
Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
I'd just drop the test for !dev and assume that dev is always valid.
I suggest to change the int return value to void and return nothing.
Re-reading the actual implementation of this function, this location
might be too early. Further below there already is a test for
irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
the vblank handlers will be disabled and erroneous callers will see a
warning in the kernel's log.
Best regards
Thomas
> irq_enabled = dev->irq_enabled;
> dev->irq_enabled = false;
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/irq: Add irq as false detection
2020-11-02 12:09 ` Thomas Zimmermann
@ 2020-11-02 12:32 ` tiantao (H)
2020-11-02 12:40 ` Thomas Zimmermann
1 sibling, 0 replies; 6+ messages in thread
From: tiantao (H) @ 2020-11-02 12:32 UTC (permalink / raw)
To: Thomas Zimmermann, Tian Tao, maarten.lankhorst, mripard, airlied,
daniel, dri-devel, linux-kernel
在 2020/11/2 20:09, Thomas Zimmermann 写道:
> Hi
>
> Am 02.11.20 um 12:19 schrieb Tian Tao:
>> Add the detection of false for irq, so that the EINVAL is not
>> returned when dev->irq_enabled is false.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>> drivers/gpu/drm/drm_irq.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 09d6e9e..7537a3d 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
>> bool irq_enabled;
>> int i;
>>
>> + if (!dev->irq_enabled || !dev)
>> + return 0;
>> +
>
> Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
> I'd just drop the test for !dev and assume that dev is always valid.
>
> I suggest to change the int return value to void and return nothing.
>
> Re-reading the actual implementation of this function, this location
> might be too early. Further below there already is a test for
> irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
> the vblank handlers will be disabled and erroneous callers will see a
> warning in the kernel's log.
>
thank you，I will send a new patch to fix that.
> Best regards
> Thomas
>
>> irq_enabled = dev->irq_enabled;
>> dev->irq_enabled = false;
>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/irq: Add irq as false detection
2020-11-02 12:09 ` Thomas Zimmermann
2020-11-02 12:32 ` tiantao (H)
@ 2020-11-02 12:40 ` Thomas Zimmermann
2020-11-02 12:51 ` Daniel Vetter
1 sibling, 1 reply; 6+ messages in thread
From: Thomas Zimmermann @ 2020-11-02 12:40 UTC (permalink / raw)
To: Tian Tao, maarten.lankhorst, mripard, airlied, daniel, dri-devel,
linux-kernel
[-- Attachment #1.1.1: Type: text/plain, Size: 2267 bytes --]
Hi
Am 02.11.20 um 13:09 schrieb Thomas Zimmermann:
> Hi
>
> Am 02.11.20 um 12:19 schrieb Tian Tao:
>> Add the detection of false for irq, so that the EINVAL is not
>> returned when dev->irq_enabled is false.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>> drivers/gpu/drm/drm_irq.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 09d6e9e..7537a3d 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
>> bool irq_enabled;
>> int i;
>>
>> + if (!dev->irq_enabled || !dev)
>> + return 0;
>> +
>
> Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
> I'd just drop the test for !dev and assume that dev is always valid.
>
> I suggest to change the int return value to void and return nothing.
>
> Re-reading the actual implementation of this function, this location
> might be too early. Further below there already is a test for
> irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
> the vblank handlers will be disabled and erroneous callers will see a
> warning in the kernel's log.
In addition to these changes, you could also add a managed
implementation of drm_irq_install(). The canonical name should be
devm_drm_irq_install(). The function would call drm_irq_install() and
register a cleanup handler via devm_add_action(). Example code is at [1].
KMS drivers, such as hibmc, would then not have to bother about
uninstalling the DRM irq.
Best regards
Thomas
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_drv.c#n664
>
> Best regards
> Thomas
>
>> irq_enabled = dev->irq_enabled;
>> dev->irq_enabled = false;
>>
>>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/irq: Add irq as false detection
2020-11-02 12:40 ` Thomas Zimmermann
@ 2020-11-02 12:51 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-11-02 12:51 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Tian Tao, Maarten Lankhorst, Maxime Ripard, Dave Airlie,
dri-devel, Linux Kernel Mailing List
On Mon, Nov 2, 2020 at 1:40 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 02.11.20 um 13:09 schrieb Thomas Zimmermann:
> > Hi
> >
> > Am 02.11.20 um 12:19 schrieb Tian Tao:
> >> Add the detection of false for irq, so that the EINVAL is not
> >> returned when dev->irq_enabled is false.
> >>
> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> >> ---
> >> drivers/gpu/drm/drm_irq.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >> index 09d6e9e..7537a3d 100644
> >> --- a/drivers/gpu/drm/drm_irq.c
> >> +++ b/drivers/gpu/drm/drm_irq.c
> >> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
> >> bool irq_enabled;
> >> int i;
> >>
> >> + if (!dev->irq_enabled || !dev)
> >> + return 0;
> >> +
> >
> > Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
> > I'd just drop the test for !dev and assume that dev is always valid.
> >
> > I suggest to change the int return value to void and return nothing.
> >
> > Re-reading the actual implementation of this function, this location
> > might be too early. Further below there already is a test for
> > irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
> > the vblank handlers will be disabled and erroneous callers will see a
> > warning in the kernel's log.
>
> In addition to these changes, you could also add a managed
> implementation of drm_irq_install(). The canonical name should be
> devm_drm_irq_install(). The function would call drm_irq_install() and
> register a cleanup handler via devm_add_action(). Example code is at [1].
>
> KMS drivers, such as hibmc, would then not have to bother about
> uninstalling the DRM irq.
Yup, devm_ is the right fix here imo, not trying to make the uninstall
hook resilient against drivers which can't keep track of stuff. So if
that's all there is, imo this patch is a bad idea, since we have
proper tools to make driver unloading easier on driver author's
nowadays.
-Daniel
> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_drv.c#n664
>
>
> >
> > Best regards
> > Thomas
> >
> >> irq_enabled = dev->irq_enabled;
> >> dev->irq_enabled = false;
> >>
> >>
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kbuild] Re: [PATCH] drm/irq: Add irq as false detection
2020-11-02 11:19 [PATCH] drm/irq: Add irq as false detection Tian Tao
2020-11-02 12:09 ` Thomas Zimmermann
@ 2020-11-02 18:30 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-11-02 18:30 UTC (permalink / raw)
To: kbuild, Tian Tao, maarten.lankhorst, mripard, tzimmermann,
airlied, daniel, dri-devel, linux-kernel
Cc: lkp, Dan Carpenter, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]
Hi Tian,
url: https://github.com/0day-ci/linux/commits/Tian-Tao/drm-irq-Add-irq-as-false-detection/20201102-192137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cea11cd5e3b00d91caf0b4730194039b45c5891
config: x86_64-randconfig-m001-20201102 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
New smatch warnings:
drivers/gpu/drm/drm_irq.c:175 drm_irq_uninstall() warn: variable dereferenced before check 'dev' (see line 175)
Old smatch warnings:
drivers/gpu/drm/drm_irq.c:149 drm_irq_install() warn: 'irq' not released on lines: 133.
vim +/dev +175 drivers/gpu/drm/drm_irq.c
84b1fd103dbbe01 drivers/char/drm/drm_irq.c Dave Airlie 2007-07-11 169 int drm_irq_uninstall(struct drm_device *dev)
^1da177e4c3f415 drivers/char/drm/drm_irq.c Linus Torvalds 2005-04-16 170 {
dc1336ff4fe08ae drivers/gpu/drm/drm_irq.c Jesse Barnes 2009-01-06 171 unsigned long irqflags;
4423843cde65232 drivers/gpu/drm/drm_irq.c Ville Syrjälä 2013-10-04 172 bool irq_enabled;
4423843cde65232 drivers/gpu/drm/drm_irq.c Ville Syrjälä 2013-10-04 173 int i;
^1da177e4c3f415 drivers/char/drm/drm_irq.c Linus Torvalds 2005-04-16 174
fa889e8baa5377b drivers/gpu/drm/drm_irq.c Tian Tao 2020-11-02 @175 if (!dev->irq_enabled || !dev)
^^^^^^^^^^^^^^^^^^^^^^^^^^
This is checking "dev" after dereferencing it. Can "dev" even be NULL?
fa889e8baa5377b drivers/gpu/drm/drm_irq.c Tian Tao 2020-11-02 176 return 0;
fa889e8baa5377b drivers/gpu/drm/drm_irq.c Tian Tao 2020-11-02 177
^1da177e4c3f415 drivers/char/drm/drm_irq.c Linus Torvalds 2005-04-16 178 irq_enabled = dev->irq_enabled;
4423843cde65232 drivers/gpu/drm/drm_irq.c Ville Syrjälä 2013-10-04 179 dev->irq_enabled = false;
^1da177e4c3f415 drivers/char/drm/drm_irq.c Linus Torvalds 2005-04-16 180
dc1336ff4fe08ae drivers/gpu/drm/drm_irq.c Jesse Barnes 2009-01-06 181 /*
3bff93d64cf59f0 drivers/gpu/drm/drm_irq.c Daniel Vetter 2015-02-22 182 * Wake up any waiters so they don't hang. This is just to paper over
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30701 bytes --]
[-- Attachment #3: Type: text/plain, Size: 149 bytes --]
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-02 18:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 11:19 [PATCH] drm/irq: Add irq as false detection Tian Tao
2020-11-02 12:09 ` Thomas Zimmermann
2020-11-02 12:32 ` tiantao (H)
2020-11-02 12:40 ` Thomas Zimmermann
2020-11-02 12:51 ` Daniel Vetter
2020-11-02 18:30 ` [kbuild] " Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®