Patchwork Do not try loading option ROM for hotplug PCI device in pc-0.11 compat mode

login
register
mail settings
Submitter Jes Sorensen
Date July 23, 2010, 3:56 p.m.
Message ID <1279900598-8432-1-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/59814/
State New
Headers show

Comments

Jes Sorensen - July 23, 2010, 3:56 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
at init time, it is not possible to load an option ROM for a hotplug
device when running in compat mode.

v2: Alex Williamson pointed out that one can get to qdev directly from
pci_dev, so no need to pass it down.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hw/pci.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
Aurelien Jarno - July 30, 2010, 9:08 p.m.
On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
> at init time, it is not possible to load an option ROM for a hotplug
> device when running in compat mode.
> 
> v2: Alex Williamson pointed out that one can get to qdev directly from
> pci_dev, so no need to pass it down.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  hw/pci.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index a98d6f3..2d38643 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
>          return 0;
>  
>      if (!pdev->rom_bar) {
> +        int class;
>          /*
>           * Load rom via fw_cfg instead of creating a rom bar,
> -         * for 0.11 compatibility.
> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
> +         * we cannot do hotplug load of option roms.
>           */
> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> +        if (pdev->qdev.hotplugged)
> +            return 0;

Missing braces around the return 0 line. 

> +        class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
>          if (class == 0x0300) {
>              rom_add_vga(pdev->romfile);
>          } else {
> -- 
> 1.7.1.1

Otherwise looks good.
Jes Sorensen - July 31, 2010, 9:16 a.m.
On 07/30/10 23:08, Aurelien Jarno wrote:
> On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
>> at init time, it is not possible to load an option ROM for a hotplug
>> device when running in compat mode.
>>
>> v2: Alex Williamson pointed out that one can get to qdev directly from
>> pci_dev, so no need to pass it down.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  hw/pci.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index a98d6f3..2d38643 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>          return 0;
>>  
>>      if (!pdev->rom_bar) {
>> +        int class;
>>          /*
>>           * Load rom via fw_cfg instead of creating a rom bar,
>> -         * for 0.11 compatibility.
>> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
>> +         * we cannot do hotplug load of option roms.
>>           */
>> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
>> +        if (pdev->qdev.hotplugged)
>> +            return 0;
> 
> Missing braces around the return 0 line. 

Half the QEMU code base doesn't have braces around single line if
statements, including in hw/pci.c. Adding braces here would be inconsistent.

Jes
Aurelien Jarno - July 31, 2010, 2:19 p.m.
On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
> On 07/30/10 23:08, Aurelien Jarno wrote:
> > On Fri, Jul 23, 2010 at 05:56:38PM +0200, Jes.Sorensen@redhat.com wrote:
> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>
> >> pc-0.11 and older uses fw_cfg to provide option ROMs. As fw_cfg is setup
> >> at init time, it is not possible to load an option ROM for a hotplug
> >> device when running in compat mode.
> >>
> >> v2: Alex Williamson pointed out that one can get to qdev directly from
> >> pci_dev, so no need to pass it down.
> >>
> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> ---
> >>  hw/pci.c |    8 ++++++--
> >>  1 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/pci.c b/hw/pci.c
> >> index a98d6f3..2d38643 100644
> >> --- a/hw/pci.c
> >> +++ b/hw/pci.c
> >> @@ -1810,11 +1810,15 @@ static int pci_add_option_rom(PCIDevice *pdev)
> >>          return 0;
> >>  
> >>      if (!pdev->rom_bar) {
> >> +        int class;
> >>          /*
> >>           * Load rom via fw_cfg instead of creating a rom bar,
> >> -         * for 0.11 compatibility.
> >> +         * for 0.11 compatibility. fw_cfg is initialized at boot, so
> >> +         * we cannot do hotplug load of option roms.
> >>           */
> >> -        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
> >> +        if (pdev->qdev.hotplugged)
> >> +            return 0;
> > 
> > Missing braces around the return 0 line. 
> 
> Half the QEMU code base doesn't have braces around single line if
> statements, including in hw/pci.c. Adding braces here would be inconsistent.
> 

If we follow the coding style in new patches, it will eventually become
consistent.
Jes Sorensen - July 31, 2010, 2:40 p.m.
On 07/31/10 16:19, Aurelien Jarno wrote:
> On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
>> On 07/30/10 23:08, Aurelien Jarno wrote:
>>> Missing braces around the return 0 line. 
>>
>> Half the QEMU code base doesn't have braces around single line if
>> statements, including in hw/pci.c. Adding braces here would be inconsistent.
>>
> 
> If we follow the coding style in new patches, it will eventually become
> consistent.
> 

If you want that, please do it in a separate patch for the entire file,
otherwise it will never become consistent. However it doesn't change the
issue either that putting braces around a single line like this is bad
coding style.

Jes
Aurelien Jarno - July 31, 2010, 3:13 p.m.
On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
> On 07/31/10 16:19, Aurelien Jarno wrote:
> > On Sat, Jul 31, 2010 at 11:16:45AM +0200, Jes Sorensen wrote:
> >> On 07/30/10 23:08, Aurelien Jarno wrote:
> >>> Missing braces around the return 0 line. 
> >>
> >> Half the QEMU code base doesn't have braces around single line if
> >> statements, including in hw/pci.c. Adding braces here would be inconsistent.
> >>
> > 
> > If we follow the coding style in new patches, it will eventually become
> > consistent.
> > 
> 
> If you want that, please do it in a separate patch for the entire file,
> otherwise it will never become consistent. However it doesn't change the
> issue either that putting braces around a single line like this is bad
> coding style.

We got this discussions numerous times, and I am not going to start it
again. If you want to see the patch applied, just follow the rules.
Jes Sorensen - July 31, 2010, 3:21 p.m.
On 07/31/10 17:13, Aurelien Jarno wrote:
> On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
>> If you want that, please do it in a separate patch for the entire file,
>> otherwise it will never become consistent. However it doesn't change the
>> issue either that putting braces around a single line like this is bad
>> coding style.
> 
> We got this discussions numerous times, and I am not going to start it
> again. If you want to see the patch applied, just follow the rules.
> 

Then  I would like to know why you do not apply this silly rule to every
patch submitted. There's patches going in almost every day that doesn't
use this hopeless formatting.

Jes
Aurelien Jarno - July 31, 2010, 3:29 p.m.
On Sat, Jul 31, 2010 at 05:21:34PM +0200, Jes Sorensen wrote:
> On 07/31/10 17:13, Aurelien Jarno wrote:
> > On Sat, Jul 31, 2010 at 04:40:05PM +0200, Jes Sorensen wrote:
> >> If you want that, please do it in a separate patch for the entire file,
> >> otherwise it will never become consistent. However it doesn't change the
> >> issue either that putting braces around a single line like this is bad
> >> coding style.
> > 
> > We got this discussions numerous times, and I am not going to start it
> > again. If you want to see the patch applied, just follow the rules.
> > 
> 
> Then  I would like to know why you do not apply this silly rule to every
> patch submitted. There's patches going in almost every day that doesn't
> use this hopeless formatting.
> 

Because not everyone try to enforce the rules.

Patch

diff --git a/hw/pci.c b/hw/pci.c
index a98d6f3..2d38643 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1810,11 +1810,15 @@  static int pci_add_option_rom(PCIDevice *pdev)
         return 0;
 
     if (!pdev->rom_bar) {
+        int class;
         /*
          * Load rom via fw_cfg instead of creating a rom bar,
-         * for 0.11 compatibility.
+         * for 0.11 compatibility. fw_cfg is initialized at boot, so
+         * we cannot do hotplug load of option roms.
          */
-        int class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
+        if (pdev->qdev.hotplugged)
+            return 0;
+        class = pci_get_word(pdev->config + PCI_CLASS_DEVICE);
         if (class == 0x0300) {
             rom_add_vga(pdev->romfile);
         } else {