On Thu, Mar 25, 2021 at 09:46:46AM +0100, Roger Pau Monné wrote:

On Wed, Mar 24, 2021 at 06:57:12PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote:

quoted

On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote:

quoted

On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:

...

quoted

quoted

quoted

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.

I'm not sure I understand the above reply. My question was whether the
MMIO regions used by the pinctrl device (as fetched from the ACPI DSDT
table) are supposed belong to regions marked as RESERVED in the
firmware memory map (ie: either the e820 or the EFI one).

I don't actually know. I guess it should be done in order to have ACPI device
a possibility to claim the resource.

quoted

quoted

quoted

quoted

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.

OK, I'm not going to resend this anymore. I'm happy with just getting
the first patch in.

I think you trust the hardware more that I would do, and I also think
the check added here is very minimal an unintrusive and serves as a
way to sanitize the data fetched from the hardware in order to prevent
a kernel page fault if such data turns out to be wrong.

Taking a reactive approach of requiring a broken piece of hardware to
exist in order to sanitize a fetched value seems too risky. I could
add a WARN_ON or similar if you want some kind of splat that's very
noticeable when this goes wrong but that doesn't end up in a fatal
kernel page fault.

You found the issue anyway as long as you had a crash, so current code already
proved that it does it work perfectly.

Since I know what hardware this driver is for, I can assure you, that it will
be quite unlikely to have wrong data in the capability register. The data sheet
is crystal clear about the register's contents: on real hardware it must be
present and be set to a sane value.

-- 
With Best Regards,
Andy Shevchenko