[PATCH v2] Fix loading of module radeonfb on PowerMac

[PATCH v2] Fix loading of module radeonfb on PowerMac

[Mathieu Malaterre]

When the linux kernel is build with (typical kernel ship with Debian
installer):

CONFIG_FB_OF=y
CONFIG_VT_HW_CONSOLE_BINDING=y
CONFIG_FB_RADEON=m

The offb driver takes precedence over module radeonfb. It is then
impossible to load the module, error reported is:

[   96.551486] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
[   96.551526] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem 0x98000000-0x9fffffff pref]
[   96.551531] radeonfb (0000:00:10.0): cannot request region 0.
[   96.551545] radeonfb: probe of 0000:00:10.0 failed with error -16

This patch reproduce the behavior of the module radeon, so as to make it
possible to load radeonfb when offb is first loaded.

It should be noticed that `offb_destroy` is never called which explain the
need to skip error detection on the radeon side.

Signed-off-by: Mathieu Malaterre <malat@debian.org>
Link: https://bugs.debian.org/826629#57
Link: https://bugzilla.kernel.org/show_bug.cgi?id=119741
Suggested-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
---
 drivers/video/fbdev/aty/radeon_base.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
index 218339a..84d634b 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -2259,6 +2259,22 @@ static struct bin_attribute edid2_attr = {
 	.read	= radeon_show_edid2,
 };
 
+static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
+{
+	struct apertures_struct *ap;
+
+	ap = alloc_apertures(1);
+	if (!ap)
+		return -ENOMEM;
+
+	ap->ranges[0].base = pci_resource_start(pdev, 0);
+	ap->ranges[0].size = pci_resource_len(pdev, 0);
+
+	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
+	kfree(ap);
+
+	return 0;
+}
 
 static int radeonfb_pci_register(struct pci_dev *pdev,
 				 const struct pci_device_id *ent)
@@ -2314,19 +2330,27 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
 	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
 	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
 
+	ret = radeon_kick_out_firmware_fb(pdev);
+	if (ret)
+		return ret;
+
 	/* request the mem regions */
 	ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
 	if (ret < 0) {
 		printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
 			pci_name(rinfo->pdev));
+#ifndef CONFIG_PPC
 		goto err_release_fb;
+#endif
 	}
 
 	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));
+#ifndef CONFIG_PPC
 		goto err_release_pci0;
+#endif
 	}
 
 	/* map the regions */
@@ -2511,10 +2535,12 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
 	iounmap(rinfo->mmio_base);
 err_release_pci2:
 	pci_release_region(pdev, 2);
+#ifndef CONFIG_PPC
 err_release_pci0:
 	pci_release_region(pdev, 0);
 err_release_fb:
         framebuffer_release(info);
+#endif
 err_disable:
 err_out:
 	return ret;
-- 
2.1.4

Re: [PATCH v2] Fix loading of module radeonfb on PowerMac

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 3866 bytes --]

Hi,

On 14/11/16 21:59, Mathieu Malaterre wrote:
> When the linux kernel is build with (typical kernel ship with Debian
> installer):
> 
> CONFIG_FB_OF=y
> CONFIG_VT_HW_CONSOLE_BINDING=y
> CONFIG_FB_RADEON=m
> 
> The offb driver takes precedence over module radeonfb. It is then
> impossible to load the module, error reported is:
> 
> [   96.551486] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
> [   96.551526] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem 0x98000000-0x9fffffff pref]
> [   96.551531] radeonfb (0000:00:10.0): cannot request region 0.
> [   96.551545] radeonfb: probe of 0000:00:10.0 failed with error -16
> 
> This patch reproduce the behavior of the module radeon, so as to make it
> possible to load radeonfb when offb is first loaded.
> 
> It should be noticed that `offb_destroy` is never called which explain the
> need to skip error detection on the radeon side.
> 
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> Link: https://bugs.debian.org/826629#57
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=119741
> Suggested-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> ---

Please always have a changelog in patch new revisions. For individual
patches, you can insert the changes here.

>  drivers/video/fbdev/aty/radeon_base.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
> index 218339a..84d634b 100644
> --- a/drivers/video/fbdev/aty/radeon_base.c
> +++ b/drivers/video/fbdev/aty/radeon_base.c
> @@ -2259,6 +2259,22 @@ static struct bin_attribute edid2_attr = {
>  	.read	= radeon_show_edid2,
>  };
>  
> +static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
> +{
> +	struct apertures_struct *ap;
> +
> +	ap = alloc_apertures(1);
> +	if (!ap)
> +		return -ENOMEM;
> +
> +	ap->ranges[0].base = pci_resource_start(pdev, 0);
> +	ap->ranges[0].size = pci_resource_len(pdev, 0);
> +
> +	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
> +	kfree(ap);
> +
> +	return 0;
> +}
>  
>  static int radeonfb_pci_register(struct pci_dev *pdev,
>  				 const struct pci_device_id *ent)
> @@ -2314,19 +2330,27 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>  	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
>  	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
>  
> +	ret = radeon_kick_out_firmware_fb(pdev);
> +	if (ret)
> +		return ret;
> +
>  	/* request the mem regions */
>  	ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
>  	if (ret < 0) {
>  		printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
>  			pci_name(rinfo->pdev));
> +#ifndef CONFIG_PPC
>  		goto err_release_fb;
> +#endif

If this is not a problem on PPC, the kernel shouldn't print an error either.

>  	}
>  
>  	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));
> +#ifndef CONFIG_PPC
>  		goto err_release_pci0;
> +#endif

Same here.

>  	}
>  
>  	/* map the regions */
> @@ -2511,10 +2535,12 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>  	iounmap(rinfo->mmio_base);
>  err_release_pci2:
>  	pci_release_region(pdev, 2);
> +#ifndef CONFIG_PPC
>  err_release_pci0:
>  	pci_release_region(pdev, 0);
>  err_release_fb:
>          framebuffer_release(info);
> +#endif
>  err_disable:
>  err_out:
>  	return ret;
> 

So I don't quite follow what's going on here. Why the CONFIG_PPC
conditionals? Is this problem only with OFFB and only with PPC?

And I think the code itself should have comments on this rather strange
behavior: the driver fails to get HW resources, but decides to ignore
the failure on PPC.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Fix loading of module radeonfb on PowerMac

[Lennart Sorensen]

On Tue, Nov 15, 2016 at 01:46:10PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/11/16 21:59, Mathieu Malaterre wrote:
> > When the linux kernel is build with (typical kernel ship with Debian
> > installer):
> > 
> > CONFIG_FB_OF=y
> > CONFIG_VT_HW_CONSOLE_BINDING=y
> > CONFIG_FB_RADEON=m
> > 
> > The offb driver takes precedence over module radeonfb. It is then
> > impossible to load the module, error reported is:
> > 
> > [   96.551486] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
> > [   96.551526] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem 0x98000000-0x9fffffff pref]
> > [   96.551531] radeonfb (0000:00:10.0): cannot request region 0.
> > [   96.551545] radeonfb: probe of 0000:00:10.0 failed with error -16
> > 
> > This patch reproduce the behavior of the module radeon, so as to make it
> > possible to load radeonfb when offb is first loaded.
> > 
> > It should be noticed that `offb_destroy` is never called which explain the
> > need to skip error detection on the radeon side.
> > 
> > Signed-off-by: Mathieu Malaterre <malat@debian.org>
> > Link: https://bugs.debian.org/826629#57
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=119741
> > Suggested-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
> > ---
> 
> Please always have a changelog in patch new revisions. For individual
> patches, you can insert the changes here.
> 
> >  drivers/video/fbdev/aty/radeon_base.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
> > index 218339a..84d634b 100644
> > --- a/drivers/video/fbdev/aty/radeon_base.c
> > +++ b/drivers/video/fbdev/aty/radeon_base.c
> > @@ -2259,6 +2259,22 @@ static struct bin_attribute edid2_attr = {
> >  	.read	= radeon_show_edid2,
> >  };
> >  
> > +static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
> > +{
> > +	struct apertures_struct *ap;
> > +
> > +	ap = alloc_apertures(1);
> > +	if (!ap)
> > +		return -ENOMEM;
> > +
> > +	ap->ranges[0].base = pci_resource_start(pdev, 0);
> > +	ap->ranges[0].size = pci_resource_len(pdev, 0);
> > +
> > +	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
> > +	kfree(ap);
> > +
> > +	return 0;
> > +}
> >  
> >  static int radeonfb_pci_register(struct pci_dev *pdev,
> >  				 const struct pci_device_id *ent)
> > @@ -2314,19 +2330,27 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
> >  	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
> >  	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
> >  
> > +	ret = radeon_kick_out_firmware_fb(pdev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* request the mem regions */
> >  	ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
> >  	if (ret < 0) {
> >  		printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
> >  			pci_name(rinfo->pdev));
> > +#ifndef CONFIG_PPC
> >  		goto err_release_fb;
> > +#endif
> 
> If this is not a problem on PPC, the kernel shouldn't print an error either.
> 
> >  	}
> >  
> >  	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));
> > +#ifndef CONFIG_PPC
> >  		goto err_release_pci0;
> > +#endif
> 
> Same here.
> 
> >  	}
> >  
> >  	/* map the regions */
> > @@ -2511,10 +2535,12 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
> >  	iounmap(rinfo->mmio_base);
> >  err_release_pci2:
> >  	pci_release_region(pdev, 2);
> > +#ifndef CONFIG_PPC
> >  err_release_pci0:
> >  	pci_release_region(pdev, 0);
> >  err_release_fb:
> >          framebuffer_release(info);
> > +#endif
> >  err_disable:
> >  err_out:
> >  	return ret;
> > 
> 
> So I don't quite follow what's going on here. Why the CONFIG_PPC
> conditionals? Is this problem only with OFFB and only with PPC?

I don't think so.  I am no convinced the pci_request_region even serves
a purpose.  Certainly a number of the other drivers don't even bother
doing it.

The problem is that offb does do it, and radeon_fb tries to do it,
and since one is trying to take over from the other, it can't do that
because the area is already reserved.

> And I think the code itself should have comments on this rather strange
> behavior: the driver fails to get HW resources, but decides to ignore
> the failure on PPC.

It seems the simpler answer is to just not do it at all.

Now if some architectures require you to do it (no idea), then that
could be an issue.

Looking at all the other FB drivers, none of the ones that support
kicking out another driver to takeover call pci_request_region.
The ones that do call it all appear to be older drivers, likely not
updated much lately.

-- 
Len Sorensen

Re: [PATCH v2] Fix loading of module radeonfb on PowerMac

[Mathieu Malaterre]

Tomi,

On Tue, Nov 15, 2016 at 12:46 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On 14/11/16 21:59, Mathieu Malaterre wrote:
>> When the linux kernel is build with (typical kernel ship with Debian
>> installer):
>>
>> CONFIG_FB_OF=y
>> CONFIG_VT_HW_CONSOLE_BINDING=y
>> CONFIG_FB_RADEON=m
>>
>> The offb driver takes precedence over module radeonfb. It is then
>> impossible to load the module, error reported is:
>>
>> [   96.551486] radeonfb 0000:00:10.0: enabling device (0006 -> 0007)
>> [   96.551526] radeonfb 0000:00:10.0: BAR 0: can't reserve [mem 0x98000000-0x9fffffff pref]
>> [   96.551531] radeonfb (0000:00:10.0): cannot request region 0.
>> [   96.551545] radeonfb: probe of 0000:00:10.0 failed with error -16
>>
>> This patch reproduce the behavior of the module radeon, so as to make it
>> possible to load radeonfb when offb is first loaded.
>>
>> It should be noticed that `offb_destroy` is never called which explain the
>> need to skip error detection on the radeon side.
>>
>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>> Link: https://bugs.debian.org/826629#57
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=119741
>> Suggested-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
>> ---
>
> Please always have a changelog in patch new revisions. For individual
> patches, you can insert the changes here.
>
>>  drivers/video/fbdev/aty/radeon_base.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
>> index 218339a..84d634b 100644
>> --- a/drivers/video/fbdev/aty/radeon_base.c
>> +++ b/drivers/video/fbdev/aty/radeon_base.c
>> @@ -2259,6 +2259,22 @@ static struct bin_attribute edid2_attr = {
>>       .read   = radeon_show_edid2,
>>  };
>>
>> +static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
>> +{
>> +     struct apertures_struct *ap;
>> +
>> +     ap = alloc_apertures(1);
>> +     if (!ap)
>> +             return -ENOMEM;
>> +
>> +     ap->ranges[0].base = pci_resource_start(pdev, 0);
>> +     ap->ranges[0].size = pci_resource_len(pdev, 0);
>> +
>> +     remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
>> +     kfree(ap);
>> +
>> +     return 0;
>> +}
>>
>>  static int radeonfb_pci_register(struct pci_dev *pdev,
>>                                const struct pci_device_id *ent)
>> @@ -2314,19 +2330,27 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>>       rinfo->fb_base_phys = pci_resource_start (pdev, 0);
>>       rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
>>
>> +     ret = radeon_kick_out_firmware_fb(pdev);
>> +     if (ret)
>> +             return ret;
>> +
>>       /* request the mem regions */
>>       ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
>>       if (ret < 0) {
>>               printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
>>                       pci_name(rinfo->pdev));
>> +#ifndef CONFIG_PPC
>>               goto err_release_fb;
>> +#endif
>
> If this is not a problem on PPC, the kernel shouldn't print an error either.
>
>>       }
>>
>>       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));
>> +#ifndef CONFIG_PPC
>>               goto err_release_pci0;
>> +#endif
>
> Same here.
>
>>       }
>>
>>       /* map the regions */
>> @@ -2511,10 +2535,12 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>>       iounmap(rinfo->mmio_base);
>>  err_release_pci2:
>>       pci_release_region(pdev, 2);
>> +#ifndef CONFIG_PPC
>>  err_release_pci0:
>>       pci_release_region(pdev, 0);
>>  err_release_fb:
>>          framebuffer_release(info);
>> +#endif
>>  err_disable:
>>  err_out:
>>       return ret;
>>
>
> So I don't quite follow what's going on here. Why the CONFIG_PPC
> conditionals? Is this problem only with OFFB and only with PPC?

The radeonfb fails to load only (conflict with offb):
- when on PPC (PMAC actually)
- when compiled as module (CONFIG_FB_RADEON=y is OK)

> And I think the code itself should have comments on this rather strange
> behavior: the driver fails to get HW resources, but decides to ignore
> the failure on PPC.

Actually you are right, I missed configuration such as Pegasos[0]
machine (no openfirmware-framebuffer implemented in their firmware at
all). So I cannot simply assume that offb already allocated stuff on
all PPC out there.

Will resent a v3 with proper changelog ASAP.

[0] https://en.wikipedia.org/wiki/Pegasos