Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #55322 > unrolled thread

Bug#826629: Possible offb unload fix.

Started byMathieu Malaterre <malat@debian.org>
First post2016-10-04 20:50 +0200
Last post2016-10-05 18:40 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.debian.kernel

Contents

  Bug#826629: Possible offb unload fix. Mathieu Malaterre <malat@debian.org> - 2016-10-04 20:50 +0200
    Bug#826629: Possible offb unload fix. lsorense@csclub.uwaterloo.ca (Lennart Sorensen) - 2016-10-04 21:20 +0200
      Bug#826629: Possible offb unload fix. Mathieu Malaterre <malat@debian.org> - 2016-10-04 21:50 +0200
        Bug#826629: Possible offb unload fix. lsorense@csclub.uwaterloo.ca (Lennart Sorensen) - 2016-10-04 23:40 +0200
          Bug#826629: Possible offb unload fix. Mathieu Malaterre <malat@debian.org> - 2016-10-05 18:40 +0200

#55322 — Bug#826629: Possible offb unload fix.

FromMathieu Malaterre <malat@debian.org>
Date2016-10-04 20:50 +0200
SubjectBug#826629: Possible offb unload fix.
Message-ID<soCWR-6rC-3@gated-at.bofh.it>
Hi Len,

Here is the release function I am using:

static void offb_destroy(struct fb_info *info)
{
        struct offb_par *par = (struct offb_par *) info->par;
        if (info->screen_base)
                iounmap(info->screen_base);
        if (par->cmap_adr != NULL) {
                iounmap(par->cmap_adr);
                par->cmap_adr = NULL;
        }
        release_mem_region(info->apertures->ranges[0].base,
info->apertures->ranges[0].size);
        framebuffer_release(info);
}


(you need the cast to avoid warning about deref of void*).

And if I do `modprobe radeonfb`:

[   72.163546] bus: 'pci': add driver radeonfb
[   72.163618] bus: 'pci': driver_probe_device: matched device
0000:00:10.0 with driver radeonfb
[   72.163627] bus: 'pci': really_probe: probing driver radeonfb with
device 0000:00:10.0
[   72.163651] devices_kset: Moving 0000:00:10.0 to end of list
[   72.163659] radeonfb_pci_register BEGIN
[   72.163680] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
[   72.163721] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
0x98000000-0x9fffffff pref]
[   72.163726] radeonfb (0000:00:10.0): cannot request region 0.
[   72.163746] radeonfb: probe of 0000:00:10.0 failed with error -16

[toc] | [next] | [standalone]

#55323

Fromlsorense@csclub.uwaterloo.ca (Lennart Sorensen)
Date2016-10-04 21:20 +0200
Message-ID<soDpT-6Rr-11@gated-at.bofh.it>
In reply to#55322
On Tue, Oct 04, 2016 at 08:41:45PM +0200, Mathieu Malaterre wrote:
> Hi Len,
> 
> Here is the release function I am using:
> 
> static void offb_destroy(struct fb_info *info)
> {
>         struct offb_par *par = (struct offb_par *) info->par;
>         if (info->screen_base)
>                 iounmap(info->screen_base);
>         if (par->cmap_adr != NULL) {
>                 iounmap(par->cmap_adr);
>                 par->cmap_adr = NULL;
>         }
>         release_mem_region(info->apertures->ranges[0].base,
> info->apertures->ranges[0].size);
>         framebuffer_release(info);
> }
> 
> 
> (you need the cast to avoid warning about deref of void*).
> 
> And if I do `modprobe radeonfb`:
> 
> [   72.163546] bus: 'pci': add driver radeonfb
> [   72.163618] bus: 'pci': driver_probe_device: matched device
> 0000:00:10.0 with driver radeonfb
> [   72.163627] bus: 'pci': really_probe: probing driver radeonfb with
> device 0000:00:10.0
> [   72.163651] devices_kset: Moving 0000:00:10.0 to end of list
> [   72.163659] radeonfb_pci_register BEGIN
> [   72.163680] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
> [   72.163721] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
> 0x98000000-0x9fffffff pref]
> [   72.163726] radeonfb (0000:00:10.0): cannot request region 0.
> [   72.163746] radeonfb: probe of 0000:00:10.0 failed with error -16

Could you put a print statement in offb_destroy to make sure that is
actually being called?

And this is radeonfb with code added to actually try to kick out offb,
right?

-- 
Len Sorensen

[toc] | [prev] | [next] | [standalone]

#55324

FromMathieu Malaterre <malat@debian.org>
Date2016-10-04 21:50 +0200
Message-ID<soDSV-71V-13@gated-at.bofh.it>
In reply to#55323

[Multipart message — attachments visible in raw view] — view raw

On Tue, Oct 4, 2016 at 9:13 PM, Lennart Sorensen
<lsorense@csclub.uwaterloo.ca> wrote:
> On Tue, Oct 04, 2016 at 08:41:45PM +0200, Mathieu Malaterre wrote:
>> Hi Len,
>>
>> Here is the release function I am using:
>>
>> static void offb_destroy(struct fb_info *info)
>> {
>>         struct offb_par *par = (struct offb_par *) info->par;
>>         if (info->screen_base)
>>                 iounmap(info->screen_base);
>>         if (par->cmap_adr != NULL) {
>>                 iounmap(par->cmap_adr);
>>                 par->cmap_adr = NULL;
>>         }
>>         release_mem_region(info->apertures->ranges[0].base,
>> info->apertures->ranges[0].size);
>>         framebuffer_release(info);
>> }
>>
>>
>> (you need the cast to avoid warning about deref of void*).
>>
>> And if I do `modprobe radeonfb`:
>>
>> [   72.163546] bus: 'pci': add driver radeonfb
>> [   72.163618] bus: 'pci': driver_probe_device: matched device
>> 0000:00:10.0 with driver radeonfb
>> [   72.163627] bus: 'pci': really_probe: probing driver radeonfb with
>> device 0000:00:10.0
>> [   72.163651] devices_kset: Moving 0000:00:10.0 to end of list
>> [   72.163659] radeonfb_pci_register BEGIN
>> [   72.163680] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
>> [   72.163721] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
>> 0x98000000-0x9fffffff pref]
>> [   72.163726] radeonfb (0000:00:10.0): cannot request region 0.
>> [   72.163746] radeonfb: probe of 0000:00:10.0 failed with error -16
>
> Could you put a print statement in offb_destroy to make sure that is
> actually being called?
>
> And this is radeonfb with code added to actually try to kick out offb,
> right?

Here is what I see:

[   52.270154] bus: 'pci': add driver radeonfb
[   52.270224] bus: 'pci': driver_probe_device: matched device
0000:00:10.0 with driver radeonfb
[   52.270233] bus: 'pci': really_probe: probing driver radeonfb with
device 0000:00:10.0
[   52.270256] devices_kset: Moving 0000:00:10.0 to end of list
[   52.270264] radeonfb_pci_register BEGIN
[   52.270267] radeonfb: MM1
[   52.275001] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
[   52.279727] radeonfb: MM2
[   52.295202] radeonfb: MM3
[   52.299816] radeonfb: MM4
[   52.304934] radeonfb: MM0 98000000 8000000 radeonfb
[   52.309768] checking generic (9c008000 96000) vs hw (98000000 8000000)
[   52.309776] fb: switching to radeonfb from OFfb ATY,RockHo
[   52.315075] Console: switching to colour dummy device 80x25
[   52.315595] device: 'fb0': device_unregister
[   52.315736] PM: Removing info for No Bus:fb0
[   52.317348] device: 'fb0': device_create_release
[   52.317407] radeonfb: MM5 0
[   52.317447] device: 'vtcon1': device_unregister
[   52.317500] PM: Removing info for No Bus:vtcon1
[   52.317565] device: 'vtcon1': device_create_release
[   52.318992] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
0x98000000-0x9fffffff pref]
[   52.319029] radeonfb (0000:00:10.0): cannot request region 0.
[   52.319066] radeonfb: probe of 0000:00:10.0 failed with error -16

With patch attached.

[toc] | [prev] | [next] | [standalone]

#55328

Fromlsorense@csclub.uwaterloo.ca (Lennart Sorensen)
Date2016-10-04 23:40 +0200
Message-ID<soFBn-8cl-5@gated-at.bofh.it>
In reply to#55324
On Tue, Oct 04, 2016 at 09:39:12PM +0200, Mathieu Malaterre wrote:
> Here is what I see:
> 
> [   52.270154] bus: 'pci': add driver radeonfb
> [   52.270224] bus: 'pci': driver_probe_device: matched device
> 0000:00:10.0 with driver radeonfb
> [   52.270233] bus: 'pci': really_probe: probing driver radeonfb with
> device 0000:00:10.0
> [   52.270256] devices_kset: Moving 0000:00:10.0 to end of list
> [   52.270264] radeonfb_pci_register BEGIN
> [   52.270267] radeonfb: MM1
> [   52.275001] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
> [   52.279727] radeonfb: MM2
> [   52.295202] radeonfb: MM3
> [   52.299816] radeonfb: MM4
> [   52.304934] radeonfb: MM0 98000000 8000000 radeonfb
> [   52.309768] checking generic (9c008000 96000) vs hw (98000000 8000000)
> [   52.309776] fb: switching to radeonfb from OFfb ATY,RockHo
> [   52.315075] Console: switching to colour dummy device 80x25
> [   52.315595] device: 'fb0': device_unregister
> [   52.315736] PM: Removing info for No Bus:fb0
> [   52.317348] device: 'fb0': device_create_release
> [   52.317407] radeonfb: MM5 0
> [   52.317447] device: 'vtcon1': device_unregister
> [   52.317500] PM: Removing info for No Bus:vtcon1
> [   52.317565] device: 'vtcon1': device_create_release
> [   52.318992] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
> 0x98000000-0x9fffffff pref]
> [   52.319029] radeonfb (0000:00:10.0): cannot request region 0.
> [   52.319066] radeonfb: probe of 0000:00:10.0 failed with error -16

So offb_destroy is NOT called.  Well that explains the problem.

Unfortunately it seems the reason it isn't called is that the
fb->info->count is not zero, because it is still open.  I am not sure
how the ability to unregister a conflicting framebuffer is supposed to
work if it stays active until it is replaced.

The kick out appears to successfully remove offb from being the active
fb, but since the release hasn't been called yet, the resources are not
yet freed, so radeonfb fails when trying to reserve them.  If offb_destroy
had been called the resources would be free and there would almost
certainly not be a problem.

Now the radeon.ko gets away with this because it doesn't try to reserve
the memory and never calls pci_request_region at all.

How about adding this to your patch:

--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -2319,14 +2319,14 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
 	if (ret < 0) {
 		printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
 			pci_name(rinfo->pdev));
-		goto err_release_fb;
+		//goto err_release_fb;
 	}
 
 	ret = pci_request_region(pdev, 2, "radeonfb mmio");
 	if (ret < 0) {
 		printk( KERN_ERR "radeonfb (%s): cannot request region 2.\n",
 			pci_name(rinfo->pdev));
-		goto err_release_pci0;
+		//goto err_release_pci0;
 	}
 
 	/* map the regions */

So it will still complain, but it will ignore the fact the memory
reservation failed, and still continue and do the ioremap and try to
use it.

That is how radeon.ko works as far as I can tell.

Of course once that is done, then the changes to offb.c become irrelevant.

-- 
Len Sorensen

[toc] | [prev] | [next] | [standalone]

#55332

FromMathieu Malaterre <malat@debian.org>
Date2016-10-05 18:40 +0200
Message-ID<soXoC-3mA-19@gated-at.bofh.it>
In reply to#55328
Control: tags -1 + patch

On Tue, Oct 4, 2016 at 11:37 PM, Lennart Sorensen
<lsorense@csclub.uwaterloo.ca> wrote:
> On Tue, Oct 04, 2016 at 09:39:12PM +0200, Mathieu Malaterre wrote:
>> Here is what I see:
>>
>> [   52.270154] bus: 'pci': add driver radeonfb
>> [   52.270224] bus: 'pci': driver_probe_device: matched device
>> 0000:00:10.0 with driver radeonfb
>> [   52.270233] bus: 'pci': really_probe: probing driver radeonfb with
>> device 0000:00:10.0
>> [   52.270256] devices_kset: Moving 0000:00:10.0 to end of list
>> [   52.270264] radeonfb_pci_register BEGIN
>> [   52.270267] radeonfb: MM1
>> [   52.275001] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
>> [   52.279727] radeonfb: MM2
>> [   52.295202] radeonfb: MM3
>> [   52.299816] radeonfb: MM4
>> [   52.304934] radeonfb: MM0 98000000 8000000 radeonfb
>> [   52.309768] checking generic (9c008000 96000) vs hw (98000000 8000000)
>> [   52.309776] fb: switching to radeonfb from OFfb ATY,RockHo
>> [   52.315075] Console: switching to colour dummy device 80x25
>> [   52.315595] device: 'fb0': device_unregister
>> [   52.315736] PM: Removing info for No Bus:fb0
>> [   52.317348] device: 'fb0': device_create_release
>> [   52.317407] radeonfb: MM5 0
>> [   52.317447] device: 'vtcon1': device_unregister
>> [   52.317500] PM: Removing info for No Bus:vtcon1
>> [   52.317565] device: 'vtcon1': device_create_release
>> [   52.318992] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem
>> 0x98000000-0x9fffffff pref]
>> [   52.319029] radeonfb (0000:00:10.0): cannot request region 0.
>> [   52.319066] radeonfb: probe of 0000:00:10.0 failed with error -16
>
> So offb_destroy is NOT called.  Well that explains the problem.
>
> Unfortunately it seems the reason it isn't called is that the
> fb->info->count is not zero, because it is still open.  I am not sure
> how the ability to unregister a conflicting framebuffer is supposed to
> work if it stays active until it is replaced.
>
> The kick out appears to successfully remove offb from being the active
> fb, but since the release hasn't been called yet, the resources are not
> yet freed, so radeonfb fails when trying to reserve them.  If offb_destroy
> had been called the resources would be free and there would almost
> certainly not be a problem.
>
> Now the radeon.ko gets away with this because it doesn't try to reserve
> the memory and never calls pci_request_region at all.
>
> How about adding this to your patch:
>
> --- a/drivers/video/fbdev/aty/radeon_base.c
> +++ b/drivers/video/fbdev/aty/radeon_base.c
> @@ -2319,14 +2319,14 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>         if (ret < 0) {
>                 printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
>                         pci_name(rinfo->pdev));
> -               goto err_release_fb;
> +               //goto err_release_fb;
>         }
>
>         ret = pci_request_region(pdev, 2, "radeonfb mmio");
>         if (ret < 0) {
>                 printk( KERN_ERR "radeonfb (%s): cannot request region 2.\n",
>                         pci_name(rinfo->pdev));
> -               goto err_release_pci0;
> +               //goto err_release_pci0;
>         }
>
>         /* map the regions */
>
> So it will still complain, but it will ignore the fact the memory
> reservation failed, and still continue and do the ioremap and try to
> use it.
>
> That is how radeon.ko works as far as I can tell.
>
> Of course once that is done, then the changes to offb.c become irrelevant.

Very impressive ! I'll clean it up and ask upstream for review.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.kernel

csiph-web