PCI: Make reset warning messages different

We have same warning message for FLR and AF FLR and users can't
know which type of resets the PCI device is taking when there are
pending transactions. The patch makes them different for FLR and
AF FLR cases.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Wed, Oct 15, 2014 at 5:41 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> We have same warning message for FLR and AF FLR and users can't
> know which type of resets the PCI device is taking when there are
> pending transactions. The patch makes them different for FLR and
> AF FLR cases.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  drivers/pci/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2d708cd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3144,7 +3144,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>                 return 0;
>
>         if (!pci_wait_for_pending_transaction(dev))
> -               dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
> +               dev_err(&dev->dev, "Force FLR with pending transaction\n");
>
>         pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>
> @@ -3178,7 +3178,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>                                  PCI_AF_STATUS_TP << 8))
>                 goto clear;
>
> -       dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
> +       dev_err(&dev->dev, "Force AF FLR with pending transaction\n");

Making the text different is fine, but I don't think "FLR" and "AF
FLR" are meaningful except to extremely technical people.  So I think
"reset" needs to stay spelled out in the message.

>  clear:
>         pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
> --
> 1.8.3.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 15, 2014 at 05:48:35PM -0600, Bjorn Helgaas wrote:
>On Wed, Oct 15, 2014 at 5:41 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> We have same warning message for FLR and AF FLR and users can't
>> know which type of resets the PCI device is taking when there are
>> pending transactions. The patch makes them different for FLR and
>> AF FLR cases.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/pci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 625a4ac..2d708cd 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3144,7 +3144,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>>                 return 0;
>>
>>         if (!pci_wait_for_pending_transaction(dev))
>> -               dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>> +               dev_err(&dev->dev, "Force FLR with pending transaction\n");
>>
>>         pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>>
>> @@ -3178,7 +3178,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>>                                  PCI_AF_STATUS_TP << 8))
>>                 goto clear;
>>
>> -       dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>> +       dev_err(&dev->dev, "Force AF FLR with pending transaction\n");
>
>Making the text different is fine, but I don't think "FLR" and "AF
>FLR" are meaningful except to extremely technical people.  So I think
>"reset" needs to stay spelled out in the message.
>

Agree, it's worthy to keep "reset". How about something like this:

   "Force function level reset with pending transaction"         - FLR
   "Force AF function level reset with pending transaction"      - AF FLR

If above messages look good to you, I'll send out another revision.

Thanks,
Gavin

>>  clear:
>>         pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
>> --
>> 1.8.3.2
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 15, 2014 at 6:19 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> On Wed, Oct 15, 2014 at 05:48:35PM -0600, Bjorn Helgaas wrote:
>>On Wed, Oct 15, 2014 at 5:41 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>>> We have same warning message for FLR and AF FLR and users can't
>>> know which type of resets the PCI device is taking when there are
>>> pending transactions. The patch makes them different for FLR and
>>> AF FLR cases.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>  drivers/pci/pci.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 625a4ac..2d708cd 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3144,7 +3144,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>>>                 return 0;
>>>
>>>         if (!pci_wait_for_pending_transaction(dev))
>>> -               dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>>> +               dev_err(&dev->dev, "Force FLR with pending transaction\n");
>>>
>>>         pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>>>
>>> @@ -3178,7 +3178,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>>>                                  PCI_AF_STATUS_TP << 8))
>>>                 goto clear;
>>>
>>> -       dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>>> +       dev_err(&dev->dev, "Force AF FLR with pending transaction\n");
>>
>>Making the text different is fine, but I don't think "FLR" and "AF
>>FLR" are meaningful except to extremely technical people.  So I think
>>"reset" needs to stay spelled out in the message.
>>
>
> Agree, it's worthy to keep "reset". How about something like this:
>
>    "Force function level reset with pending transaction"         - FLR
>    "Force AF function level reset with pending transaction"      - AF FLR
>
> If above messages look good to you, I'll send out another revision.

How about something like "timed out waiting for pending transaction;
performing function level reset"?

"Force reset with pending transaction" sounds like a pending
transaction might be the mechanism we're using to perform the reset.

Out of curiosity, is there some issue you tripped over where it's
important for users to know this difference?

Just FYI, I'll be on vacation the rest of this week.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 15, 2014 at 09:43:32PM -0600, Bjorn Helgaas wrote:
>On Wed, Oct 15, 2014 at 6:19 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> On Wed, Oct 15, 2014 at 05:48:35PM -0600, Bjorn Helgaas wrote:
>>>On Wed, Oct 15, 2014 at 5:41 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>>>> We have same warning message for FLR and AF FLR and users can't
>>>> know which type of resets the PCI device is taking when there are
>>>> pending transactions. The patch makes them different for FLR and
>>>> AF FLR cases.
>>>>
>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> ---
>>>>  drivers/pci/pci.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index 625a4ac..2d708cd 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3144,7 +3144,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>>>>                 return 0;
>>>>
>>>>         if (!pci_wait_for_pending_transaction(dev))
>>>> -               dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>>>> +               dev_err(&dev->dev, "Force FLR with pending transaction\n");
>>>>
>>>>         pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>>>>
>>>> @@ -3178,7 +3178,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>>>>                                  PCI_AF_STATUS_TP << 8))
>>>>                 goto clear;
>>>>
>>>> -       dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>>>> +       dev_err(&dev->dev, "Force AF FLR with pending transaction\n");
>>>
>>>Making the text different is fine, but I don't think "FLR" and "AF
>>>FLR" are meaningful except to extremely technical people.  So I think
>>>"reset" needs to stay spelled out in the message.
>>>
>>
>> Agree, it's worthy to keep "reset". How about something like this:
>>
>>    "Force function level reset with pending transaction"         - FLR
>>    "Force AF function level reset with pending transaction"      - AF FLR
>>
>> If above messages look good to you, I'll send out another revision.
>
>How about something like "timed out waiting for pending transaction;
>performing function level reset"?
>
>"Force reset with pending transaction" sounds like a pending
>transaction might be the mechanism we're using to perform the reset.
>

Yep, I'll change according to your suggestion.

>Out of curiosity, is there some issue you tripped over where it's
>important for users to know this difference?
>

I saw this message when issuing FLR (not AF case) to (EEH) frozen device.
Since 0xFF's is always returned from the frozen device, this message was
surely printed. However, I didn't know it was FLR or AF-FLR from the
original message.

>Just FYI, I'll be on vacation the rest of this week.
>

Ok. Have a good vacation.

Thanks,
Gavin

>Bjorn
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html