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;
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;
在 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;
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/driv...
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
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/driv...
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
Hi Tian,
url: https://github.com/0day-ci/linux/commits/Tian-Tao/drm-irq-Add-irq-as-false-d... 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
_______________________________________________ kbuild mailing list -- kbuild@lists.01.org To unsubscribe send an email to kbuild-leave@lists.01.org
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Daniel Vetter -
Thomas Zimmermann -
Tian Tao -
tiantao (H)