* [PATCH 0/4] pcie: cleanup code and add trace point
@ 2023-02-07 12:09 Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro Vladimir Sementsov-Ogievskiy
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
Hi all!
Here is tiny code cleanup + on trace point to track power indicator
changes (which may help to analyze
"Hot-unplug failed: guest is busy (power indicator blinking)" error
message).
Vladimir Sementsov-Ogievskiy (4):
pcie: pcie_cap_slot_write_config(): use correct macro
pcie_regs: drop duplicated indicator value macros
pcie: drop unused PCIExpressIndicator
pcie: add trace-point for power indicator transitions
include/hw/pci/pcie.h | 8 --------
include/hw/pci/pcie_regs.h | 14 --------------
hw/pci/pcie.c | 33 +++++++++++++++++++++++++++------
hw/pci/trace-events | 3 +++
4 files changed, 30 insertions(+), 28 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:09 ` Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 2/4] pcie_regs: drop duplicated indicator value macros Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
PCI_EXP_SLTCTL_PIC_OFF is a value, and PCI_EXP_SLTCTL_PIC is a mask.
Happily PCI_EXP_SLTCTL_PIC_OFF is a maximum value for this mask and is
equal to the mask itself. Still the code looks like a bug. Let's make
it more reader-friendly.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
hw/pci/pcie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 924fdabd15..82ef723983 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -770,9 +770,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
* control of powered off slots before powering them on.
*/
if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
- (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF &&
+ (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
(!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
- (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) {
+ (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
pcie_cap_slot_do_unplug(dev);
}
pcie_cap_update_power(dev);
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] pcie_regs: drop duplicated indicator value macros
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:09 ` Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 3/4] pcie: drop unused PCIExpressIndicator Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
We already have indicator values in
include/standard-headers/linux/pci_regs.h , no reason to reinvent them
in include/hw/pci/pcie_regs.h. (and we already have usage of
PCI_EXP_SLTCTL_PWR_IND_BLINK and PCI_EXP_SLTCTL_PWR_IND_OFF in
hw/pci/pcie.c, so let's be consistent)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/hw/pci/pcie_regs.h | 9 ---------
hw/pci/pcie.c | 13 +++++++------
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 963dc2e170..00b595a82e 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -70,15 +70,6 @@ typedef enum PCIExpLinkWidth {
#define PCI_EXP_SLTCTL_IND_ON 0x1
#define PCI_EXP_SLTCTL_IND_BLINK 0x2
#define PCI_EXP_SLTCTL_IND_OFF 0x3
-#define PCI_EXP_SLTCTL_AIC_SHIFT ctz32(PCI_EXP_SLTCTL_AIC)
-#define PCI_EXP_SLTCTL_AIC_OFF \
- (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_AIC_SHIFT)
-
-#define PCI_EXP_SLTCTL_PIC_SHIFT ctz32(PCI_EXP_SLTCTL_PIC)
-#define PCI_EXP_SLTCTL_PIC_OFF \
- (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT)
-#define PCI_EXP_SLTCTL_PIC_ON \
- (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT)
#define PCI_EXP_SLTCTL_SUPPORTED \
(PCI_EXP_SLTCTL_ABPE | \
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 82ef723983..ccdb2377e1 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -634,8 +634,8 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
PCI_EXP_SLTCTL_PIC |
PCI_EXP_SLTCTL_AIC);
pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL,
- PCI_EXP_SLTCTL_PIC_OFF |
- PCI_EXP_SLTCTL_AIC_OFF);
+ PCI_EXP_SLTCTL_PWR_IND_OFF |
+ PCI_EXP_SLTCTL_ATTN_IND_OFF);
pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
PCI_EXP_SLTCTL_PIC |
PCI_EXP_SLTCTL_AIC |
@@ -679,7 +679,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
PCI_EXP_SLTCTL_PDCE |
PCI_EXP_SLTCTL_ABPE);
pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
- PCI_EXP_SLTCTL_AIC_OFF);
+ PCI_EXP_SLTCTL_ATTN_IND_OFF);
if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
/* Downstream ports enforce device number 0. */
@@ -694,7 +694,8 @@ void pcie_cap_slot_reset(PCIDevice *dev)
PCI_EXP_SLTCTL_PCC);
}
- pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
+ pic = populated ?
+ PCI_EXP_SLTCTL_PWR_IND_ON : PCI_EXP_SLTCTL_PWR_IND_OFF;
pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic);
}
@@ -770,9 +771,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
* control of powered off slots before powering them on.
*/
if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
- (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
+ (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF &&
(!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
- (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
+ (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) {
pcie_cap_slot_do_unplug(dev);
}
pcie_cap_update_power(dev);
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] pcie: drop unused PCIExpressIndicator
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 2/4] pcie_regs: drop duplicated indicator value macros Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:09 ` Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-poing for power indicator transitions Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
The structure type is unused. Also, it's the only user of corresponding
macros, so drop them too.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/hw/pci/pcie.h | 8 --------
include/hw/pci/pcie_regs.h | 5 -----
2 files changed, 13 deletions(-)
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 798a262a0a..3cc2b15957 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -27,14 +27,6 @@
#include "hw/pci/pcie_sriov.h"
#include "hw/hotplug.h"
-typedef enum {
- /* for attention and power indicator */
- PCI_EXP_HP_IND_RESERVED = PCI_EXP_SLTCTL_IND_RESERVED,
- PCI_EXP_HP_IND_ON = PCI_EXP_SLTCTL_IND_ON,
- PCI_EXP_HP_IND_BLINK = PCI_EXP_SLTCTL_IND_BLINK,
- PCI_EXP_HP_IND_OFF = PCI_EXP_SLTCTL_IND_OFF,
-} PCIExpressIndicator;
-
typedef enum {
/* these bits must match the bits in Slot Control/Status registers.
* PCI_EXP_HP_EV_xxx = PCI_EXP_SLTCTL_xxxE = PCI_EXP_SLTSTA_xxx
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 00b595a82e..1fe0bdd25b 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -66,11 +66,6 @@ typedef enum PCIExpLinkWidth {
#define PCI_EXP_SLTCAP_PSN_SHIFT ctz32(PCI_EXP_SLTCAP_PSN)
-#define PCI_EXP_SLTCTL_IND_RESERVED 0x0
-#define PCI_EXP_SLTCTL_IND_ON 0x1
-#define PCI_EXP_SLTCTL_IND_BLINK 0x2
-#define PCI_EXP_SLTCTL_IND_OFF 0x3
-
#define PCI_EXP_SLTCTL_SUPPORTED \
(PCI_EXP_SLTCTL_ABPE | \
PCI_EXP_SLTCTL_PDCE | \
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] pcie: add trace-poing for power indicator transitions
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2023-02-07 12:09 ` [PATCH 3/4] pcie: drop unused PCIExpressIndicator Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:09 ` Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-point " Vladimir Sementsov-Ogievskiy
2023-02-07 12:11 ` [PATCH 0/4] pcie: cleanup code and add trace point DROP THIS Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] pcie: add trace-point for power indicator transitions
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-poing for power indicator transitions Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:09 ` Vladimir Sementsov-Ogievskiy
2023-02-07 12:11 ` [PATCH 0/4] pcie: cleanup code and add trace point DROP THIS Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:09 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, vsementsov, philmd
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] pcie: cleanup code and add trace point DROP THIS
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-point " Vladimir Sementsov-Ogievskiy
@ 2023-02-07 12:11 ` Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 12:11 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel.apfelbaum, philmd
Please ignore this accidental resend, I'm sorry for the noise.
On 07.02.23 15:09, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is tiny code cleanup + on trace point to track power indicator
> changes (which may help to analyze
> "Hot-unplug failed: guest is busy (power indicator blinking)" error
> message).
>
> Vladimir Sementsov-Ogievskiy (4):
> pcie: pcie_cap_slot_write_config(): use correct macro
> pcie_regs: drop duplicated indicator value macros
> pcie: drop unused PCIExpressIndicator
> pcie: add trace-point for power indicator transitions
>
> include/hw/pci/pcie.h | 8 --------
> include/hw/pci/pcie_regs.h | 14 --------------
> hw/pci/pcie.c | 33 +++++++++++++++++++++++++++------
> hw/pci/trace-events | 3 +++
> 4 files changed, 30 insertions(+), 28 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions
2023-02-07 10:39 ` Vladimir Sementsov-Ogievskiy
@ 2023-03-01 20:53 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-03-01 20:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Philippe Mathieu-Daudé, qemu-devel, marcel.apfelbaum
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions
2023-02-05 10:56 ` [PATCH 4/4] pcie: add trace-point for power indicator transitions Philippe Mathieu-Daudé
@ 2023-02-07 10:39 ` Vladimir Sementsov-Ogievskiy
2023-03-01 20:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-02-07 10:39 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: mst, marcel.apfelbaum
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions
[not found] ` <20230204174758.234951-6-vsementsov@yandex-team.ru>
@ 2023-02-05 10:56 ` Philippe Mathieu-Daudé
2023-02-07 10:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-05 10:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel; +Cc: mst, marcel.apfelbaum
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>
> + }
> +}
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-01 20:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-07 12:09 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 1/4] pcie: pcie_cap_slot_write_config(): use correct macro Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 2/4] pcie_regs: drop duplicated indicator value macros Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 3/4] pcie: drop unused PCIExpressIndicator Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-poing for power indicator transitions Vladimir Sementsov-Ogievskiy
2023-02-07 12:09 ` [PATCH 4/4] pcie: add trace-point " Vladimir Sementsov-Ogievskiy
2023-02-07 12:11 ` [PATCH 0/4] pcie: cleanup code and add trace point DROP THIS Vladimir Sementsov-Ogievskiy
-- strict thread matches above, loose matches on Subject: below --
2023-02-04 17:47 [PATCH 0/4] pcie: cleanup code and add trace point Vladimir Sementsov-Ogievskiy
[not found] ` <20230204174758.234951-6-vsementsov@yandex-team.ru>
2023-02-05 10:56 ` [PATCH 4/4] pcie: add trace-point for power indicator transitions Philippe Mathieu-Daudé
2023-02-07 10:39 ` Vladimir Sementsov-Ogievskiy
2023-03-01 20:53 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.