(Trimming text)
On Wed, Jun 03, 2020 at 12:23:48AM +0000, Rajat Jain wrote:
> On Tue, Jun 2, 2020 at 4:49 PM Prashant Malani <pmal...@google.com> wrote:
> >
> > Hi Rajat,
>
> Hi Prashant, thanks for taking a look.
>
> >
> > On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> > > +static bool risky_device(struct pci_dev *pdev)
> > > +{
> > > + if (pdev->untrusted) {
> > > + pci_warn(pdev,
> > > + "Skipping IOMMU quirk for dev (%04X:%04X) on
> > > untrusted"
> > > + " PCI link. Please check with your BIOS/Platform"
> > > + " vendor about this\n", pdev->vendor,
> > > pdev->device);
> > > + return true;
> > > + }
> > > + return false;
> > minor suggestion: Perhaps you could use a guard clause here? It would save
> > you
> > a level of indentation, and possibly allow better string splitting
> > (e.g keeping "untrusted PCI" together). So something like:
> >
> > if (!pdev->untrusted)
> > return false;
>
> I personally have found double negation expressions always confusing,
> even if negation is part of the variable. (For e.g. I have found I
> need to be always stop and convince myself that:
>
> "if (!pdev->untrusted)"
> <do something>
>
> conceptually implies
>
> "if (pdev->trusted)".
> <do something>
>
> So I tend to keep negations to minimum. In this case, it doesn't buy
> us much either, so I'd prefer to keep it the same unless there are
> more opinions on this. OTOH I don't mind changing it too if you feel
> strongly about this.
Ordinarily, I'd agree with you regarding double-negatives.
However, in this case the condition phrasing is so brief ("not untrusted") that
I'd
argue the indentation savings outweigh possible interpretation issues.
That said, I don't have a strong opinion here, so will defer to the
maintainer's preference.
Best,
>
> Thanks,
>
> Rajat
>
>
> >
> > pci_warn(...);
> >
> > I also hear the column limit warning is now for 100 chars [1], though
> > I'm not sure how it's being handled in this file.
> >
> > Best regards,
> >
> > -Prashant
> >
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> >
> > > +}
> > > +
> > > const struct iommu_ops intel_iommu_ops = {
> > > .capable = intel_iommu_capable,
> > > .domain_alloc = intel_iommu_domain_alloc,
> > > @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
> > >
> > > static void quirk_iommu_igfx(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > > dmar_map_gfx = 0;
> > > }
> > > @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,
> > > 0x163D, quirk_iommu_igfx);
> > >
> > > static void quirk_iommu_rwbf(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > /*
> > > * Mobile 4 Series Chipset neglects to set RWBF capability,
> > > * but needs it. Same seems to hold for the desktop versions.
> > > @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct
> > > pci_dev *dev)
> > > {
> > > unsigned short ggc;
> > >
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > if (pci_read_config_word(dev, GGC, &ggc))
> > > return;
> > >
> > > @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> > > pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> > > if (!pdev)
> > > return;
> > > +
> > > + if (risky_device(pdev)) {
> > > + pci_dev_put(pdev);
> > > + return;
> > > + }
> > > +
> > > pci_dev_put(pdev);
> > >
> > > /* System Management Registers. Might be hidden, in which case
> > > @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> > > if (!pdev)
> > > return;
> > >
> > > + if (risky_device(pdev)) {
> > > + pci_dev_put(pdev);
> > > + return;
> > > + }
> > > +
> > > if (pci_read_config_dword(pdev, 0x188, &vtisochctrl)) {
> > > pci_dev_put(pdev);
> > > return;
> > > --
> > > 2.27.0.rc2.251.g90737beb825-goog
> > >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
