Re: [PATCH] iommu/vt-d: Add RPLS to quirk list to skip TE disabling

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2/24/22 9:39 PM, Vivi, Rodrigo wrote:
> On Thu, 2022-02-24 at 13:42 +0800, Lu Baolu wrote:
>> On 2/23/22 2:29 PM, Tejas Upadhyay wrote:
>>> The VT-d spec requires (10.4.4 Global Command Register, TE
>>> field) that:
>>>
>>> Hardware implementations supporting DMA draining must drain
>>> any in-flight DMA read/write requests queued within the
>>> Root-Complex before completing the translation enable
>>> command and reflecting the status of the command through
>>> the TES field in the Global Status register.
>>>
>>> Unfortunately, some integrated graphic devices fail to do
>>> so after some kind of power state transition. As the
>>> result, the system might stuck in iommu_disable_translati
>>> on(), waiting for the completion of TE transition.
>>>
>>> This adds RPLS to a quirk list for those devices and skips
>>> TE disabling if the qurik hits.
>>>
>>> Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/4898
>>> Fixes: LCK-10789
>>
>> Remove this please.
> 
> good catch. Wrong use of Fixes tag.
> "Fixes:" should only be used for patches fixing other patches and
> mentioning the commit id.

This is still a fix patch, right? If so,

Fixes: b1012ca8dc4f9 "iommu/vt-d: Skip TE disabling on quirky gfx 
dedicated iommu"
Cc: stable@vger.kernel.org

> 
> Baolu,
> could you mind if we use
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4898
> 
> or maybe
> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/4898
> 
> This last one seems to be the one use in drivers/iommu
> and the Closes is what we use in drm-intel, hence the one used
> with gitlab.freedesktop links in general.

How about "Link:"?

As Documentation/process/submitting-patches.rst states:

If related discussions or any other background information behind the change
can be found on the web, add 'Link:' tags pointing to it. In case your patch
fixes a bug, for example, add a tag with a URL referencing the report in the
mailing list archives or a bug tracker; if the patch is a result of some
earlier mailing list discussion or something documented on the web, point to
it.


> 
>>
>>> Tested-by: Raviteja Goud Talla <ravitejax.goud.talla@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Tejas Upadhyay
>>> <tejaskumarx.surendrakumar.upadhyay@intel.com>
> 
> you can keep my rv-b
> 
>>> ---
>>>    drivers/iommu/intel/iommu.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c
>>> b/drivers/iommu/intel/iommu.c
>>> index 92fea3fbbb11..be9487516617 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -5743,7 +5743,7 @@ static void quirk_igfx_skip_te_disable(struct
>>> pci_dev *dev)
>>>          ver = (dev->device >> 8) & 0xff;
>>>          if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
>>>              ver != 0x4e && ver != 0x8a && ver != 0x98 &&
>>> -           ver != 0x9a)
>>> +           ver != 0x9a && ver != 0xa7)
>>>                  return;
>>>    
>>>          if (risky_device(dev))
>>
>> This is a quirk for integrated graphic device. Rodrigo, does this
>> hardware needs this quirk as well?
> 
> Yes, 0xa7* are RPL-S that are integrated and very similar to 0x46*
> (ADL-S)

Thank you for confirming this quirk.

> 
> I would prefer to have a more expanded list instead of group or at
> least some defines with the platform names.

Yeah! That's better.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu