Re: [TECH TOPIC] Rust for Linux

[Linus Walleij <linus.walleij@linaro.org>]

On Thu, Jul 22, 2021 at 1:34 PM Wedson Almeida Filho
<wedsonaf@google.com> wrote:

> I had two more questions from reading the code that you may be able to answer:
> 1. Drivers like the one for pl061 call `irq_set_irq_wake` from their
>    `irq_set_wake` callbacks. These are "counted", which is great because it
>    allows us to just call the same function for each gpio/interrupt. However,
>    when a gpio irq chip is removed, what ensures that these increments are
>    accordingly decremented?

Really a question for the irqchip people.

I think you will just see many shortcomings of this type since
the irqchip has very little state in general.
IIUC the framework is designed to be be very lightweight.

The following is based on my relatively superficial understanding
of irqchips.

I don't really get what you mean with "counted" here?

It's just consumers setting a flag that that IRQ should be able to wake
up the system.

In PL061 that percolates up to the parent, the cascaded IRQ.
The parent doesn't "count" the number of wake flags either, not
any driver I know of, the framework only provides these callbacks,
it has no state.

The only practical effect is setting a bit in the hardware (if available)
that the IRQ can wake up the system so the hardware bit *is* the
state. The driver may record it in its state container to ascertain not
to shut off clocks when sleeping for example but in practice it's often
not very thorough.

The IRQ wake status of the parent before we probe is unknown and
there is no way to retrieve it. It can very well be a power-on default,
so by definition undefined.

It could be dangerous to attempt to set it back to disabled when we
leave the since the IRQ may be shared and the irqchip doesn't count
so that will yield a first come-first serve bug.

In practice there are a few calls to this function and the same bit
will be set in hardware a few times from different callers and that
is fine because it is all that is needed most times. Then the whole
state goes away with the hardware it is in when the system goes
down.

I think this is partly by design to keep the state in the hardware.
If the hardware changes state because of external factors, then
the state reflects reality through reading the registers etc.
This is maybe a questionable design principle.

> 2. `irq_domain_remove` requires that all mappings have been disposed of before
>    being called. I see that `gpiochip_irqchip_remove` calls
>    `irq_dispose_mapping` for each pin before calling `irq_domain_remove`. But
>    what prevents *new* mappings from being added back before `irq_domain_remove`
>    is called?

Again mostly a question for the irqchip people I guess but nothing I
think since the irqdomain does not have any synchronization
primitives or any counting.

We have made it mostly work fine in practice for many years
because in practice there is often just one way in which the
IRQ tree can come up and down and once up it doesn't change
much.

It could be more elegant I guess. Especially since it could very well
have state as the domain is just a piece of software.

If you start to think that irqchip could use some more developers
and maintainers you are probably right.

Yours,
Linus Walleij