linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 related	[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;
as well as URLs for NNTP newsgroup(s).