[Qemu-devel] [PATCH] pci_aer_init: cleanup

[Qemu-devel] [PATCH] pci_aer_init: cleanup

[Cao jin]

log_max have no chance to be PCIE_AER_LOG_MAX_UNSET, unless user specify it.

Bonus:
1. remove unnecessary local variable.
2. fix a typo.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pcie_aer.c         | 10 +---------
 include/hw/pci/pcie_aer.h |  2 +-
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index ac47f34..6cf088b 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -99,18 +99,10 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
 int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
                   uint16_t size)
 {
-    PCIExpressDevice *exp;
-
     pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, cap_ver,
                         offset, size);
-    exp = &dev->exp;
-    exp->aer_cap = offset;
+    dev->exp.aer_cap = offset;
 
-    /* log_max is property */
-    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
-        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
-    }
-    /* clip down the value to avoid unreasobale memory usage */
     if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
         return -EINVAL;
     }
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index c373591..31755ef 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -40,7 +40,7 @@ struct PCIEAERLog {
      * The specified value will be clipped down to PCIE_AER_LOG_MAX_LIMIT
      * to avoid unreasonable memory usage.
      * I bet that 128 log size would be big enough, otherwise too many errors
-     * for system to function normaly. But could consecutive errors occur?
+     * for system to function normally. But could consecutive errors occur?
      */
 #define PCIE_AER_LOG_MAX_DEFAULT        8
 #define PCIE_AER_LOG_MAX_LIMIT          128
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pci_aer_init: cleanup

[Cao jin]

ping

On 10/13/2016 10:32 AM, Cao jin wrote:
> log_max have no chance to be PCIE_AER_LOG_MAX_UNSET, unless user specify it.
>
> Bonus:
> 1. remove unnecessary local variable.
> 2. fix a typo.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>   hw/pci/pcie_aer.c         | 10 +---------
>   include/hw/pci/pcie_aer.h |  2 +-
>   2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index ac47f34..6cf088b 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -99,18 +99,10 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
>   int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
>                     uint16_t size)
>   {
> -    PCIExpressDevice *exp;
> -
>       pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, cap_ver,
>                           offset, size);
> -    exp = &dev->exp;
> -    exp->aer_cap = offset;
> +    dev->exp.aer_cap = offset;
>
> -    /* log_max is property */
> -    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> -        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> -    }
> -    /* clip down the value to avoid unreasobale memory usage */
>       if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
>           return -EINVAL;
>       }
> diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
> index c373591..31755ef 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -40,7 +40,7 @@ struct PCIEAERLog {
>        * The specified value will be clipped down to PCIE_AER_LOG_MAX_LIMIT
>        * to avoid unreasonable memory usage.
>        * I bet that 128 log size would be big enough, otherwise too many errors
> -     * for system to function normaly. But could consecutive errors occur?
> +     * for system to function normally. But could consecutive errors occur?
>        */
>   #define PCIE_AER_LOG_MAX_DEFAULT        8
>   #define PCIE_AER_LOG_MAX_LIMIT          128
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] pci_aer_init: cleanup

[Marcel Apfelbaum]

On 10/13/2016 05:32 AM, Cao jin wrote:
> log_max have no chance to be PCIE_AER_LOG_MAX_UNSET, unless user specify it.
>

Hi,


> Bonus:
> 1. remove unnecessary local variable.
> 2. fix a typo.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie_aer.c         | 10 +---------
>  include/hw/pci/pcie_aer.h |  2 +-
>  2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index ac47f34..6cf088b 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -99,18 +99,10 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
>  int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
>                    uint16_t size)
>  {
> -    PCIExpressDevice *exp;
> -
>      pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, cap_ver,
>                          offset, size);
> -    exp = &dev->exp;
> -    exp->aer_cap = offset;
> +    dev->exp.aer_cap = offset;
>
> -    /* log_max is property */
> -    if (dev->exp.aer_log.log_max == PCIE_AER_LOG_MAX_UNSET) {
> -        dev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
> -    }

OK, maybe this check is not needed, the default value is PCIE_AER_LOG_MAX_DEFAULT -> 8.

The user can pass -device ioh3420,aer_log_max=0xffff and until now would be automatically
modified to PCIE_AER_LOG_MAX_DEFAULT -> 15, from now on he will fail. However the behavior
change is OK.



> -    /* clip down the value to avoid unreasobale memory usage */
>      if (dev->exp.aer_log.log_max > PCIE_AER_LOG_MAX_LIMIT) {
>          return -EINVAL;
>      }


If you already started working on it, maybe you can fix the error message when the user passes
aer_log_max over PCIE_AER_LOG_MAX_LIMIT.
For the moment we get:
     qemu-system-x86_64: -device ioh3420,aer_log_max=0xfff: Device initialization failed

And I would suggest a more clear patch subject.

Thanks,
Marcel

> diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
> index c373591..31755ef 100644
> --- a/include/hw/pci/pcie_aer.h
> +++ b/include/hw/pci/pcie_aer.h
> @@ -40,7 +40,7 @@ struct PCIEAERLog {
>       * The specified value will be clipped down to PCIE_AER_LOG_MAX_LIMIT
>       * to avoid unreasonable memory usage.
>       * I bet that 128 log size would be big enough, otherwise too many errors
> -     * for system to function normaly. But could consecutive errors occur?
> +     * for system to function normally. But could consecutive errors occur?
>       */
>  #define PCIE_AER_LOG_MAX_DEFAULT        8
>  #define PCIE_AER_LOG_MAX_LIM