| Message ID | dc4fe3d857849ac63131c5620f1bacf1a3d7172e.1722191367.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbdev/hpfb: Fix an error handling path in hpfb_dio_probe() | expand |
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)
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
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
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
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
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)
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(-)