[PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Christophe JAILLET]

If an error occurs after request_mem_region(), a corresponding
release_mem_region() should be called, as already done in the remove
function.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
*Not* even compile tested only.
I don't know on what architecture it relies on.

So it is provided as-is
---
 drivers/video/fbdev/hpfb.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/hpfb.c b/drivers/video/fbdev/hpfb.c
index 66fac8e5393e..87b8dcdc1cf3 100644
--- a/drivers/video/fbdev/hpfb.c
+++ b/drivers/video/fbdev/hpfb.c
@@ -342,12 +342,17 @@ static int hpfb_dio_probe(struct dio_dev *d, const struct dio_device_id *ent)
 	}
 	printk(KERN_INFO "Topcat found at DIO select code %d "
 	       "(secondary id %02x)\n", d->scode, (d->id >> 8) & 0xff);
-	if (hpfb_init_one(paddr, vaddr)) {
-		if (d->scode >= DIOII_SCBASE)
-			iounmap((void *)vaddr);
-		return -ENOMEM;
-	}
+	if (hpfb_init_one(paddr, vaddr))
+		goto err_unmap;
+
 	return 0;
+
+err_unmap:
+	if (d->scode >= DIOII_SCBASE)
+		iounmap((void *)vaddr);
+	release_mem_region(d->resource.start, resource_size(&d->resource));
+
+	return -ENOMEM;
 }
 
 static void hpfb_remove_one(struct dio_dev *d)
-- 
2.45.2

Re: [PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Helge Deller]

On 7/28/24 20:29, Christophe JAILLET wrote:
> If an error occurs after request_mem_region(), a corresponding
> release_mem_region() should be called, as already done in the remove
> function.

True.

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

I think we can drop this "Fixes" tag, as it gives no real info.

> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> *Not* even compile tested only.

Ok.

> I don't know on what architecture it relies on.

HP300 are old HP machines with an m68k CPU.
Not sure if someone still has such a machine :-)

> So it is provided as-is
> ---
>   drivers/video/fbdev/hpfb.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/hpfb.c b/drivers/video/fbdev/hpfb.c
> index 66fac8e5393e..87b8dcdc1cf3 100644
> --- a/drivers/video/fbdev/hpfb.c
> +++ b/drivers/video/fbdev/hpfb.c
> @@ -342,12 +342,17 @@ static int hpfb_dio_probe(struct dio_dev *d, const struct dio_device_id *ent)
>   	}
>   	printk(KERN_INFO "Topcat found at DIO select code %d "
>   	       "(secondary id %02x)\n", d->scode, (d->id >> 8) & 0xff);
> -	if (hpfb_init_one(paddr, vaddr)) {
> -		if (d->scode >= DIOII_SCBASE)
> -			iounmap((void *)vaddr);

This driver hasn't changed in years, and I don't expect we will
have many other changes, so in this case I think simply adding the one line:
+	release_mem_region(d->resource.start, resource_size(&d->resource));
here is sufficient without adding additional jump targets.

I can fix it up here, or please send a new patch.

Helge


> -		return -ENOMEM;
> -	}
> +	if (hpfb_init_one(paddr, vaddr))
> +		goto err_unmap;
> +
>   	return 0;
> +
> +err_unmap:
> +	if (d->scode >= DIOII_SCBASE)
> +		iounmap((void *)vaddr);
> +	release_mem_region(d->resource.start, resource_size(&d->resource));
> +
> +	return -ENOMEM;
>   }
>
>   static void hpfb_remove_one(struct dio_dev *d)

Re: [PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Dan Carpenter]

On Mon, Jul 29, 2024 at 10:13:17AM +0200, Helge Deller wrote:
> On 7/28/24 20:29, Christophe JAILLET wrote:
> > If an error occurs after request_mem_region(), a corresponding
> > release_mem_region() should be called, as already done in the remove
> > function.
> 
> True.
> 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> I think we can drop this "Fixes" tag, as it gives no real info.
> 

If we're backporting patches then these tags really are useful.  As
I've been doing more and more backporting, I've come to believe this
more firmly.

I don't necessarily think this patch is worth backporting, but leaving
the Fixes tag off doesn't mean it won't happen.  People quite often
leave the Fixes tags off of real fixes by mistake so AUTOSEL could still
pick it up.  You'd have to add:
Cc: <stable+noautosel@kernel.org> # reason goes here, and must be present

> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > *Not* even compile tested only.
> 
> Ok.
> 
> > I don't know on what architecture it relies on.
> 
> HP300 are old HP machines with an m68k CPU.
> Not sure if someone still has such a machine :-)
> 

It surprised me how many patches we backport for ancient stuff.  But I
guess the risk/reward equation still works because if the code isn't
used there the risk is very small.

regards,
dan carpenter

Re: [PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Helge Deller]

On 7/29/24 17:59, Dan Carpenter wrote:
> On Mon, Jul 29, 2024 at 10:13:17AM +0200, Helge Deller wrote:
>> On 7/28/24 20:29, Christophe JAILLET wrote:
>>> If an error occurs after request_mem_region(), a corresponding
>>> release_mem_region() should be called, as already done in the remove
>>> function.
>>
>> True.
>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>
>> I think we can drop this "Fixes" tag, as it gives no real info.
>
> If we're backporting patches then these tags really are useful.  As
> I've been doing more and more backporting, I've come to believe this
> more firmly.

Sure, "Fixes" tags are useful, but only if they really refer
to another patch which introduced the specific issue.

But the tag 1da177e4c3f4 ("Linux-2.6.12-rc2") isn't useful, as it's
just the initial git commit. It has no relation to why release_mem_region()
might have been initially missed. See:

  commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 (tag: v2.6.12-rc2)
Author: Linus Torvalds <torvalds@ppc970.osdl.org>
Date:   Sat Apr 16 15:20:36 2005 -0700

     Linux-2.6.12-rc2

     Initial git repository build. I'm not bothering with the full history,
     even though we have it. We can create a separate "historical" git
     archive of that later if we want to, and in the meantime it's about
     3.2GB when imported into git - space that would just make the early
     git days unnecessarily complicated, when we don't have a lot of good
     infrastructure for it.

Helge

Re: [PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Christophe JAILLET]

Le 29/07/2024 à 22:09, Helge Deller a écrit :
> On 7/29/24 17:59, Dan Carpenter wrote:
>> On Mon, Jul 29, 2024 at 10:13:17AM +0200, Helge Deller wrote:
>>> On 7/28/24 20:29, Christophe JAILLET wrote:
>>>> If an error occurs after request_mem_region(), a corresponding
>>>> release_mem_region() should be called, as already done in the remove
>>>> function.
>>>
>>> True.
>>>
>>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>>
>>> I think we can drop this "Fixes" tag, as it gives no real info.
>>
>> If we're backporting patches then these tags really are useful.  As
>> I've been doing more and more backporting, I've come to believe this
>> more firmly.
> 
> Sure, "Fixes" tags are useful, but only if they really refer
> to another patch which introduced the specific issue.
> 
> But the tag 1da177e4c3f4 ("Linux-2.6.12-rc2") isn't useful, as it's
> just the initial git commit. It has no relation to why release_mem_region()
> might have been initially missed. See:

I agree that the description of this specific tag is not useful by itself.

But at least it means: should it be backported, it can be done up to 
there. (and sometimes LWN gives some statistics on how long it took to 
fix an "issue", should it be considered as such)

Without it, it is not easy to guess in which branch the patch is meaningful.

I'll sent a v2 with your suggested minimal change, but I'll keep the 
Fixes tag.


Up to you to remove it or not, and to add a <stable@kernel.org> or a 
<stable+noautosel@kernel.org> or none of them.

Any solution is fine with me.


> 
>   commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 (tag: v2.6.12-rc2)
> Author: Linus Torvalds <torvalds@ppc970.osdl.org>
> Date:   Sat Apr 16 15:20:36 2005 -0700
> 
>      Linux-2.6.12-rc2
> 
>      Initial git repository build. I'm not bothering with the full history,
>      even though we have it. We can create a separate "historical" git
>      archive of that later if we want to, and in the meantime it's about
>      3.2GB when imported into git - space that would just make the early
>      git days unnecessarily complicated, when we don't have a lot of good
>      infrastructure for it.
> 
> Helge
> 
> 

On:

 > HP300 are old HP machines with an m68k CPU.
 > Not sure if someone still has such a machine 🙂

so it really was the one I found on wikipedia, LoL!

So, another way to "fix" the issue is maybe to deprecate the driver or 
everything related to this old architecture?

No strong opinion about it.

CJ

Re: [PATCH] fbdev/hpfb: Fix an error handling path in hpfb_dio_probe()

[Dan Carpenter]

On Mon, Jul 29, 2024 at 10:09:39PM +0200, Helge Deller wrote:
> On 7/29/24 17:59, Dan Carpenter wrote:
> > On Mon, Jul 29, 2024 at 10:13:17AM +0200, Helge Deller wrote:
> > > On 7/28/24 20:29, Christophe JAILLET wrote:
> > > > If an error occurs after request_mem_region(), a corresponding
> > > > release_mem_region() should be called, as already done in the remove
> > > > function.
> > > 
> > > True.
> > > 
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > 
> > > I think we can drop this "Fixes" tag, as it gives no real info.
> > 
> > If we're backporting patches then these tags really are useful.  As
> > I've been doing more and more backporting, I've come to believe this
> > more firmly.
> 
> Sure, "Fixes" tags are useful, but only if they really refer
> to another patch which introduced the specific issue.
> 
> But the tag 1da177e4c3f4 ("Linux-2.6.12-rc2") isn't useful, as it's
> just the initial git commit. It has no relation to why release_mem_region()
> might have been initially missed. See:

In the last couple stable kernels we've backported some pretty serious
networking commits that have Linux-2.6.12-rc2 for the Fixes tag.  So if
it's security related that's really important information.

For minor stuff like this, the commit will be backported as far back as
possible and until it ends up in a list of failed commits.

When I'm reviewing the list of failed patches and there is no Fixes tag
I think maybe it was backported to all the affected kernels?  In that
case, I could have skipped the manual review if the patch was just
tagged correctly.  Then I wonder, why wasn't it tagged?  I just assume
it was sloppiness honestly.  I'm probably not going to spend that much
time on it, but it's annoying.

When a commit lists Linux-2.6.12-rc2 as the Fixes then it still ends up
in the failed list.  But it can't affect too many users if we're only
getting around to fixing it now.  It's easier to ignore.

regards,
dan carpenter