Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pcie.c       | 20 ++++++++++++++++++++
 hw/pci/trace-events |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ccdb2377e1..1a19368994 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -28,6 +28,7 @@
 #include "hw/pci/pcie_regs.h"
 #include "hw/pci/pcie_port.h"
 #include "qemu/range.h"
+#include "trace.h"
 
 //#define DEBUG_PCIE
 #ifdef DEBUG_PCIE
@@ -718,6 +719,20 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta)
     *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 }
 
+static const char *pcie_sltctl_pic_str(uint16_t sltctl)
+{
+    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
+    case PCI_EXP_SLTCTL_PWR_IND_ON:
+        return "on";
+    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+        return "blink";
+    case PCI_EXP_SLTCTL_PWR_IND_OFF:
+        return "off";
+    default:
+        return "?";
+    }
+}
+
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint16_t old_slt_ctl, uint16_t old_slt_sta,
                                 uint32_t addr, uint32_t val, int len)
@@ -762,6 +777,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    if ((val & PCI_EXP_SLTCTL_PIC) != (old_slt_ctl & PCI_EXP_SLTCTL_PIC)) {
+        trace_pcie_power_indicator(pcie_sltctl_pic_str(old_slt_ctl),
+                                   pcie_sltctl_pic_str(val));
+    }
+
     /*
      * If the slot is populated, power indicator is off and power
      * controller is off, it is safe to detach the devices.
diff --git a/hw/pci/trace-events b/hw/pci/trace-events
index aaf46bc92d..ec4a5ff43d 100644
--- a/hw/pci/trace-events
+++ b/hw/pci/trace-events
@@ -15,3 +15,6 @@ msix_write_config(char *name, bool enabled, bool masked) "dev %s enabled %d mask
 sriov_register_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: creating %d vf devs"
 sriov_unregister_vfs(const char *name, int slot, int function, int num_vfs) "%s %02x:%x: Unregistering %d vf devs"
 sriov_config_write(const char *name, int slot, int fun, uint32_t offset, uint32_t val, uint32_t len) "%s %02x:%x: sriov offset 0x%x val 0x%x len %d"
+
+# pcie.c
+pcie_power_indicator(const char *old, const char *new) "%s -> %s"
-- 
2.34.1

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/pci/pcie.c       | 20 ++++++++++++++++++++
>   hw/pci/trace-events |  3 +++
>   2 files changed, 23 insertions(+)

> +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
> +{
> +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
> +    case PCI_EXP_SLTCTL_PWR_IND_ON:
> +        return "on";
> +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> +        return "blink";
> +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
> +        return "off";
> +    default:
> +        return "?";

Maybe "illegal"?

Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    }
> +}

Thanks for reviewing!

On 05.02.23 13:56, Philippe Mathieu-Daudé wrote:
> On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/pci/pcie.c       | 20 ++++++++++++++++++++
>>   hw/pci/trace-events |  3 +++
>>   2 files changed, 23 insertions(+)
> 
>> +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
>> +{
>> +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
>> +    case PCI_EXP_SLTCTL_PWR_IND_ON:
>> +        return "on";
>> +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
>> +        return "blink";
>> +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
>> +        return "off";
>> +    default:
>> +        return "?";
> 
> Maybe "illegal"?

I just was unsure about it.

For SHPC, 0 is correct, and means that this command don't change the led state.

But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right?


Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed.

> 
> Otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +    }
>> +}
> 

-- 
Best regards,
Vladimir

On Tue, Feb 07, 2023 at 01:39:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Thanks for reviewing!
> 
> On 05.02.23 13:56, Philippe Mathieu-Daudé wrote:
> > On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >   hw/pci/pcie.c       | 20 ++++++++++++++++++++
> > >   hw/pci/trace-events |  3 +++
> > >   2 files changed, 23 insertions(+)
> > 
> > > +static const char *pcie_sltctl_pic_str(uint16_t sltctl)
> > > +{
> > > +    switch (sltctl & PCI_EXP_SLTCTL_PIC) {
> > > +    case PCI_EXP_SLTCTL_PWR_IND_ON:
> > > +        return "on";
> > > +    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
> > > +        return "blink";
> > > +    case PCI_EXP_SLTCTL_PWR_IND_OFF:
> > > +        return "off";
> > > +    default:
> > > +        return "?";
> > 
> > Maybe "illegal"?
> 
> I just was unsure about it.
> 
> For SHPC, 0 is correct, and means that this command don't change the led state.
> 
> But with PCI-e hotplug we don't have such commands but change the led directly, so it must be one of "on"/"blink"/"off", and zero is really wrong, right?
> 
> 
> Also, I'm now looking at /* TODO: send event to monitor */ in shpc code, and working on it. So, I think, I'll soon send patches with such event for both SHPC and PCI-e, and probably that trace point becomes not needed.


I think it's ok to queue as is, change it with a patch on top.

> > 
> > Otherwise:
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > > +    }
> > > +}
> > 
> 
> -- 
> Best regards,
> Vladimir