Re: Second-source devices and interrupt-mapping race

[Johan Hovold]

Hi Rob,

and sorry about the late follow up. I received your reply just before my
holiday and then ended up in a bit of a rabbit hole with the Qualcomm
PHY drivers so this got put on the back burner.

On Thu, Aug 04, 2022 at 09:42:01AM -0600, Rob Herring wrote:
> On Thu, Jul 28, 2022 at 3:30 AM Johan Hovold <johan@xxxxxxxxxx> wrote:

> > 2. Rob, Krzysztof, I assume that handling second-source devices by
> > enabling multiple variants in the devicetree can not be considered
> > correct?
> 
> Probably not, but there's not really any defined policy there. What
> that looks like in DT depends on the component. Displays are a common
> one and don't lend well to populating multiple in the DT. For those,
> the only solution so far is we require the 2nd source to be compatible
> with the default/1st. I think that was QCom chromebooks...
> 
> The easy answer is firmware should deal with figuring out what's
> actually there and update the DT accordingly.

Right.
 
> > What about the related case of simply non-populated devices (e.g. laptop
> > variants without a touchscreen)?
> 
> Shouldn't that just be a case of the driver not screaming if there's
> no device?

Right, that's simple, but what I'm getting at is that this is also a
case of the devicetree not describing the actual hardware. If we allow
that, does that imply that we should also allow having second-source
devices in the devicetree even if we know that at least one of them will
be non-populated?

The big difference is dealing with any "shared" resources, such has the
pinconfig and interrupt in my HID example, which would now appear to be
claimed by more than one device.

It seems we'd need some way to describe the devices as mutually
exclusive, and perhaps that's reason enough not to try to generalise the
single-non-populated-device case.

> > Note that we have at least two cases of "second-source" nodes in mainline
> > ("rtc" and "trackpad", respectively):
> >
> >         85a9efcd4e29 ("ARM: mvebu: add DT support for Seagate NAS 2 and 4-Bay")
> >         689b937bedde ("arm64: dts: mediatek: add mt8173 elm and hana board")
> >
> > and that, for example, the i2c-hid driver explicitly supports
> > non-populated devices:
> >
> >         b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before really probing")
> >
> > and the commit message indicates that this is something that Chromebooks
> > rely on.
> >
> > For the X13s, I'm not sure how we would go about to tell the variants
> > apart (the ACPI tables that Windows use include both touchpads and an
> > optional touchscreen). In the end, the boot firmware might need to
> > resort to a similar kind of probing if we don't allow the kernel to do
> > it.
> >
> > Finally, note that while disabling async probing for "second-source"
> > nodes (e.g. if we could mark them as requiring that) would take care of
> > the irq-mapping race, we'd still currently also need to move any
> > pinconfig handles to the parent bus node (as is also done in one of the
> > in-tree examples above) to suppress the corresponding pinctrl errors in
> > case the populated device is probed and bound first:
> >
> > [  +0.010217] sc8280xp-tlmm f100000.pinctrl: pin GPIO_182 already requested by 0-0015; cannot claim for 0-002c
> 
> If the config is the same for both we could suppress that warning. If
> not the same, seems a bit dangerous to configure the wrong config...

Indeed, the pin configs would need to be at least compatible.

I'm sure this has the potential to mess things up with respect to other
resources too. And even if it may be possible to get this to work in
many cases it seems firmware needs to be involved for it to work
generally.

Johan