Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote:
> On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote:
> > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote:
> > > > Moreover, it seems you are bailing out and basically denying driver to load.
> > > > This does look that capability is simply the first register that blows the setup.
> > > > I think you have to fix something into Xen to avoid loading these drivers or
> > > > check with something like pci_device_is_present() approach.
> > > 
> > > Is there a backing PCI device BAR for those MMIO regions that the
> > > pinctrl driver is trying to access? AFAICT those regions are only
> > > reported in the ACPI DSDT table on the _CRS method of the object (at
> > > least on my system).
> > 
> > Unfortunately it does not expose PCI configuration space.
> 
> Are those regions supposed to be marked as reserved in the memory map,
> or that's left to the discretion of the hardware vendor?

I didn't get. The OS doesn't see them and an internal backbone simply drops any
IO access to that region.

> > > Doing something like pci_device_is_present would require a register
> > > that we know will never return ~0 unless the device is not present. As
> > > said above, maybe we could use REVID to that end?
> > 
> > Yes, that's good, see above.
> > 
> > WRT capabilities, if we crash we will see the report immediately on the
> > hardware which has such an issue. (It's quite unlikely we will ever have one,
> > that's why I consider it's not critical)
> 
> I would rather prefer to not crash, because I think the kernel should
> only resort to crashing when there's no alternative, and here it's
> perfectly fine to just print an error message and don't load the
> driver.

Are we speaking about real hardware that has an issue? I eagerly want to know
what is that beast.

> IMO I would rather boot without pinctrl than get a panic if
> it turns out pinctrl capabilities list is somehow corrupted.

Again, do you have a hardware that does this?

> It's a
> long shot, but the check added in order to prevent this scenario is
> minimal.

> In any case I will send a new version with the REVID check and this
> current patch.

Okay, let's continue there, but I'm pessimistic about accepting this patch.

-- 
With Best Regards,
Andy Shevchenko