[0/4] pcie: cleanup code and add trace point

Message

Vladimir Sementsov-Ogievskiy Feb. 4, 2023, 5:47 p.m. UTC

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(-)

Comments

Philippe Mathieu-Daudé Feb. 5, 2023, 10:53 a.m. UTC | #1

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> 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 <[e-mail]>
> ---
>   include/hw/pci/pcie_regs.h |  9 ---------
>   hw/pci/pcie.c              | 13 +++++++------
>   2 files changed, 7 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Philippe Mathieu-Daudé Feb. 5, 2023, 10:54 a.m. UTC | #2

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> 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 <[e-mail]>
> ---
>   hw/pci/pcie.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Philippe Mathieu-Daudé Feb. 5, 2023, 10:56 a.m. UTC | #3

On 4/2/23 18:47, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>   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é <[e-mail]>

> +    }
> +}

Vladimir Sementsov-Ogievskiy Feb. 7, 2023, 10:39 a.m. UTC | #4

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 <[e-mail]>
>> ---
>>   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é <[e-mail]>
> 
>> +    }
>> +}
>

Michael S. Tsirkin March 1, 2023, 8:53 p.m. UTC | #5

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 <[e-mail]>
> > > ---
> > >   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é <[e-mail]>
> > 
> > > +    }
> > > +}
> > 
> 
> -- 
> Best regards,
> Vladimir