* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
@ 2023-10-06 8:09 ` Helge Deller
0 siblings, 0 replies; 9+ messages in thread
From: Helge Deller @ 2023-10-06 8:09 UTC (permalink / raw)
To: Zhang Shurong
Cc: jiapeng.chong, linux-fbdev, javierm, dri-devel, linux-kernel,
tzimmermann
On 10/5/23 09:01, Zhang Shurong wrote:
> Add missing pci_disable_device() in error path in ark_pci_probe().
Do you have this hardware and tested your patch?
I'm sure there is a reason, why "pci_disable_device()" was commented
out in the original submission in commit 681e14730c73c...
Additionally I'm wondering why your patch doesn't show up in
the fbdev patchwork, although you added linux-fbdev mailing list.
Probably a vger issue.
Helge
> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> ---
> drivers/video/fbdev/arkfb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> index 60a96fdb5dd8..6c4e5065646f 100644
> --- a/drivers/video/fbdev/arkfb.c
> +++ b/drivers/video/fbdev/arkfb.c
> @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> err_dac:
> pci_release_regions(dev);
> err_request_regions:
> -/* pci_disable_device(dev); */
> + pci_disable_device(dev);
> err_enable_device:
> framebuffer_release(info);
> return rc;
> @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
>
> pci_iounmap(dev, info->screen_base);
> pci_release_regions(dev);
> -/* pci_disable_device(dev); */
> + pci_disable_device(dev);
>
> framebuffer_release(info);
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
2023-10-06 8:09 ` Helge Deller
@ 2023-10-06 11:19 ` Zhang Shurong
-1 siblings, 0 replies; 9+ messages in thread
From: Zhang Shurong @ 2023-10-06 11:19 UTC (permalink / raw)
To: Helge Deller
Cc: tzimmermann, jiapeng.chong, javierm, linux-fbdev, dri-devel,
linux-kernel
Since the graphic card in question is quite old, I haven't had the
opportunity
to personally test it yet.
I decided to send the patch because, upon careful consideration,
I couldn't find any compelling reasons not to include the call to
pci_disable_device(). However, I completely understand your point,
and I agree that if there is no definitive explanation for why
it should be ignored, it would be prudent to locate the actual hardware
and conduct proper testing.
I genuinely appreciate your thorough review and valuable feedback on
this matter. Maybe I will find this old card and test this patch.
Thank you and best regards,
Shurong
On 2023/10/6 16:09, Helge Deller wrote:
> On 10/5/23 09:01, Zhang Shurong wrote:
>> Add missing pci_disable_device() in error path in ark_pci_probe().
>
> Do you have this hardware and tested your patch?
> I'm sure there is a reason, why "pci_disable_device()" was commented
> out in the original submission in commit 681e14730c73c...
>
> Additionally I'm wondering why your patch doesn't show up in
> the fbdev patchwork, although you added linux-fbdev mailing list.
> Probably a vger issue.
>
> Helge
>
>
>> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
>> ---
>> drivers/video/fbdev/arkfb.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
>> index 60a96fdb5dd8..6c4e5065646f 100644
>> --- a/drivers/video/fbdev/arkfb.c
>> +++ b/drivers/video/fbdev/arkfb.c
>> @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev,
>> const struct pci_device_id *id)
>> err_dac:
>> pci_release_regions(dev);
>> err_request_regions:
>> -/* pci_disable_device(dev); */
>> + pci_disable_device(dev);
>> err_enable_device:
>> framebuffer_release(info);
>> return rc;
>> @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
>>
>> pci_iounmap(dev, info->screen_base);
>> pci_release_regions(dev);
>> -/* pci_disable_device(dev); */
>> + pci_disable_device(dev);
>>
>> framebuffer_release(info);
>> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
@ 2023-10-06 11:19 ` Zhang Shurong
0 siblings, 0 replies; 9+ messages in thread
From: Zhang Shurong @ 2023-10-06 11:19 UTC (permalink / raw)
To: Helge Deller
Cc: jiapeng.chong, linux-fbdev, javierm, dri-devel, linux-kernel,
tzimmermann
Since the graphic card in question is quite old, I haven't had the
opportunity
to personally test it yet.
I decided to send the patch because, upon careful consideration,
I couldn't find any compelling reasons not to include the call to
pci_disable_device(). However, I completely understand your point,
and I agree that if there is no definitive explanation for why
it should be ignored, it would be prudent to locate the actual hardware
and conduct proper testing.
I genuinely appreciate your thorough review and valuable feedback on
this matter. Maybe I will find this old card and test this patch.
Thank you and best regards,
Shurong
On 2023/10/6 16:09, Helge Deller wrote:
> On 10/5/23 09:01, Zhang Shurong wrote:
>> Add missing pci_disable_device() in error path in ark_pci_probe().
>
> Do you have this hardware and tested your patch?
> I'm sure there is a reason, why "pci_disable_device()" was commented
> out in the original submission in commit 681e14730c73c...
>
> Additionally I'm wondering why your patch doesn't show up in
> the fbdev patchwork, although you added linux-fbdev mailing list.
> Probably a vger issue.
>
> Helge
>
>
>> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
>> ---
>> drivers/video/fbdev/arkfb.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
>> index 60a96fdb5dd8..6c4e5065646f 100644
>> --- a/drivers/video/fbdev/arkfb.c
>> +++ b/drivers/video/fbdev/arkfb.c
>> @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev,
>> const struct pci_device_id *id)
>> err_dac:
>> pci_release_regions(dev);
>> err_request_regions:
>> -/* pci_disable_device(dev); */
>> + pci_disable_device(dev);
>> err_enable_device:
>> framebuffer_release(info);
>> return rc;
>> @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
>>
>> pci_iounmap(dev, info->screen_base);
>> pci_release_regions(dev);
>> -/* pci_disable_device(dev); */
>> + pci_disable_device(dev);
>>
>> framebuffer_release(info);
>> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
2023-10-06 8:09 ` Helge Deller
@ 2023-10-06 14:34 ` Ondrej Zary
-1 siblings, 0 replies; 9+ messages in thread
From: Ondrej Zary @ 2023-10-06 14:34 UTC (permalink / raw)
To: Helge Deller
Cc: Zhang Shurong, tzimmermann, jiapeng.chong, javierm, linux-fbdev,
dri-devel, linux-kernel
On Friday 06 October 2023, Helge Deller wrote:
> On 10/5/23 09:01, Zhang Shurong wrote:
> > Add missing pci_disable_device() in error path in ark_pci_probe().
>
> Do you have this hardware and tested your patch?
> I'm sure there is a reason, why "pci_disable_device()" was commented
> out in the original submission in commit 681e14730c73c...
pci_disable_device() call is disabled in many fbdev drivers because calling it might prevent display from working.
>
> Additionally I'm wondering why your patch doesn't show up in
> the fbdev patchwork, although you added linux-fbdev mailing list.
> Probably a vger issue.
>
> Helge
>
>
> > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> > ---
> > drivers/video/fbdev/arkfb.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> > index 60a96fdb5dd8..6c4e5065646f 100644
> > --- a/drivers/video/fbdev/arkfb.c
> > +++ b/drivers/video/fbdev/arkfb.c
> > @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > err_dac:
> > pci_release_regions(dev);
> > err_request_regions:
> > -/* pci_disable_device(dev); */
> > + pci_disable_device(dev);
> > err_enable_device:
> > framebuffer_release(info);
> > return rc;
> > @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
> >
> > pci_iounmap(dev, info->screen_base);
> > pci_release_regions(dev);
> > -/* pci_disable_device(dev); */
> > + pci_disable_device(dev);
> >
> > framebuffer_release(info);
> > }
>
>
--
Ondrej Zary
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
@ 2023-10-06 14:34 ` Ondrej Zary
0 siblings, 0 replies; 9+ messages in thread
From: Ondrej Zary @ 2023-10-06 14:34 UTC (permalink / raw)
To: Helge Deller
Cc: jiapeng.chong, linux-fbdev, javierm, dri-devel, linux-kernel,
tzimmermann, Zhang Shurong
On Friday 06 October 2023, Helge Deller wrote:
> On 10/5/23 09:01, Zhang Shurong wrote:
> > Add missing pci_disable_device() in error path in ark_pci_probe().
>
> Do you have this hardware and tested your patch?
> I'm sure there is a reason, why "pci_disable_device()" was commented
> out in the original submission in commit 681e14730c73c...
pci_disable_device() call is disabled in many fbdev drivers because calling it might prevent display from working.
>
> Additionally I'm wondering why your patch doesn't show up in
> the fbdev patchwork, although you added linux-fbdev mailing list.
> Probably a vger issue.
>
> Helge
>
>
> > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> > ---
> > drivers/video/fbdev/arkfb.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> > index 60a96fdb5dd8..6c4e5065646f 100644
> > --- a/drivers/video/fbdev/arkfb.c
> > +++ b/drivers/video/fbdev/arkfb.c
> > @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > err_dac:
> > pci_release_regions(dev);
> > err_request_regions:
> > -/* pci_disable_device(dev); */
> > + pci_disable_device(dev);
> > err_enable_device:
> > framebuffer_release(info);
> > return rc;
> > @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
> >
> > pci_iounmap(dev, info->screen_base);
> > pci_release_regions(dev);
> > -/* pci_disable_device(dev); */
> > + pci_disable_device(dev);
> >
> > framebuffer_release(info);
> > }
>
>
--
Ondrej Zary
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
2023-10-06 14:34 ` Ondrej Zary
@ 2023-10-10 11:45 ` Daniel Vetter
-1 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2023-10-10 11:45 UTC (permalink / raw)
To: Ondrej Zary
Cc: Helge Deller, jiapeng.chong, linux-fbdev, javierm, dri-devel,
linux-kernel, tzimmermann, Zhang Shurong
On Fri, Oct 06, 2023 at 04:34:01PM +0200, Ondrej Zary wrote:
> On Friday 06 October 2023, Helge Deller wrote:
> > On 10/5/23 09:01, Zhang Shurong wrote:
> > > Add missing pci_disable_device() in error path in ark_pci_probe().
> >
> > Do you have this hardware and tested your patch?
> > I'm sure there is a reason, why "pci_disable_device()" was commented
> > out in the original submission in commit 681e14730c73c...
>
> pci_disable_device() call is disabled in many fbdev drivers because
> calling it might prevent display from working.
Yeah I've recently hit some _really_ interesting regression report where
essentially fbdev driver and vgacon where managing the hardware
collaboratively. There's enormous amounts of "this shouldn't work like
that, but it is how it does" going on in old fbdev drivers, and what you
brought up is also what I immediately thought of.
This patch feels way too risky to me that we should apply it ...
-Sima
>
> >
> > Additionally I'm wondering why your patch doesn't show up in
> > the fbdev patchwork, although you added linux-fbdev mailing list.
> > Probably a vger issue.
> >
> > Helge
> >
> >
> > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> > > ---
> > > drivers/video/fbdev/arkfb.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> > > index 60a96fdb5dd8..6c4e5065646f 100644
> > > --- a/drivers/video/fbdev/arkfb.c
> > > +++ b/drivers/video/fbdev/arkfb.c
> > > @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > > err_dac:
> > > pci_release_regions(dev);
> > > err_request_regions:
> > > -/* pci_disable_device(dev); */
> > > + pci_disable_device(dev);
> > > err_enable_device:
> > > framebuffer_release(info);
> > > return rc;
> > > @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
> > >
> > > pci_iounmap(dev, info->screen_base);
> > > pci_release_regions(dev);
> > > -/* pci_disable_device(dev); */
> > > + pci_disable_device(dev);
> > >
> > > framebuffer_release(info);
> > > }
> >
> >
>
>
>
> --
> Ondrej Zary
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] video: fbdev: arkfb: fix possible object reference leak
@ 2023-10-10 11:45 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2023-10-10 11:45 UTC (permalink / raw)
To: Ondrej Zary
Cc: jiapeng.chong, linux-fbdev, Helge Deller, linux-kernel, dri-devel,
javierm, tzimmermann, Zhang Shurong
On Fri, Oct 06, 2023 at 04:34:01PM +0200, Ondrej Zary wrote:
> On Friday 06 October 2023, Helge Deller wrote:
> > On 10/5/23 09:01, Zhang Shurong wrote:
> > > Add missing pci_disable_device() in error path in ark_pci_probe().
> >
> > Do you have this hardware and tested your patch?
> > I'm sure there is a reason, why "pci_disable_device()" was commented
> > out in the original submission in commit 681e14730c73c...
>
> pci_disable_device() call is disabled in many fbdev drivers because
> calling it might prevent display from working.
Yeah I've recently hit some _really_ interesting regression report where
essentially fbdev driver and vgacon where managing the hardware
collaboratively. There's enormous amounts of "this shouldn't work like
that, but it is how it does" going on in old fbdev drivers, and what you
brought up is also what I immediately thought of.
This patch feels way too risky to me that we should apply it ...
-Sima
>
> >
> > Additionally I'm wondering why your patch doesn't show up in
> > the fbdev patchwork, although you added linux-fbdev mailing list.
> > Probably a vger issue.
> >
> > Helge
> >
> >
> > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> > > ---
> > > drivers/video/fbdev/arkfb.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> > > index 60a96fdb5dd8..6c4e5065646f 100644
> > > --- a/drivers/video/fbdev/arkfb.c
> > > +++ b/drivers/video/fbdev/arkfb.c
> > > @@ -1064,7 +1064,7 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > > err_dac:
> > > pci_release_regions(dev);
> > > err_request_regions:
> > > -/* pci_disable_device(dev); */
> > > + pci_disable_device(dev);
> > > err_enable_device:
> > > framebuffer_release(info);
> > > return rc;
> > > @@ -1085,7 +1085,7 @@ static void ark_pci_remove(struct pci_dev *dev)
> > >
> > > pci_iounmap(dev, info->screen_base);
> > > pci_release_regions(dev);
> > > -/* pci_disable_device(dev); */
> > > + pci_disable_device(dev);
> > >
> > > framebuffer_release(info);
> > > }
> >
> >
>
>
>
> --
> Ondrej Zary
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread