Message ID | 20210324154312.61878-1-roger.pau@citrix.com |
---|---|
Headers | show |
Series | intel/pinctrl: check some registers reads | expand |
On Wed, Mar 24, 2021 at 04:43:11PM +0100, Roger Pau Monne wrote: Thanks for a fix! My comments below. > Use the value read from the REVID register in order to check for the > presence of the device. A read of all ones is treated as if the device > is not present, and hence probing is ended. > > This fixes an issue when running as a Xen PVH dom0, where the ACPI > DSDT table is provided unmodified to dom0 and hence contains the > pinctrl devices, but the MMIO region(s) containing the device > registers might not be mapped in the guest physical memory map if such > region(s) are not exposed on a PCI device BAR or marked as reserved in > the host memory map. Any particular point that we can use in the Fixes tag? ... > Suggested-by: Andy Shevchenko <andy@kernel.org> Hmm... was it that address I have used? In any case I think my @linux.intel.com is better. ... > /* Determine community features based on the revision */ > value = readl(regs + REVID); > + if (value == ~0u) > + return -ENODATA; I think -ENODEV is more appropriate here. Also comment above should be adjusted to explain this check.
On Wed, Mar 24, 2021 at 07:01:18PM +0200, Andy Shevchenko wrote: > On Wed, Mar 24, 2021 at 04:43:11PM +0100, Roger Pau Monne wrote: > > Thanks for a fix! My comments below. > > > Use the value read from the REVID register in order to check for the > > presence of the device. A read of all ones is treated as if the device > > is not present, and hence probing is ended. > > > > This fixes an issue when running as a Xen PVH dom0, where the ACPI > > DSDT table is provided unmodified to dom0 and hence contains the > > pinctrl devices, but the MMIO region(s) containing the device > > registers might not be mapped in the guest physical memory map if such > > region(s) are not exposed on a PCI device BAR or marked as reserved in > > the host memory map. > > Any particular point that we can use in the Fixes tag? Hm, I haven't seen those issues up until 91d898e51e60 ('pinctrl: intel: Convert capability list to features'), but the device wasn't working properly for sure, as the registers where not accessible, it just didn't lead to a kernel crash. > ... > > > Suggested-by: Andy Shevchenko <andy@kernel.org> > > Hmm... was it that address I have used? In any case I think my @linux.intel.com > is better. I just used the same as the one that's on the MAINTAINERS file, because I already had that n my Cc list. I can change to the @intel one if that's your preference. > ... > > > /* Determine community features based on the revision */ > > value = readl(regs + REVID); > > + if (value == ~0u) > > + return -ENODATA; > > I think -ENODEV is more appropriate here. > Also comment above should be adjusted to explain this check. Right, will change and send v3. Thanks.