Re: [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence
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
>
> 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.
Re: [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence
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 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. -- With Best Regards, Andy Shevchenko
[PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence
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.
Suggested-by: Andy Shevchenko
Signed-off-by: Roger Pau Monné
---
Changes since v1:
- New in this version.
---
Cc: Mika Westerberg
Cc: Andy Shevchenko
Cc: Linus Walleij
Cc: linux-g...@vger.kernel.org
---
drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
b/drivers/pinctrl/intel/pinctrl-intel.c
index 8085782cd8f9..59d13342caf6 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1493,6 +1493,8 @@ static int intel_pinctrl_probe(struct platform_device
*pdev,
/* Determine community features based on the revision */
value = readl(regs + REVID);
+ if (value == ~0u)
+ return -ENODATA;
if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
community->features |= PINCTRL_FEATURE_DEBOUNCE;
community->features |= PINCTRL_FEATURE_1K_PD;
--
2.30.1
