Re: [PATCH v5 07/10] PCI/IDE: Add IDE establishment helpers

[<dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
[..]
> >> +/**
> >> + * pci_ide_stream_enable() - enable a Selective IDE Stream
> >> + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> >> + * @ide: registered and setup IDE settings descriptor
> >> + *
> >> + * Activate the stream by writing to the Selective IDE Stream Control
> >> + * Register.
> >> + *
> >> + * Return: 0 if the stream successfully entered the "secure" state, and -ENXIO
> >> + * otherwise.
> >> + *
> >> + * Note that the state may go "insecure" at any point after returning 0, but
> >> + * those events are equivalent to a "link down" event and handled via
> >> + * asynchronous error reporting.
> >> + */
> >> +int pci_ide_stream_enable(struct pci_dev *pdev, struct pci_ide *ide)
> >> +{
> >> +    struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> >> +    int pos;
> >> +    u32 val;
> >> +
> >> +    if (!settings)
> >> +        return -ENXIO;
> >> +
> >> +    pos = sel_ide_offset(pdev, settings);
> >> +
> >> +    set_ide_sel_ctl(pdev, ide, pos, true);
> >> +
> >> +    pci_read_config_dword(pdev, pos + PCI_IDE_SEL_STS, &val);
> >> +    if (FIELD_GET(PCI_IDE_SEL_STS_STATE, val) !=
> >> +        PCI_IDE_SEL_STS_STATE_SECURE) {
> >> +        set_ide_sel_ctl(pdev, ide, pos, false);
> 
> 
> Ah this is an actual problem, this is not right. The PCIe r6.1 spec says:
> 
> "It is permitted, but strongly not recommended, to Set the Enable bit in the IDE Extended Capability
> entry for a Stream prior to the completion of key programming for that Stream".

This ordering is controlled by the TSM driver though...

> 
> And I have a device like that where the links goes secure after the last
> key is SET_GO. So it is okay to return an error here but not ok to clear
> the Enabled bit.

...can you not simply wait to call pci_ide_stream_enable() until after the
SET_GO?

Are you saying the problem is that the shutdown path needs to do the
reverse SET_STOP before disabling the stream?

> Was it "Do or do not, there is no try for pci_ide_stream_enable() (Bjorn)" in the changelog? Not very descriptive :-/ Thanks,

I understand he was taking issue with the comment, but this practical issue
is much more serious. I will push error detection and cleanup out of this
helper, and make it return void.

Thanks for the hardware testing!