| Message ID | 1279900598-8432-1-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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
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.
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 {