Re: [PATCH 4/4] pcie: add trace-point for power indicator transitions

[Michael S. Tsirkin]

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